[PATCH v2 05/12] pulsecore: SHM: Introduce memfd support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 2016-02-12 01:14, Ahmed S. Darwish wrote:
> Memfd is a simple memory sharing mechanism, added by the systemd/kdbus
> developers, to share pages between processes in an anonymous, no global
> registry needed, no mount-point required, relatively secure, manner.
>
> This patch introduces the necessary building blocks for using memfd
> shared memory transfers in PulseAudio.
>
> Memfd support shall also help us in laying out the necessary (but not
> yet sufficient) groundwork for application sandboxing, protecting PA
> from ts clients, and protecting clients data from each other.
>
> We plan to exclusively use memfds, instead of POSIX SHM, on the way
> forward.
>
> Signed-off-by: Ahmed S. Darwish <darwish.07 at gmail.com>
> ---
>   configure.ac                   |  19 ++++++
>   src/Makefile.am                |   5 ++
>   src/pulsecore/memfd-wrappers.h |  68 +++++++++++++++++++
>   src/pulsecore/shm.c            | 146 ++++++++++++++++++++++++++++-------------
>   src/pulsecore/shm.h            |  16 ++++-
>   5 files changed, 209 insertions(+), 45 deletions(-)
>   create mode 100644 src/pulsecore/memfd-wrappers.h
>
> diff --git a/configure.ac b/configure.ac
> index dcd0110..256a196 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -607,6 +607,23 @@ AC_DEFINE(HAVE_DLADDR, [1], [Have dladdr?])
>
>   AM_ICONV
>
> +#### Linux memfd_create(2) SHM support ####
> +
> +AC_ARG_ENABLE([memfd],
> +    AS_HELP_STRING([--disable-memfd], [Disable Linux memfd shared memory]))
> +
> +AS_IF([test "x$enable_memfd" != "xno"],
> +    AC_CHECK_DECL(SYS_memfd_create, [HAVE_MEMFD=1], [HAVE_MEMFD=0], [#include <sys/syscall.h>]),
> +    [HAVE_MEMFD=0])
> +
> +AS_IF([test "x$enable_memfd" = "xyes" && test "x$HAVE_MEMFD" = "x0"],
> +    [AC_MSG_ERROR([*** Your Linux kernel does not support memfd shared memory.
> +                  *** Use linux v3.17 or higher for such a feature.])])
> +
> +AC_SUBST(HAVE_MEMFD)
> +AM_CONDITIONAL([HAVE_MEMFD], [test "x$HAVE_MEMFD" = x1])
> +AS_IF([test "x$HAVE_MEMFD" = "x1"], AC_DEFINE([HAVE_MEMFD], 1, [Have memfd shared memory.]))
> +
>   #### X11 (optional) ####
>
>   AC_ARG_ENABLE([x11],
> @@ -1539,6 +1556,7 @@ AC_OUTPUT
>
>   # ==========================================================================
>
> +AS_IF([test "x$HAVE_MEMFD" = "x1"], ENABLE_MEMFD=yes, ENABLE_MEMFD=no)
>   AS_IF([test "x$HAVE_X11" = "x1"], ENABLE_X11=yes, ENABLE_X11=no)
>   AS_IF([test "x$HAVE_OSS_OUTPUT" = "x1"], ENABLE_OSS_OUTPUT=yes, ENABLE_OSS_OUTPUT=no)
>   AS_IF([test "x$HAVE_OSS_WRAPPER" = "x1"], ENABLE_OSS_WRAPPER=yes, ENABLE_OSS_WRAPPER=no)
> @@ -1600,6 +1618,7 @@ echo "
>       CPPFLAGS:                      ${CPPFLAGS}
>       LIBS:                          ${LIBS}
>
> +    Enable memfd shared memory:    ${ENABLE_MEMFD}
>       Enable X11:                    ${ENABLE_X11}
>       Enable OSS Output:             ${ENABLE_OSS_OUTPUT}
>       Enable OSS Wrapper:            ${ENABLE_OSS_WRAPPER}
> diff --git a/src/Makefile.am b/src/Makefile.am
> index f3a62fb..be8d3d7 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -729,6 +729,11 @@ libpulsecommon_ at PA_MAJORMINOR@_la_CFLAGS = $(AM_CFLAGS) $(LIBJSON_CFLAGS) $(LIBS
>   libpulsecommon_ at PA_MAJORMINOR@_la_LDFLAGS = $(AM_LDFLAGS) $(AM_LIBLDFLAGS) -avoid-version
>   libpulsecommon_ at PA_MAJORMINOR@_la_LIBADD = $(AM_LIBADD) $(LIBJSON_LIBS)  $(LIBWRAP_LIBS) $(WINSOCK_LIBS) $(LTLIBICONV) $(LIBSNDFILE_LIBS)
>
> +if HAVE_MEMFD
> +libpulsecommon_ at PA_MAJORMINOR@_la_SOURCES += \
> +		pulsecore/memfd-wrappers.h
> +endif
> +
>   if HAVE_X11
>   libpulsecommon_ at PA_MAJORMINOR@_la_SOURCES += \
>   		pulse/client-conf-x11.c pulse/client-conf-x11.h \
> diff --git a/src/pulsecore/memfd-wrappers.h b/src/pulsecore/memfd-wrappers.h
> new file mode 100644
> index 0000000..3bed9b2
> --- /dev/null
> +++ b/src/pulsecore/memfd-wrappers.h
> @@ -0,0 +1,68 @@
> +#ifndef foopulsememfdwrappershfoo
> +#define foopulsememfdwrappershfoo
> +
> +/***
> +  This file is part of PulseAudio.
> +
> +  Copyright 2016 Ahmed S. Darwish <darwish.07 at gmail.com>
> +
> +  PulseAudio is free software; you can redistribute it and/or modify
> +  it under the terms of the GNU Lesser General Public License as
> +  published by the Free Software Foundation; either version 2.1 of the
> +  License, or (at your option) any later version.
> +
> +  PulseAudio is distributed in the hope that it will be useful, but
> +  WITHOUT ANY WARRANTY; without even the implied warranty of
> +  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +  Lesser General Public License for more details.
> +
> +  You should have received a copy of the GNU Lesser General Public
> +  License along with PulseAudio; if not, see <http://www.gnu.org/licenses/>.
> +***/
> +
> +#ifdef HAVE_MEMFD
> +
> +#include <sys/syscall.h>
> +#include <fcntl.h>
> +
> +/*
> + * No glibc wrappers exist for memfd_create(2), so provide our own.
> + *
> + * Also define memfd fcntl sealing macros. While they are already
> + * defined in the kernel header file <linux/fcntl.h>, that file as
> + * a whole conflicts with the original glibc header <fnctl.h>.
> + */
> +
> +static inline int memfd_create(const char *name, unsigned int flags) {
> +    return syscall(SYS_memfd_create, name, flags);
> +}
> +
> +/* memfd_create(2) flags */
> +
> +#ifndef MFD_CLOEXEC
> +#define MFD_CLOEXEC       0x0001U
> +#endif
> +
> +#ifndef MFD_ALLOW_SEALING
> +#define MFD_ALLOW_SEALING 0x0002U
> +#endif
> +
> +/* fcntl() seals-related flags */
> +
> +#ifndef F_LINUX_SPECIFIC_BASE
> +#define F_LINUX_SPECIFIC_BASE 1024
> +#endif
> +
> +#ifndef F_ADD_SEALS
> +#define F_ADD_SEALS (F_LINUX_SPECIFIC_BASE + 9)
> +#define F_GET_SEALS (F_LINUX_SPECIFIC_BASE + 10)
> +
> +#define F_SEAL_SEAL     0x0001  /* prevent further seals from being set */
> +#define F_SEAL_SHRINK   0x0002  /* prevent file from shrinking */
> +#define F_SEAL_GROW     0x0004  /* prevent file from growing */
> +#define F_SEAL_WRITE    0x0008  /* prevent writes */
> +#endif
> +
> +#endif /* HAVE_MEMFD */
> +
> +#endif
> diff --git a/src/pulsecore/shm.c b/src/pulsecore/shm.c
> index 4c0ecf1..0ca4fb0 100644
> --- a/src/pulsecore/shm.c
> +++ b/src/pulsecore/shm.c
> @@ -45,6 +45,7 @@
>   #include <pulse/xmalloc.h>
>   #include <pulse/gccmacro.h>
>
> +#include <pulsecore/memfd-wrappers.h>
>   #include <pulsecore/core-error.h>
>   #include <pulsecore/log.h>
>   #include <pulsecore/random.h>
> @@ -88,7 +89,12 @@ struct shm_marker {
>       uint64_t _reserved4;
>   } PA_GCC_PACKED;
>
> -#define SHM_MARKER_SIZE PA_ALIGN(sizeof(struct shm_marker))
> +static inline size_t shm_marker_size(pa_shm *m) {
> +    if (m->type == PA_MEM_TYPE_SHARED_POSIX)
> +        return PA_ALIGN(sizeof(struct shm_marker));
> +
> +    return 0;
> +}
>
>   #ifdef HAVE_SHM_OPEN
>   static char *segment_name(char *fn, size_t l, unsigned id) {
> @@ -100,8 +106,8 @@ static char *segment_name(char *fn, size_t l, unsigned id) {
>   int pa_shm_create(pa_shm *m, pa_mem_type_t type, size_t size, mode_t mode) {
>   #ifdef HAVE_SHM_OPEN
>       char fn[32];
> -    int fd = -1;
>   #endif
> +    int fd = -1;
>       struct shm_marker *marker;
>
>       pa_assert(m);
> @@ -118,16 +124,33 @@ int pa_shm_create(pa_shm *m, pa_mem_type_t type, size_t size, mode_t mode) {
>       /* Round up to make it page aligned */
>       size = PA_PAGE_ALIGN(size);
>
> -#ifdef HAVE_SHM_OPEN
>       pa_random(&m->id, sizeof(m->id));
> -    segment_name(fn, sizeof(fn), m->id);
>
> -    if ((fd = shm_open(fn, O_RDWR|O_CREAT|O_EXCL, mode)) < 0) {
> -        pa_log("shm_open() failed: %s", pa_cstrerror(errno));
> +    switch (type) {
> +#ifdef HAVE_SHM_OPEN
> +    case PA_MEM_TYPE_SHARED_POSIX:
> +        segment_name(fn, sizeof(fn), m->id);
> +        fd = shm_open(fn, O_RDWR|O_CREAT|O_EXCL, mode);
> +        m->per_type.posix_shm.do_unlink = true;
> +        break;
> +#endif
> +#ifdef HAVE_MEMFD
> +    case PA_MEM_TYPE_SHARED_MEMFD:
> +        fd = memfd_create("memfd", MFD_ALLOW_SEALING);
> +        m->per_type.memfd.fd = fd;

These failure paths look a bit hairy. If mmap() fails e g, will this not 
lead to double close of the same fd, first in this function and then 
later in pa_shm_free?

I e, we should not set "m->per_type.memfd.fd = fd" until after all "goto 
fail"s?

> +        break;
> +#endif
> +    default:
>           goto fail;
>       }
>
> -    m->mem.size = size + SHM_MARKER_SIZE;
> +    if (fd < 0) {
> +        pa_log("%s open() failed: %s", pa_mem_type_to_string(type), pa_cstrerror(errno));
> +        goto fail;
> +    }
> +
> +    m->type = type;
> +    m->mem.size = size + shm_marker_size(m);
>
>       if (ftruncate(fd, (off_t) m->mem.size) < 0) {
>           pa_log("ftruncate() failed: %s", pa_cstrerror(errno));
> @@ -143,25 +166,29 @@ int pa_shm_create(pa_shm *m, pa_mem_type_t type, size_t size, mode_t mode) {
>           goto fail;
>       }
>
> -    /* We store our PID at the end of the shm block, so that we
> -     * can check for dead shm segments later */
> -    marker = (struct shm_marker*) ((uint8_t*) m->mem.ptr + m->mem.size - SHM_MARKER_SIZE);
> -    pa_atomic_store(&marker->pid, (int) getpid());
> -    pa_atomic_store(&marker->marker, SHM_MARKER);
> +    if (type == PA_MEM_TYPE_SHARED_POSIX) {
> +        /* We store our PID at the end of the shm block, so that we
> +         * can check for dead shm segments later */
> +        marker = (struct shm_marker*) ((uint8_t*) m->mem.ptr + m->mem.size - shm_marker_size(m));
> +        pa_atomic_store(&marker->pid, (int) getpid());
> +        pa_atomic_store(&marker->marker, SHM_MARKER);
> +    }
>
> -    pa_assert_se(pa_close(fd) == 0);
> -    m->do_unlink = true;
> -#else
> -    goto fail;
> -#endif
> +    /* For memfds, we keep the fd open until we pass it to the other
> +     * PA endpoint over the unix domain socket */
> +    if (type != PA_MEM_TYPE_SHARED_MEMFD)
> +        pa_assert_se(pa_close(fd) == 0);
>
>       return 0;
>
>   fail:
>
> -#ifdef HAVE_SHM_OPEN
> +#if defined(HAVE_SHM_OPEN) || defined(HAVE_MEMFD)
>       if (fd >= 0) {
> -        shm_unlink(fn);
> +#ifdef HAVE_SHM_OPEN
> +        if (type == PA_MEM_TYPE_SHARED_POSIX)
> +            shm_unlink(fn);
> +#endif
>           pa_close(fd);
>       }
>   #endif
> @@ -178,19 +205,28 @@ void pa_shm_free(pa_shm *m) {
>       pa_assert(m->mem.ptr != MAP_FAILED);
>   #endif
>
> -#ifdef HAVE_SHM_OPEN
> +#if defined(HAVE_SHM_OPEN) || defined(HAVE_MEMFD)
>       if (munmap(m->mem.ptr, PA_PAGE_ALIGN(m->mem.size)) < 0)
>           pa_log("munmap() failed: %s", pa_cstrerror(errno));
>
> -    if (m->do_unlink) {
> +#ifdef HAVE_SHM_OPEN
> +    if (m->type == PA_MEM_TYPE_SHARED_POSIX && m->per_type.posix_shm.do_unlink) {
>           char fn[32];
>
>           segment_name(fn, sizeof(fn), m->id);
>           if (shm_unlink(fn) < 0)
>               pa_log(" shm_unlink(%s) failed: %s", fn, pa_cstrerror(errno));
>       }
> +#endif
> +#ifdef HAVE_MEMFD
> +    /* Memfds are the only SHM type where the file descriptor is not
> +     * directly closed after memory mapping. */
> +    if (m->type == PA_MEM_TYPE_SHARED_MEMFD && m->per_type.memfd.fd != -1)
> +        pa_assert_se(pa_close(m->per_type.memfd.fd) == 0);
> +#endif
> +
>   #else
> -    /* We shouldn't be here without shm support */
> +    /* We shouldn't be here without shm or memfd support */
>       pa_assert_not_reached();
>   #endif
>
> @@ -243,9 +279,9 @@ void pa_shm_punch(pa_shm *m, size_t offset, size_t size) {
>   #endif
>   }
>
> -#ifdef HAVE_SHM_OPEN
> +#if defined(HAVE_SHM_OPEN) || defined(HAVE_MEMFD)
>
> -static int shm_attach(pa_shm *m, unsigned id, bool writable, bool for_cleanup) {
> +static int shm_attach(pa_shm *m, pa_mem_type_t type, unsigned id, int memfd_fd, bool writable, bool for_cleanup) {
>       char fn[32];
>       int fd = -1;
>       int prot;
> @@ -253,11 +289,27 @@ static int shm_attach(pa_shm *m, unsigned id, bool writable, bool for_cleanup) {
>
>       pa_assert(m);
>
> -    segment_name(fn, sizeof(fn), m->id = id);
> -
> -    if ((fd = shm_open(fn, writable ? O_RDWR : O_RDONLY, 0)) < 0) {
> -        if ((errno != EACCES && errno != ENOENT) || !for_cleanup)
> -            pa_log("shm_open() failed: %s", pa_cstrerror(errno));
> +    switch (type) {
> +#ifdef HAVE_SHM_OPEN
> +    case PA_MEM_TYPE_SHARED_POSIX:
> +        pa_assert(memfd_fd == -1);
> +        segment_name(fn, sizeof(fn), id);
> +        if ((fd = shm_open(fn, writable ? O_RDWR : O_RDONLY, 0)) < 0) {
> +            if ((errno != EACCES && errno != ENOENT) || !for_cleanup)
> +                pa_log("shm_open() failed: %s", pa_cstrerror(errno));
> +            goto fail;
> +        }
> +        m->per_type.posix_shm.do_unlink = false;
> +        break;
> +#endif
> +#ifdef HAVE_MEMFD
> +    case PA_MEM_TYPE_SHARED_MEMFD:
> +        pa_assert(memfd_fd != -1);
> +        fd = memfd_fd;
> +        m->per_type.memfd.fd = -1;      /* -1; check comments above pa_shm_attch() for rationale */
> +        break;
> +#endif
> +    default:
>           goto fail;
>       }
>
> @@ -267,13 +319,15 @@ static int shm_attach(pa_shm *m, unsigned id, bool writable, bool for_cleanup) {
>       }
>
>       if (st.st_size <= 0 ||
> -        st.st_size > (off_t) (PA_MAX_SHM_SIZE+SHM_MARKER_SIZE) ||
> +        st.st_size > (off_t) PA_MAX_SHM_SIZE + (off_t) shm_marker_size(m) ||
>           PA_ALIGN((size_t) st.st_size) != (size_t) st.st_size) {
>           pa_log("Invalid shared memory segment size");
>           goto fail;
>       }
>
> +    m->type = type;
>       m->mem.size = (size_t) st.st_size;
> +    m->id = id;
>
>       prot = writable ? PROT_READ | PROT_WRITE : PROT_READ;
>       if ((m->mem.ptr = mmap(NULL, PA_PAGE_ALIGN(m->mem.size), prot, MAP_SHARED, fd, (off_t) 0)) == MAP_FAILED) {
> @@ -281,9 +335,9 @@ static int shm_attach(pa_shm *m, unsigned id, bool writable, bool for_cleanup) {
>           goto fail;
>       }
>
> -    m->do_unlink = false;
> -
> -    pa_assert_se(pa_close(fd) == 0);
> +    /* Do not close file descriptors which are not our own */
> +    if (type != PA_MEM_TYPE_SHARED_MEMFD)
> +        pa_assert_se(pa_close(fd) == 0);

I believe the fd should be closed regardless of type?

Now you seem to leak an fd if type == PA_MEM_TYPE_SHARED_MEMFD?

>
>       return 0;
>
> @@ -293,18 +347,22 @@ fail:
>
>       return -1;
>   }
> +#endif
>
> -int pa_shm_attach(pa_shm *m, unsigned id, bool writable) {
> -    return shm_attach(m, id, writable, false);
> -}
> -
> -#else /* HAVE_SHM_OPEN */
> -
> -int pa_shm_attach(pa_shm *m, unsigned id, bool writable) {
> +/* Note: In case of attaching memfd SHM areas, the caller maintains ownership
> + * of the passed fd and has the responsibility of closing it when appropriate. */
> +static int NEW_API_pa_shm_attach(pa_shm *m, pa_mem_type_t type, unsigned id, int memfd_fd, bool writable) {

It's more common to just do _pa_shm_attach instead of 
NEW_API_pa_shm_attach, but since you remove it again in the next patch, 
it doesn't matter.

> +#if defined(HAVE_SHM_OPEN) || defined(HAVE_MEMFD)
> +    return shm_attach(m, type, id, memfd_fd, writable, false);
> +#else
>       return -1;
> +#endif
>   }
>
> -#endif /* HAVE_SHM_OPEN */
> +/* Compatibility version until the new API is used in external sources */
> +int pa_shm_attach(pa_shm *m, unsigned id, bool writable) {
> +    return NEW_API_pa_shm_attach(m, PA_MEM_TYPE_SHARED_POSIX, id, -1, writable);
> +}
>
>   int pa_shm_cleanup(void) {
>
> @@ -335,15 +393,15 @@ int pa_shm_cleanup(void) {
>           if (pa_atou(de->d_name + SHM_ID_LEN, &id) < 0)
>               continue;
>
> -        if (shm_attach(&seg, id, false, true) < 0)
> +        if (shm_attach(&seg, PA_MEM_TYPE_SHARED_POSIX, id, -1, false, true) < 0)
>               continue;
>
> -        if (seg.mem.size < SHM_MARKER_SIZE) {
> +        if (seg.mem.size < shm_marker_size(&seg)) {
>               pa_shm_free(&seg);
>               continue;
>           }
>
> -        m = (struct shm_marker*) ((uint8_t*) seg.mem.ptr + seg.mem.size - SHM_MARKER_SIZE);
> +        m = (struct shm_marker*) ((uint8_t*) seg.mem.ptr + seg.mem.size - shm_marker_size(&seg));
>
>           if (pa_atomic_load(&m->marker) != SHM_MARKER) {
>               pa_shm_free(&seg);
> diff --git a/src/pulsecore/shm.h b/src/pulsecore/shm.h
> index 887f516..1c8ce83 100644
> --- a/src/pulsecore/shm.h
> +++ b/src/pulsecore/shm.h
> @@ -32,7 +32,21 @@ typedef struct pa_shm {
>       pa_mem mem;             /* Parent; must be first */
>       pa_mem_type_t type;
>       unsigned id;
> -    bool do_unlink:1;
> +
> +    union {
> +        struct {
> +            bool do_unlink;
> +        } posix_shm;
> +        struct {
> +            /* To avoid fd leaks, we keep this fd open only until we pass it
> +             * to the other PA endpoint over the unix domain socket.
> +             *
> +             * When we don't have ownership for the memfd fd in question (e.g.
> +             * memfd segment attachment), or the file descriptor has now been
> +             * closed, this is set to -1. */
> +            int fd;
> +        } memfd;
> +    } per_type;
>   } pa_shm;
>
>   int pa_shm_create(pa_shm *m, pa_mem_type_t type, size_t size, mode_t mode);
>
>
> Regards,
>

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic


[Index of Archives]     [Linux Audio Users]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux