Re: [PATCH 2/3] usb: gadget: uvc: cleanup request when not in correct state

[Avichal Rakesh <arakesh@google.com>]

On 9/19/23 13:07, Michael Grzeschik wrote:
> On Tue, Sep 19, 2023 at 12:55:02PM -0700, Avichal Rakesh wrote:
>> On 9/19/23 12:13, Michael Grzeschik wrote:
>>> On Mon, Sep 18, 2023 at 04:40:07PM -0700, Avichal Rakesh wrote:
>>>>
>>>>
>>>> On 9/18/23 14:43, Michael Grzeschik wrote:
>>>>> On Mon, Sep 18, 2023 at 12:02:11PM -0700, Avichal Rakesh wrote:
>>>>>> On 9/16/23 16:23, Michael Grzeschik wrote:
>>>>>>> On Fri, Sep 15, 2023 at 07:41:05PM -0700, Avichal Rakesh wrote:
>>>>>>>> On 9/15/23 16:32, Michael Grzeschik wrote:
>>>>>>>>> On Mon, Sep 11, 2023 at 09:52:22PM -0700, Avichal Rakesh wrote:
>>>>>>>>>> On 9/10/23 17:24, Michael Grzeschik wrote:
>>>>>>>>>>> The 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.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>>>>>>>>>>> ---
>>>>>>>>>>>  drivers/usb/gadget/function/uvc_video.c | 6 ++++++
>>>>>>>>>>>  1 file changed, 6 insertions(+)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
>>>>>>>>>>> index 4b6e854e30c58c..52e3666b51f743 100644
>>>>>>>>>>> --- a/drivers/usb/gadget/function/uvc_video.c
>>>>>>>>>>> +++ b/drivers/usb/gadget/function/uvc_video.c
>>>>>>>>>>> @@ -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);
>>>>>>>>>> nit: You can probably just call usb_ep_free_request with req instead of ureq->req.
>>>>>>>>>
>>>>>>>>> Thanks, thats a good point.
>>>>>>>>>
>>>>>>>>>>> +        ureq->req = NULL;
>>>>>>>>>>> +        return;
>>>>>>>>>>> +    }
>>>>>>>>>>> +
>>>>>>>>>>>      switch (req->status) {
>>>>>>>>>>>      case 0:
>>>>>>>>>>>          break;
>>>>>>>>>>
>>>>>>>>>> Perhaps I am missing something here, but I am not sure how this alone
>>>>>>>>>> fixes the use-after-free issue. uvcg_video_enable still deallocates
>>>>>>>>>> _all_ usb_requests right after calling usb_ep_dequeue, so it is still
>>>>>>>>>> possible that an unreturned request is deallocated, and now it is
>>>>>>>>>> possible that the complete callback accesses a deallocated ureq :(
>>>>>>>>>
>>>>>>>>> Since the issue I saw was usually coming from the list_del_entry_valid check in
>>>>>>>>> the list_del_entry of the giveback function, the issue was probably just not
>>>>>>>>> triggered anymore as the complete function did exit early.
>>>>>>>>>
>>>>>>>>> So this fix alone is actually bogus without a second patch I had in the stack.
>>>>>>>>> The second patch I am refering should change the actual overall issue:
>>>>>>>>>
>>>>>>>>> https://lore.kernel.org/linux-usb/20230915233113.2903645-1-m.grzeschik@pengutronix.de/T/#u
>>>>>>>>>
>>>>>>>>> This early list_del and this patch here should ensure that the
>>>>>>>>> concurrent functions are not handling already freed memory.
>>>>>>>>
>>>>>>>> Oh, the patch linked above is interesting. It effectively force removes the dwc3_request
>>>>>>>> from whatever list it belongs to? So if DWC3's interrupt handler is delayed past
>>>>>>>> UVC gadget's ep_free_request call, then it won't see the requests in its cancelled
>>>>>>>> list at all. However, this setup is still prone to errors. For example, there is now
>>>>>>>> a chance that gadget_ep_free_request is called twice for one request. A scheduling
>>>>>>>> like the following might cause double kfree:
>>>>>>>>
>>>>>>>> 1. uvcg_video_enable calls usb_ep_dequeue for all usb_requests
>>>>>>>> 2. While the usb_ep_dequeues are being processed, dwc3's interrupt handler starts
>>>>>>>>   calling the complete callbacks.
>>>>>>>> 3. The complete callback calls gadget_ep_free_request (calling kfree as a result)
>>>>>>>> 4. Meanwhile, uvcg_video_enable has moved to uvc_video_free_requests which also
>>>>>>>>   calls gadget_ep_free_request (calling kfree).
>>>>>>>>
>>>>>>>> There is currently (even in your patches) no synchronization between calls to
>>>>>>>> gadget_ep_free_request via complete callback and uvcg_video_enable, which will
>>>>>>>> inevitably call usb_ep_free_request twice for one request.
>>>>>>>>
>>>>>>>> Does that make sense, or am I misunderstanding some part of the patch?
>>>>>>>
>>>>>>> The overall concept is correct. But in detail the
>>>>>>> uvc_video_free_requests is checking that video->ureq[i].req is not NULL.
>>>>>>>
>>>>>>> With our previous call of ep_free_request in the complete handler, the
>>>>>>> ureq->req pointer in focus was already set to NULL. So the
>>>>>>> uvc_video_free_requests function will skip that extra free.
>>>>>>>
>>>>>>
>>>>>> Is there any form of synchronization between uvc_video_request and the
>>>>>> complete callback? As I see it, the dwc3 interrupt thread and the v4l2
>>>>>> ioctl thread (which calls uvcg_video_enable) are fully independent, so
>>>>>> the calls made by them are free to be interleaved arbitrarily, so an
>>>>>> interleaving like this is technically possible:
>>>>>>
>>>>>> +------+------------------------------------+---------------------------------------------+
>>>>>> | time |            ioctl_thread            |            dwc3 interrupt handler           |
>>>>>> +======+====================================+=============================================+
>>>>>> |   1  | -uvc_v4l2_streamoff                |                                             |
>>>>>> |   2  | |-uvcg_video_enable                |                                             |
>>>>>> |   3  | ||-usb_ep_dequeue                  |                                             |
>>>>>> |   4  | ||                                 | -dwc3_process_event_buf                     |
>>>>>> |   5  | ||-uvc_video_free_requests         | |                                           |
>>>>>> |   6  | |||                                | |-dwc3_gadget_ep_cleanup_cancelled_requests |
>>>>>> |   7  | |||                                | ||-dwc3_gadget_giveback                     |
>>>>>> |   8  | |||                                | |||-uvc_video_complete                      |
>>>>>> |   9  | |||-check ureq->req != NULL [true] | ||||                                        |
>>>>>> |  10  | ||||-usb_ep_free_request           | ||||                                        |
>>>>>> |  11  | |||||-dwc3_ep_free_request         | ||||                                        |
>>>>>> |  12  | ||||||-kfree [first call]          | ||||                                        |
>>>>>> |  13  | ||||                               | ||||-usb_ep_free_request                    |
>>>>>> |  14  | ||||                               | |||||-dwc3_ep_free_request                  |
>>>>>> |  15  | ||||                               | ||||||-kfree [second call]                  |
>>>>>> |  16  | ||||                               | ||||-set ureq->req = NULL                   |
>>>>>> |  17  | ||||-set ureq->req = NULL          |                                             |
>>>>>> +------+------------------------------------+---------------------------------------------+
>>>>>>
>>>>>> A situation like this means that dwc3_ep_free_request can be called
>>>>>> twice for a particular usb_request. This is obviously low probability,
>>>>>> but a race condition here means we'll start seeing very vague and hard
>>>>>> to repro crashes or memory inconsistencies when using the uvc gadget.
>>>>>>
>>>>>> I do apologize if I've missed something obvious with your changes that
>>>>>> prevents such interleaving. I don't currently see any locking or
>>>>>> other synchronization mechanism in your changes. Is there something
>>>>>> in dwc3 that prevents this situation?
>>>>>
>>>>> I think you have pointed it out totally clear. This is obviously the
>>>>> case. It just did not trigger here. But the window is there and has to
>>>>> be locked in some way.
>>>>>
>>>>> For now we have two options to solve it.
>>>>>
>>>>> 1) Trying to avoid this double code path of the complete callback and
>>>>> uvc_video_free_requests. This is what your patches are already doing.
>>>>>
>>>>> But for now I am not so pleased with the timeout concept by waiting for
>>>>> the complete interrupt to be called. This is also a shot in the dark as
>>>>> the latency depends on the scheduler and the amount of potential
>>>>> requests that are being handled.
>>>>
>>>> I agree, a timeout is not the most elegant of solutions and given a
>>>> weird enough scheduler, will run into issues as well.
>>>>
>>>>>
>>>>> 2) Locking both codepathes around the resource in question so the issue
>>>>> is avoided.
>>>>>
>>>>> However, I am also not a fried of many locks.
>>>>>
>>>>> Perhaps it is possible to use a combination of wait_for_completion in
>>>>> the uvc_video_free_requests and a complete callback in
>>>>> uvc_video_complete for those requests that are not listed in the
>>>>> req_free list.
>>>>>
>>>>> What do you think?
>>>>>
>>>> There might be a way that builds on your idea of cleaning up in the complete callback.
>>>> It would rely on having a uvc_requests that aren't bulk allocated, which may have a
>>>> performance impact.
>>>
>>> Since the allocation will only be done once, this performance impact is
>>> should not be critical.
>>>
>>>> I am imagining something like the following:
>>>>  1. Instead of allocating a bulk of uvc_requests, we allocate them
>>>>     one at a time and add them to uvc_video.ureq
>>>>  2. uvc_video.ureq becomes a list_head containing all the individual
>>>>     requests
>>>>  3. We add a sentinel flag in uvc_request that says the request is
>>>>     now stale. This flag is protected by uvc_video->req_lock
>>>>  4. uvc_video_complete looks at  this flag to deallocate both
>>>>     usb_request and uvc_request.
>>>>  5. uvcg_video_enable looks something like the following:
>>>>       uvcg_video_enable(...) {
>>>>         ...
>>>>         lock(req_lock);
>>>>         forall (uvc_requests->ureqs) {ureq->stale = true}
>>>>         unlock(req_lock);
>>>>         usb_ep_dequeue all reqs
>>>>
>>>>         uvc_video_free_requests(...)
>>>>         ...
>>>>       }
>>>>  6. uvc_video_complete looks something like:
>>>>       uvc_video_complete(...) {
>>>>         // at the start
>>>>         lock(req_lock)
>>>>         is_stale = ureq->stale;
>>>>         unlock(req_lock);
>>>>
>>>>         if (is_stale) {
>>>>           usb_ep_free_request();
>>>>           dealloc corresponding uvc_request();
>>>>           return;
>>>>         }
>>>>
>>>>         ...
>>>>
>>>>         lock(req_lock);
>>>>         // possible that request became stale while we were handling stuff
>>>>         if (!ureq->stale) {
>>>>           list_add_tail(&req->list, &video->req_free);
>>>>         } else {
>>>>           usb_ep_free_request();
>>>>           dealloc corresponding uvc_request();
>>>>         }
>>>>         unlock(req_lock);
>>>>       }
>>>>  7. uvc_video_free_requests can freely dealloc usb_requests/uvc_requests in
>>>>     req_free because we can be certain that uvc_video_complete won't modify
>>>>     it once requests have been marked stale, and the stale requests in flight
>>>>     will be cleaned up by the complete callback.
>>>>
>>>> Effectively, we freeze the state of req_free before dequeuing, and all
>>>> inflight requests are considered the responsibility of the complete handler
>>>> from that point onwards. The gadget is only responsible for freeing requests it
>>>> currently owns.
>>>>
>>>> I think this should ensure that we never have a situation where the ownership of the
>>>> requests are undefined, and only one thread is responsible for freeing any given request.
>>>>
>>>> Hope that makes sense!
>>>
>>> So you found a way to secure this also with the already available
>>> req_lock then. Nice!
>>>
>>> Also what you suggest is to move from the array model we currently have
>>> to dynamic allocation in a linked list.
>>>
>>> I would suggest some more adaptions.
>>>
>>> Keep to allocate all requests dynamicaly as you suggest instead of the
>>> bulk array.
>>>
>>> Rewrite the uvc_video_free_requests to iterate over the video->req_free
>>> list instead of all available requests to take care of all requests
>>> that are truely freed.
>>>
>>> Take this patch we started this thread with and expand it to
>>> clean up not only the usb_request but also the uvc_request
>>> like you suggested in your pseudo code.
>>>
>>> Since we check for UVC_STATE_CONNECTED already in the comlete handler
>>> this is a superset of your stale flag anyway. And every request
>>> that is currently in flight is not part of the req_free list, which
>>> makes the uvc_video_free_requests function free to run without making
>>> no harm.
>>
>> The downside of freeing based on UVC_STATE_CONNECTED and why it might be
>> problematic is that without any other synchronization method, the complete
>> callback can be arbitrarily delayed for a given usb_request.
>>
>> A STREAMOFF quickly followed by a STREAMON, might set uvc->state to
>> UVC_STATE_STREAMING before the controller has had a chance to return the
>> stale requests. This won't cause any functional issues AFAICT, but will
>> cause a memory "leak" of sorts where every successive quick
>> STREAMOFF-->STREAMON will lead to some extra usb_requests sticking around.
>> They'll eventually get freed, but it doesn't seem very responsible to increase
>> the memory load unless required.
>>
>> The stale flag ensures that this situation never happens and even if the
>> complete callbacks comes back well after  the new STREAMON event, we correctly
>> free the associated usb_request and uvc_request.
> 
> In that case we could use stale then, but I would suggest also keeping
> the change the functionality of uvc_video_free_requests aswell to loop
> over the requests in req_free list.

Agreed, uvc_video_free_requests should only free the requests in
req_free. 

Just to clear any confusion: are you working on incorporating these changes
into your patchset, or do you want me to include them in 
https://lore.kernel.org/20230912041910.726442-3-arakesh@google.com/ 
instead?

Regards,
Avi.