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

[Ahmed S. Darwish]

Hi!

On Fri, Feb 12, 2016 at 04:49:23PM +0100, David Henningsson wrote:
> 
> On 2016-02-12 01:14, Ahmed S. Darwish wrote:
...
> >@@ -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?
>

This will not happen since callers of pa_shm_create() does not
call pa_shm_free() if _create() failed.

But I definitely see your point from a resillience perspective
and PA coding style. I'll thus make sure that in case of failure,
the returned object is never touched and remains as originally
received.

...
> >@@ -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?
>

Hmm, that comment should've been further clarified..

By "not close file descriptors which are not our own", I meant
not to close the fd behind our caller's back. It was just passed
to us as a parameter and thus we do not have any kind of
ownership to close it.

This is unlike attaching to a Posix SHM  where the fd was created
by us in the same code path [ using shm_open() ] and thus we have
its complete ownership -- and must close it afterwards.

So for memfd attachment, the caller owns the passed fd and is
responsible for closing it when it sees fit. That's also why we
do not cache the passed fd in the callee and set pa_shm.memfd.fd
to -1.

Meanwhile, the caller does indeed do the appropriate cleanup.

Kindly check packet_complete() at patch #9, and more specifically:

  /* Finished reading a packet */
  static void packet_complete(pa_pstream *p, pa_packet *packet,
  pa_cmsg_ancil_data *ancil, bool shmid_to_memfd_packet) {
    ...
    memfd_fd = ancil->fds[0];
    shm_id = *(unsigned *)pa_packet_data(packet, &packet_len);
    ...
    if (pa_memimport_add_permanent_shmid_to_memfd_mapping(
        p->import, shm_id, memfd_fd, true)) {
      ...
      pa_assert_se(pa_close(memfd_fd) == 0);
      return;
    }
    ...
    pa_assert_se(pa_close(memfd_fd) == 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.
>

Yup..

Point of making it big and bold is to communicate to the reviewer
that this is temporary and will be removed in the next commit.
(and this is done to improve commit locality, bisection, etc.)

Thanks a lot,

-- 
Darwish
http://darwish.chasingpointers.com