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 20:14:17 +0000 From: Thinh Nguyen <Thinh.Nguyen@...opsys.com> To: Wesley Cheng <wcheng@...eaurora.org>, Thinh Nguyen <Thinh.Nguyen@...opsys.com>, John Stultz <john.stultz@...aro.org> CC: lkml <linux-kernel@...r.kernel.org>, Felipe Balbi <balbi@...nel.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.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() Wesley Cheng wrote: > Hi Thinh, > > On 8/9/2021 3:57 PM, 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. >> >> Thanks, >> Thinh >> > > Another suggestion would just be to replace the loop with a while() loop > and using list_entry() instead. That was what was discussed in the > earlier patch series which also addresses the problem as well. Issue > here is the tmp variable still carries a stale request after the dwc3 > giveback is called. We can avoid that by always fetching the > list_entry() instead of relying on list_for_each_safe() > > https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/1620716636-12422-1-git-send-email-wcheng@codeaurora.org/__;!!A4F2R9G_pg!P0E1pv3C0PStDepKyy8iqKgUaOhDy0ZDhYdz-_cZwnJRQjNjvw0MdJQCdU6Xwnt3YAs_$ > This should work, but the awkward thing is 2 loops from 2 separate threads competing to remove/giveback the requests and may report mix status. BR, Thinh
Powered by blists - more mailing lists