Re: [PATCH v2] media: uvcvideo: defer cancelled buffer completion until copies finish

[Ricardo Ribalda]

Hi

On Mon, 7 Sept 2026 at 05:23, 胡洪兵 <huhb04@xxxxxxxxx> wrote:
>
> Hi Ricardo,
> Sorry for the late reply and thanks for your review.
> The issue is exactly as you described.
>
> Ricardo Ribalda <ribalda@xxxxxxxxxxxx> 于2026年9月4日周五 21:00写道：
>>
>> Hi Hongbing
>>
>> Let me rewrite the commit message for me, because it took me a while
>> to understand what was going on. (totally on me). Please correct me if
>> I miss-understood the issue.
>>
>> The UVC driver processes frames in urbs. A frame usually is divided in
>> multiple urbs. The urb handler parses the header and the metadata, but
>> leaves the expensive memcpy to a workqueue.
>>
>> At any given moment, during streamon we will have:
>> - the current frame with (0-N) async memcpys waiting to happen. The
>> current frame is in queue->irqqueue. It has a refcnt of 1 + N memcpy
>> - Previous frames with (N) async memcpys waiting to happen. They are
>> not in queue->irqqueue. They have a refcnt of N memcpy
>>
>> With the current behaviour, when there is urb with an error we flush
>> the queue->irqque() irrespetively if they have any pending memcpys.
>>
>> There can be situations where the memcpy occurs after we have flushed
>> the frames and they are returned to the user. Instead, we should
>> exploit the refcnt mechanism to return the frames only after the
>> memcpys have been completed.
>>
>> This patch does so
>>
>> On Thu, 3 Sept 2026 at 16:33, Hongbing Hu <huhb04@xxxxxxxxx> wrote:
>> >
>> > From: Hongbing Hu <huhb04@xxxxxxxxx>
>> >
>> > uvc_queue_cancel() returns queued buffers to videobuf2 directly via
>> > vb2_buffer_done(). This is unsafe because asynchronous memcpy workers may
>> > still hold a reference on the same uvc_buffer. Once vb2_buffer_done() has
>> > run, userspace can dequeue and requeue the buffer while the old worker is
>> > still running, leading to the same list_head being inserted into irqqueue
>> > twice and corrupting the list.
>> >
>> > The crash manifests in two ways depending on the drop-corrupted-frames
>> > module parameter:
>> >
>> > 1. With the drop corrupted frames enabled, the old worker's
>> >    final kref_put reaches uvc_queue_buffer_complete(), sees buf->error set,
>> >    and calls uvc_queue_buffer_requeue(). This performs list_add_tail() on
>> >    buf->queue even though userspace has already QBUF'd the same buffer and
>> >    added it to irqqueue.
>> >
>> > 2. With nodrop=1, uvc_queue_buffer_complete() calls vb2_buffer_done() a
>> >    second time. If userspace has already dequeued and requeued the buffer,
>> >    the second completion exposes it to userspace again, and the next
>> >    DQBUF/QBUF inserts the still-linked list_head into irqqueue a second
>> >    time.
>> >
>> > Both cases produce the kind of list_del corruption seen here:
>> >
>> >     list_del corruption. next->prev should be ..., but was ...
>> >     kernel BUG at lib/list_debug.c:64!
>> >
>> > Fix the cancel path to remove buffers from irqqueue and drop the queue's
>> > reference through uvc_queue_buffer_release() (i.e. kref_put()). The final
>> > VB2 completion then only happens from uvc_queue_buffer_complete() once the
>> > last async reference is released, so userspace cannot reuse the buffer
>> > while an old worker still accesses it.
>> >
>> > A buffer whose state is already UVC_BUF_STATE_ERROR is forced to complete
>> > as an error and must not be requeued by the corrupted-frame policy. Normal
>> > malformed frames are still requeued/dropped as before.
>> >
>> > Fixes: 01e90464e42e ("media: uvcvideo: queue: Support asynchronous buffer handling")
>> > Cc: stable@xxxxxxxxxxxxxxx
>> > Signed-off-by: Hongbing Hu <huhb04@xxxxxxxxx>
>> > ---
>> > v2: corrected author real name in From/SOB per maintainer feedback
>> >
>> >  drivers/media/usb/uvc/uvc_queue.c | 30 ++++++++++++++++++++++++++++--
>> >  1 file changed, 28 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
>> > index 3c002c8f44..9206e0a355 100644
>> > --- a/drivers/media/usb/uvc/uvc_queue.c
>> > +++ b/drivers/media/usb/uvc/uvc_queue.c
>> > @@ -289,10 +289,19 @@ int uvc_queue_init(struct uvc_streaming *stream, struct uvc_video_queue *queue,
>> >   */
>> >  void uvc_queue_cancel(struct uvc_video_queue *queue, int disconnect)
>> >  {
>> > +       struct uvc_buffer *buf, *next;
>> > +       struct list_head local_list;
>> >         unsigned long flags;
>> >
>> > +       INIT_LIST_HEAD(&local_list);
>> > +
>> >         spin_lock_irqsave(&queue->irqlock, flags);
>> > -       __uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
>> > +       while (!list_empty(&queue->irqqueue)) {
>> > +               buf = list_first_entry(&queue->irqqueue, struct uvc_buffer, queue);
>> > +               list_del(&buf->queue);
>> > +               buf->state = UVC_BUF_STATE_ERROR;
>> > +               list_add_tail(&buf->queue, &local_list);
>> > +       }
>> >         /*
>> >          * This must be protected by the irqlock spinlock to avoid race
>> >          * conditions between uvc_buffer_queue and the disconnection event that
>> > @@ -303,6 +312,17 @@ void uvc_queue_cancel(struct uvc_video_queue *queue, int disconnect)
>> >         if (disconnect)
>> >                 queue->flags |= UVC_QUEUE_DISCONNECTED;
>> >         spin_unlock_irqrestore(&queue->irqlock, flags);
>>
>>
>> I *think* that you do not need to call uvc_queue_buffer_release
>> outisde spin_unlock_irqrestore(). You could do it
>> inside the previous while() and simplify the code.
>>
>> The only way to deadlock is  if uvc_queue_buffer_requeue() is called
>> and you are actively blocking that in your next snippet.
>>
>> I would recommend to add a comment in uvc_queue_buffer_complete if you
>> go that way.
>
> OK， good idea。
> I checked the code again.
> Reusing `buf->state` might make the code look a little confusing. Would it be better to add a new member "buf->cancelled"? For example, modify it like this:
> vc_queue_cancel()：buf->cancelled = true; buf->error = 1;uvc_queue_buffer_release(buf);
> uvc_buffer_prepare()：buf->cancelled = false;
> uvc_queue_buffer_complete: !buf->cancelled && buf->error && !nodrop
>
> What do you think of this change? Thanks!


Both approaches work for me. I have no strong preference for one or
the other. Lets wait for Laurent or Hans. They might think otherwise.

>
> diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
> index 3c002c8f44..1026d7def3 100644
> --- a/drivers/media/usb/uvc/uvc_queue.c
> +++ b/drivers/media/usb/uvc/uvc_queue.c
> @@ -122,6 +122,7 @@ static int uvc_buffer_prepare(struct vb2_buffer *vb)
>
>   buf->state = UVC_BUF_STATE_QUEUED;
>   buf->error = 0;
> + buf->cancelled = false;
>   buf->mem = vb2_plane_vaddr(vb, 0);
>   buf->length = vb2_plane_size(vb, 0);
>   if (vb->type != V4L2_BUF_TYPE_VIDEO_OUTPUT)
> @@ -289,10 +290,17 @@ int uvc_queue_init(struct uvc_streaming *stream, struct uvc_video_queue *queue,
>   */
>  void uvc_queue_cancel(struct uvc_video_queue *queue, int disconnect)
>  {
> + struct uvc_buffer *buf;
>   unsigned long flags;
>
>   spin_lock_irqsave(&queue->irqlock, flags);
> - __uvc_queue_return_buffers(queue, UVC_BUF_STATE_ERROR);
> + while (!list_empty(&queue->irqqueue)) {
> + buf = list_first_entry(&queue->irqqueue, struct uvc_buffer, queue);
> + list_del(&buf->queue);
> + buf->error = 1;
> + buf->cancelled = true;
> + uvc_queue_buffer_release(buf);
> + }
>   /*
>   * This must be protected by the irqlock spinlock to avoid race
>   * conditions between uvc_buffer_queue and the disconnection event that
> @@ -356,7 +364,12 @@ static void uvc_queue_buffer_complete(struct kref *ref)
>   struct vb2_buffer *vb = &buf->buf.vb2_buf;
>   struct uvc_video_queue *queue = vb2_get_drv_priv(vb->vb2_queue);
>
> - if (buf->error && !uvc_no_drop_param) {
> + /*
> + * Buffers cancelled from uvc_queue_cancel() are forced to complete as
> + * errors. They must not be requeued by the corrupted-frame policy even
> + * when buf->error is set.
> + */
> + if (!buf->cancelled && buf->error && !uvc_no_drop_param) {
>   uvc_queue_buffer_requeue(queue, buf);
>   return;
>   }
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index b6bcee4a22..1b611715fa 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -317,6 +317,7 @@ struct uvc_buffer {
>
>   enum uvc_buffer_state state;
>   unsigned int error;
> + bool cancelled;
>
>   void *mem;
>   unsigned int length;
> --
> 2.34.1
>
>> > +
>> > +       /*
>> > +        * Release the queue-owned kref outside the irqlock. The final VB2
>> > +        * completion only happens from uvc_queue_buffer_complete() once all
>> > +        * asynchronous copy references have been released, preventing userspace
>> > +        * from reusing the buffer while an old worker still accesses it.
>> > +        */
>> > +       list_for_each_entry_safe(buf, next, &local_list, queue) {
>> > +               list_del(&buf->queue);
>> > +               uvc_queue_buffer_release(buf);
>> > +       }
>> >  }
>> >
>> >  /*
>> > @@ -356,7 +376,13 @@ static void uvc_queue_buffer_complete(struct kref *ref)
>> >         struct vb2_buffer *vb = &buf->buf.vb2_buf;
>> >         struct uvc_video_queue *queue = vb2_get_drv_priv(vb->vb2_queue);
>>
>> If we called uvc_queue_cancel() buf->error is not 1, but the frame is
>> not complete.
>>
>> You cannot return VB2_BUF_STATE_DONE in that situation. I think you
>> should set buf->error =1 inside uvc_queue_cancel
>>
>> >
>> > -       if (buf->error && !uvc_no_drop_param) {
>> > +       /*
>> > +        * Buffers cancelled from uvc_queue_cancel() are forced to complete as
>> > +        * errors. They must not be requeued by the corrupted-frame policy even
>> > +        * when buf->error is set.
>> > +        */
>> > +       if (buf->state != UVC_BUF_STATE_ERROR &&
>> > +           buf->error && !uvc_no_drop_param) {
>> >                 uvc_queue_buffer_requeue(queue, buf);
>> >                 return;
>> >         }
>> > --
>> > 2.34.1
>> >
>> >
>>
>>
>> --
>> Ricardo Ribalda



-- 
Ricardo Ribalda