| Message ID | 20240731135936.2105-2-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi, On 7/31/24 3:59 PM, Laurent Pinchart wrote: > libcamera creates memfds in two locations already, duplicating some > code. Move the code to a new MemFd helper class. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Thanks, patch looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > --- > include/libcamera/base/memfd.h | 34 ++++++++ > include/libcamera/base/meson.build | 1 + > src/libcamera/base/memfd.cpp | 116 ++++++++++++++++++++++++++++ > src/libcamera/base/meson.build | 1 + > src/libcamera/dma_buf_allocator.cpp | 46 +---------- > src/libcamera/shared_mem_object.cpp | 21 ++--- > 6 files changed, 161 insertions(+), 58 deletions(-) > create mode 100644 include/libcamera/base/memfd.h > create mode 100644 src/libcamera/base/memfd.cpp > > diff --git a/include/libcamera/base/memfd.h b/include/libcamera/base/memfd.h > new file mode 100644 > index 000000000000..b0edd2de5d83 > --- /dev/null > +++ b/include/libcamera/base/memfd.h > @@ -0,0 +1,34 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2024, Ideas on Board Oy > + * > + * Anonymous file creation > + */ > + > +#pragma once > + > +#include <stddef.h> > + > +#include <libcamera/base/flags.h> > +#include <libcamera/base/unique_fd.h> > + > +namespace libcamera { > + > +class MemFd > +{ > +public: > + enum class Seal { > + None = 0, > + Shrink = (1 << 0), > + Grow = (1 << 1), > + }; > + > + using Seals = Flags<Seal>; > + > + static UniqueFD create(const char *name, std::size_t size, > + Seals seals = Seal::None); > +}; > + > +LIBCAMERA_FLAGS_ENABLE_OPERATORS(MemFd::Seal) > + > +} /* namespace libcamera */ > diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build > index bace25d56b13..2a0cee317204 100644 > --- a/include/libcamera/base/meson.build > +++ b/include/libcamera/base/meson.build > @@ -21,6 +21,7 @@ libcamera_base_private_headers = files([ > 'event_notifier.h', > 'file.h', > 'log.h', > + 'memfd.h', > 'message.h', > 'mutex.h', > 'private.h', > diff --git a/src/libcamera/base/memfd.cpp b/src/libcamera/base/memfd.cpp > new file mode 100644 > index 000000000000..72474307af09 > --- /dev/null > +++ b/src/libcamera/base/memfd.cpp > @@ -0,0 +1,116 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2024, Ideas on Board Oy > + * > + * Anonymous file creation > + */ > + > +#include <libcamera/base/memfd.h> > + > +#include <fcntl.h> > +#include <string.h> > +#include <sys/mman.h> > +#include <sys/syscall.h> > +#include <unistd.h> > + > +#include <libcamera/base/log.h> > + > +/** > + * \file base/memfd.h > + * \brief Anonymous file creation > + */ > + > +/* uClibc doesn't provide the file sealing API. */ > +#ifndef __DOXYGEN__ > +#if not HAVE_FILE_SEALS > +#define F_ADD_SEALS 1033 > +#define F_SEAL_SHRINK 0x0002 > +#define F_SEAL_GROW 0x0004 > +#endif > +#endif > + > +namespace libcamera { > + > +LOG_DECLARE_CATEGORY(File) > + > +/** > + * \class MemFd > + * \brief Helper class to create anonymous files > + * > + * Anonymous files behave like regular files, and can be modified, truncated, > + * memory-mapped and so on. Unlike regular files, they however live in RAM and > + * don't have permanent backing storage. > + */ > + > +/** > + * \enum MemFd::Seal > + * \brief Seals for the MemFd::create() function > + * \var MemFd::Seal::None > + * \brief No seals (used as default value) > + * \var MemFd::Seal::Shrink > + * \brief Prevent the memfd from shrinking > + * \var MemFd::Seal::Grow > + * \brief Prevent the memfd from growing > + */ > + > +/** > + * \typedef MemFd::Seals > + * \brief A bitwise combination of MemFd::Seal values > + */ > + > +/** > + * \brief Create an anonymous file > + * \param[in] name The file name (displayed in symbolic links in /proc/self/fd/) > + * \param[in] size The file size > + * \param[in] seals The file seals > + * > + * This function is a helper that wraps anonymous file (memfd) creation and > + * sets the file size and optional seals. > + * > + * \return The descriptor of the anonymous file if creation succeeded, or an > + * invalid UniqueFD otherwise > + */ > +UniqueFD MemFd::create(const char *name, std::size_t size, Seals seals) > +{ > +#if HAVE_MEMFD_CREATE > + int ret = memfd_create(name, MFD_ALLOW_SEALING | MFD_CLOEXEC); > +#else > + int ret = syscall(SYS_memfd_create, name, MFD_ALLOW_SEALING | MFD_CLOEXEC); > +#endif > + if (ret < 0) { > + ret = errno; > + LOG(File, Error) > + << "Failed to allocate memfd storage for " << name > + << ": " << strerror(ret); > + return {}; > + } > + > + UniqueFD memfd(ret); > + > + ret = ftruncate(memfd.get(), size); > + if (ret < 0) { > + ret = errno; > + LOG(File, Error) > + << "Failed to set memfd size for " << name > + << ": " << strerror(ret); > + return {}; > + } > + > + if (seals) { > + int fileSeals = (seals & Seal::Shrink ? F_SEAL_SHRINK : 0) > + | (seals & Seal::Grow ? F_SEAL_GROW : 0); > + > + ret = fcntl(memfd.get(), F_ADD_SEALS, fileSeals); > + if (ret < 0) { > + ret = errno; > + LOG(File, Error) > + << "Failed to seal the memfd for " << name > + << ": " << strerror(ret); > + return {}; > + } > + } > + > + return memfd; > +} > + > +} /* namespace libcamera */ > diff --git a/src/libcamera/base/meson.build b/src/libcamera/base/meson.build > index 7a7fd7e4ca87..4a228d335ba4 100644 > --- a/src/libcamera/base/meson.build > +++ b/src/libcamera/base/meson.build > @@ -10,6 +10,7 @@ libcamera_base_sources = files([ > 'file.cpp', > 'flags.cpp', > 'log.cpp', > + 'memfd.cpp', > 'message.cpp', > 'mutex.cpp', > 'object.cpp', > diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp > index c06eca7d04ec..be6efb89fbb7 100644 > --- a/src/libcamera/dma_buf_allocator.cpp > +++ b/src/libcamera/dma_buf_allocator.cpp > @@ -13,7 +13,6 @@ > #include <sys/ioctl.h> > #include <sys/mman.h> > #include <sys/stat.h> > -#include <sys/syscall.h> > #include <sys/types.h> > #include <unistd.h> > > @@ -22,6 +21,7 @@ > #include <linux/udmabuf.h> > > #include <libcamera/base/log.h> > +#include <libcamera/base/memfd.h> > > /** > * \file dma_buf_allocator.cpp > @@ -126,54 +126,16 @@ DmaBufAllocator::~DmaBufAllocator() = default; > * \brief Check if the DmaBufAllocator instance is valid > * \return True if the DmaBufAllocator is valid, false otherwise > */ > - > -/* uClibc doesn't provide the file sealing API. */ > -#ifndef __DOXYGEN__ > -#if not HAVE_FILE_SEALS > -#define F_ADD_SEALS 1033 > -#define F_SEAL_SHRINK 0x0002 > -#endif > -#endif > - > UniqueFD DmaBufAllocator::allocFromUDmaBuf(const char *name, std::size_t size) > { > /* Size must be a multiple of the page size. Round it up. */ > std::size_t pageMask = sysconf(_SC_PAGESIZE) - 1; > size = (size + pageMask) & ~pageMask; > > -#if HAVE_MEMFD_CREATE > - int ret = memfd_create(name, MFD_ALLOW_SEALING | MFD_CLOEXEC); > -#else > - int ret = syscall(SYS_memfd_create, name, MFD_ALLOW_SEALING | MFD_CLOEXEC); > -#endif > - if (ret < 0) { > - ret = errno; > - LOG(DmaBufAllocator, Error) > - << "Failed to allocate memfd storage for " << name > - << ": " << strerror(ret); > - return {}; > - } > - > - UniqueFD memfd(ret); > - > - ret = ftruncate(memfd.get(), size); > - if (ret < 0) { > - ret = errno; > - LOG(DmaBufAllocator, Error) > - << "Failed to set memfd size for " << name > - << ": " << strerror(ret); > - return {}; > - } > - > /* udmabuf dma-buffers *must* have the F_SEAL_SHRINK seal. */ > - ret = fcntl(memfd.get(), F_ADD_SEALS, F_SEAL_SHRINK); > - if (ret < 0) { > - ret = errno; > - LOG(DmaBufAllocator, Error) > - << "Failed to seal the memfd for " << name > - << ": " << strerror(ret); > + UniqueFD memfd = MemFd::create(name, size, MemFd::Seal::Shrink); > + if (!memfd.isValid()) > return {}; > - } > > struct udmabuf_create create; > > @@ -182,7 +144,7 @@ UniqueFD DmaBufAllocator::allocFromUDmaBuf(const char *name, std::size_t size) > create.offset = 0; > create.size = size; > > - ret = ::ioctl(providerHandle_.get(), UDMABUF_CREATE, &create); > + int ret = ::ioctl(providerHandle_.get(), UDMABUF_CREATE, &create); > if (ret < 0) { > ret = errno; > LOG(DmaBufAllocator, Error) > diff --git a/src/libcamera/shared_mem_object.cpp b/src/libcamera/shared_mem_object.cpp > index 809fbdaf95de..022645e71a35 100644 > --- a/src/libcamera/shared_mem_object.cpp > +++ b/src/libcamera/shared_mem_object.cpp > @@ -13,9 +13,9 @@ > #include <stddef.h> > #include <stdint.h> > #include <sys/mman.h> > -#include <sys/syscall.h> > #include <sys/types.h> > -#include <unistd.h> > + > +#include <libcamera/base/memfd.h> > > /** > * \file shared_mem_object.cpp > @@ -58,22 +58,11 @@ SharedMem::SharedMem() = default; > */ > SharedMem::SharedMem(const std::string &name, std::size_t size) > { > -#if HAVE_MEMFD_CREATE > - int fd = memfd_create(name.c_str(), MFD_CLOEXEC); > -#else > - int fd = syscall(SYS_memfd_create, name.c_str(), MFD_CLOEXEC); > -#endif > - if (fd < 0) > + UniqueFD memfd = MemFd::create(name.c_str(), size); > + if (!memfd.isValid()) > return; > > - fd_ = SharedFD(std::move(fd)); > - if (!fd_.isValid()) > - return; > - > - if (ftruncate(fd_.get(), size) < 0) { > - fd_ = SharedFD(); > - return; > - } > + fd_ = SharedFD(std::move(memfd)); > > void *mem = mmap(nullptr, size, PROT_READ | PROT_WRITE, MAP_SHARED, > fd_.get(), 0);
diff --git a/include/libcamera/base/memfd.h b/include/libcamera/base/memfd.h new file mode 100644 index 000000000000..b0edd2de5d83 --- /dev/null +++ b/include/libcamera/base/memfd.h @@ -0,0 +1,34 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2024, Ideas on Board Oy + * + * Anonymous file creation + */ + +#pragma once + +#include <stddef.h> + +#include <libcamera/base/flags.h> +#include <libcamera/base/unique_fd.h> + +namespace libcamera { + +class MemFd +{ +public: + enum class Seal { + None = 0, + Shrink = (1 << 0), + Grow = (1 << 1), + }; + + using Seals = Flags<Seal>; + + static UniqueFD create(const char *name, std::size_t size, + Seals seals = Seal::None); +}; + +LIBCAMERA_FLAGS_ENABLE_OPERATORS(MemFd::Seal) + +} /* namespace libcamera */ diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build index bace25d56b13..2a0cee317204 100644 --- a/include/libcamera/base/meson.build +++ b/include/libcamera/base/meson.build @@ -21,6 +21,7 @@ libcamera_base_private_headers = files([ 'event_notifier.h', 'file.h', 'log.h', + 'memfd.h', 'message.h', 'mutex.h', 'private.h', diff --git a/src/libcamera/base/memfd.cpp b/src/libcamera/base/memfd.cpp new file mode 100644 index 000000000000..72474307af09 --- /dev/null +++ b/src/libcamera/base/memfd.cpp @@ -0,0 +1,116 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2024, Ideas on Board Oy + * + * Anonymous file creation + */ + +#include <libcamera/base/memfd.h> + +#include <fcntl.h> +#include <string.h> +#include <sys/mman.h> +#include <sys/syscall.h> +#include <unistd.h> + +#include <libcamera/base/log.h> + +/** + * \file base/memfd.h + * \brief Anonymous file creation + */ + +/* uClibc doesn't provide the file sealing API. */ +#ifndef __DOXYGEN__ +#if not HAVE_FILE_SEALS +#define F_ADD_SEALS 1033 +#define F_SEAL_SHRINK 0x0002 +#define F_SEAL_GROW 0x0004 +#endif +#endif + +namespace libcamera { + +LOG_DECLARE_CATEGORY(File) + +/** + * \class MemFd + * \brief Helper class to create anonymous files + * + * Anonymous files behave like regular files, and can be modified, truncated, + * memory-mapped and so on. Unlike regular files, they however live in RAM and + * don't have permanent backing storage. + */ + +/** + * \enum MemFd::Seal + * \brief Seals for the MemFd::create() function + * \var MemFd::Seal::None + * \brief No seals (used as default value) + * \var MemFd::Seal::Shrink + * \brief Prevent the memfd from shrinking + * \var MemFd::Seal::Grow + * \brief Prevent the memfd from growing + */ + +/** + * \typedef MemFd::Seals + * \brief A bitwise combination of MemFd::Seal values + */ + +/** + * \brief Create an anonymous file + * \param[in] name The file name (displayed in symbolic links in /proc/self/fd/) + * \param[in] size The file size + * \param[in] seals The file seals + * + * This function is a helper that wraps anonymous file (memfd) creation and + * sets the file size and optional seals. + * + * \return The descriptor of the anonymous file if creation succeeded, or an + * invalid UniqueFD otherwise + */ +UniqueFD MemFd::create(const char *name, std::size_t size, Seals seals) +{ +#if HAVE_MEMFD_CREATE + int ret = memfd_create(name, MFD_ALLOW_SEALING | MFD_CLOEXEC); +#else + int ret = syscall(SYS_memfd_create, name, MFD_ALLOW_SEALING | MFD_CLOEXEC); +#endif + if (ret < 0) { + ret = errno; + LOG(File, Error) + << "Failed to allocate memfd storage for " << name + << ": " << strerror(ret); + return {}; + } + + UniqueFD memfd(ret); + + ret = ftruncate(memfd.get(), size); + if (ret < 0) { + ret = errno; + LOG(File, Error) + << "Failed to set memfd size for " << name + << ": " << strerror(ret); + return {}; + } + + if (seals) { + int fileSeals = (seals & Seal::Shrink ? F_SEAL_SHRINK : 0) + | (seals & Seal::Grow ? F_SEAL_GROW : 0); + + ret = fcntl(memfd.get(), F_ADD_SEALS, fileSeals); + if (ret < 0) { + ret = errno; + LOG(File, Error) + << "Failed to seal the memfd for " << name + << ": " << strerror(ret); + return {}; + } + } + + return memfd; +} + +} /* namespace libcamera */ diff --git a/src/libcamera/base/meson.build b/src/libcamera/base/meson.build index 7a7fd7e4ca87..4a228d335ba4 100644 --- a/src/libcamera/base/meson.build +++ b/src/libcamera/base/meson.build @@ -10,6 +10,7 @@ libcamera_base_sources = files([ 'file.cpp', 'flags.cpp', 'log.cpp', + 'memfd.cpp', 'message.cpp', 'mutex.cpp', 'object.cpp', diff --git a/src/libcamera/dma_buf_allocator.cpp b/src/libcamera/dma_buf_allocator.cpp index c06eca7d04ec..be6efb89fbb7 100644 --- a/src/libcamera/dma_buf_allocator.cpp +++ b/src/libcamera/dma_buf_allocator.cpp @@ -13,7 +13,6 @@ #include <sys/ioctl.h> #include <sys/mman.h> #include <sys/stat.h> -#include <sys/syscall.h> #include <sys/types.h> #include <unistd.h> @@ -22,6 +21,7 @@ #include <linux/udmabuf.h> #include <libcamera/base/log.h> +#include <libcamera/base/memfd.h> /** * \file dma_buf_allocator.cpp @@ -126,54 +126,16 @@ DmaBufAllocator::~DmaBufAllocator() = default; * \brief Check if the DmaBufAllocator instance is valid * \return True if the DmaBufAllocator is valid, false otherwise */ - -/* uClibc doesn't provide the file sealing API. */ -#ifndef __DOXYGEN__ -#if not HAVE_FILE_SEALS -#define F_ADD_SEALS 1033 -#define F_SEAL_SHRINK 0x0002 -#endif -#endif - UniqueFD DmaBufAllocator::allocFromUDmaBuf(const char *name, std::size_t size) { /* Size must be a multiple of the page size. Round it up. */ std::size_t pageMask = sysconf(_SC_PAGESIZE) - 1; size = (size + pageMask) & ~pageMask; -#if HAVE_MEMFD_CREATE - int ret = memfd_create(name, MFD_ALLOW_SEALING | MFD_CLOEXEC); -#else - int ret = syscall(SYS_memfd_create, name, MFD_ALLOW_SEALING | MFD_CLOEXEC); -#endif - if (ret < 0) { - ret = errno; - LOG(DmaBufAllocator, Error) - << "Failed to allocate memfd storage for " << name - << ": " << strerror(ret); - return {}; - } - - UniqueFD memfd(ret); - - ret = ftruncate(memfd.get(), size); - if (ret < 0) { - ret = errno; - LOG(DmaBufAllocator, Error) - << "Failed to set memfd size for " << name - << ": " << strerror(ret); - return {}; - } - /* udmabuf dma-buffers *must* have the F_SEAL_SHRINK seal. */ - ret = fcntl(memfd.get(), F_ADD_SEALS, F_SEAL_SHRINK); - if (ret < 0) { - ret = errno; - LOG(DmaBufAllocator, Error) - << "Failed to seal the memfd for " << name - << ": " << strerror(ret); + UniqueFD memfd = MemFd::create(name, size, MemFd::Seal::Shrink); + if (!memfd.isValid()) return {}; - } struct udmabuf_create create; @@ -182,7 +144,7 @@ UniqueFD DmaBufAllocator::allocFromUDmaBuf(const char *name, std::size_t size) create.offset = 0; create.size = size; - ret = ::ioctl(providerHandle_.get(), UDMABUF_CREATE, &create); + int ret = ::ioctl(providerHandle_.get(), UDMABUF_CREATE, &create); if (ret < 0) { ret = errno; LOG(DmaBufAllocator, Error) diff --git a/src/libcamera/shared_mem_object.cpp b/src/libcamera/shared_mem_object.cpp index 809fbdaf95de..022645e71a35 100644 --- a/src/libcamera/shared_mem_object.cpp +++ b/src/libcamera/shared_mem_object.cpp @@ -13,9 +13,9 @@ #include <stddef.h> #include <stdint.h> #include <sys/mman.h> -#include <sys/syscall.h> #include <sys/types.h> -#include <unistd.h> + +#include <libcamera/base/memfd.h> /** * \file shared_mem_object.cpp @@ -58,22 +58,11 @@ SharedMem::SharedMem() = default; */ SharedMem::SharedMem(const std::string &name, std::size_t size) { -#if HAVE_MEMFD_CREATE - int fd = memfd_create(name.c_str(), MFD_CLOEXEC); -#else - int fd = syscall(SYS_memfd_create, name.c_str(), MFD_CLOEXEC); -#endif - if (fd < 0) + UniqueFD memfd = MemFd::create(name.c_str(), size); + if (!memfd.isValid()) return; - fd_ = SharedFD(std::move(fd)); - if (!fd_.isValid()) - return; - - if (ftruncate(fd_.get(), size) < 0) { - fd_ = SharedFD(); - return; - } + fd_ = SharedFD(std::move(memfd)); void *mem = mmap(nullptr, size, PROT_READ | PROT_WRITE, MAP_SHARED, fd_.get(), 0);