Message ID | 20231019185319.2714000-5-arakesh@google.com |
---|---|
State | Superseded |
Headers | show |
Series | usb: gadget: uvc: stability fixes on STREAMOFF. | expand |
On Thu, Oct 19, 2023 at 03:30:00PM -0700, Avichal Rakesh wrote: > > > On 10/19/23 13:32, kernel test robot wrote: > > Hi Avichal, > > > > kernel test robot noticed the following build warnings: > > > > [auto build test WARNING on usb/usb-testing] > > [also build test WARNING on usb/usb-next usb/usb-linus linus/master v6.6-rc6 next-20231019] > > [If your patch is applied to the wrong git tree, kindly drop us a note. > > And when submitting patch, we suggest to use '--base' as documented in > > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > > > url: https://github.com/intel-lab-lkp/linux/commits/Avichal-Rakesh/usb-gadget-uvc-prevent-use-of-disabled-endpoint/20231020-025512 > > base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing > > patch link: https://lore.kernel.org/r/20231019185319.2714000-5-arakesh%40google.com > > patch subject: [PATCH v6 4/4] usb: gadget: uvc: Fix use-after-free for inflight usb_requests > > config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20231020/202310200457.GwPPFuHX-lkp@intel.com/config) > > compiler: m68k-linux-gcc (GCC) 13.2.0 > > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231020/202310200457.GwPPFuHX-lkp@intel.com/reproduce) > > > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > > the same patch/commit), kindly add following tags > > | Reported-by: kernel test robot <lkp@intel.com> > > | Closes: https://lore.kernel.org/oe-kbuild-all/202310200457.GwPPFuHX-lkp@intel.com/ > > > > All warnings (new ones prefixed by >>): > > > >>> drivers/usb/gadget/function/uvc_video.c:231: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst > > * Must be called with req_lock held as it modifies the list ureq is held in > > > > > > Greg, apologies for the newb question: do you want me to upload > the fix for this as a reply to [PATCH v6 4/4], or upload a new chain of > v7s with this patch fixed? A whole new v7 series please. thanks, greg k-h
On Sat, Oct 21, 2023 at 3:05 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Thu, Oct 19, 2023 at 03:30:00PM -0700, Avichal Rakesh wrote: > > > > > > On 10/19/23 13:32, kernel test robot wrote: > > > Hi Avichal, > > > > > > kernel test robot noticed the following build warnings: > > > > > > [auto build test WARNING on usb/usb-testing] > > > [also build test WARNING on usb/usb-next usb/usb-linus linus/master v6.6-rc6 next-20231019] > > > [If your patch is applied to the wrong git tree, kindly drop us a note. > > > And when submitting patch, we suggest to use '--base' as documented in > > > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > > > > > url: https://github.com/intel-lab-lkp/linux/commits/Avichal-Rakesh/usb-gadget-uvc-prevent-use-of-disabled-endpoint/20231020-025512 > > > base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing > > > patch link: https://lore.kernel.org/r/20231019185319.2714000-5-arakesh%40google.com > > > patch subject: [PATCH v6 4/4] usb: gadget: uvc: Fix use-after-free for inflight usb_requests > > > config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20231020/202310200457.GwPPFuHX-lkp@intel.com/config) > > > compiler: m68k-linux-gcc (GCC) 13.2.0 > > > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231020/202310200457.GwPPFuHX-lkp@intel.com/reproduce) > > > > > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > > > the same patch/commit), kindly add following tags > > > | Reported-by: kernel test robot <lkp@intel.com> > > > | Closes: https://lore.kernel.org/oe-kbuild-all/202310200457.GwPPFuHX-lkp@intel.com/ > > > > > > All warnings (new ones prefixed by >>): > > > > > >>> drivers/usb/gadget/function/uvc_video.c:231: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst > > > * Must be called with req_lock held as it modifies the list ureq is held in > > > > > > > > > > Greg, apologies for the newb question: do you want me to upload > > the fix for this as a reply to [PATCH v6 4/4], or upload a new chain of > > v7s with this patch fixed? > > A whole new v7 series please. > Had a feeling, so sent out v7 series preemptively. Let me know if that doesn't work. v7: https://lore.kernel.org/20231020173626.2978356-1-arakesh@google.com/ Thank you! - Avi
On Mon, Oct 23, 2023 at 02:25:30PM -0700, Avichal Rakesh wrote: > On Sat, Oct 21, 2023 at 3:05 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > On Thu, Oct 19, 2023 at 03:30:00PM -0700, Avichal Rakesh wrote: > > > > > > > > > On 10/19/23 13:32, kernel test robot wrote: > > > > Hi Avichal, > > > > > > > > kernel test robot noticed the following build warnings: > > > > > > > > [auto build test WARNING on usb/usb-testing] > > > > [also build test WARNING on usb/usb-next usb/usb-linus linus/master v6.6-rc6 next-20231019] > > > > [If your patch is applied to the wrong git tree, kindly drop us a note. > > > > And when submitting patch, we suggest to use '--base' as documented in > > > > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > > > > > > > url: https://github.com/intel-lab-lkp/linux/commits/Avichal-Rakesh/usb-gadget-uvc-prevent-use-of-disabled-endpoint/20231020-025512 > > > > base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing > > > > patch link: https://lore.kernel.org/r/20231019185319.2714000-5-arakesh%40google.com > > > > patch subject: [PATCH v6 4/4] usb: gadget: uvc: Fix use-after-free for inflight usb_requests > > > > config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20231020/202310200457.GwPPFuHX-lkp@intel.com/config) > > > > compiler: m68k-linux-gcc (GCC) 13.2.0 > > > > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231020/202310200457.GwPPFuHX-lkp@intel.com/reproduce) > > > > > > > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > > > > the same patch/commit), kindly add following tags > > > > | Reported-by: kernel test robot <lkp@intel.com> > > > > | Closes: https://lore.kernel.org/oe-kbuild-all/202310200457.GwPPFuHX-lkp@intel.com/ > > > > > > > > All warnings (new ones prefixed by >>): > > > > > > > >>> drivers/usb/gadget/function/uvc_video.c:231: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst > > > > * Must be called with req_lock held as it modifies the list ureq is held in > > > > > > > > > > > > > > Greg, apologies for the newb question: do you want me to upload > > > the fix for this as a reply to [PATCH v6 4/4], or upload a new chain of > > > v7s with this patch fixed? > > > > A whole new v7 series please. > > > > Had a feeling, so sent out v7 series preemptively. Let me know if that > doesn't work. > > v7: https://lore.kernel.org/20231020173626.2978356-1-arakesh@google.com/ I have already dropped that from my review queue as your emails crossed with that, so I thought it was obsolete by now, sorry. Can you send a v8 please? thanks, greg k-h
On Tue, Oct 24, 2023 at 2:27 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Mon, Oct 23, 2023 at 02:25:30PM -0700, Avichal Rakesh wrote: > > On Sat, Oct 21, 2023 at 3:05 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > > > On Thu, Oct 19, 2023 at 03:30:00PM -0700, Avichal Rakesh wrote: > > > > > > > > > > > > On 10/19/23 13:32, kernel test robot wrote: > > > > > Hi Avichal, > > > > > > > > > > kernel test robot noticed the following build warnings: > > > > > > > > > > [auto build test WARNING on usb/usb-testing] > > > > > [also build test WARNING on usb/usb-next usb/usb-linus linus/master v6.6-rc6 next-20231019] > > > > > [If your patch is applied to the wrong git tree, kindly drop us a note. > > > > > And when submitting patch, we suggest to use '--base' as documented in > > > > > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > > > > > > > > > url: https://github.com/intel-lab-lkp/linux/commits/Avichal-Rakesh/usb-gadget-uvc-prevent-use-of-disabled-endpoint/20231020-025512 > > > > > base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing > > > > > patch link: https://lore.kernel.org/r/20231019185319.2714000-5-arakesh%40google.com > > > > > patch subject: [PATCH v6 4/4] usb: gadget: uvc: Fix use-after-free for inflight usb_requests > > > > > config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20231020/202310200457.GwPPFuHX-lkp@intel.com/config) > > > > > compiler: m68k-linux-gcc (GCC) 13.2.0 > > > > > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231020/202310200457.GwPPFuHX-lkp@intel.com/reproduce) > > > > > > > > > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > > > > > the same patch/commit), kindly add following tags > > > > > | Reported-by: kernel test robot <lkp@intel.com> > > > > > | Closes: https://lore.kernel.org/oe-kbuild-all/202310200457.GwPPFuHX-lkp@intel.com/ > > > > > > > > > > All warnings (new ones prefixed by >>): > > > > > > > > > >>> drivers/usb/gadget/function/uvc_video.c:231: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst > > > > > * Must be called with req_lock held as it modifies the list ureq is held in > > > > > > > > > > > > > > > > > > Greg, apologies for the newb question: do you want me to upload > > > > the fix for this as a reply to [PATCH v6 4/4], or upload a new chain of > > > > v7s with this patch fixed? > > > > > > A whole new v7 series please. > > > > > > > Had a feeling, so sent out v7 series preemptively. Let me know if that > > doesn't work. > > > > v7: https://lore.kernel.org/20231020173626.2978356-1-arakesh@google.com/ > > I have already dropped that from my review queue as your emails crossed > with that, so I thought it was obsolete by now, sorry. > > Can you send a v8 please? > Sent out v8! PTAL when you get the chance. https://lore.kernel.org/20231024183605.908253-1-arakesh@google.com/ Thank you! - Avi.
diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h index 993694da0bbc..be0d012aa244 100644 --- a/drivers/usb/gadget/function/uvc.h +++ b/drivers/usb/gadget/function/uvc.h @@ -102,6 +102,7 @@ struct uvc_video { unsigned int uvc_num_requests; /* Requests */ + bool is_enabled; /* tracks whether video stream is enabled */ unsigned int req_size; struct list_head ureqs; /* all uvc_requests allocated by uvc_video */ struct list_head req_free; diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c index 7cb8d027ff0c..f4d2e24835d4 100644 --- a/drivers/usb/gadget/function/uvc_v4l2.c +++ b/drivers/usb/gadget/function/uvc_v4l2.c @@ -451,8 +451,8 @@ uvc_v4l2_streamon(struct file *file, void *fh, enum v4l2_buf_type type) * Complete the alternate setting selection setup phase now that * userspace is ready to provide video frames. */ - uvc_function_setup_continue(uvc, 0); uvc->state = UVC_STATE_STREAMING; + uvc_function_setup_continue(uvc, 0); return 0; } @@ -468,11 +468,11 @@ uvc_v4l2_streamoff(struct file *file, void *fh, enum v4l2_buf_type type) if (type != video->queue.queue.type) return -EINVAL; - uvc->state = UVC_STATE_CONNECTED; ret = uvcg_video_enable(video, 0); if (ret < 0) return ret; + uvc->state = UVC_STATE_CONNECTED; uvc_function_setup_continue(uvc, 1); return 0; } @@ -507,14 +507,6 @@ uvc_v4l2_subscribe_event(struct v4l2_fh *fh, static void uvc_v4l2_disable(struct uvc_device *uvc) { uvc_function_disconnect(uvc); - /* - * Drop uvc->state to CONNECTED if it was streaming before. - * This ensures that the usb_requests are no longer queued - * to the controller. - */ - if (uvc->state == UVC_STATE_STREAMING) - uvc->state = UVC_STATE_CONNECTED; - uvcg_video_enable(&uvc->video, 0); uvcg_free_buffers(&uvc->video.queue); uvc->func_connected = false; diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c index 80b8eaea2d39..41fb4f24e829 100644 --- a/drivers/usb/gadget/function/uvc_video.c +++ b/drivers/usb/gadget/function/uvc_video.c @@ -227,6 +227,9 @@ uvc_video_encode_isoc(struct usb_request *req, struct uvc_video *video, * Request handling */ +/** + * Must be called with req_lock held as it modifies the list ureq is held in + */ static void uvc_video_free_request(struct uvc_request *ureq, struct usb_ep *ep) { @@ -271,9 +274,25 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req) struct uvc_request *ureq = req->context; struct uvc_video *video = ureq->video; struct uvc_video_queue *queue = &video->queue; - struct uvc_device *uvc = video->uvc; + struct uvc_buffer *last_buf = NULL; unsigned long flags; + spin_lock_irqsave(&video->req_lock, flags); + if (!video->is_enabled) { + /* + * When is_enabled is false, uvc_video_disable ensures that + * in-flight uvc_buffers are returned, so we can safely + * call free_request without worrying about last_buf. + */ + uvc_video_free_request(ureq, ep); + spin_unlock_irqrestore(&video->req_lock, flags); + return; + } + + last_buf = ureq->last_buf; + ureq->last_buf = NULL; + spin_unlock_irqrestore(&video->req_lock, flags); + switch (req->status) { case 0: break; @@ -295,17 +314,26 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req) uvcg_queue_cancel(queue, 0); } - if (ureq->last_buf) { - uvcg_complete_buffer(&video->queue, ureq->last_buf); - ureq->last_buf = NULL; + if (last_buf) { + spin_lock_irqsave(&queue->irqlock, flags); + uvcg_complete_buffer(&video->queue, last_buf); + spin_unlock_irqrestore(&queue->irqlock, flags); } spin_lock_irqsave(&video->req_lock, flags); - list_add_tail(&req->list, &video->req_free); - spin_unlock_irqrestore(&video->req_lock, flags); - - if (uvc->state == UVC_STATE_STREAMING) + /* + * Video stream might have been disabled while we were + * processing the current usb_request. So make sure + * we're still streaming before queueing the usb_request + * back to req_free + */ + if (video->is_enabled) { + list_add_tail(&req->list, &video->req_free); queue_work(video->async_wq, &video->pump); + } else { + uvc_video_free_request(ureq, ep); + } + spin_unlock_irqrestore(&video->req_lock, flags); } static int @@ -393,20 +421,22 @@ static void uvcg_video_pump(struct work_struct *work) struct uvc_video_queue *queue = &video->queue; /* video->max_payload_size is only set when using bulk transfer */ bool is_bulk = video->max_payload_size; - struct uvc_device *uvc = video->uvc; struct usb_request *req = NULL; struct uvc_buffer *buf; unsigned long flags; bool buf_done; int ret; - while (uvc->state == UVC_STATE_STREAMING && video->ep->enabled) { + while (true) { + if (!video->ep->enabled) + return; + /* - * Retrieve the first available USB request, protected by the - * request lock. + * Check is_enabled and retrieve the first available USB + * request, protected by the request lock. */ spin_lock_irqsave(&video->req_lock, flags); - if (list_empty(&video->req_free)) { + if (!video->is_enabled || list_empty(&video->req_free)) { spin_unlock_irqrestore(&video->req_lock, flags); return; } @@ -488,9 +518,11 @@ static void uvcg_video_pump(struct work_struct *work) return; spin_lock_irqsave(&video->req_lock, flags); - list_add_tail(&req->list, &video->req_free); + if (video->is_enabled) + list_add_tail(&req->list, &video->req_free); + else + uvc_video_free_request(req->context, video->ep); spin_unlock_irqrestore(&video->req_lock, flags); - return; } /* @@ -499,17 +531,64 @@ static void uvcg_video_pump(struct work_struct *work) static int uvcg_video_disable(struct uvc_video *video) { - struct uvc_request *ureq; + unsigned long flags; + struct list_head inflight_bufs; + struct usb_request *req, *temp; + struct uvc_buffer *buf, *btemp; + struct uvc_request *ureq, *utemp; + + INIT_LIST_HEAD(&inflight_bufs); + spin_lock_irqsave(&video->req_lock, flags); + video->is_enabled = false; + + /* + * Remove any in-flight buffers from the uvc_requests + * because we want to return them before cancelling the + * queue. This ensures that we aren't stuck waiting for + * all complete callbacks to come through before disabling + * vb2 queue. + */ + list_for_each_entry(ureq, &video->ureqs, list) { + if (ureq->last_buf) { + list_add_tail(&ureq->last_buf->queue, &inflight_bufs); + ureq->last_buf = NULL; + } + } + spin_unlock_irqrestore(&video->req_lock, flags); cancel_work_sync(&video->pump); uvcg_queue_cancel(&video->queue, 0); - list_for_each_entry(ureq, &video->ureqs, list) { - if (ureq->req) - usb_ep_dequeue(video->ep, ureq->req); + spin_lock_irqsave(&video->req_lock, flags); + /* + * Remove all uvc_reqeusts from ureqs with list_del_init + * This lets uvc_video_free_request correctly identify + * if the uvc_request is attached to a list or not when freeing + * memory. + */ + list_for_each_entry_safe(ureq, utemp, &video->ureqs, list) + list_del_init(&ureq->list); + + list_for_each_entry_safe(req, temp, &video->req_free, list) { + list_del(&req->list); + uvc_video_free_request(req->context, video->ep); } - uvc_video_free_requests(video); + INIT_LIST_HEAD(&video->ureqs); + INIT_LIST_HEAD(&video->req_free); + video->req_size = 0; + spin_unlock_irqrestore(&video->req_lock, flags); + + /* + * Return all the video buffers before disabling the queue. + */ + spin_lock_irqsave(&video->queue.irqlock, flags); + list_for_each_entry_safe(buf, btemp, &inflight_bufs, queue) { + list_del(&buf->queue); + uvcg_complete_buffer(&video->queue, buf); + } + spin_unlock_irqrestore(&video->queue.irqlock, flags); + uvcg_queue_enable(&video->queue, 0); return 0; } @@ -530,6 +609,14 @@ int uvcg_video_enable(struct uvc_video *video, int enable) if (!enable) return uvcg_video_disable(video); + /* + * Safe to access request related fields without req_lock because + * this is the only thread currently active, and no other + * request handling thread will become active until this function + * returns. + */ + video->is_enabled = true; + if ((ret = uvcg_queue_enable(&video->queue, 1)) < 0) return ret; @@ -555,6 +642,7 @@ int uvcg_video_enable(struct uvc_video *video, int enable) */ int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc) { + video->is_enabled = false; INIT_LIST_HEAD(&video->ureqs); INIT_LIST_HEAD(&video->req_free); spin_lock_init(&video->req_lock);