lkml.org 
[lkml]   [2023]   [Oct]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v1 0/3] usb: gadget: uvc: stability fixes on STREAMOFF.
    From


    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.

    \
     
     \ /
      Last update: 2023-10-07 01:49    [W:9.518 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site