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

[Ricardo Ribalda]

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.
My worry is the outside apps that we do not control.

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


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