linux-kernel - Re: [RFC][PATCH] dwc3: gadget: Fix losing list items in dwc3_gadget_ep_cleanup_completed

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YRImkgCDSvz11ytQ@kroah.com>
Date:   Tue, 10 Aug 2021 09:11:14 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Thinh Nguyen <Thinh.Nguyen@...opsys.com>
Cc:     John Stultz <john.stultz@...aro.org>,
        lkml <linux-kernel@...r.kernel.org>,
        Wesley Cheng <wcheng@...eaurora.org>,
        Felipe Balbi <balbi@...nel.org>,
        Alan Stern <stern@...land.harvard.edu>,
        Jack Pham <jackp@...eaurora.org>, Todd Kjos <tkjos@...gle.com>,
        Amit Pundir <amit.pundir@...aro.org>,
        YongQin Liu <yongqin.liu@...aro.org>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Petri Gynther <pgynther@...gle.com>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>
Subject: Re: [RFC][PATCH] dwc3: gadget: Fix losing list items in
 dwc3_gadget_ep_cleanup_completed_requests()

On Tue, Aug 10, 2021 at 08:05:49AM +0200, Greg Kroah-Hartman wrote:
> On Mon, Aug 09, 2021 at 10:57:27PM +0000, Thinh Nguyen wrote:
> > John Stultz wrote:
> > > On Mon, Aug 9, 2021 at 3:44 PM Thinh Nguyen <Thinh.Nguyen@...opsys.com> wrote:
> > >>
> > >> John Stultz wrote:
> > >>> In commit d25d85061bd8 ("usb: dwc3: gadget: Use
> > >>> list_replace_init() before traversing lists"), a local list_head
> > >>> was introduced to process the started_list items to avoid races.
> > >>>
> > >>> However, in dwc3_gadget_ep_cleanup_completed_requests() if
> > >>> dwc3_gadget_ep_cleanup_completed_request() fails, we break early,
> > >>> causing the items on the local list_head to be lost.
> > >>>
> > >>> This issue showed up as problems on the db845c/RB3 board, where
> > >>> adb connetions would fail, showing the device as "offline".
> > >>>
> > >>> This patch tries to fix the issue by if we are returning early
> > >>> we splice in the local list head back into the started_list
> > >>> and return (avoiding an infinite loop, as the started_list is
> > >>> now non-null).
> > >>>
> > >>> Not sure if this is fully correct, but seems to work for me so I
> > >>> wanted to share for feedback.
> > >>>
> > >>> Cc: Wesley Cheng <wcheng@...eaurora.org>
> > >>> Cc: Felipe Balbi <balbi@...nel.org>
> > >>> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > >>> Cc: Alan Stern <stern@...land.harvard.edu>
> > >>> Cc: Jack Pham <jackp@...eaurora.org>
> > >>> Cc: Thinh Nguyen <thinh.nguyen@...opsys.com>
> > >>> Cc: Todd Kjos <tkjos@...gle.com>
> > >>> Cc: Amit Pundir <amit.pundir@...aro.org>
> > >>> Cc: YongQin Liu <yongqin.liu@...aro.org>
> > >>> Cc: Sumit Semwal <sumit.semwal@...aro.org>
> > >>> Cc: Petri Gynther <pgynther@...gle.com>
> > >>> Cc: linux-usb@...r.kernel.org
> > >>> Fixes: d25d85061bd8 ("usb: dwc3: gadget: Use list_replace_init() before traversing lists")
> > >>> Signed-off-by: John Stultz <john.stultz@...aro.org>
> > >>> ---
> > >>>  drivers/usb/dwc3/gadget.c | 6 ++++++
> > >>>  1 file changed, 6 insertions(+)
> > >>>
> > >>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > >>> index b8d4b2d327b23..a73ebe8e75024 100644
> > >>> --- a/drivers/usb/dwc3/gadget.c
> > >>> +++ b/drivers/usb/dwc3/gadget.c
> > >>> @@ -2990,6 +2990,12 @@ static void dwc3_gadget_ep_cleanup_completed_requests(struct dwc3_ep *dep,
> > >>>                       break;
> > >>>       }
> > >>>
> > >>> +     if (!list_empty(&local)) {
> > >>> +             list_splice_tail(&local, &dep->started_list);
> > >>> +             /* Return so we don't hit the restart case and loop forever */
> > >>> +             return;
> > >>> +     }
> > >>> +
> > >>>       if (!list_empty(&dep->started_list))
> > >>>               goto restart;
> > >>>  }
> > >>>
> > >>
> > >> No, we should revert the change for
> > >> dwc3_gadget_ep_cleaup_completed_requests(). As I mentioned previously,
> > >> we don't cleanup the entire started_list. If the original problem is due
> > >> to disconnection in the middle of request completion, then we can just
> > >> check for pullup_connected and exit the loop and let the
> > >> dwc3_remove_requests() do the cleanup.
> > > 
> > > Ok, sorry, I didn't read your mail in depth until I had this patch
> > > sent out. If a revert of d25d85061bd8 is the better fix, I'm fine with
> > > that too.
> > > 
> > > thanks
> > > -john
> > > 
> > 
> > IMO, we should revert this patch for now since it will cause regression.
> > We can review and test a proper fix at a later time.
> 
> Ok, can someone send me a revert please?  That will go faster than me
> having to create it myself...

I'll go do this now...