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