Re: [PATCH v2 2/2] media: videobuf2: order buffer initialization before bufs_bitmap publication

[Marek Szyprowski]

On 03.11.2026 11:56, Ricardo Ribalda wrote:
> Stateless mem2mem decoders call vb2_find_buffer() -> vb2_get_buffer()
> from .device_run() without holding q->lock or q->mmap_lock, which can
> run concurrently with VIDIOC_CREATE_BUFS on a streaming queue.
>
> Both set_bit() and test_bit() are unordered, and mutex_lock(&q->mmap_lock)
> is only an acquire barrier. On weakly-ordered architectures such as
> arm64, a lockless vb2_get_buffer() caller can observe a newly set bit in
> q->bufs_bitmap before the kzalloc() zeroing, buffer initialization, and
> q->bufs[] pointer store performed by __vb2_queue_alloc() become visible.
>
> Pair smp_mb__before_atomic() in vb2_queue_publish_buffers() with
> test_bit_acquire() in vb2_get_buffer() so that any reader observing a
> set bit in q->bufs_bitmap also sees a fully initialized vb2_buffer.
>
> Fixes: a286b0837e13 ("media: core: Add bitmap manage bufs array entries")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>


Can't we use ordinary locking instead of those memory barriers and relying
on not-always-atomic bit operations? For me this looks like over-optimization,
taking into account that those operations are not on the critical path nor
anyhow time-comparable with filling the whole buffer with image data.


> ---
>  drivers/media/common/videobuf2/videobuf2-core.c | 8 ++++++++
>  include/media/videobuf2-core.h                  | 6 +++++-
>  2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 70ef38c3d16a..d0dd4b22b646 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -461,6 +461,14 @@ static void vb2_queue_publish_buffers(struct vb2_queue *q, unsigned int start,
>  
>  	lockdep_assert_held(&q->mmap_lock);
>  
> +	/*
> +	 * Order the buffer initialization and the q->bufs[] stores done by
> +	 * __vb2_queue_alloc() before the bits that publish them. Pairs with
> +	 * test_bit_acquire() in vb2_get_buffer(), which is also called
> +	 * without q->lock or q->mmap_lock held, e.g. by vb2_find_buffer()
> +	 * from a mem2mem .device_run().
> +	 */
> +	smp_mb__before_atomic();
>  	for (i = start; i < start + count; i++) {
>  		if (WARN_ON(i >= q->max_num_buffers || !q->bufs[i] ||
>  			    test_bit(i, q->bufs_bitmap)))
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 2e93e6985d5f..20b961da21e0 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -1288,7 +1288,11 @@ static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
>  	if (index >= q->max_num_buffers)
>  		return NULL;
>  
> -	if (test_bit(index, q->bufs_bitmap))
> +	/*
> +	 * A set bit guarantees that the buffer is fully initialized, pairs
> +	 * with smp_mb__before_atomic() in vb2_queue_publish_buffers().
> +	 */
> +	if (test_bit_acquire(index, q->bufs_bitmap))
>  		return q->bufs[index];
>  	return NULL;
>  }
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland