Re: [PATCH] usb: gadget: uvc: Move usb_ep_disable() to uvcg_video_enable()

[Michael Grzeschik <mgr@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 4862 bytes --]

Hi Avichal

On Mon, Sep 11, 2023 at 09:26:09PM -0700, Avichal Rakesh wrote:
>On 9/10/23 17:50, Michael Grzeschik wrote:
>> On Fri, Sep 08, 2023 at 11:54:40PM +0800, Avichal Rakesh wrote:
>>> Apologies for the late reply, I have been out travelling.
>>> On Mon, Sep 4, 2023 at 10:36 PM Michael Grzeschik <mgr@pengutronix.de> wrote:
>>>> Cc'ing: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>>>>
>>>> ...
>>>> I am currently trying to solve that by preparing a patch that is
>>>> fixing the use of the requests when deallocating them. Since currently
>>>> the uvc_gadget is also running into wild use after free issues because
>>>> of exactly that async dequeue and dealloc situation.
>>>
>>> Do you already have a patch up for this? It seems my LKML-fu is
>>> failing and I can't seem to find the thread. If you aren't too deep
>>> into the patch, can you take a look at the request counting mechanism
>>> added in my patch? If you have a (somewhat) consistent repro of the
>>> use-after-dealloc issue, runnin it through the whole patch would be
>>> very appreciated! It is supposed to fix the exact problem you've
>>> described.
>>
>> I just send out v1:
>>
>> https://lore.kernel.org/linux-usb/20230911002451.2860049-1-m.grzeschik@pengutronix.de/
>
>Thank you for the patch. I do have a few comments on it, will respond on that thread.

Thanks for the comments.

>>
>> My patches did go back and forth between changes in the uvc-gadget
>> driver and the device-controller driver. My latest version was including
>> calling free_request from the complete handler. I found that option
>> while looking into the uac2 gadget code. It took away a lot of
>> pain while trying to fix the issue in the dwc3 gadget driver.
>>
>>>> IMHO it should be
>>>> save to call dealloc after calling dequeue. Which is probably true for
>>>> other usb device controller driver other then dwc3.
>>>
>>> Perhaps Thinh or someone better versed in Gadget API can chime in on
>>> this, but as it stands usb_ep_dequeue specifically says that it is
>>> async, and gadget drivers must wait on the complete callbacks to
>>> regain ownership of the usb_request. Until the API change is made, UVC
>>> should adhere to the current API?
>>
>> Since you mention that usb_ep_dequeue is async I am very confident
>> that it is safe to free the request in the completion handler.
>>
>> Although we could cleanup and improve the uvc_video_free_requests
>> function itself. But with the patches I have here the use
>> after free was gone so far. So they should be good so far.
>>
>>>> For some background. The dwc3 is putting the requests into a cancelled list
>>>> that will be cleared by the interrupt handler and that is dequeuing them
>>>> instead. In between the dequeue call and the interrupt call the uvc layer could
>>>> dealloc the request which leads the interrupt handler to dequeue an
>>>> already freed request.
>>>
>>> This roughly tracks with what I gleaned from skimming the DWC3 code as
>>> well. In local tests the complete calls were always timely and I never
>>> actually ran into the situation where UVC deallocated an unowned
>>> request, but as someone (I think it was Alan?)  said in a previous
>>> thread: technically possible just means it will happen eventually
>>>
>>> Please do review/test the patch. I'll send out a formal patch on
>>> Monday once I am back, but would love to have some early eyes take a
>>> look in case there is something obvious I missed.
>>
>> First I tested your patch with my sketchy setup where I more then once
>> run into the use after free condition. But with the patch this was not
>> gone.
>
>Just to confirm, use-after-free issue was _not_ fixed with this patch?

Yes, it did somehow not help the issues I see.

I will come back with the issues I saw on your actual patch series.

>> I also looked over the patch. As what I saw this is a possible
>> alternative to my patches. The changes are doing some similar things.
>> But the code is changing to many things at once. Please split the code
>> up into more logical chunks. Perhaps you could try to rebase it on
>> my patches. And start from there.
>
>You're right, there are two issues this patch fixes. One of which is the
>same as that fixed by your series of patches. Uploaded v1 of the series at
>https://lore.kernel.org/20230912041910.726442-1-arakesh@google.com/
>(cc'ed to you) which splits the fixes into two separate patches.

Thanks for seperating them.

Regards,
Michael

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]