Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 24 Mar 2022 15:23:29 -0500
From:   Dan Vacura <w36195@motorola.com>
To:     Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc:     linux-usb@vger.kernel.org, stable@vger.kernel.org,
        Felipe Balbi <balbi@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Bhupesh Sharma <bhupesh.sharma@st.com>,
        linux-kernel@vger.kernel.org,
        Paul Elder <paul.elder@ideasonboard.com>
Subject: Re: [PATCH v2] usb: gadget: uvc: Fix crash when encoding data for
 usb request
Message-ID: <YjzTQX3PlkBG2q6U@p1g3>
References: <20220318164706.22365-1-w36195@motorola.com>
 <YjyDp4l37XimcoZh@pendragon.ideasonboard.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YjyDp4l37XimcoZh@pendragon.ideasonboard.com>
Precedence: bulk

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