usb: gadget: uvc: restart fixes
Subsystems (3)
v2 cover usb: gadget: uvc: restart fixes
This series is improving the stability of the usb uvc gadget driver. On the unconditional event of a crash or intentional stop while using the uvc v4l2 userspace device and streaming to the host, the setup was sometimes running into use after free cases. We fix that.
Michael Grzeschik (3):
usb: gadget: uvc: stop pump thread on video disable
usb: gadget: uvc: cleanup request when not in correct state
usb: gadget: uvc: rework pump worker to avoid while loop
drivers/usb/gadget/function/uvc_video.c | 32 ++++++++++++++++++++----- 1 file changed, 26 insertions(+), 6 deletions(-)
Signature · 1 line
2.39.2
PATCH v2 1/3 usb: gadget: uvc: stop pump thread on video disable
Since the uvc-video gadget driver is using the v4l2 interface, the streamon and streamoff can be triggered at any times. To ensure that the pump worker will be closed as soon the userspace is calling streamoff we synchronize the state of the gadget ensuring the pump worker to bail out.
---
v1 -> v2: Fixed the missing uvc variable in uvcg_video_enable
drivers/usb/gadget/function/uvc_video.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
drivers/usb/gadget/function/uvc_video.c +7 -1
@@ -384,13 +384,14 @@ 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 (video->ep->enabled) { +while (video->ep->enabled && uvc->state ==UVC_STATE_STREAMING ) {/* * Retrieve the first available USB request, protected by the * request lock.
@@ -488,6 +489,7 @@ static void uvcg_video_pump(struct work_struct *work)*/ int uvcg_video_enable (struct uvc_video *video,int enable) { +struct uvc_device *uvc = video->uvc;unsigned int i;int ret;
@@ -498,6 +500,8 @@ int uvcg_video_enable(struct uvc_video *video, int enable) }if (!enable) { + uvc->state =UVC_STATE_CONNECTED ; +cancel_work_sync (&video->pump);uvcg_queue_cancel (&video->queue,0 );
@@ -523,6 +527,8 @@ int uvcg_video_enable(struct uvc_video *video, int enable) video->encode = video->queue.use_sg ? uvc_video_encode_isoc_sg : uvc_video_encode_isoc; + uvc->state =UVC_STATE_STREAMING ; + video->req_int_count =0 ;queue_work (video->async_wq, &video->pump);
Signature · 1 line
2.39.2
Re: [PATCH v2 1/3] usb: gadget: uvc: stop pump thread on video disable
Hi Michael,
Thank you for the patch.
Quoted 5 lines · Michael Grzeschik
Since the uvc-video gadget driver is using the v4l2 interface, the streamon and streamoff can be triggered at any times. To ensure that the pump worker will be closed as soon the userspace is calling streamoff we synchronize the state of the gadget ensuring the pump worker to bail out.
I'm sorry but I really dislike this. Not only does the patch fail to ensure real synchronization, as the uvcg_video_pump() function still runs asynchronously, it messes up the usage of the state field that now tracks the state both from a host point of view (which it was doing so far, updating the state based on callbacks from the UDC), and from a gadget userspace point of view. This lacks clarity and is confusing. Furthermore, the commit message doesn't even explain what issue is being fixed here.
Greg, I think this series has been merged too soon :-(
Quoted 45 lines
Signed-off-by Michael Grzeschik <[email protected]>---
v1 -> v2: Fixed the missing uvc variable inuvcg_video_enabledrivers/usb/gadget/function/uvc_video.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)drivers/usb/gadget/function/uvc_video.c +5
-1@@ -384,13 +384,14 @@ 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 (video->ep->enabled) { +while (video->ep->enabled && uvc->state ==UVC_STATE_STREAMING ) {/* * Retrieve the first available USB request, protected by the * request lock. @@ -488,6 +489,7 @@ static void uvcg_video_pump(struct work_struct *work)*/ int uvcg_video_enable (struct uvc_video *video,int enable) { +struct uvc_device *uvc = video->uvc;unsigned int i;int ret;@@ -498,6 +500,8 @@ int uvcg_video_enable(struct uvc_video *video, int enable) }if (!enable) { + uvc->state =UVC_STATE_CONNECTED ; +cancel_work_sync (&video->pump);uvcg_queue_cancel (&video->queue,0 );
drivers/usb/gadget/function/uvc_video.c int uvcg_video_enable(struct uvc_video *video, int enable) · line 531
Quoted 5 lines
drivers/usb/gadget/function/uvc_video.c +2
-0@@ -523,6 +527,8 @@ int uvcg_video_enable(struct uvc_video *video, int enable) video->encode = video->queue.use_sg ? uvc_video_encode_isoc_sg : uvc_video_encode_isoc; + uvc->state =UVC_STATE_STREAMING ; +
You're now setting the state to UVC_STATE_STREAMING both here and in uvc_v4l2_streamon(). That's confusing.
Quoted 3 lines
drivers/usb/gadget/function/uvc_video.c +0
-0video->req_int_count =0 ;queue_work (video->async_wq, &video->pump);
Signature · 3 lines
Regards, Laurent Pinchart
Greg KH
maintainerRe: [PATCH v2 1/3] usb: gadget: uvc: stop pump thread on video disable
Quoted 20 lines · Laurent Pinchart
Hi Michael,
Thank you for the patch.
Quoted 5 lines · Michael Grzeschik
Since the uvc-video gadget driver is using the v4l2 interface, the streamon and streamoff can be triggered at any times. To ensure that the pump worker will be closed as soon the userspace is calling streamoff we synchronize the state of the gadget ensuring the pump worker to bail out.
I'm sorry but I really dislike this. Not only does the patch fail to ensure real synchronization, as the
uvcg_video_pump()function still runs asynchronously, it messes up the usage of the state field that now tracks the state both from a host point of view (which it was doing so far, updating the state based on callbacks from the UDC), and from a gadget userspace point of view. This lacks clarity and is confusing. Furthermore, the commit message doesn't even explain what issue is being fixed here.
Quoted 1 line
Greg, I think this series has been merged too soon :-(
Ok, I'll go revert them now, thanks for the review.
greg k-h
Re: [PATCH v2 1/3] usb: gadget: uvc: stop pump thread on video disable
Quoted 21 lines · Greg KH
Quoted 20 lines · Laurent Pinchart
Hi Michael,
Thank you for the patch.
Quoted 5 lines · Michael Grzeschik
Since the uvc-video gadget driver is using the v4l2 interface, the streamon and streamoff can be triggered at any times. To ensure that the pump worker will be closed as soon the userspace is calling streamoff we synchronize the state of the gadget ensuring the pump worker to bail out.
I'm sorry but I really dislike this. Not only does the patch fail to ensure real synchronization, as the
uvcg_video_pump()function still runs asynchronously, it messes up the usage of the state field that now tracks the state both from a host point of view (which it was doing so far, updating the state based on callbacks from the UDC), and from a gadget userspace point of view. This lacks clarity and is confusing. Furthermore, the commit message doesn't even explain what issue is being fixed here.
Quoted 3 lines
Quoted 1 line
Greg, I think this series has been merged too soon :-(
Ok, I'll go revert them now, thanks for the review.
Or we can wait a day for Michael to reply, in case this can quickly be fixed on top for v6.7. I'm now reading on the loooon discussion from v1, and reviewing the other pending patches that try to tackle the same issue.
Signature · 3 lines
Regards, Laurent Pinchart
Greg KH
maintainerRe: [PATCH v2 1/3] usb: gadget: uvc: stop pump thread on video disable
Quoted 22 lines · Laurent Pinchart
Quoted 21 lines · Greg KH
Quoted 20 lines · Laurent Pinchart
Hi Michael,
Thank you for the patch.
Quoted 5 lines · Michael Grzeschik
Since the uvc-video gadget driver is using the v4l2 interface, the streamon and streamoff can be triggered at any times. To ensure that the pump worker will be closed as soon the userspace is calling streamoff we synchronize the state of the gadget ensuring the pump worker to bail out.
I'm sorry but I really dislike this. Not only does the patch fail to ensure real synchronization, as the
uvcg_video_pump()function still runs asynchronously, it messes up the usage of the state field that now tracks the state both from a host point of view (which it was doing so far, updating the state based on callbacks from the UDC), and from a gadget userspace point of view. This lacks clarity and is confusing. Furthermore, the commit message doesn't even explain what issue is being fixed here.
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
Greg, I think this series has been merged too soon :-(
Ok, I'll go revert them now, thanks for the review.
Or we can wait a day for Michael to reply, in case this can quickly be fixed on top for v6.7. I'm now reading on the loooon discussion from v1, and reviewing the other pending patches that try to tackle the same issue.
I'd rather take a patchset that everyone agrees with, reverting was easy and now done.
thanks,
greg k-h
PATCH v2 2/3 usb: gadget: uvc: cleanup request when not in correct state
In reply to Michael GrzeschikThe uvc_video_enable function of the uvc-gadget driver is dequeing and immediately deallocs all requests on its disable codepath. This is not save since the dequeue function is async and does not ensure that the requests are left unlinked in the controller driver.
By adding the ep_free_request into the completion path of the requests we ensure that the request will be properly deallocated.
---
v1 == v2
drivers/usb/gadget/function/uvc_video.c | 6 ++++++ 1 file changed, 6 insertions(+)
drivers/usb/gadget/function/uvc_video.c +6 -0
@@ -256,6 +256,12 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)struct uvc_device *uvc = video->uvc;unsigned long flags; +if (uvc->state ==UVC_STATE_CONNECTED ) { +usb_ep_free_request (video->ep, ureq->req); + ureq->req =NULL ; +return ; + } +switch (req->status) {case 0 :break ;
Signature · 1 line
2.39.2
Re: [PATCH v2 2/3] usb: gadget: uvc: cleanup request when not in correct state
Hi Michael,
Thank you for the patch.
Quoted 7 lines · Michael Grzeschik
The
uvc_video_enablefunction of the uvc-gadget driver is dequeing and immediately deallocs all requests on its disable codepath. This is not save since the dequeue function is async and does not ensure that the requests are left unlinked in the controller driver.By adding the
ep_free_requestinto the completion path of the requests we ensure that the request will be properly deallocated.
You're swapping one race condition for a different one. With this patch, request can now be double-freed.
I'll reply to the long discussion on v1 to try and find a proper solution. As for 1/3, I think this got merged too soon :-(
Quoted 24 lines
Signed-off-by Michael Grzeschik <[email protected]>---
v1 == v2drivers/usb/gadget/function/uvc_video.c | 6 ++++++ 1 file changed, 6 insertions(+)drivers/usb/gadget/function/uvc_video.c +6
-0@@ -256,6 +256,12 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)struct uvc_device *uvc = video->uvc;unsigned long flags; +if (uvc->state ==UVC_STATE_CONNECTED ) { +usb_ep_free_request (video->ep, ureq->req); + ureq->req =NULL ; +return ; + } +switch (req->status) {case 0 :break ;
Signature · 3 lines
Regards, Laurent Pinchart
PATCH v2 3/3 usb: gadget: uvc: rework pump worker to avoid while loop
In reply to Michael GrzeschikThe uvc_video_enable function is calling cancel_work_sync which will be blocking as long as new requests will be queued with the while loop. To ensure an earlier stop in the pumping loop in this particular case we rework the worker to requeue itself on every requests. Since the worker is already running prioritized, the scheduling overhad did not have real impact on the performance.
---
v1 == v2
drivers/usb/gadget/function/uvc_video.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)
drivers/usb/gadget/function/uvc_video.c +14 -6
@@ -397,7 +397,7 @@ static void uvcg_video_pump(struct work_struct *work)bool buf_done;int ret; -while (video->ep->enabled && uvc->state ==UVC_STATE_STREAMING ) { +if (video->ep->enabled && uvc->state ==UVC_STATE_STREAMING ) {/* * Retrieve the first available USB request, protected by the * request lock.
@@ -409,6 +409,11 @@ static void uvcg_video_pump(struct work_struct *work) } req = list_first_entry(&video->req_free, struct usb_request, list); +if (!req) { +spin_unlock_irqrestore (&video->req_lock, flags); +return ; + } +list_del (&req->list);spin_unlock_irqrestore (&video->req_lock, flags);
@@ -437,7 +442,7 @@ static void uvcg_video_pump(struct work_struct *work)* further. */ spin_unlock_irqrestore (&queue->irqlock, flags); -break ; + goto out; }/*
@@ -470,20 +475,23 @@ static void uvcg_video_pump(struct work_struct *work)/* Queue the USB request */ ret =uvcg_video_ep_queue (video, req);spin_unlock_irqrestore (&queue->irqlock, flags); -if (ret <0 ) {uvcg_queue_cancel (queue,0 ); -break ; + goto out; }/* Endpoint now owns the request */ req =NULL ; video->req_int_count++; + }else { +return ; } -if (!req) -return ; +if (uvc->state ==UVC_STATE_STREAMING ) +queue_work (video->async_wq, &video->pump); +return ; +out:spin_lock_irqsave (&video->req_lock, flags);list_add_tail (&req->list, &video->req_free);spin_unlock_irqrestore (&video->req_lock, flags);
Signature · 1 line
2.39.2