| Message ID | 20220318164706.22365-1-w36195@motorola.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] usb: gadget: uvc: Fix crash when encoding data for usb request | expand |
Hi Dan, (CC'ing Paul Elder) Thank you for the patch. On Fri, Mar 18, 2022 at 11:47:06AM -0500, Dan Vacura wrote: > During the uvcg_video_pump() process, if an error occurs and > uvcg_queue_cancel() is called, the buffer queue will be cleared out, but > the current marker (queue->buf_used) of the active buffer (no longer > active) is not reset. On the next iteration of uvcg_video_pump() the > stale buf_used count will be used and the logic of min((unsigned > int)len, buf->bytesused - queue->buf_used) may incorrectly calculate a > nbytes size, causing an invalid memory access. When uvcg_queue_cancel() is called, it will empty the queue->irqqueue. The next uvcg_video_pump() iteration should thus get a NULL buffer when calling uvcg_queue_head(), and shouldn't proceed to calling video->encode(). Is the issue that the application queues further buffers after cancellation, which puts a new buffer in the irqqueue ? I wonder if we need to expand the discussion here to what should be done if an error occurs in uvcg_video_pump(). We currently cancel the queue and drop all queued buffers, but don't prevent more buffers to be queued. Should we force the application to stop streaming in case of error, clean up and restart ? Or are usb_ep_queue() errors expected to happen from time to time, with graceful error recovery a required feature of the gadget driver ? > [80802.185460][ T315] configfs-gadget gadget: uvc: VS request completed > with status -18. > [80802.185519][ T315] configfs-gadget gadget: uvc: VS request completed > with status -18. > ... > uvcg_queue_cancel() is called and the queue is cleared out, but the > marker queue->buf_used is not reset. > ... > [80802.262328][ T8682] Unable to handle kernel paging request at virtual > address ffffffc03af9f000 > ... > ... > [80802.263138][ T8682] Call trace: > [80802.263146][ T8682] __memcpy+0x12c/0x180 > [80802.263155][ T8682] uvcg_video_pump+0xcc/0x1e0 > [80802.263165][ T8682] process_one_work+0x2cc/0x568 > [80802.263173][ T8682] worker_thread+0x28c/0x518 > [80802.263181][ T8682] kthread+0x160/0x170 > [80802.263188][ T8682] ret_from_fork+0x10/0x18 > [80802.263198][ T8682] Code: a8c12829 a88130cb a8c130 > > Fixes: d692522577c0 ("usb: gadget/uvc: Port UVC webcam gadget to use videobuf2 framework") > Signed-off-by: Dan Vacura <w36195@motorola.com> > > --- > Changes in v2: > - Add Fixes tag > > drivers/usb/gadget/function/uvc_queue.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c > index d852ac9e47e7..2cda982f3765 100644 > --- a/drivers/usb/gadget/function/uvc_queue.c > +++ b/drivers/usb/gadget/function/uvc_queue.c > @@ -264,6 +264,8 @@ void uvcg_queue_cancel(struct uvc_video_queue *queue, int disconnect) > buf->state = UVC_BUF_STATE_ERROR; > vb2_buffer_done(&buf->buf.vb2_buf, VB2_BUF_STATE_ERROR); > } > + queue->buf_used = 0; > + > /* This must be protected by the irqlock spinlock to avoid race > * conditions between uvc_queue_buffer and the disconnection event that > * could result in an interruptible wait in uvc_dequeue_buffer. Do not
On Fri, Mar 18, 2022 at 11:47:06AM -0500, Dan Vacura wrote: > During the uvcg_video_pump() process, if an error occurs and > uvcg_queue_cancel() is called, the buffer queue will be cleared out, but > the current marker (queue->buf_used) of the active buffer (no longer > active) is not reset. On the next iteration of uvcg_video_pump() the > stale buf_used count will be used and the logic of min((unsigned > int)len, buf->bytesused - queue->buf_used) may incorrectly calculate a > nbytes size, causing an invalid memory access. > > [80802.185460][ T315] configfs-gadget gadget: uvc: VS request completed > with status -18. > [80802.185519][ T315] configfs-gadget gadget: uvc: VS request completed > with status -18. > ... > uvcg_queue_cancel() is called and the queue is cleared out, but the > marker queue->buf_used is not reset. > ... > [80802.262328][ T8682] Unable to handle kernel paging request at virtual > address ffffffc03af9f000 > ... > ... > [80802.263138][ T8682] Call trace: > [80802.263146][ T8682] __memcpy+0x12c/0x180 > [80802.263155][ T8682] uvcg_video_pump+0xcc/0x1e0 > [80802.263165][ T8682] process_one_work+0x2cc/0x568 > [80802.263173][ T8682] worker_thread+0x28c/0x518 > [80802.263181][ T8682] kthread+0x160/0x170 > [80802.263188][ T8682] ret_from_fork+0x10/0x18 > [80802.263198][ T8682] Code: a8c12829 a88130cb a8c130 > > Fixes: d692522577c0 ("usb: gadget/uvc: Port UVC webcam gadget to use videobuf2 framework") > Signed-off-by: Dan Vacura <w36195@motorola.com> > > --- > Changes in v2: > - Add Fixes tag <formletter> This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html for how to do this properly. </formletter>
Hi Laurent, Appreciate the feedback. On Thu, Mar 24, 2022 at 04:43:51PM +0200, Laurent Pinchart wrote: > Hi Dan, > > (CC'ing Paul Elder) > > Thank you for the patch. > > On Fri, Mar 18, 2022 at 11:47:06AM -0500, Dan Vacura wrote: > > During the uvcg_video_pump() process, if an error occurs and > > uvcg_queue_cancel() is called, the buffer queue will be cleared out, but > > the current marker (queue->buf_used) of the active buffer (no longer > > active) is not reset. On the next iteration of uvcg_video_pump() the > > stale buf_used count will be used and the logic of min((unsigned > > int)len, buf->bytesused - queue->buf_used) may incorrectly calculate a > > nbytes size, causing an invalid memory access. > > When uvcg_queue_cancel() is called, it will empty the queue->irqqueue. > The next uvcg_video_pump() iteration should thus get a NULL buffer when > calling uvcg_queue_head(), and shouldn't proceed to calling > video->encode(). Is the issue that the application queues further > buffers after cancellation, which puts a new buffer in the irqqueue ? Yes, that's exactly what's happening. The application has one thread that is receiving camera frames and queuing them to the gadget driver, the other thread is waiting for empty buffers to dequeue, generated via uvcg_queue_next_buffer(), to send back for camera to fill. Is there a requirement to serialize this logic? In addition, I can check for the state of the buffers that have just been dequeued, but not sure what to do if there's a failure. > > I wonder if we need to expand the discussion here to what should be done > if an error occurs in uvcg_video_pump(). We currently cancel the queue > and drop all queued buffers, but don't prevent more buffers to be > queued. Should we force the application to stop streaming in case of > error, clean up and restart ? Or are usb_ep_queue() errors expected to > happen from time to time, with graceful error recovery a required > feature of the gadget driver ? Good question, this is out of my expertise, but I can comment about what I see in our current setup, qcom snapdragon chipsets with the dwc3 drivers on 5.10-android. Depending on the host I connect to, the -18 errors in uvc_video_complete() can occur during normal use, sometimes several times in a span of a few seconds. I wasn't seeing usb_eq_queue() errors. When the error occurs the device application doesn't do anything special and continues queuing subsequent buffers. Sometimes there is visible corruption in the received data, but the streaming gracefully recovers, mpeg or yuv. > > > [80802.185460][ T315] configfs-gadget gadget: uvc: VS request completed > > with status -18. > > [80802.185519][ T315] configfs-gadget gadget: uvc: VS request completed > > with status -18. > > ... > > uvcg_queue_cancel() is called and the queue is cleared out, but the > > marker queue->buf_used is not reset. > > ... > > [80802.262328][ T8682] Unable to handle kernel paging request at virtual > > address ffffffc03af9f000 > > ... > > ... > > [80802.263138][ T8682] Call trace: > > [80802.263146][ T8682] __memcpy+0x12c/0x180 > > [80802.263155][ T8682] uvcg_video_pump+0xcc/0x1e0 > > [80802.263165][ T8682] process_one_work+0x2cc/0x568 > > [80802.263173][ T8682] worker_thread+0x28c/0x518 > > [80802.263181][ T8682] kthread+0x160/0x170 > > [80802.263188][ T8682] ret_from_fork+0x10/0x18 > > [80802.263198][ T8682] Code: a8c12829 a88130cb a8c130 > > > > Fixes: d692522577c0 ("usb: gadget/uvc: Port UVC webcam gadget to use videobuf2 framework") > > Signed-off-by: Dan Vacura <w36195@motorola.com> > > > > --- > > Changes in v2: > > - Add Fixes tag > > > > drivers/usb/gadget/function/uvc_queue.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c > > index d852ac9e47e7..2cda982f3765 100644 > > --- a/drivers/usb/gadget/function/uvc_queue.c > > +++ b/drivers/usb/gadget/function/uvc_queue.c > > @@ -264,6 +264,8 @@ void uvcg_queue_cancel(struct uvc_video_queue *queue, int disconnect) > > buf->state = UVC_BUF_STATE_ERROR; > > vb2_buffer_done(&buf->buf.vb2_buf, VB2_BUF_STATE_ERROR); > > } > > + queue->buf_used = 0; > > + > > /* This must be protected by the irqlock spinlock to avoid race > > * conditions between uvc_queue_buffer and the disconnection event that > > * could result in an interruptible wait in uvc_dequeue_buffer. Do not > > -- > Regards, > > Laurent Pinchart
Hi Dan, On Thu, Mar 24, 2022 at 03:23:29PM -0500, Dan Vacura wrote: > On Thu, Mar 24, 2022 at 04:43:51PM +0200, Laurent Pinchart wrote: > > On Fri, Mar 18, 2022 at 11:47:06AM -0500, Dan Vacura wrote: > > > During the uvcg_video_pump() process, if an error occurs and > > > uvcg_queue_cancel() is called, the buffer queue will be cleared out, but > > > the current marker (queue->buf_used) of the active buffer (no longer > > > active) is not reset. On the next iteration of uvcg_video_pump() the > > > stale buf_used count will be used and the logic of min((unsigned > > > int)len, buf->bytesused - queue->buf_used) may incorrectly calculate a > > > nbytes size, causing an invalid memory access. > > > > When uvcg_queue_cancel() is called, it will empty the queue->irqqueue. > > The next uvcg_video_pump() iteration should thus get a NULL buffer when > > calling uvcg_queue_head(), and shouldn't proceed to calling > > video->encode(). Is the issue that the application queues further > > buffers after cancellation, which puts a new buffer in the irqqueue ? > > Yes, that's exactly what's happening. The application has one thread > that is receiving camera frames and queuing them to the gadget driver, > the other thread is waiting for empty buffers to dequeue, generated via > uvcg_queue_next_buffer(), to send back for camera to fill. Is there a > requirement to serialize this logic? Not in userspace, any required serialization should be handled in the kernel. Unless I'm mistaken we already have a lock to handle concurrent QBUF and DQBUF calls, so it should be fine. > In addition, I can check for the > state of the buffers that have just been dequeued, but not sure what to > do if there's a failure. That's a good question, discussed below. > > I wonder if we need to expand the discussion here to what should be done > > if an error occurs in uvcg_video_pump(). We currently cancel the queue > > and drop all queued buffers, but don't prevent more buffers to be > > queued. Should we force the application to stop streaming in case of > > error, clean up and restart ? Or are usb_ep_queue() errors expected to > > happen from time to time, with graceful error recovery a required > > feature of the gadget driver ? > > Good question, this is out of my expertise, but I can comment about what > I see in our current setup, qcom snapdragon chipsets with the dwc3 > drivers on 5.10-android. Depending on the host I connect to, the -18 > errors in uvc_video_complete() can occur during normal use, sometimes > several times in a span of a few seconds. I wasn't seeing usb_eq_queue() > errors. When the error occurs the device application doesn't do anything > special and continues queuing subsequent buffers. Sometimes there is > visible corruption in the received data, but the streaming gracefully > recovers, mpeg or yuv. So it looks like we need to handle this condition gracefully. This means that we shouldn't cancel the queue, but continue with the next transfer, doesn't it ? > > > [80802.185460][ T315] configfs-gadget gadget: uvc: VS request completed > > > with status -18. > > > [80802.185519][ T315] configfs-gadget gadget: uvc: VS request completed > > > with status -18. > > > ... > > > uvcg_queue_cancel() is called and the queue is cleared out, but the > > > marker queue->buf_used is not reset. > > > ... > > > [80802.262328][ T8682] Unable to handle kernel paging request at virtual > > > address ffffffc03af9f000 > > > ... > > > ... > > > [80802.263138][ T8682] Call trace: > > > [80802.263146][ T8682] __memcpy+0x12c/0x180 > > > [80802.263155][ T8682] uvcg_video_pump+0xcc/0x1e0 > > > [80802.263165][ T8682] process_one_work+0x2cc/0x568 > > > [80802.263173][ T8682] worker_thread+0x28c/0x518 > > > [80802.263181][ T8682] kthread+0x160/0x170 > > > [80802.263188][ T8682] ret_from_fork+0x10/0x18 > > > [80802.263198][ T8682] Code: a8c12829 a88130cb a8c130 > > > > > > Fixes: d692522577c0 ("usb: gadget/uvc: Port UVC webcam gadget to use videobuf2 framework") > > > Signed-off-by: Dan Vacura <w36195@motorola.com> > > > > > > --- > > > Changes in v2: > > > - Add Fixes tag > > > > > > drivers/usb/gadget/function/uvc_queue.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c > > > index d852ac9e47e7..2cda982f3765 100644 > > > --- a/drivers/usb/gadget/function/uvc_queue.c > > > +++ b/drivers/usb/gadget/function/uvc_queue.c > > > @@ -264,6 +264,8 @@ void uvcg_queue_cancel(struct uvc_video_queue *queue, int disconnect) > > > buf->state = UVC_BUF_STATE_ERROR; > > > vb2_buffer_done(&buf->buf.vb2_buf, VB2_BUF_STATE_ERROR); > > > } > > > + queue->buf_used = 0; > > > + > > > /* This must be protected by the irqlock spinlock to avoid race > > > * conditions between uvc_queue_buffer and the disconnection event that > > > * could result in an interruptible wait in uvc_dequeue_buffer. Do not
diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c index d852ac9e47e7..2cda982f3765 100644 --- a/drivers/usb/gadget/function/uvc_queue.c +++ b/drivers/usb/gadget/function/uvc_queue.c @@ -264,6 +264,8 @@ void uvcg_queue_cancel(struct uvc_video_queue *queue, int disconnect) buf->state = UVC_BUF_STATE_ERROR; vb2_buffer_done(&buf->buf.vb2_buf, VB2_BUF_STATE_ERROR); } + queue->buf_used = 0; + /* This must be protected by the irqlock spinlock to avoid race * conditions between uvc_queue_buffer and the disconnection event that * could result in an interruptible wait in uvc_dequeue_buffer. Do not
During the uvcg_video_pump() process, if an error occurs and uvcg_queue_cancel() is called, the buffer queue will be cleared out, but the current marker (queue->buf_used) of the active buffer (no longer active) is not reset. On the next iteration of uvcg_video_pump() the stale buf_used count will be used and the logic of min((unsigned int)len, buf->bytesused - queue->buf_used) may incorrectly calculate a nbytes size, causing an invalid memory access. [80802.185460][ T315] configfs-gadget gadget: uvc: VS request completed with status -18. [80802.185519][ T315] configfs-gadget gadget: uvc: VS request completed with status -18. ... uvcg_queue_cancel() is called and the queue is cleared out, but the marker queue->buf_used is not reset. ... [80802.262328][ T8682] Unable to handle kernel paging request at virtual address ffffffc03af9f000 ... ... [80802.263138][ T8682] Call trace: [80802.263146][ T8682] __memcpy+0x12c/0x180 [80802.263155][ T8682] uvcg_video_pump+0xcc/0x1e0 [80802.263165][ T8682] process_one_work+0x2cc/0x568 [80802.263173][ T8682] worker_thread+0x28c/0x518 [80802.263181][ T8682] kthread+0x160/0x170 [80802.263188][ T8682] ret_from_fork+0x10/0x18 [80802.263198][ T8682] Code: a8c12829 a88130cb a8c130 Fixes: d692522577c0 ("usb: gadget/uvc: Port UVC webcam gadget to use videobuf2 framework") Signed-off-by: Dan Vacura <w36195@motorola.com> --- Changes in v2: - Add Fixes tag drivers/usb/gadget/function/uvc_queue.c | 2 ++ 1 file changed, 2 insertions(+)