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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 10 Aug 2021 08:05:49 +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 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... thanks, greg k-h
Powered by blists - more mailing lists