Re: [PATCH v2 1/2] media: uvcvideo: Fix race condition for meta buffer list

[Ricardo Ribalda]

Hi Ricardo :P

On Wed, 8 Jul 2026 at 12:57, Ricardo Ribalda <ribalda@xxxxxxxxxxxx> wrote:
>
> Hi Hans
>
> On Wed, 1 Jul 2026 at 17:30, Hans de Goede <hansg@xxxxxxxxxx> wrote:
> >
> > Hi Ricardo,
> >
> > On 30-Jun-26 16:10, Ricardo Ribalda wrote:
> > > Hi Hans,
> > >
> > > On Tue, 30 Jun 2026 at 15:21, Hans de Goede <hansg@xxxxxxxxxx> wrote:
> > >>
> > >> Hi Ricardo,
> > >>
> > >> On 30-Jun-26 12:17, Ricardo Ribalda wrote:
> > >>> Hi Hans,
> > >>>
> > >>> Thanks for the prompt reply.
> > >>>
> > >>> On Tue, 30 Jun 2026 at 11:47, Hans de Goede <hansg@xxxxxxxxxx> wrote:
> > >>>>
> > >>>> Hi Ricardo,
> > >>>>
> > >>>> On 29-Jun-26 19:31, Ricardo Ribalda wrote:
> > >>>>> queue->irqueue contains a list of the buffers owned by the driver. The
> > >>>>> list is protected by queue->irqlock. uvc_queue_get_current_buffer()
> > >>>>> returns a pointer to the current buffer in that list, but does not
> > >>>>> remove the buffer from it. This can lead to race conditions.
> > >>>>>
> > >>>>> Inspecting the code, it seems that the candidate for such race is
> > >>>>> uvc_queue_return_buffers(). For the capture queue, that function is
> > >>>>> called with the device streamoff, so no race can occur. On the other
> > >>>>> hand, the metadata queue, could trigger a race condition, because
> > >>>>> stop_streaming can be called with the device in any streaming state.
> > >>>>>
> > >>>>> We can solve this issue introducing a flag, stream->meta.in_flight,
> > >>>>> protected with a spinlock. When there is a buffer in flight that can
> > >>>>> write into metadata the flag is raised, notifying the stop streaming
> > >>>>> that it needs to wait.
> > >>>>>
> > >>>>> Reported-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > >>>>> Closes: https://lore.kernel.org/linux-media/20250630141707.GG20333@xxxxxxxxxxxxxxxxxxxxxxxxxx/
> > >>>>> Cc: stable@xxxxxxxxxxxxxxx
> > >>>>> Fixes: 088ead255245 ("media: uvcvideo: Add a metadata device node")
> > >>>>> Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> > >>>>
> > >>>> First of all thank you for looking into fixing this.
> > >>>>
> > >>>> I'm sorry, but this feels more like a band-aid then a proper fix.
> > >>>>
> > >>>> How about adding a started bool to struct uvc_streaming which gets
> > >>>> set to 1 by uvc_video_start_streaming() and 0 by uvc_video_stop_streaming().
> > >>>>
> > >>>> And then call uvc_video_stop_streaming() from either
> > >>>> uvc_stop_streaming_video() or uvc_stop_streaming_meta()
> > >>>> depending on which one gets called first ?
> > >>>>
> > >>>> With a mutex protecting the started bool and being held
> > >>>> over calling uvc_video_stop_streaming() ?
> > >>>>
> > >>>> So stop the actual hw streaming when either of the
> > >>>> 2 possible /dev/video0 nodes gets its vb2_ops.stop_streaming
> > >>>> callback called?
> > >>>>
> > >>>> And to this before draining the buffer queue.
> > >>>>
> > >>>> That seems cleaner then this approach?
> > >>>
> > >>> Assuming /dev/video0 is the video node and /dev/video1 is the meta device.
> > >>>
> > >>> Currently, we support something like:
> > >>>
> > >>> 1) yavta -c /dev/video0 &
> > >>> 2) yavta --capture=2 /dev/video1
> > >>> 3) yavta --capture=2 /dev/video1
> > >>> 4) kill %1
> > >>>
> > >>>
> > >>> If I understood correctly, your proposal would cause the camera to
> > >>> stop streaming when step 2 completes.
> > >>
> > >> Yes. But this very much feels like a case of:
> > >>
> > >> https://quotefancy.com/media/wallpaper/1600x900/5523002-Henny-Youngman-Quote-The-patient-says-Doctor-it-hurts-when-I-do.jpg
> > >
> > > We have a similar joke in Spanish:
> > > Doctor, doctor, it hurts here, here, here, here, here. What do I have?
> > > A broken finger :P
> > >
> > >>
> > >>> I think this risks breaking use cases.
> > >>
> > >> That would have to be some rather convoluted use-case.
> > >
> > > I believe we have a similar scheme to test the metadata node in
> > > ChromeOS... but we can change that.
> >
> > That seems unlikely? Either I would expect some app/lib/dameon to
> > do a quick test stream for a few frames at init time to determine metadata
> > support, in which case I would expect streaming on both queues to get
> > stopped after the quick test.
> >
> > Or testing is delayed till the first real start-streaming moment in which
> > case it makes no sense to stop + restart the metadata queue. What I guess
> > may happen is stopping the metadata queue when it does not generate any
> > data for a few frames, assuming there simply is no metadata support.
> >
> > Hmm, that might actually be a troublesome case.
> >
> > > My worry is the outside apps that we do not control.
> >
> > Ack, thinking more about this, this might be more likely then
> > I assumed in the non metadata available case, see above.
> >
> > So I'm no longer really convinced of my own proposal.
> >
> > >> IMHO the simplicity of fixing the race you're trying to fix is
> > >> worth the userspace regression risk (which I deem low).
> > >>
> > >> Worst case we revert the fix and go back to the drawing board.
> > >
> > > Are you concerned of this asymmetric behaviour, or do you think that it is fine?
> > >
> > > open /dev/video0 (streaming starts)
> > > open /dev/video1
> > > close /dev/video1 (streaming stops)
> > > open /dev/video1 (streaming still off)
> > >
> > >
> > > vs
> > >
> > > open /dev/video0 (streaming starts)
> > > open /dev/video1
> > > close /dev/video0 (streaming stops)
> > > open /dev/video0 (streaming resumes)
> >
> > That second one actually is broken already, we don't flush
> > the metadata queue on streaming stop on the regular queue, so
> > it will possibly contain a half-filled metadata buffer which
> > we then continue to append to with fresh metadata. So any
> > multi-packet metadata will get corrupted for the first frame
> > in the second stream start in that case.
>
> Luckily for us, it is currently "kind of" working.
>
> uvc_stop_streaming_video() -> uvc_video_stop_streaming() ->
> uvc_video_stop_transfer() -> usb_poison_urb()
>
> This usually (though I do not think always) forces an -ENOENT at the
> URBs callback:
>
> uvc_video_complete() -> uvc_queue_cancel(qmeta) -> uvc_queue_return_buffers()
>
> I will send a patch to explicitly call uvc_queue_return_buffers(qmeta)
> in uvc_stop_streaming_video() so we don't have to rely on this
> implicit behavior.
>
> But I have a question: Do we *really* need to call
> __uvc_queue_return_buffers() from uvc_queue_cancel() if it is properly
> handled in uvc_stop_streaming_video()?

Yes we do. Otherwise if userspace is locked in dqbuf and a usb error
happens, userspace will be locked forever.
Could we probably add a flag or something to avoid the 5x
__uvc_queue_return_buffers(), probably... but worth it? Nahhh


>
> A side effect of relying on the URB cancellation is that we end up
> performing one flush per URB, which is not really needed. For example:
>
> 41 (1) [-] none 176 5500 B 15166.417603 15166.449738 16.665 fps ts mono/SoE
> 42 (2) [-] none 177 5478 B 15166.477640 15166.509783 16.656 fps ts mono/SoE
> 43 (3) [-] none 178 5522 B 15166.537639 15166.569758 16.667 fps ts mono/SoE
> 44 (4) [E] none 0 0 B 0.000000 15166.599067 -0.000 fps ts mono/SoE
> 45 (5) [E] none 0 0 B 0.000000 15166.599115 0.000 fps ts mono/SoE
> 46 (6) [E] none 0 0 B 0.000000 15166.599125 0.000 fps ts mono/SoE
> 47 (7) [E] none 0 0 B 0.000000 15166.599133 0.000 fps ts mono/SoE
> 48 (0) [E] none 0 0 B 0.000000 15166.599141 0.000 fps ts mono/SoE
> 49 (1) [E] none 0 0 B 0.000000 15166.599148 0.000 fps ts mono/SoE
> 50 (2) [E] none 0 0 B 0.000000 15166.599157 0.000 fps ts mono/SoE
> 51 (3) [E] none 0 0 B 0.000000 15166.599167 0.000 fps ts mono/SoE
> 52 (4) [E] none 0 0 B 0.000000 15166.599177 0.000 fps ts mono/SoE
> 53 (5) [E] none 0 0 B 0.000000 15166.599187 0.000 fps ts mono/SoE
> 54 (6) [E] none 0 0 B 0.000000 15166.599197 0.000 fps ts mono/SoE
> 55 (7) [E] none 0 0 B 0.000000 15166.599224 0.000 fps ts mono/SoE
> 56 (0) [E] none 0 0 B 0.000000 15166.599234 0.000 fps ts mono/SoE
> 57 (1) [E] none 0 0 B 0.000000 15166.599243 0.000 fps ts mono/SoE
> 58 (2) [E] none 0 0 B 0.000000 15166.599253 0.000 fps ts mono/SoE
> 59 (3) [E] none 0 0 B 0.000000 15166.599261 0.000 fps ts mono/SoE
> 60 (4) [E] none 0 0 B 0.000000 15166.599269 0.000 fps ts mono/SoE
> 61 (5) [E] none 0 0 B 0.000000 15166.599278 0.000 fps ts mono/SoE
> 62 (6) [E] none 0 0 B 0.000000 15166.599462 0.000 fps ts mono/SoE
> 63 (7) [E] none 0 0 B 0.000000 15166.599485 0.000 fps ts mono/SoE
> 64 (0) [E] none 0 0 B 0.000000 15166.599497 0.000 fps ts mono/SoE
> 65 (1) [E] none 0 0 B 0.000000 15166.599510 0.000 fps ts mono/SoE
> 66 (2) [E] none 0 0 B 0.000000 15166.599521 0.000 fps ts mono/SoE
> 67 (3) [E] none 0 0 B 0.000000 15166.599528 0.000 fps ts mono/SoE
> 68 (4) [E] none 0 0 B 0.000000 15166.599538 0.000 fps ts mono/SoE
> 69 (5) [E] none 0 0 B 0.000000 15166.599548 0.000 fps ts mono/SoE
> 70 (6) [E] none 0 0 B 0.000000 15166.599557 0.000 fps ts mono/SoE
> 71 (7) [E] none 0 0 B 0.000000 15166.599567 0.000 fps ts mono/SoE
> 72 (0) [E] none 0 0 B 0.000000 15166.599576 0.000 fps ts mono/SoE
>
> >
> > In hindsight having the metadata queue be a fully independent
> > queue without clearly defining how start/stop on both queues
> > works and enforcing the defined behavior at the driver level
> > was a mistake.
> >
> > I'm starting to think that ideally we would simply flush both
> > queues on the stop on the regular node and not have a stop
> > queue-op on the metadata queue at all, but that is not possible
> > I'm afraid.
> >
> > So I think we do need something like this series +
> > flush metadata-queue on regular queue stop.
> >
> > I'll try to make some time to review this series as is, since
> > although the waiting solution still feels ugly it may be the
> > best we can do.
> >
> > Regards,
> >
> > Hans
> >
> >
> >
> >
> >
> > >
> > >
> > >>
> > >>> As I see it, the issue is that the camera's live capture cycle is
> > >>> controlled solely by video0. We need some kind of synchronization
> > >>> mechanism with video1 if we do not want to change the behaviour and
> > >>> risk breaking apps.
> > >>
> > >> IMHO for a device with multiple /dev/video# nodes it makes sense
> > >> to wait with actually starting streaming/DMA-engines until all
> > >> enabled queues are started and stop when the first queue is stopped.
> > >>
> > >> The problem with uvcvideo is that we do not know if the metadata
> > >> queue is going to get used at all. In hindsight we should maybe
> > >> have had some way for userspace to explictly enable/disable metadata
> > >> support.
> > >>
> > >> So we start as soon as the main video node is opened, still I think
> > >> that stopping as soon as one of the queues is stopped makes sense.
> > >>
> > >> Laurent, do you have any input here?
> > >>
> > >> Regards,
> > >>
> > >> Hans
> > >>
> > >>
> > >>
> > >>
> > >>>> p.s.
> > >>>>
> > >>>> 1. It is tempting to also apply the same approach to
> > >>>> vb2_ops.start_streaming, but allowing the meta queue to be
> > >>>> the one to start streaming will likely cause issues. E.g.
> > >>>> the streaming code assumes having a meta-queue active is
> > >>>> optional, but not the other way around.
> > >>>>
> > >>>> TL;DR: vb2_ops.start_streaming should stay as is.
> > >>>>
> > >>>> 2. While looking into this I noticed that struct uvc_streaming
> > >>>> already has an active member, but unless I'm missing something
> > >>>> that ever only gets initialized to 0. So I think that can be
> > >>>> dropped. (If you re-use this please change it to a bool, no
> > >>>> need to have it atomic while protected by a mutex).
> > >>>
> > >>> I will send a patch to fix this. Thanks for noticing :)
> > >>>
> > >>>>
> > >>>>
> > >>>>
> > >>>>> ---
> > >>>>>  drivers/media/usb/uvc/uvc_queue.c | 14 ++++++++++++++
> > >>>>>  drivers/media/usb/uvc/uvc_video.c | 30 +++++++++++++++++++++++++++++-
> > >>>>>  drivers/media/usb/uvc/uvcvideo.h  |  2 ++
> > >>>>>  3 files changed, 45 insertions(+), 1 deletion(-)
> > >>>>>
> > >>>>> diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
> > >>>>> index 3c002c8f442f..af9dbfcf6f53 100644
> > >>>>> --- a/drivers/media/usb/uvc/uvc_queue.c
> > >>>>> +++ b/drivers/media/usb/uvc/uvc_queue.c
> > >>>>> @@ -209,10 +209,24 @@ static void uvc_stop_streaming_video(struct vb2_queue *vq)
> > >>>>>  static void uvc_stop_streaming_meta(struct vb2_queue *vq)
> > >>>>>  {
> > >>>>>       struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> > >>>>> +     struct uvc_streaming *stream = queue->stream;
> > >>>>>
> > >>>>>       lockdep_assert_irqs_enabled();
> > >>>>>
> > >>>>> +     spin_lock_irq(&stream->meta.irqlock);
> > >>>>> +     while (stream->meta.in_flight) {
> > >>>>> +             spin_unlock_irq(&stream->meta.irqlock);
> > >>>>> +             schedule();
> > >>>>> +             spin_lock_irq(&stream->meta.irqlock);
> > >>>>> +     }
> > >>>>> +     stream->meta.in_flight = true;
> > >>>>> +     spin_unlock_irq(&stream->meta.irqlock);
> > >>>>> +
> > >>>>>       uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
> > >>>>> +
> > >>>>> +     scoped_guard(spinlock_irq, &stream->meta.irqlock) {
> > >>>>> +             stream->meta.in_flight = false;
> > >>>>> +     }
> > >>>>>  }
> > >>>>>
> > >>>>>  static const struct vb2_ops uvc_queue_qops = {
> > >>>>> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > >>>>> index fc3536a4399f..f6b55b3a3308 100644
> > >>>>> --- a/drivers/media/usb/uvc/uvc_video.c
> > >>>>> +++ b/drivers/media/usb/uvc/uvc_video.c
> > >>>>> @@ -1732,6 +1732,26 @@ static void uvc_video_encode_bulk(struct uvc_urb *uvc_urb,
> > >>>>>       urb->transfer_buffer_length = stream->urb_size - len;
> > >>>>>  }
> > >>>>>
> > >>>>> +static struct uvc_buffer *
> > >>>>> +uvc_video_get_current_meta_buffer(struct uvc_streaming *stream)
> > >>>>> +{
> > >>>>> +     struct uvc_video_queue *queue = &stream->meta.queue;
> > >>>>> +     struct uvc_buffer *buf;
> > >>>>> +
> > >>>>> +     buf = uvc_queue_get_current_buffer(queue);
> > >>>>> +     if (!buf)
> > >>>>> +             return NULL;
> > >>>>> +
> > >>>>> +     guard(spinlock_irqsave)(&stream->meta.irqlock);
> > >>>>> +
> > >>>>> +     if (stream->meta.in_flight)
> > >>>>> +             return NULL;
> > >>>>> +
> > >>>>> +     stream->meta.in_flight = true;
> > >>>>> +
> > >>>>> +     return buf;
> > >>>>> +}
> > >>>>> +
> > >>>>>  static void uvc_video_complete(struct urb *urb)
> > >>>>>  {
> > >>>>>       struct uvc_urb *uvc_urb = urb->context;
> > >>>>> @@ -1767,7 +1787,7 @@ static void uvc_video_complete(struct urb *urb)
> > >>>>>       buf = uvc_queue_get_current_buffer(queue);
> > >>>>>
> > >>>>>       if (vb2_qmeta)
> > >>>>> -             buf_meta = uvc_queue_get_current_buffer(qmeta);
> > >>>>> +             buf_meta = uvc_video_get_current_meta_buffer(stream);
> > >>>>>
> > >>>>>       /* Re-initialise the URB async work. */
> > >>>>>       uvc_urb->async_operations = 0;
> > >>>>> @@ -1778,6 +1798,12 @@ static void uvc_video_complete(struct urb *urb)
> > >>>>>        */
> > >>>>>       stream->decode(uvc_urb, buf, buf_meta);
> > >>>>>
> > >>>>> +     if (buf_meta) {
> > >>>>> +             scoped_guard(spinlock_irqsave, &stream->meta.irqlock) {
> > >>>>> +                     stream->meta.in_flight = false;
> > >>>>> +             }
> > >>>>> +     }
> > >>>>> +
> > >>>>>       /* If no async work is needed, resubmit the URB immediately. */
> > >>>>>       if (!uvc_urb->async_operations) {
> > >>>>>               ret = usb_submit_urb(uvc_urb->urb, GFP_ATOMIC);
> > >>>>> @@ -2330,6 +2356,8 @@ int uvc_video_init(struct uvc_streaming *stream)
> > >>>>>       for_each_uvc_urb(uvc_urb, stream)
> > >>>>>               INIT_WORK(&uvc_urb->work, uvc_video_copy_data_work);
> > >>>>>
> > >>>>> +     spin_lock_init(&stream->meta.irqlock);
> > >>>>> +
> > >>>>>       return 0;
> > >>>>>  }
> > >>>>>
> > >>>>> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > >>>>> index b6bcee4a222f..6f1a3381d392 100644
> > >>>>> --- a/drivers/media/usb/uvc/uvcvideo.h
> > >>>>> +++ b/drivers/media/usb/uvc/uvcvideo.h
> > >>>>> @@ -484,6 +484,8 @@ struct uvc_streaming {
> > >>>>>               struct uvc_video_queue queue;
> > >>>>>               u32 format;
> > >>>>>               u32 buffersize;
> > >>>>> +             bool in_flight;
> > >>>>> +             spinlock_t irqlock; /* Protects in_flight. */
> > >>>>>       } meta;
> > >>>>>
> > >>>>>       /* Context data used by the bulk completion handler. */
> > >>>>>
> > >>>>
> > >>>
> > >>>
> > >>
> > >
> > >
> >
>
>
> --
> Ricardo Ribalda



-- 
Ricardo Ribalda