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@xxxxxxxxxxxxxx> wrote:Cc'ing: Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> ... 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@xxxxxxxxxxxxxx/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@xxxxxxxxxx/ (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:
signature.asc
Description: PGP signature
