[pulseaudio-discuss] [PATCH 09/11] pulsecore: memexport/memimport: Introduce memfd blocks support

[Tanu Kaskinen]

On Sun, 2015-09-20 at 23:36 +0200, Ahmed S. Darwish wrote:
> @@ -537,6 +561,46 @@ pa_mempool* pa_memblock_get_pool(pa_memblock *b) {
>      return b->pool;
>  }
>  
> +static pa_mem_type_t memblock_get_mem_type(pa_memblock *b) {
> +    switch (b->type) {
> +    case PA_MEMBLOCK_IMPORTED:
> +        pa_assert(b->per_type.imported.segment);
> +        return b->per_type.imported.segment->mem_type;
> +
> +    case PA_MEMBLOCK_POOL:
> +    case PA_MEMBLOCK_POOL_EXTERNAL:
> +        return b->pool->mem_type;
> +
> +    case PA_MEMBLOCK_APPENDED:
> +    case PA_MEMBLOCK_FIXED:
> +    case PA_MEMBLOCK_USER:
> +        return PA_MEMORY_PRIVATE;
> +
> +    default:
> +        pa_assert_not_reached();
> +    }
> +};

You can move the assertion outside the switch, and drop the default
case. That will give the compiler a chance to warn about unhandled
cases, if the memblock type enum gets extended.

> +
> +int pa_memblock_get_memfd_fd(pa_memblock *b) {
> +    pa_assert(memblock_get_mem_type(b) == PA_MEMORY_SHARED_MEMFD);
> +
> +    switch (b->type) {
> +    case PA_MEMBLOCK_IMPORTED:
> +        pa_assert(b->per_type.imported.segment);
> +        return b->per_type.imported.segment->per_type.memfd.fd;
> +
> +    case PA_MEMBLOCK_POOL:
> +    case PA_MEMBLOCK_POOL_EXTERNAL:
> +        return b->pool->per_type.memfd.fd;
> +
> +    case PA_MEMBLOCK_APPENDED:
> +    case PA_MEMBLOCK_FIXED:
> +    case PA_MEMBLOCK_USER:
> +    default:
> +        pa_assert_not_reached();
> +    }
> +}

Same as above.

> @@ -978,25 +1043,59 @@ pa_memimport* pa_memimport_new(pa_mempool *p, pa_memimport_release_cb_t cb, void
>  
>  static void memexport_revoke_blocks(pa_memexport *e, pa_memimport *i);
>  
> +static void segment_late_init(pa_memimport_segment *seg, pa_memimport *i, pa_mem_type_t mem_type, bool writable) {
> +    seg->writable = writable;
> +    seg->import = i;
> +    seg->mem_type = mem_type;
> +
> +    pa_assert(seg->mem.ptr != NULL);
> +    seg->trap = pa_memtrap_add(seg->mem.ptr, seg->mem.size);
> +}
> +
>  /* Should be called locked */
> -static pa_memimport_segment* segment_attach(pa_memimport *i, uint32_t shm_id, bool writable) {
> +static pa_memimport_segment* segment_shm_attach(pa_memimport *i, uint32_t shm_id, bool writable) {
>      pa_memimport_segment* seg;
>  
> -    if (pa_hashmap_size(i->segments) >= PA_MEMIMPORT_SEGMENTS_MAX)
> +    if ((seg = pa_hashmap_get(i->shm_segments, PA_UINT32_TO_PTR(shm_id))))
> +        return seg;
> +
> +    if (pa_hashmap_size(i->shm_segments) >= PA_MEMIMPORT_SEGMENTS_MAX)
>          return NULL;
>  
>      seg = pa_xnew0(pa_memimport_segment, 1);
>  
> -    if (pa_shm_attach(&seg->memory, shm_id, writable) < 0) {
> +    if (pa_shm_attach(&seg->per_type.shm, shm_id, writable) < 0) {
>          pa_xfree(seg);
>          return NULL;
>      }
>  
> -    seg->writable = writable;
> -    seg->import = i;
> -    seg->trap = pa_memtrap_add(seg->memory.ptr, seg->memory.size);
> +    segment_late_init(seg, i, PA_MEMORY_SHARED_POSIX, writable);
> +
> +    pa_hashmap_put(i->shm_segments, PA_UINT32_TO_PTR(seg->per_type.shm.id), seg);
> +    return seg;
> +}
> +
> +/* Should be called locked */
> +static pa_memimport_segment* segment_memfd_attach(pa_memimport *i, int fd, bool writable) {
> +    pa_memimport_segment* seg;
>  
> -    pa_hashmap_put(i->segments, PA_UINT32_TO_PTR(seg->memory.id), seg);
> +    /* FIXME: Introduce a proper memfd-tracking mechanism. We receive
> +     * different fd numbers even for the very same fds passed by the
> +     * other endpoint. This can lead to an _unbounded_ increase in
> +     * the number of `pa_memimport_segment' allocations created here. */
> +/*  if (i->n_memfd_segments >= PA_MEMIMPORT_SEGMENTS_MAX)
> +        return NULL; */

It's not very clear from the comment why the code is commented out.

After looking at pstream.c, I think the situation is this: when
receiving memblocks over regular shm, the other side tells us the block
id and the shm segment id. Normally there will be only one shm segment
per client (and hence one segment per memimport object), right? I
wonder then why we bother with segment ids at all in each memblock
transmission, if there's only one segment anyway.
PA_MEMIMPORT_SEGMENTS_MAX is 16, though, so maybe I'm wrong with the
one segment per client assumption...

Now the problem is that you tried to implement a similar scheme for
memfds, with the fd as the segment id, which didn't work out so well.
Even when there's only one segment, we actually receive a new fd for
every memblock transmission, and attach to the same segment multiple
times, easily exceeding PA_MEMIMPORT_SEGMENTS_MAX even with non
-malicious clients. Is that correct?

This looks like something that really should be fixed before merging
the patch set. Do you already have an idea how to fix it? Could we send
the fd just once during the connection set-up phase, and always use
that fd from then on?

...

Ok, now that I look at the memexport changes, it looks like we can re
-export an imported memblock. That explains why the segment id is
needed.

> @@ -1048,10 +1159,10 @@ void pa_memimport_free(pa_memimport *i) {
>  }
>  
>  /* Self-locked */
> -pa_memblock* pa_memimport_get(pa_memimport *i, uint32_t block_id, uint32_t shm_id,
> -                              size_t offset, size_t size, bool writable) {
> +static pa_memblock* pa_memimport_get(pa_memimport *i, pa_mem_type_t type, uint32_t block_id,

The pa_ prefix should be removed.

> diff --git a/src/pulsecore/memblock.h b/src/pulsecore/memblock.h
> index 184ba55..fc84c17 100644
> --- a/src/pulsecore/memblock.h
> +++ b/src/pulsecore/memblock.h
> @@ -135,14 +136,17 @@ size_t pa_mempool_block_size_max(pa_mempool *p);
>  /* For receiving blocks from other nodes */
>  pa_memimport* pa_memimport_new(pa_mempool *p, pa_memimport_release_cb_t cb, void *userdata);
>  void pa_memimport_free(pa_memimport *i);
> -pa_memblock* pa_memimport_get(pa_memimport *i, uint32_t block_id, uint32_t shm_id,
> +pa_memblock *pa_memimport_shm_get(pa_memimport *i, uint32_t block_id, uint32_t shm_id,
>                                size_t offset, size_t size, bool writable);
> +pa_memblock *pa_memimport_memfd_get(pa_memimport *i, uint32_t block_id, int fd, size_t offset,
> +        size_t size, bool writable);

Wrapped lines should be aligned with the opening parenthesis.

Phew, finally I'm done with this patch! Trying to understand the
relationship with memimports, segments and pstream and their
interactions took some effort. That makes me pretty impressed with your
patches, which didn't seem to take that much time nor help to write,
and I haven't found many issues beyond trivial coding style stuff :)

-- 
Tanu