Hi Michael, On 9/10/23 17:50, Michael Grzeschik wrote: > Hi Avichal > > 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. > > 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? > > 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! - Avi.
