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

[Hans Verkuil <hverkuil+cisco@kernel.org>]

On 11/3/26 14:39, Marek Szyprowski wrote:
> 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@vger.kernel.org
>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> 
> 
> 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.

I was thinking the same thing.

I *think* the code is correct, but I'm not sure. Memory barriers are hard to get
right, and I consider myself to be a newbie when it comes to that. A simple spinlock
is much easier to understand, and this is hardly a high-performance loop where you
need to squeeze the last bit of performance out of it.

It's why I wanted to split this off from the original patch, so it is easier to
review.

Regards,

	Hans

> 
> 
>> ---
>>  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