| Message ID | 20230930184821.310143-1-arakesh@google.com |
|---|---|
| Headers | show |
| Series | usb: gadget: uvc: stability fixes on STREAMOFF. | expand |
On Tue, Oct 03, 2023 at 01:09:06PM +0200, Michael Grzeschik wrote: > On Sat, Sep 30, 2023 at 11:48:18AM -0700, Avichal Rakesh wrote: > > We have been seeing two main stability issues that uvc gadget driver > > runs into when stopping streams: > > 1. Attempting to queue usb_requests to a disabled usb_ep > > 2. use-after-free issue for inflight usb_requests > > > > The three patches below fix the two issues above. Patch 1/3 fixes the > > first issue, and Patch 2/3 and 3/3 fix the second issue. > > > > Avichal Rakesh (3): > > usb: gadget: uvc: prevent use of disabled endpoint > > usb: gadget: uvc: Allocate uvc_requests one at a time > > usb: gadget: uvc: Fix use-after-free for inflight usb_requests > > > > drivers/usb/gadget/function/f_uvc.c | 11 +- > > drivers/usb/gadget/function/f_uvc.h | 2 +- > > drivers/usb/gadget/function/uvc.h | 6 +- > > drivers/usb/gadget/function/uvc_v4l2.c | 21 ++- > > drivers/usb/gadget/function/uvc_video.c | 189 +++++++++++++++++------- > > 5 files changed, 164 insertions(+), 65 deletions(-) > > These patches are not applying on gregkh/usb-testing since > Greg did take my patches first. I have already rebased them. I think they got merged too soon :-( We could fix things on top, but there's very little time to do so for v6.7. > In the updated version I the stack runs into the > following error, when enabling lockdep. Could you > try your version with lockdep enabled? > > [ 41.278520] configfs-gadget.vz gadget.0: uvc: reset UVC > [ 47.156261] configfs-gadget.vz gadget.0: uvc: uvc_function_set_alt(2, 0) > [ 47.169177] > [ 47.170903] ============================================ > [ 47.176857] WARNING: possible recursive locking detected > [ 47.182798] 6.5.0-20230919-1+ #19 Tainted: G C > [ 47.189323] -------------------------------------------- > [ 47.195256] vzuvcd/412 is trying to acquire lock: > [ 47.200511] ffffff8009560928 (&video->req_lock){....}-{3:3}, at: uvc_video_complete+0x44/0x2e0 > [ 47.210172] > [ 47.210172] but task is already holding lock: > [ 47.216687] ffffff8009560928 (&video->req_lock){....}-{3:3}, at: uvcg_video_enable+0x2d0/0x5c0 > [ 47.226333] > [ 47.226333] other info that might help us debug this: > [ 47.233625] Possible unsafe locking scenario: > [ 47.233625] > [ 47.240242] CPU0 > [ 47.242974] ---- > [ 47.245709] lock(&video->req_lock); > [ 47.249802] lock(&video->req_lock); > [ 47.253897] > [ 47.253897] *** DEADLOCK *** > [ 47.253897] > [ 47.260511] May be due to missing lock nesting notation > [ 47.260511]
Hi Laurent On Thu, Oct 05, 2023 at 11:23:27AM +0300, Laurent Pinchart wrote: >On Tue, Oct 03, 2023 at 01:09:06PM +0200, Michael Grzeschik wrote: >> On Sat, Sep 30, 2023 at 11:48:18AM -0700, Avichal Rakesh wrote: >> > We have been seeing two main stability issues that uvc gadget driver >> > runs into when stopping streams: >> > 1. Attempting to queue usb_requests to a disabled usb_ep >> > 2. use-after-free issue for inflight usb_requests >> > >> > The three patches below fix the two issues above. Patch 1/3 fixes the >> > first issue, and Patch 2/3 and 3/3 fix the second issue. >> > >> > Avichal Rakesh (3): >> > usb: gadget: uvc: prevent use of disabled endpoint >> > usb: gadget: uvc: Allocate uvc_requests one at a time >> > usb: gadget: uvc: Fix use-after-free for inflight usb_requests >> > >> > drivers/usb/gadget/function/f_uvc.c | 11 +- >> > drivers/usb/gadget/function/f_uvc.h | 2 +- >> > drivers/usb/gadget/function/uvc.h | 6 +- >> > drivers/usb/gadget/function/uvc_v4l2.c | 21 ++- >> > drivers/usb/gadget/function/uvc_video.c | 189 +++++++++++++++++------- >> > 5 files changed, 164 insertions(+), 65 deletions(-) >> >> These patches are not applying on gregkh/usb-testing since >> Greg did take my patches first. I have already rebased them. > >I think they got merged too soon :-( We could fix things on top, but >there's very little time to do so for v6.7. Agreed. I was jumping from one workaround to another one, since this is not easy to fix in a proper way. And still after this long discussion with Avichal I don't think we are there yet. So far the first two patches from Avichal look legit. But the overall Use-After-Free fix is yet to be done properly. The "abondoned" method he suggested is really bad to follow and will add too much complexity and will be hard to debug. IMHO it should be possible to introduce two cleanup pathes. One path would be in the uvc_cleanup_requests that will cleanup the requests that are actually not used in the controller and are registered in the req_free list. The second path would be the complete functions that are being run from the controller and will ensure that the cleanup will really free the requests from the controller after they were consumed. What do you think? Regards, Michael >> In the updated version I the stack runs into the >> following error, when enabling lockdep. Could you >> try your version with lockdep enabled? >> >> [ 41.278520] configfs-gadget.vz gadget.0: uvc: reset UVC >> [ 47.156261] configfs-gadget.vz gadget.0: uvc: uvc_function_set_alt(2, 0) >> [ 47.169177] >> [ 47.170903] ============================================ >> [ 47.176857] WARNING: possible recursive locking detected >> [ 47.182798] 6.5.0-20230919-1+ #19 Tainted: G C >> [ 47.189323] -------------------------------------------- >> [ 47.195256] vzuvcd/412 is trying to acquire lock: >> [ 47.200511] ffffff8009560928 (&video->req_lock){....}-{3:3}, at: uvc_video_complete+0x44/0x2e0 >> [ 47.210172] >> [ 47.210172] but task is already holding lock: >> [ 47.216687] ffffff8009560928 (&video->req_lock){....}-{3:3}, at: uvcg_video_enable+0x2d0/0x5c0 >> [ 47.226333] >> [ 47.226333] other info that might help us debug this: >> [ 47.233625] Possible unsafe locking scenario: >> [ 47.233625] >> [ 47.240242] CPU0 >> [ 47.242974] ---- >> [ 47.245709] lock(&video->req_lock); >> [ 47.249802] lock(&video->req_lock); >> [ 47.253897] >> [ 47.253897] *** DEADLOCK *** >> [ 47.253897] >> [ 47.260511] May be due to missing lock nesting notation >> [ 47.260511] > >-- >Regards, > >Laurent Pinchart >
On 10/5/23 00:11, Greg Kroah-Hartman wrote: > On Tue, Oct 03, 2023 at 04:16:00PM -0700, Avichal Rakesh wrote: >> Thank you for testing the patch, Michael! >> >> On 10/3/23 04:09, Michael Grzeschik wrote: >>> Hi >>> >>> On Sat, Sep 30, 2023 at 11:48:18AM -0700, Avichal Rakesh wrote: >>>> We have been seeing two main stability issues that uvc gadget driver >>>> runs into when stopping streams: >>>> 1. Attempting to queue usb_requests to a disabled usb_ep >>>> 2. use-after-free issue for inflight usb_requests >>>> >>>> The three patches below fix the two issues above. Patch 1/3 fixes the >>>> first issue, and Patch 2/3 and 3/3 fix the second issue. >>>> >>>> Avichal Rakesh (3): >>>> usb: gadget: uvc: prevent use of disabled endpoint >>>> usb: gadget: uvc: Allocate uvc_requests one at a time >>>> usb: gadget: uvc: Fix use-after-free for inflight usb_requests >>>> >>>> drivers/usb/gadget/function/f_uvc.c | 11 +- >>>> drivers/usb/gadget/function/f_uvc.h | 2 +- >>>> drivers/usb/gadget/function/uvc.h | 6 +- >>>> drivers/usb/gadget/function/uvc_v4l2.c | 21 ++- >>>> drivers/usb/gadget/function/uvc_video.c | 189 +++++++++++++++++------- >>>> 5 files changed, 164 insertions(+), 65 deletions(-) >>> >>> These patches are not applying on gregkh/usb-testing since >>> Greg did take my patches first. I have already rebased them. >> >> Ah, I didn't realize Greg had picked up your changes in his tree. >> Rebased the patches in V2. > > The "v2" series here is almost impossible to follow, sorry. > > Please send it as a new thread, not as responses to the individual > commits, how am I supposed to pick them up that way? > > And make it v3 please. Sent out v3 as a new thread. Sorry about that! - Avi.
On 10/5/23 03:14, Michael Grzeschik wrote: > Hi Laurent > > On Thu, Oct 05, 2023 at 11:23:27AM +0300, Laurent Pinchart wrote: >> On Tue, Oct 03, 2023 at 01:09:06PM +0200, Michael Grzeschik wrote: >>> On Sat, Sep 30, 2023 at 11:48:18AM -0700, Avichal Rakesh wrote: >>> > We have been seeing two main stability issues that uvc gadget driver >>> > runs into when stopping streams: >>> > 1. Attempting to queue usb_requests to a disabled usb_ep >>> > 2. use-after-free issue for inflight usb_requests >>> > >>> > The three patches below fix the two issues above. Patch 1/3 fixes the >>> > first issue, and Patch 2/3 and 3/3 fix the second issue. >>> > >>> > Avichal Rakesh (3): >>> > usb: gadget: uvc: prevent use of disabled endpoint >>> > usb: gadget: uvc: Allocate uvc_requests one at a time >>> > usb: gadget: uvc: Fix use-after-free for inflight usb_requests >>> > >>> > drivers/usb/gadget/function/f_uvc.c | 11 +- >>> > drivers/usb/gadget/function/f_uvc.h | 2 +- >>> > drivers/usb/gadget/function/uvc.h | 6 +- >>> > drivers/usb/gadget/function/uvc_v4l2.c | 21 ++- >>> > drivers/usb/gadget/function/uvc_video.c | 189 +++++++++++++++++------- >>> > 5 files changed, 164 insertions(+), 65 deletions(-) >>> >>> These patches are not applying on gregkh/usb-testing since >>> Greg did take my patches first. I have already rebased them. >> >> I think they got merged too soon :-( We could fix things on top, but >> there's very little time to do so for v6.7. > > Agreed. I was jumping from one workaround to another one, since this > is not easy to fix in a proper way. And still after this long discussion > with Avichal I don't think we are there yet. > > > So far the first two patches from Avichal look legit. But the overall > Use-After-Free fix is yet to be done properly. > > The "abondoned" method he suggested is really bad to follow and will > add too much complexity and will be hard to debug. > > IMHO it should be possible to introduce two cleanup pathes. > > One path would be in the uvc_cleanup_requests that will cleanup the > requests that are actually not used in the controller and are registered > in the req_free list. > > The second path would be the complete functions that are being run > from the controller and will ensure that the cleanup will really free > the requests from the controller after they were consumed. > > What do you think? I am not sure I follow. Patch 3/3 does exactly what you say here. There are two cleanup paths: 1. uvcg_video_disable cleans up only the requests in req_free, and 2. complete handler cleans up the in-flight requests. The "abandoned" flag is simply to let the completion handler know which requests to clean up and which ones to re-queue back to the gadget driver. The other "complications" are around making sure we can trust the values in an inherently racey situation. The reasoning can admittedly be difficult to follow at a glance, which incidentally is why I went with a simple to prove timed wait in the past (https://lore.kernel.org/20230912041910.726442-3-arakesh@google.com). I am not suggesting we go back to a timed wait, but please do look at the patch and let me know which parts don't make sense, or are difficult to understand. We can add more documentation about our assumptions there, or if you have a way to do this that you think is simpler to reason about, then please let me know and I'll be more than happy to use that! Regards, Avi.
Hi Avichal, On Thu, Oct 05, 2023 at 11:30:32AM -0700, Avichal Rakesh wrote: >On 10/5/23 03:14, Michael Grzeschik wrote: >> On Thu, Oct 05, 2023 at 11:23:27AM +0300, Laurent Pinchart wrote: >>> On Tue, Oct 03, 2023 at 01:09:06PM +0200, Michael Grzeschik wrote: >>>> On Sat, Sep 30, 2023 at 11:48:18AM -0700, Avichal Rakesh wrote: >>>> > We have been seeing two main stability issues that uvc gadget driver >>>> > runs into when stopping streams: >>>> > 1. Attempting to queue usb_requests to a disabled usb_ep >>>> > 2. use-after-free issue for inflight usb_requests >>>> > >>>> > The three patches below fix the two issues above. Patch 1/3 fixes the >>>> > first issue, and Patch 2/3 and 3/3 fix the second issue. >>>> > >>>> > Avichal Rakesh (3): >>>> > usb: gadget: uvc: prevent use of disabled endpoint >>>> > usb: gadget: uvc: Allocate uvc_requests one at a time >>>> > usb: gadget: uvc: Fix use-after-free for inflight usb_requests >>>> > >>>> > drivers/usb/gadget/function/f_uvc.c | 11 +- >>>> > drivers/usb/gadget/function/f_uvc.h | 2 +- >>>> > drivers/usb/gadget/function/uvc.h | 6 +- >>>> > drivers/usb/gadget/function/uvc_v4l2.c | 21 ++- >>>> > drivers/usb/gadget/function/uvc_video.c | 189 +++++++++++++++++------- >>>> > 5 files changed, 164 insertions(+), 65 deletions(-) >>>> >>>> These patches are not applying on gregkh/usb-testing since >>>> Greg did take my patches first. I have already rebased them. >>> >>> I think they got merged too soon :-( We could fix things on top, but >>> there's very little time to do so for v6.7. >> >> Agreed. I was jumping from one workaround to another one, since this >> is not easy to fix in a proper way. And still after this long discussion >> with Avichal I don't think we are there yet. >> >> >> So far the first two patches from Avichal look legit. But the overall >> Use-After-Free fix is yet to be done properly. >> >> The "abondoned" method he suggested is really bad to follow and will >> add too much complexity and will be hard to debug. >> >> IMHO it should be possible to introduce two cleanup pathes. >> >> One path would be in the uvc_cleanup_requests that will cleanup the >> requests that are actually not used in the controller and are registered >> in the req_free list. >> >> The second path would be the complete functions that are being run >> from the controller and will ensure that the cleanup will really free >> the requests from the controller after they were consumed. >> >> What do you think? > >I am not sure I follow. Patch 3/3 does exactly what you say here. Yes, it was just to summ up what the latest state of the idea was, so Laurent does not read the whole thread in detail. Sorry for not being clear enough about that. >There are two cleanup paths: > 1. uvcg_video_disable cleans up only the requests in req_free, and > 2. complete handler cleans up the in-flight requests. > >The "abandoned" flag is simply to let the completion handler know >which requests to clean up and which ones to re-queue back to >the gadget driver. What I don't get is, why in the case of shutdown there needs to be something re-queued back to the gadget driver. There should not need to be any sort of barrier flag for the requests. Just the complete handler running past a barrier where it knows that the whole device is stopped. So every call on complete should then clean that exact request it is touching currently. I don't know where the extra complexity comes from. >The other "complications" are around making sure we can trust >the values in an inherently racey situation. The reasoning >can admittedly be difficult to follow at a glance, which incidentally >is why I went with a simple to prove timed wait in the past >(https://lore.kernel.org/20230912041910.726442-3-arakesh@google.com). > >I am not suggesting we go back to a timed wait, but please do look >at the patch and let me know which parts don't make sense, or are >difficult to understand. We can add more documentation about our >assumptions there, or if you have a way to do this that you >think is simpler to reason about, then please let me know and I'll >be more than happy to use that! I really try to spin my head around the idea of the is_abondoned flag you are using. Unfortunatly for now I am out to debug the issues I see with your series. So I did try these patches you send. Yes the deadlock error is gone with v3. But the linked list is still running into cases where dwc3_gadget_giveback(complete) is touching requests that are already freed. [ 61.408715] ------------[ cut here ]------------ [ 61.413897] kernel BUG at lib/list_debug.c:56! ... [ 61.590762] Call trace: [ 61.596890] __list_del_entry_valid+0xb8/0xe8 [ 61.603408] dwc3_gadget_giveback+0x3c/0x1b0 [ 61.607594] dwc3_remove_requests.part.0+0xcc/0x100 [ 61.612948] __dwc3_gadget_ep_disable+0xbc/0x1b8 [ 61.621019] dwc3_gadget_ep_disable+0x48/0x100 [ 61.627925] usb_ep_disable+0x3c/0x138 [ 61.638230] uvc_function_setup_continue+0x3c/0x60 [ 61.645040] uvc_v4l2_streamoff+0x5c/0x80 [ 61.659812] v4l_streamoff+0x40/0x60 [ 61.668950] __video_do_ioctl+0x344/0x420 [ 61.679548] video_usercopy+0x1d0/0x788 [ 61.685677] video_ioctl2+0x40/0x70 [ 61.697439] v4l2_ioctl+0x68/0xa0 [ 61.709200] __arm64_sys_ioctl+0x304/0xda0 [ 61.720768] invoke_syscall.constprop.0+0x70/0x130 Regards, Michael
On 10/5/23 15:05, Michael Grzeschik wrote: > Hi Avichal, > > On Thu, Oct 05, 2023 at 11:30:32AM -0700, Avichal Rakesh wrote: >> On 10/5/23 03:14, Michael Grzeschik wrote: >>> On Thu, Oct 05, 2023 at 11:23:27AM +0300, Laurent Pinchart wrote: >>>> On Tue, Oct 03, 2023 at 01:09:06PM +0200, Michael Grzeschik wrote: >>>>> On Sat, Sep 30, 2023 at 11:48:18AM -0700, Avichal Rakesh wrote: >>>>> > We have been seeing two main stability issues that uvc gadget driver >>>>> > runs into when stopping streams: >>>>> > 1. Attempting to queue usb_requests to a disabled usb_ep >>>>> > 2. use-after-free issue for inflight usb_requests >>>>> > >>>>> > The three patches below fix the two issues above. Patch 1/3 fixes the >>>>> > first issue, and Patch 2/3 and 3/3 fix the second issue. >>>>> > >>>>> > Avichal Rakesh (3): >>>>> > usb: gadget: uvc: prevent use of disabled endpoint >>>>> > usb: gadget: uvc: Allocate uvc_requests one at a time >>>>> > usb: gadget: uvc: Fix use-after-free for inflight usb_requests >>>>> > >>>>> > drivers/usb/gadget/function/f_uvc.c | 11 +- >>>>> > drivers/usb/gadget/function/f_uvc.h | 2 +- >>>>> > drivers/usb/gadget/function/uvc.h | 6 +- >>>>> > drivers/usb/gadget/function/uvc_v4l2.c | 21 ++- >>>>> > drivers/usb/gadget/function/uvc_video.c | 189 +++++++++++++++++------- >>>>> > 5 files changed, 164 insertions(+), 65 deletions(-) >>>>> >>>>> These patches are not applying on gregkh/usb-testing since >>>>> Greg did take my patches first. I have already rebased them. >>>> >>>> I think they got merged too soon :-( We could fix things on top, but >>>> there's very little time to do so for v6.7. >>> >>> Agreed. I was jumping from one workaround to another one, since this >>> is not easy to fix in a proper way. And still after this long discussion >>> with Avichal I don't think we are there yet. >>> >>> >>> So far the first two patches from Avichal look legit. But the overall >>> Use-After-Free fix is yet to be done properly. >>> >>> The "abondoned" method he suggested is really bad to follow and will >>> add too much complexity and will be hard to debug. >>> >>> IMHO it should be possible to introduce two cleanup pathes. >>> >>> One path would be in the uvc_cleanup_requests that will cleanup the >>> requests that are actually not used in the controller and are registered >>> in the req_free list. >>> >>> The second path would be the complete functions that are being run >>> from the controller and will ensure that the cleanup will really free >>> the requests from the controller after they were consumed. >>> >>> What do you think? >> >> I am not sure I follow. Patch 3/3 does exactly what you say here. > > Yes, it was just to summ up what the latest state of the idea was, > so Laurent does not read the whole thread in detail. Sorry for not > being clear enough about that. Whoops! Sorry about the misunderstanding! > >> There are two cleanup paths: >> 1. uvcg_video_disable cleans up only the requests in req_free, and >> 2. complete handler cleans up the in-flight requests. >> >> The "abandoned" flag is simply to let the completion handler know >> which requests to clean up and which ones to re-queue back to >> the gadget driver. > > What I don't get is, why in the case of shutdown there needs to > be something re-queued back to the gadget driver. There should not > need to be any sort of barrier flag for the requests. Just the > complete handler running past a barrier where it knows that the > whole device is stopped. So every call on complete should then clean > that exact request it is touching currently. > > I don't know where the extra complexity comes from. A lot of this complexity comes from assuming a back to back STREAMOFF -> STREAMON sequence is possible where the gadget driver doesn't have the time to clean up all in-flight usb_requests. However, looking through the usb gadget APIs again, and it looks like usb_ep_disable enforces that all requests will be sent back to the gadget driver before it returns. So you're right: With Patch 1/3 in place, I think we can just guard on uvc->state alone, because control requests are blocked until usb_ep_disable is finished anyway. I'll upload v4 with the "is_abandoned" flag removed and the checks simplified once I've verified the fix locally. That should also remove any bookkeeping issues that may have triggered the stack below. Regards, Avi. > >> The other "complications" are around making sure we can trust >> the values in an inherently racey situation. The reasoning >> can admittedly be difficult to follow at a glance, which incidentally >> is why I went with a simple to prove timed wait in the past >> (https://lore.kernel.org/20230912041910.726442-3-arakesh@google.com). >> >> I am not suggesting we go back to a timed wait, but please do look >> at the patch and let me know which parts don't make sense, or are >> difficult to understand. We can add more documentation about our >> assumptions there, or if you have a way to do this that you >> think is simpler to reason about, then please let me know and I'll >> be more than happy to use that! > > I really try to spin my head around the idea of the is_abondoned flag > you are using. Unfortunatly for now I am out to debug the issues I see > with your series. > > So I did try these patches you send. Yes the deadlock error is gone with > v3. But the linked list is still running into cases where > dwc3_gadget_giveback(complete) is touching requests that are already > freed. > > [ 61.408715] ------------[ cut here ]------------ > [ 61.413897] kernel BUG at lib/list_debug.c:56! > ... > [ 61.590762] Call trace: > [ 61.596890] __list_del_entry_valid+0xb8/0xe8 > [ 61.603408] dwc3_gadget_giveback+0x3c/0x1b0 > [ 61.607594] dwc3_remove_requests.part.0+0xcc/0x100 > [ 61.612948] __dwc3_gadget_ep_disable+0xbc/0x1b8 > [ 61.621019] dwc3_gadget_ep_disable+0x48/0x100 > [ 61.627925] usb_ep_disable+0x3c/0x138 > [ 61.638230] uvc_function_setup_continue+0x3c/0x60 > [ 61.645040] uvc_v4l2_streamoff+0x5c/0x80 > [ 61.659812] v4l_streamoff+0x40/0x60 > [ 61.668950] __video_do_ioctl+0x344/0x420 > [ 61.679548] video_usercopy+0x1d0/0x788 > [ 61.685677] video_ioctl2+0x40/0x70 > [ 61.697439] v4l2_ioctl+0x68/0xa0 > [ 61.709200] __arm64_sys_ioctl+0x304/0xda0 > [ 61.720768] invoke_syscall.constprop.0+0x70/0x130 >
On Fri, Oct 06, 2023 at 10:00:11AM -0700, Avichal Rakesh wrote: > > >On 10/5/23 15:05, Michael Grzeschik wrote: >> Hi Avichal, >> >> On Thu, Oct 05, 2023 at 11:30:32AM -0700, Avichal Rakesh wrote: >>> On 10/5/23 03:14, Michael Grzeschik wrote: >>>> On Thu, Oct 05, 2023 at 11:23:27AM +0300, Laurent Pinchart wrote: >>>>> On Tue, Oct 03, 2023 at 01:09:06PM +0200, Michael Grzeschik wrote: >>>>>> On Sat, Sep 30, 2023 at 11:48:18AM -0700, Avichal Rakesh wrote: >>>>>> > We have been seeing two main stability issues that uvc gadget driver >>>>>> > runs into when stopping streams: >>>>>> > 1. Attempting to queue usb_requests to a disabled usb_ep >>>>>> > 2. use-after-free issue for inflight usb_requests >>>>>> > >>>>>> > The three patches below fix the two issues above. Patch 1/3 fixes the >>>>>> > first issue, and Patch 2/3 and 3/3 fix the second issue. >>>>>> > >>>>>> > Avichal Rakesh (3): >>>>>> > usb: gadget: uvc: prevent use of disabled endpoint >>>>>> > usb: gadget: uvc: Allocate uvc_requests one at a time >>>>>> > usb: gadget: uvc: Fix use-after-free for inflight usb_requests >>>>>> > >>>>>> > drivers/usb/gadget/function/f_uvc.c | 11 +- >>>>>> > drivers/usb/gadget/function/f_uvc.h | 2 +- >>>>>> > drivers/usb/gadget/function/uvc.h | 6 +- >>>>>> > drivers/usb/gadget/function/uvc_v4l2.c | 21 ++- >>>>>> > drivers/usb/gadget/function/uvc_video.c | 189 +++++++++++++++++------- >>>>>> > 5 files changed, 164 insertions(+), 65 deletions(-) >>>>>> >>>>>> These patches are not applying on gregkh/usb-testing since >>>>>> Greg did take my patches first. I have already rebased them. >>>>> >>>>> I think they got merged too soon :-( We could fix things on top, but >>>>> there's very little time to do so for v6.7. >>>> >>>> Agreed. I was jumping from one workaround to another one, since this >>>> is not easy to fix in a proper way. And still after this long discussion >>>> with Avichal I don't think we are there yet. >>>> >>>> >>>> So far the first two patches from Avichal look legit. But the overall >>>> Use-After-Free fix is yet to be done properly. >>>> >>>> The "abondoned" method he suggested is really bad to follow and will >>>> add too much complexity and will be hard to debug. >>>> >>>> IMHO it should be possible to introduce two cleanup pathes. >>>> >>>> One path would be in the uvc_cleanup_requests that will cleanup the >>>> requests that are actually not used in the controller and are registered >>>> in the req_free list. >>>> >>>> The second path would be the complete functions that are being run >>>> from the controller and will ensure that the cleanup will really free >>>> the requests from the controller after they were consumed. >>>> >>>> What do you think? >>> >>> I am not sure I follow. Patch 3/3 does exactly what you say here. >> >> Yes, it was just to summ up what the latest state of the idea was, >> so Laurent does not read the whole thread in detail. Sorry for not >> being clear enough about that. > >Whoops! Sorry about the misunderstanding! > >> >>> There are two cleanup paths: >>> 1. uvcg_video_disable cleans up only the requests in req_free, and >>> 2. complete handler cleans up the in-flight requests. >>> >>> The "abandoned" flag is simply to let the completion handler know >>> which requests to clean up and which ones to re-queue back to >>> the gadget driver. >> >> What I don't get is, why in the case of shutdown there needs to >> be something re-queued back to the gadget driver. There should not >> need to be any sort of barrier flag for the requests. Just the >> complete handler running past a barrier where it knows that the >> whole device is stopped. So every call on complete should then clean >> that exact request it is touching currently. >> >> I don't know where the extra complexity comes from. > >A lot of this complexity comes from assuming a back to back >STREAMOFF -> STREAMON sequence is possible where the gadget driver >doesn't have the time to clean up all in-flight usb_requests. >However, looking through the usb gadget APIs again, and it >looks like usb_ep_disable enforces that all requests will >be sent back to the gadget driver before it returns. Great! >So you're right: >With Patch 1/3 in place, I think we can just guard on uvc->state >alone, because control requests are blocked until usb_ep_disable >is finished anyway. I'll upload v4 with the "is_abandoned" >flag removed and the checks simplified once I've verified the >fix locally. > >That should also remove any bookkeeping issues that may have >triggered the stack below. I am currious if we should handle -ECONNRESET and -ESHUTDOWN in more detail in the completion handler and make sure that the request will not be added into the req_free list from there. Will review your v4 then. >>> The other "complications" are around making sure we can trust >>> the values in an inherently racey situation. The reasoning >>> can admittedly be difficult to follow at a glance, which incidentally >>> is why I went with a simple to prove timed wait in the past >>> (https://lore.kernel.org/20230912041910.726442-3-arakesh@google.com). >>> >>> I am not suggesting we go back to a timed wait, but please do look >>> at the patch and let me know which parts don't make sense, or are >>> difficult to understand. We can add more documentation about our >>> assumptions there, or if you have a way to do this that you >>> think is simpler to reason about, then please let me know and I'll >>> be more than happy to use that! >> >> I really try to spin my head around the idea of the is_abondoned flag >> you are using. Unfortunatly for now I am out to debug the issues I see >> with your series. >> >> So I did try these patches you send. Yes the deadlock error is gone with >> v3. But the linked list is still running into cases where >> dwc3_gadget_giveback(complete) is touching requests that are already >> freed. >> >> [ 61.408715] ------------[ cut here ]------------ >> [ 61.413897] kernel BUG at lib/list_debug.c:56! >> ... >> [ 61.590762] Call trace: >> [ 61.596890] __list_del_entry_valid+0xb8/0xe8 >> [ 61.603408] dwc3_gadget_giveback+0x3c/0x1b0 >> [ 61.607594] dwc3_remove_requests.part.0+0xcc/0x100 >> [ 61.612948] __dwc3_gadget_ep_disable+0xbc/0x1b8 >> [ 61.621019] dwc3_gadget_ep_disable+0x48/0x100 >> [ 61.627925] usb_ep_disable+0x3c/0x138 >> [ 61.638230] uvc_function_setup_continue+0x3c/0x60 >> [ 61.645040] uvc_v4l2_streamoff+0x5c/0x80 >> [ 61.659812] v4l_streamoff+0x40/0x60 >> [ 61.668950] __video_do_ioctl+0x344/0x420 >> [ 61.679548] video_usercopy+0x1d0/0x788 >> [ 61.685677] video_ioctl2+0x40/0x70 >> [ 61.697439] v4l2_ioctl+0x68/0xa0 >> [ 61.709200] __arm64_sys_ioctl+0x304/0xda0 >> [ 61.720768] invoke_syscall.constprop.0+0x70/0x130 >> > >
On 10/6/23 15:53, Michael Grzeschik wrote: > On Fri, Oct 06, 2023 at 10:00:11AM -0700, Avichal Rakesh wrote: >> >> >> On 10/5/23 15:05, Michael Grzeschik wrote: >>> Hi Avichal, >>> >>> On Thu, Oct 05, 2023 at 11:30:32AM -0700, Avichal Rakesh wrote: >>>> On 10/5/23 03:14, Michael Grzeschik wrote: >>>>> On Thu, Oct 05, 2023 at 11:23:27AM +0300, Laurent Pinchart wrote: >>>>>> On Tue, Oct 03, 2023 at 01:09:06PM +0200, Michael Grzeschik wrote: >>>>>>> On Sat, Sep 30, 2023 at 11:48:18AM -0700, Avichal Rakesh wrote: >>>>>>> > We have been seeing two main stability issues that uvc gadget driver >>>>>>> > runs into when stopping streams: >>>>>>> > 1. Attempting to queue usb_requests to a disabled usb_ep >>>>>>> > 2. use-after-free issue for inflight usb_requests >>>>>>> > >>>>>>> > The three patches below fix the two issues above. Patch 1/3 fixes the >>>>>>> > first issue, and Patch 2/3 and 3/3 fix the second issue. >>>>>>> > >>>>>>> > Avichal Rakesh (3): >>>>>>> > usb: gadget: uvc: prevent use of disabled endpoint >>>>>>> > usb: gadget: uvc: Allocate uvc_requests one at a time >>>>>>> > usb: gadget: uvc: Fix use-after-free for inflight usb_requests >>>>>>> > >>>>>>> > drivers/usb/gadget/function/f_uvc.c | 11 +- >>>>>>> > drivers/usb/gadget/function/f_uvc.h | 2 +- >>>>>>> > drivers/usb/gadget/function/uvc.h | 6 +- >>>>>>> > drivers/usb/gadget/function/uvc_v4l2.c | 21 ++- >>>>>>> > drivers/usb/gadget/function/uvc_video.c | 189 +++++++++++++++++------- >>>>>>> > 5 files changed, 164 insertions(+), 65 deletions(-) >>>>>>> >>>>>>> These patches are not applying on gregkh/usb-testing since >>>>>>> Greg did take my patches first. I have already rebased them. >>>>>> >>>>>> I think they got merged too soon :-( We could fix things on top, but >>>>>> there's very little time to do so for v6.7. >>>>> >>>>> Agreed. I was jumping from one workaround to another one, since this >>>>> is not easy to fix in a proper way. And still after this long discussion >>>>> with Avichal I don't think we are there yet. >>>>> >>>>> >>>>> So far the first two patches from Avichal look legit. But the overall >>>>> Use-After-Free fix is yet to be done properly. >>>>> >>>>> The "abondoned" method he suggested is really bad to follow and will >>>>> add too much complexity and will be hard to debug. >>>>> >>>>> IMHO it should be possible to introduce two cleanup pathes. >>>>> >>>>> One path would be in the uvc_cleanup_requests that will cleanup the >>>>> requests that are actually not used in the controller and are registered >>>>> in the req_free list. >>>>> >>>>> The second path would be the complete functions that are being run >>>>> from the controller and will ensure that the cleanup will really free >>>>> the requests from the controller after they were consumed. >>>>> >>>>> What do you think? >>>> >>>> I am not sure I follow. Patch 3/3 does exactly what you say here. >>> >>> Yes, it was just to summ up what the latest state of the idea was, >>> so Laurent does not read the whole thread in detail. Sorry for not >>> being clear enough about that. >> >> Whoops! Sorry about the misunderstanding! >> >>> >>>> There are two cleanup paths: >>>> 1. uvcg_video_disable cleans up only the requests in req_free, and >>>> 2. complete handler cleans up the in-flight requests. >>>> >>>> The "abandoned" flag is simply to let the completion handler know >>>> which requests to clean up and which ones to re-queue back to >>>> the gadget driver. >>> >>> What I don't get is, why in the case of shutdown there needs to >>> be something re-queued back to the gadget driver. There should not >>> need to be any sort of barrier flag for the requests. Just the >>> complete handler running past a barrier where it knows that the >>> whole device is stopped. So every call on complete should then clean >>> that exact request it is touching currently. >>> >>> I don't know where the extra complexity comes from. >> >> A lot of this complexity comes from assuming a back to back >> STREAMOFF -> STREAMON sequence is possible where the gadget driver >> doesn't have the time to clean up all in-flight usb_requests. >> However, looking through the usb gadget APIs again, and it >> looks like usb_ep_disable enforces that all requests will >> be sent back to the gadget driver before it returns. > > Great! Uhh...apologies, I will have to take this back. I've been trying to use uvc->state as the condition for when completion handler should clean up usb_requests, and I cannot figure out a way to do so cleanly. The fundamental problem with using uvc->state is that it is not protected by any locks. So there is no real way to assert that its value has not changed between reading uvc->state and acting on it. Naively we can write something like the following in the completion handler: void uvc_video_complete(...) { if (uvc->state != UVC_EVENT_STREAMING) { usb_ep_free_request(....); } else { // handle usb_request normally } } But without any locks, there are no guarantees that uvc->state didn't mutate immediately after the if condition was checked, and the complete handler is handling a request that it should've freed instead or vice-versa. This argument would hold for any logic we guard with uvc->state, making uvc->state effectively useless as a check for freeing memory. We can work around it by either 1. Locking uvc->state with some driver level lock to ensure that we can trust the value of uvc->state at least for a little while, or 2. Using some other barrier condition that is protected by another lock If we go with (1), we'd have to add a lock around every and every write to uvc->state, which isn't terrible, but would require more testing to ensure that it doesn't create any new deadlocks. For (2), with the realization that usb_ep_disable flushes all requests, we can add a barrier in uvc_video, protected by req_lock. That should simplify the logic a little bit and will hopefully be easier to reason about. I could of course be missing a simpler solution here, and am happy to be wrong. So please let me know if you have any other ideas on how to guarantee such a check. > >> So you're right: >> With Patch 1/3 in place, I think we can just guard on uvc->state >> alone, because control requests are blocked until usb_ep_disable >> is finished anyway. I'll upload v4 with the "is_abandoned" >> flag removed and the checks simplified once I've verified the >> fix locally. >> >> That should also remove any bookkeeping issues that may have >> triggered the stack below. > > I am currious if we should handle -ECONNRESET and -ESHUTDOWN in more > detail in the completion handler and make sure that the request will not > be added into the req_free list from there. > > Will review your v4 then. Appreciate the reviews, thank you! > >>>> The other "complications" are around making sure we can trust >>>> the values in an inherently racey situation. The reasoning >>>> can admittedly be difficult to follow at a glance, which incidentally >>>> is why I went with a simple to prove timed wait in the past >>>> (https://lore.kernel.org/20230912041910.726442-3-arakesh@google.com). >>>> >>>> I am not suggesting we go back to a timed wait, but please do look >>>> at the patch and let me know which parts don't make sense, or are >>>> difficult to understand. We can add more documentation about our >>>> assumptions there, or if you have a way to do this that you >>>> think is simpler to reason about, then please let me know and I'll >>>> be more than happy to use that! >>> >>> I really try to spin my head around the idea of the is_abondoned flag >>> you are using. Unfortunatly for now I am out to debug the issues I see >>> with your series. >>> >>> So I did try these patches you send. Yes the deadlock error is gone with >>> v3. But the linked list is still running into cases where >>> dwc3_gadget_giveback(complete) is touching requests that are already >>> freed. >>> >>> [ 61.408715] ------------[ cut here ]------------ >>> [ 61.413897] kernel BUG at lib/list_debug.c:56! >>> ... >>> [ 61.590762] Call trace: >>> [ 61.596890] __list_del_entry_valid+0xb8/0xe8 >>> [ 61.603408] dwc3_gadget_giveback+0x3c/0x1b0 >>> [ 61.607594] dwc3_remove_requests.part.0+0xcc/0x100 >>> [ 61.612948] __dwc3_gadget_ep_disable+0xbc/0x1b8 >>> [ 61.621019] dwc3_gadget_ep_disable+0x48/0x100 >>> [ 61.627925] usb_ep_disable+0x3c/0x138 >>> [ 61.638230] uvc_function_setup_continue+0x3c/0x60 >>> [ 61.645040] uvc_v4l2_streamoff+0x5c/0x80 >>> [ 61.659812] v4l_streamoff+0x40/0x60 >>> [ 61.668950] __video_do_ioctl+0x344/0x420 >>> [ 61.679548] video_usercopy+0x1d0/0x788 >>> [ 61.685677] video_ioctl2+0x40/0x70 >>> [ 61.697439] v4l2_ioctl+0x68/0xa0 >>> [ 61.709200] __arm64_sys_ioctl+0x304/0xda0 >>> [ 61.720768] invoke_syscall.constprop.0+0x70/0x130 Just to confirm: this stack was with all 3 patches applied? Want to make sure this won't happen with v4. Regards, Avi.
On Fri, Oct 06, 2023 at 04:48:19PM -0700, Avichal Rakesh wrote: >On 10/6/23 15:53, Michael Grzeschik wrote: >> On Fri, Oct 06, 2023 at 10:00:11AM -0700, Avichal Rakesh wrote: >>> >>> >>> On 10/5/23 15:05, Michael Grzeschik wrote: >>>> Hi Avichal, >>>> >>>> On Thu, Oct 05, 2023 at 11:30:32AM -0700, Avichal Rakesh wrote: >>>>> On 10/5/23 03:14, Michael Grzeschik wrote: >>>>>> On Thu, Oct 05, 2023 at 11:23:27AM +0300, Laurent Pinchart wrote: >>>>>>> On Tue, Oct 03, 2023 at 01:09:06PM +0200, Michael Grzeschik wrote: >>>>>>>> On Sat, Sep 30, 2023 at 11:48:18AM -0700, Avichal Rakesh wrote: >>>>>>>> > We have been seeing two main stability issues that uvc gadget driver >>>>>>>> > runs into when stopping streams: >>>>>>>> > 1. Attempting to queue usb_requests to a disabled usb_ep >>>>>>>> > 2. use-after-free issue for inflight usb_requests >>>>>>>> > >>>>>>>> > The three patches below fix the two issues above. Patch 1/3 fixes the >>>>>>>> > first issue, and Patch 2/3 and 3/3 fix the second issue. >>>>>>>> > >>>>>>>> > Avichal Rakesh (3): >>>>>>>> > usb: gadget: uvc: prevent use of disabled endpoint >>>>>>>> > usb: gadget: uvc: Allocate uvc_requests one at a time >>>>>>>> > usb: gadget: uvc: Fix use-after-free for inflight usb_requests >>>>>>>> > >>>>>>>> > drivers/usb/gadget/function/f_uvc.c | 11 +- >>>>>>>> > drivers/usb/gadget/function/f_uvc.h | 2 +- >>>>>>>> > drivers/usb/gadget/function/uvc.h | 6 +- >>>>>>>> > drivers/usb/gadget/function/uvc_v4l2.c | 21 ++- >>>>>>>> > drivers/usb/gadget/function/uvc_video.c | 189 +++++++++++++++++------- >>>>>>>> > 5 files changed, 164 insertions(+), 65 deletions(-) >>>>>>>> >>>>>>>> These patches are not applying on gregkh/usb-testing since >>>>>>>> Greg did take my patches first. I have already rebased them. >>>>>>> >>>>>>> I think they got merged too soon :-( We could fix things on top, but >>>>>>> there's very little time to do so for v6.7. >>>>>> >>>>>> Agreed. I was jumping from one workaround to another one, since this >>>>>> is not easy to fix in a proper way. And still after this long discussion >>>>>> with Avichal I don't think we are there yet. >>>>>> >>>>>> >>>>>> So far the first two patches from Avichal look legit. But the overall >>>>>> Use-After-Free fix is yet to be done properly. >>>>>> >>>>>> The "abondoned" method he suggested is really bad to follow and will >>>>>> add too much complexity and will be hard to debug. >>>>>> >>>>>> IMHO it should be possible to introduce two cleanup pathes. >>>>>> >>>>>> One path would be in the uvc_cleanup_requests that will cleanup the >>>>>> requests that are actually not used in the controller and are registered >>>>>> in the req_free list. >>>>>> >>>>>> The second path would be the complete functions that are being run >>>>>> from the controller and will ensure that the cleanup will really free >>>>>> the requests from the controller after they were consumed. >>>>>> >>>>>> What do you think? >>>>> >>>>> I am not sure I follow. Patch 3/3 does exactly what you say here. >>>> >>>> Yes, it was just to summ up what the latest state of the idea was, >>>> so Laurent does not read the whole thread in detail. Sorry for not >>>> being clear enough about that. >>> >>> Whoops! Sorry about the misunderstanding! >>> >>>> >>>>> There are two cleanup paths: >>>>> 1. uvcg_video_disable cleans up only the requests in req_free, and >>>>> 2. complete handler cleans up the in-flight requests. >>>>> >>>>> The "abandoned" flag is simply to let the completion handler know >>>>> which requests to clean up and which ones to re-queue back to >>>>> the gadget driver. >>>> >>>> What I don't get is, why in the case of shutdown there needs to >>>> be something re-queued back to the gadget driver. There should not >>>> need to be any sort of barrier flag for the requests. Just the >>>> complete handler running past a barrier where it knows that the >>>> whole device is stopped. So every call on complete should then clean >>>> that exact request it is touching currently. >>>> >>>> I don't know where the extra complexity comes from. >>> >>> A lot of this complexity comes from assuming a back to back >>> STREAMOFF -> STREAMON sequence is possible where the gadget driver >>> doesn't have the time to clean up all in-flight usb_requests. >>> However, looking through the usb gadget APIs again, and it >>> looks like usb_ep_disable enforces that all requests will >>> be sent back to the gadget driver before it returns. >> >> Great! > >Uhh...apologies, I will have to take this back. I've been >trying to use uvc->state as the condition for when completion >handler should clean up usb_requests, and I cannot figure >out a way to do so cleanly. > >The fundamental problem with using uvc->state is that it is >not protected by any locks. So there is no real way to >assert that its value has not changed between reading >uvc->state and acting on it. > >Naively we can write something like the following in the >completion handler: > >void uvc_video_complete(...) { > if (uvc->state != UVC_EVENT_STREAMING) { > usb_ep_free_request(....); > } else { > // handle usb_request normally > } >} > >But without any locks, there are no guarantees that >uvc->state didn't mutate immediately after the if >condition was checked, and the complete handler is >handling a request that it should've freed instead >or vice-versa. This argument would hold for any logic >we guard with uvc->state, making uvc->state effectively >useless as a check for freeing memory. Yes, this makes total sense. Since the above condition was also part of the wait_event patch you created in the first place, I bet this issue was there aswell and was probably causing the issues I saw while testing it. >We can work around it by either >1. Locking uvc->state with some driver level lock > to ensure that we can trust the value of uvc->state > at least for a little while, or >2. Using some other barrier condition that is protected by > another lock > >If we go with (1), we'd have to add a lock around every >and every write to uvc->state, which isn't terrible, but >would require more testing to ensure that it doesn't >create any new deadlocks. > >For (2), with the realization that usb_ep_disable flushes >all requests, we can add a barrier in uvc_video, protected by >req_lock. That should simplify the logic a little bit and >will hopefully be easier to reason about. > >I could of course be missing a simpler solution here, >and am happy to be wrong. So please let me know if you >have any other ideas on how to guarantee such a check. For now, I have no better Idea. Idea (2) sounds like a good compromise. But I will have to review that code to really judge. Thanks for the work! Michael
On 10/8/23 12:48, Michael Grzeschik wrote: > On Fri, Oct 06, 2023 at 04:48:19PM -0700, Avichal Rakesh wrote: >> On 10/6/23 15:53, Michael Grzeschik wrote: >>> On Fri, Oct 06, 2023 at 10:00:11AM -0700, Avichal Rakesh wrote: >>>> >>>> >>>> On 10/5/23 15:05, Michael Grzeschik wrote: >>>>> Hi Avichal, >>>>> >>>>> On Thu, Oct 05, 2023 at 11:30:32AM -0700, Avichal Rakesh wrote: >>>>>> On 10/5/23 03:14, Michael Grzeschik wrote: >>>>> <snip> >>>>> I don't know where the extra complexity comes from. >>>> >>>> A lot of this complexity comes from assuming a back to back >>>> STREAMOFF -> STREAMON sequence is possible where the gadget driver >>>> doesn't have the time to clean up all in-flight usb_requests. >>>> However, looking through the usb gadget APIs again, and it >>>> looks like usb_ep_disable enforces that all requests will >>>> be sent back to the gadget driver before it returns. >>> >>> Great! >> >> Uhh...apologies, I will have to take this back. I've been >> trying to use uvc->state as the condition for when completion >> handler should clean up usb_requests, and I cannot figure >> out a way to do so cleanly. >> >> The fundamental problem with using uvc->state is that it is >> not protected by any locks. So there is no real way to >> assert that its value has not changed between reading >> uvc->state and acting on it. >> >> Naively we can write something like the following in the >> completion handler: >> >> void uvc_video_complete(...) { >> if (uvc->state != UVC_EVENT_STREAMING) { >> usb_ep_free_request(....); >> } else { >> // handle usb_request normally >> } >> } >> >> But without any locks, there are no guarantees that >> uvc->state didn't mutate immediately after the if >> condition was checked, and the complete handler is >> handling a request that it should've freed instead >> or vice-versa. This argument would hold for any logic >> we guard with uvc->state, making uvc->state effectively >> useless as a check for freeing memory. > > Yes, this makes total sense. Since the above condition was also part of > the wait_event patch you created in the first place, I bet this issue > was there aswell and was probably causing the issues I saw while testing > it> > >> We can work around it by either >> 1. Locking uvc->state with some driver level lock >> to ensure that we can trust the value of uvc->state >> at least for a little while, or >> 2. Using some other barrier condition that is protected by >> another lock >> >> If we go with (1), we'd have to add a lock around every >> and every write to uvc->state, which isn't terrible, but >> would require more testing to ensure that it doesn't >> create any new deadlocks. >> >> For (2), with the realization that usb_ep_disable flushes >> all requests, we can add a barrier in uvc_video, protected by >> req_lock. That should simplify the logic a little bit and >> will hopefully be easier to reason about. >> >> I could of course be missing a simpler solution here, >> and am happy to be wrong. So please let me know if you >> have any other ideas on how to guarantee such a check. > > For now, I have no better Idea. Idea (2) sounds like > a good compromise. But I will have to review that code > to really judge. > Sent out v4 patches with option (2). It simplifies the logic decently because we no longer have to reason about per-request consistency. uvc_video now tracks its own state of whether requests should be flowing or not based on calls to uvcg_video_enable. This state is protected, and is the source of truth for queueing usb_requests. The last bit of complexity left is around returning in-flight video buffers. AFAICT it should now be protected, and in my local testing I didn't notice any un-returned buffers, but please to take a look and let me know if your testing uncovers anything. Thanks, Avi.
On Wed, Oct 11, 2023 at 05:24:51PM -0700, Avichal Rakesh wrote: >Currently, the uvc gadget driver allocates all uvc_requests as one array >and deallocates them all when the video stream stops. This includes >de-allocating all the usb_requests associated with those uvc_requests. >This can lead to use-after-free issues if any of those de-allocated >usb_requests were still owned by the usb controller. > >This is patch 2 of 2 in fixing the use-after-free issue. It adds a new >flag to uvc_video to track when frames and requests should be flowing. >When disabling the video stream, the flag is tripped and, instead >of de-allocating all uvc_requests and usb_requests, the gadget >driver only de-allocates those usb_requests that are currently >owned by it (as present in req_free). Other usb_requests are left >untouched until their completion handler is called which takes care >of freeing the usb_request and its corresponding uvc_request. > >Now that uvc_video does not depends on uvc->state, this patch removes >unnecessary upates to uvc->state that were made to accomodate uvc_video >logic. This should ensure that uvc gadget driver never accidentally >de-allocates a usb_request that it doesn't own. > >Link: https://lore.kernel.org/7cd81649-2795-45b6-8c10-b7df1055020d@google.com >Suggested-by: Michael Grzeschik <m.grzeschik@pengutronix.de> >Signed-off-by: Avichal Rakesh <arakesh@google.com> >--- >v1 -> v2: Rebased to ToT, and fixed deadlock reported in > https://lore.kernel.org/all/ZRv2UnKztgyqk2pt@pengutronix.de/ >v2 -> v3: Fix email threading goof-up >v3 -> v4: re-rebase to ToT & moved to a uvc_video level lock > as discussed in > https://lore.kernel.org/b14b296f-2e08-4edf-aeea-1c5b621e2d0c@google.com/ I tested this and I no longer saw any use after free errors anymore! :) Here comes some more review: > drivers/usb/gadget/function/uvc.h | 1 + > drivers/usb/gadget/function/uvc_v4l2.c | 12 +- > drivers/usb/gadget/function/uvc_video.c | 156 +++++++++++++++++++----- > 3 files changed, 128 insertions(+), 41 deletions(-) > >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 c0d77564a204..ded7d33c2a52 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,12 +468,12 @@ 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; > } >@@ -508,14 +508,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 b62b3de79153..05b89b5b6c48 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 >@@ -391,20 +419,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) { Missing space after "while". >+ 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; > } >@@ -486,9 +516,78 @@ 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); >+} >+ >+/* >+ * Disable video stream >+ */ >+static int >+uvcg_video_disable(struct uvc_video *video) { >+ 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); >- return; >+ >+ cancel_work_sync(&video->pump); >+ uvcg_queue_cancel(&video->queue, 0); >+ >+ spin_lock_irqsave(&video->req_lock, flags); >+ /* >+ * Remove all uvc_reqeusts from 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); >+ } >+ >+ 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; > } > > /* >@@ -497,28 +596,22 @@ static void uvcg_video_pump(struct work_struct *work) > int uvcg_video_enable(struct uvc_video *video, int enable) > { > int ret; >- struct uvc_request *ureq; > > if (video->ep == NULL) { > uvcg_info(&video->uvc->func, > "Video enable failed, device is uninitialized.\n"); > return -ENODEV; > } >- >- if (!enable) { >- 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); >- } >- >- uvc_video_free_requests(video); >- uvcg_queue_enable(&video->queue, 0); >- return 0; >- } >- >+ if (!enable) >+ return uvcg_video_disable(video); Could you refactor this code as it is to an separate function and prepand this change as an extra patch to this one? It would make the changes in the functions more obvious and better to review. >+ >+ /* >+ * 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; Add an extra empty line. > if ((ret = uvcg_queue_enable(&video->queue, 1)) < 0) > return ret; > >@@ -544,6 +637,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); >-- >2.42.0.609.gbb76f46606-goog > > Reviewed-by: Michael Grzeschik <m.grzeschik@pengutronix.de> Tested-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
On 10/18/23 06:10, Michael Grzeschik wrote: > On Wed, Oct 11, 2023 at 05:24:51PM -0700, Avichal Rakesh wrote: >> Currently, the uvc gadget driver allocates all uvc_requests as one array >> and deallocates them all when the video stream stops. This includes >> de-allocating all the usb_requests associated with those uvc_requests. >> This can lead to use-after-free issues if any of those de-allocated >> usb_requests were still owned by the usb controller. >> >> This is patch 2 of 2 in fixing the use-after-free issue. It adds a new >> flag to uvc_video to track when frames and requests should be flowing. >> When disabling the video stream, the flag is tripped and, instead >> of de-allocating all uvc_requests and usb_requests, the gadget >> driver only de-allocates those usb_requests that are currently >> owned by it (as present in req_free). Other usb_requests are left >> untouched until their completion handler is called which takes care >> of freeing the usb_request and its corresponding uvc_request. >> >> Now that uvc_video does not depends on uvc->state, this patch removes >> unnecessary upates to uvc->state that were made to accomodate uvc_video >> logic. This should ensure that uvc gadget driver never accidentally >> de-allocates a usb_request that it doesn't own. >> >> Link: https://lore.kernel.org/7cd81649-2795-45b6-8c10-b7df1055020d@google.com >> Suggested-by: Michael Grzeschik <m.grzeschik@pengutronix.de> >> Signed-off-by: Avichal Rakesh <arakesh@google.com> >> --- >> v1 -> v2: Rebased to ToT, and fixed deadlock reported in >> https://lore.kernel.org/all/ZRv2UnKztgyqk2pt@pengutronix.de/ >> v2 -> v3: Fix email threading goof-up >> v3 -> v4: re-rebase to ToT & moved to a uvc_video level lock >> as discussed in >> https://lore.kernel.org/b14b296f-2e08-4edf-aeea-1c5b621e2d0c@google.com/ > > I tested this and I no longer saw any use after free > errors anymore! :) Yay! Glad to hear! > > Here comes some more review: > >> drivers/usb/gadget/function/uvc.h | 1 + >> drivers/usb/gadget/function/uvc_v4l2.c | 12 +- >> drivers/usb/gadget/function/uvc_video.c | 156 +++++++++++++++++++----- >> 3 files changed, 128 insertions(+), 41 deletions(-) >> >> + >> +/* >> + * Disable video stream >> + */ >> +static int >> +uvcg_video_disable(struct uvc_video *video) { >> + 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); >> - return; >> + >> + cancel_work_sync(&video->pump); >> + uvcg_queue_cancel(&video->queue, 0); >> + >> + spin_lock_irqsave(&video->req_lock, flags); >> + /* >> + * Remove all uvc_reqeusts from 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); >> + } >> + >> + 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; >> } >> >> /* >> @@ -497,28 +596,22 @@ static void uvcg_video_pump(struct work_struct *work) >> int uvcg_video_enable(struct uvc_video *video, int enable) >> { >> int ret; >> - struct uvc_request *ureq; >> >> if (video->ep == NULL) { >> uvcg_info(&video->uvc->func, >> "Video enable failed, device is uninitialized.\n"); >> return -ENODEV; >> } >> - >> - if (!enable) { >> - 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); >> - } >> - >> - uvc_video_free_requests(video); >> - uvcg_queue_enable(&video->queue, 0); >> - return 0; >> - } >> - >> + if (!enable) >> + return uvcg_video_disable(video); > > Could you refactor this code as it is to an separate > function and prepand this change as an extra patch > to this one? It would make the changes in the functions > more obvious and better to review. Sure I can send a follow up patch, but I am curious why you think this needs to be a separate function? Refactoring into a function would have the functions structured something like: uvcg_video_disable(video) { // ... // disable impl // ... } uvcg_video_enable(video) { // ... // enable impl // ... } uvcg_video_enable(video, enable) { // ep test if (!enable) return uvcg_video_disable(video); return uvc_video_enable(video); } instead of the current structure: uvcg_video_disable(video) { // ... // disable impl // ... } uvcg_video_enable(video, enable) { // ep test if (!enable) return uvcg_video_disable(video); // ... // enable impl // ... } I am not sure if one is more readable than the other. > >> + >> + /* >> + * 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; > > Add an extra empty line. > >> if ((ret = uvcg_queue_enable(&video->queue, 1)) < 0) >> return ret; >> >> @@ -544,6 +637,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); >> -- >> 2.42.0.609.gbb76f46606-goog >> >> > > Reviewed-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > Tested-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > >
On Wed, Oct 18, 2023 at 02:50:08PM -0700, Avichal Rakesh wrote: > > >On 10/18/23 06:10, Michael Grzeschik wrote: >> On Wed, Oct 11, 2023 at 05:24:51PM -0700, Avichal Rakesh wrote: >>> Currently, the uvc gadget driver allocates all uvc_requests as one array >>> and deallocates them all when the video stream stops. This includes >>> de-allocating all the usb_requests associated with those uvc_requests. >>> This can lead to use-after-free issues if any of those de-allocated >>> usb_requests were still owned by the usb controller. >>> >>> This is patch 2 of 2 in fixing the use-after-free issue. It adds a new >>> flag to uvc_video to track when frames and requests should be flowing. >>> When disabling the video stream, the flag is tripped and, instead >>> of de-allocating all uvc_requests and usb_requests, the gadget >>> driver only de-allocates those usb_requests that are currently >>> owned by it (as present in req_free). Other usb_requests are left >>> untouched until their completion handler is called which takes care >>> of freeing the usb_request and its corresponding uvc_request. >>> >>> Now that uvc_video does not depends on uvc->state, this patch removes >>> unnecessary upates to uvc->state that were made to accomodate uvc_video >>> logic. This should ensure that uvc gadget driver never accidentally >>> de-allocates a usb_request that it doesn't own. >>> >>> Link: https://lore.kernel.org/7cd81649-2795-45b6-8c10-b7df1055020d@google.com >>> Suggested-by: Michael Grzeschik <m.grzeschik@pengutronix.de> >>> Signed-off-by: Avichal Rakesh <arakesh@google.com> >>> --- >>> v1 -> v2: Rebased to ToT, and fixed deadlock reported in >>> https://lore.kernel.org/all/ZRv2UnKztgyqk2pt@pengutronix.de/ >>> v2 -> v3: Fix email threading goof-up >>> v3 -> v4: re-rebase to ToT & moved to a uvc_video level lock >>> as discussed in >>> https://lore.kernel.org/b14b296f-2e08-4edf-aeea-1c5b621e2d0c@google.com/ >> >> I tested this and I no longer saw any use after free >> errors anymore! :) > >Yay! Glad to hear! > >> >> Here comes some more review: >> >>> drivers/usb/gadget/function/uvc.h | 1 + >>> drivers/usb/gadget/function/uvc_v4l2.c | 12 +- >>> drivers/usb/gadget/function/uvc_video.c | 156 +++++++++++++++++++----- >>> 3 files changed, 128 insertions(+), 41 deletions(-) >>> > >>> + >>> +/* >>> + * Disable video stream >>> + */ >>> +static int >>> +uvcg_video_disable(struct uvc_video *video) { >>> + 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); >>> - return; >>> + >>> + cancel_work_sync(&video->pump); >>> + uvcg_queue_cancel(&video->queue, 0); >>> + >>> + spin_lock_irqsave(&video->req_lock, flags); >>> + /* >>> + * Remove all uvc_reqeusts from 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); >>> + } >>> + >>> + 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; >>> } >>> >>> /* >>> @@ -497,28 +596,22 @@ static void uvcg_video_pump(struct work_struct *work) >>> int uvcg_video_enable(struct uvc_video *video, int enable) >>> { >>> int ret; >>> - struct uvc_request *ureq; >>> >>> if (video->ep == NULL) { >>> uvcg_info(&video->uvc->func, >>> "Video enable failed, device is uninitialized.\n"); >>> return -ENODEV; >>> } >>> - >>> - if (!enable) { >>> - 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); >>> - } >>> - >>> - uvc_video_free_requests(video); >>> - uvcg_queue_enable(&video->queue, 0); >>> - return 0; >>> - } >>> - >>> + if (!enable) >>> + return uvcg_video_disable(video); >> >> Could you refactor this code as it is to an separate >> function and prepand this change as an extra patch >> to this one? It would make the changes in the functions >> more obvious and better to review. > >Sure I can send a follow up patch, but I am curious why you think this >needs to be a separate function? Refactoring into a function would >have the functions structured something like: > >uvcg_video_disable(video) { > // ... > // disable impl > // ... >} > >uvcg_video_enable(video) { > // ... > // enable impl > // ... >} > >uvcg_video_enable(video, enable) { > // ep test > > if (!enable) > return uvcg_video_disable(video); > > return uvc_video_enable(video); >} > >instead of the current structure: > >uvcg_video_disable(video) { > // ... > // disable impl > // ... >} > >uvcg_video_enable(video, enable) { > // ep test > > if (!enable) > return uvcg_video_disable(video); > > // ... > // enable impl > // ... >} > >I am not sure if one is more readable than the other. I think you misunderstood. The second structure is all right. What I did want you to do is as follows. Lets look at your series: patch 0/3 patch 1/3 patch 2/3 <--- add a patch here that does the refactoring of the separate function uvcg_video_disable without changing the functional content of it: uvcg_video_disable(video) { // ... // disable impl // ... } uvcg_video_enable(video, enable) { // ep test if (!enable) return uvcg_video_disable(video); // ... // enable impl // ... } patch 3/3 This way in the patch 3/3 the functional changes you introduce to the uvcg_video_diable will get better to review. Regards, Michael
On 10/18/23 15:06, Michael Grzeschik wrote: > On Wed, Oct 18, 2023 at 02:50:08PM -0700, Avichal Rakesh wrote: >> >> >> On 10/18/23 06:10, Michael Grzeschik wrote: >>> On Wed, Oct 11, 2023 at 05:24:51PM -0700, Avichal Rakesh wrote: >>>> Currently, the uvc gadget driver allocates all uvc_requests as one array >>>> and deallocates them all when the video stream stops. This includes >>>> de-allocating all the usb_requests associated with those uvc_requests. >>>> This can lead to use-after-free issues if any of those de-allocated >>>> usb_requests were still owned by the usb controller. >>>> >>>> This is patch 2 of 2 in fixing the use-after-free issue. It adds a new >>>> flag to uvc_video to track when frames and requests should be flowing. >>>> When disabling the video stream, the flag is tripped and, instead >>>> of de-allocating all uvc_requests and usb_requests, the gadget >>>> driver only de-allocates those usb_requests that are currently >>>> owned by it (as present in req_free). Other usb_requests are left >>>> untouched until their completion handler is called which takes care >>>> of freeing the usb_request and its corresponding uvc_request. >>>> >>>> Now that uvc_video does not depends on uvc->state, this patch removes >>>> unnecessary upates to uvc->state that were made to accomodate uvc_video >>>> logic. This should ensure that uvc gadget driver never accidentally >>>> de-allocates a usb_request that it doesn't own. >>>> >>>> Link: https://lore.kernel.org/7cd81649-2795-45b6-8c10-b7df1055020d@google.com >>>> Suggested-by: Michael Grzeschik <m.grzeschik@pengutronix.de> >>>> Signed-off-by: Avichal Rakesh <arakesh@google.com> >>>> --- >>>> v1 -> v2: Rebased to ToT, and fixed deadlock reported in >>>> https://lore.kernel.org/all/ZRv2UnKztgyqk2pt@pengutronix.de/ >>>> v2 -> v3: Fix email threading goof-up >>>> v3 -> v4: re-rebase to ToT & moved to a uvc_video level lock >>>> as discussed in >>>> https://lore.kernel.org/b14b296f-2e08-4edf-aeea-1c5b621e2d0c@google.com/ >>> >>> I tested this and I no longer saw any use after free >>> errors anymore! :) >> >> Yay! Glad to hear! >> >>> >>> Here comes some more review: >>> >>>> drivers/usb/gadget/function/uvc.h | 1 + >>>> drivers/usb/gadget/function/uvc_v4l2.c | 12 +- >>>> drivers/usb/gadget/function/uvc_video.c | 156 +++++++++++++++++++----- >>>> 3 files changed, 128 insertions(+), 41 deletions(-) >>>> >> >>>> + >>>> +/* >>>> + * Disable video stream >>>> + */ >>>> +static int >>>> +uvcg_video_disable(struct uvc_video *video) { >>>> + 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); >>>> - return; >>>> + >>>> + cancel_work_sync(&video->pump); >>>> + uvcg_queue_cancel(&video->queue, 0); >>>> + >>>> + spin_lock_irqsave(&video->req_lock, flags); >>>> + /* >>>> + * Remove all uvc_reqeusts from 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); >>>> + } >>>> + >>>> + 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; >>>> } >>>> >>>> /* >>>> @@ -497,28 +596,22 @@ static void uvcg_video_pump(struct work_struct *work) >>>> int uvcg_video_enable(struct uvc_video *video, int enable) >>>> { >>>> int ret; >>>> - struct uvc_request *ureq; >>>> >>>> if (video->ep == NULL) { >>>> uvcg_info(&video->uvc->func, >>>> "Video enable failed, device is uninitialized.\n"); >>>> return -ENODEV; >>>> } >>>> - >>>> - if (!enable) { >>>> - 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); >>>> - } >>>> - >>>> - uvc_video_free_requests(video); >>>> - uvcg_queue_enable(&video->queue, 0); >>>> - return 0; >>>> - } >>>> - >>>> + if (!enable) >>>> + return uvcg_video_disable(video); >>> >>> Could you refactor this code as it is to an separate >>> function and prepand this change as an extra patch >>> to this one? It would make the changes in the functions >>> more obvious and better to review. >> >> Sure I can send a follow up patch, but I am curious why you think this >> needs to be a separate function? Refactoring into a function would >> have the functions structured something like: >> >> uvcg_video_disable(video) { >> // ... >> // disable impl >> // ... >> } >> >> uvcg_video_enable(video) { >> // ... >> // enable impl >> // ... >> } >> >> uvcg_video_enable(video, enable) { >> // ep test >> >> if (!enable) >> return uvcg_video_disable(video); >> >> return uvc_video_enable(video); >> } >> >> instead of the current structure: >> >> uvcg_video_disable(video) { >> // ... >> // disable impl >> // ... >> } >> >> uvcg_video_enable(video, enable) { >> // ep test >> >> if (!enable) >> return uvcg_video_disable(video); >> >> // ... >> // enable impl >> // ... >> } >> >> I am not sure if one is more readable than the other. > > I think you misunderstood. The second structure is all right. > > What I did want you to do is as follows. > > Lets look at your series: > > patch 0/3 > patch 1/3 > patch 2/3 > > <--- add a patch here that does the refactoring of the separate > function uvcg_video_disable without changing the functional > content of it: > > uvcg_video_disable(video) { > // ... > // disable impl > // ... > } > > uvcg_video_enable(video, enable) { > // ep test > > if (!enable) > return uvcg_video_disable(video); > > // ... > // enable impl > // ... > } > > patch 3/3 > > This way in the patch 3/3 the functional changes you introduce to the > uvcg_video_diable will get better to review. I see! I did indeed misunderstand. Sent out v6 with 4 patches! Thank you! - Avi.
On 10/19/23 11:53, Avichal Rakesh wrote: > We have been seeing two main stability issues that uvc gadget driver > runs into when stopping streams: > 1. Attempting to queue usb_requests to a disabled usb_ep > 2. use-after-free issue for inflight usb_requests > > The four patches below fix the two issues above. Patch 1/4 fixes the > first issue, and Patch 2/4 and 4/4 fix the second issue. Patch 3/4 > is only there to make the diff in 4/4 cleaner. > > Avichal Rakesh (4): > usb: gadget: uvc: prevent use of disabled endpoint > usb: gadget: uvc: Allocate uvc_requests one at a time > usb: gadget: uvc: move video disable logic to its own function > usb: gadget: uvc: Fix use-after-free for inflight usb_requests > > drivers/usb/gadget/function/f_uvc.c | 11 +- > drivers/usb/gadget/function/f_uvc.h | 2 +- > drivers/usb/gadget/function/uvc.h | 6 +- > drivers/usb/gadget/function/uvc_v4l2.c | 12 +- > drivers/usb/gadget/function/uvc_video.c | 231 +++++++++++++++++------- > 5 files changed, 189 insertions(+), 73 deletions(-) > > -- > 2.42.0.758.gaed0368e0e-goog Dan and Laurent, please go over the patches whenever you get a chance. I think they're ready to submit as neither Michael nor I have seen any use-after-free issues after the patches. Thank you! - Avi.
Hi Avichal On 27/10/2023 21:19, Avichal Rakesh wrote: > This patch refactors the video disable logic in uvcg_video_enable > into its own separate function 'uvcg_video_disable'. This function > is now used anywhere uvcg_video_enable(video, 0) was used. > > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> For this patch you can keep the R-b - it's fine by me now :) > Suggested-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > Signed-off-by: Avichal Rakesh <arakesh@google.com> > --- > v6: Introduced this patch to make the next one easier to review > v6 -> v7: Add Suggested-by > v7 -> v8: No change. Getting back in review queue > v8 -> v9: Call uvcg_video_disable directly instead of uvcg_video_enable(video, 0) > > drivers/usb/gadget/function/uvc_v4l2.c | 6 ++-- > drivers/usb/gadget/function/uvc_video.c | 40 ++++++++++++++++--------- > drivers/usb/gadget/function/uvc_video.h | 3 +- > 3 files changed, 31 insertions(+), 18 deletions(-) > > diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c > index 7cb8d027ff0c..904dd283cbf7 100644 > --- a/drivers/usb/gadget/function/uvc_v4l2.c > +++ b/drivers/usb/gadget/function/uvc_v4l2.c > @@ -443,7 +443,7 @@ uvc_v4l2_streamon(struct file *file, void *fh, enum v4l2_buf_type type) > return -EINVAL; > > /* Enable UVC video. */ > - ret = uvcg_video_enable(video, 1); > + ret = uvcg_video_enable(video); > if (ret < 0) > return ret; > > @@ -469,7 +469,7 @@ uvc_v4l2_streamoff(struct file *file, void *fh, enum v4l2_buf_type type) > return -EINVAL; > > uvc->state = UVC_STATE_CONNECTED; > - ret = uvcg_video_enable(video, 0); > + ret = uvcg_video_disable(video); > if (ret < 0) > return ret; > > @@ -515,7 +515,7 @@ static void uvc_v4l2_disable(struct uvc_device *uvc) > if (uvc->state == UVC_STATE_STREAMING) > uvc->state = UVC_STATE_CONNECTED; > > - uvcg_video_enable(&uvc->video, 0); > + uvcg_video_disable(&uvc->video); > uvcg_free_buffers(&uvc->video.queue); > uvc->func_connected = false; > wake_up_interruptible(&uvc->func_connected_queue); > diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c > index f8f9209fee50..1081dd790fd6 100644 > --- a/drivers/usb/gadget/function/uvc_video.c > +++ b/drivers/usb/gadget/function/uvc_video.c > @@ -494,31 +494,43 @@ static void uvcg_video_pump(struct work_struct *work) > } > > /* > - * Enable or disable the video stream. > + * Disable the video stream > */ > -int uvcg_video_enable(struct uvc_video *video, int enable) > +int > +uvcg_video_disable(struct uvc_video *video) > { > - int ret; > struct uvc_request *ureq; > > if (video->ep == NULL) { > uvcg_info(&video->uvc->func, > - "Video enable failed, device is uninitialized.\n"); > + "Video disable failed, device is uninitialized.\n"); > return -ENODEV; > } > > - if (!enable) { > - cancel_work_sync(&video->pump); > - uvcg_queue_cancel(&video->queue, 0); > + 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); > - } > + list_for_each_entry(ureq, &video->ureqs, list) { > + if (ureq->req) > + usb_ep_dequeue(video->ep, ureq->req); > + } > > - uvc_video_free_requests(video); > - uvcg_queue_enable(&video->queue, 0); > - return 0; > + uvc_video_free_requests(video); > + uvcg_queue_enable(&video->queue, 0); > + return 0; > +} > + > +/* > + * Enable the video stream. > + */ > +int uvcg_video_enable(struct uvc_video *video) > +{ > + int ret; > + > + if (video->ep == NULL) { > + uvcg_info(&video->uvc->func, > + "Video enable failed, device is uninitialized.\n"); > + return -ENODEV; > } > > if ((ret = uvcg_queue_enable(&video->queue, 1)) < 0) > diff --git a/drivers/usb/gadget/function/uvc_video.h b/drivers/usb/gadget/function/uvc_video.h > index 03adeefa343b..8ef6259741f1 100644 > --- a/drivers/usb/gadget/function/uvc_video.h > +++ b/drivers/usb/gadget/function/uvc_video.h > @@ -14,7 +14,8 @@ > > struct uvc_video; > > -int uvcg_video_enable(struct uvc_video *video, int enable); > +int uvcg_video_enable(struct uvc_video *video); > +int uvcg_video_disable(struct uvc_video *video); > > int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc); > > -- > 2.42.0.820.g83a721a137-goog