linux-kernel - Re: [PATCH v2] usb: dwc3: gadget: Avoid canceling current request for queuing error

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <87r1ikgrts.fsf@kernel.org>
Date:   Thu, 06 May 2021 12:00:15 +0300
From:   Felipe Balbi <balbi@...nel.org>
To:     Wesley Cheng <wcheng@...eaurora.org>,
        Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
Cc:     "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "jackp@...eaurora.org" <jackp@...eaurora.org>
Subject: Re: [PATCH v2] usb: dwc3: gadget: Avoid canceling current request
 for queuing error


Hi,

Wesley Cheng <wcheng@...eaurora.org> writes:
>> If I understood the whole thing correctly, we want everything except the
>> current request (the one that failed START or UPDATE transfer) to go
>> through giveback(). This really tells me that we're not handling error
>> case in kick_transfer and/or prepare_trbs() correctly.
>> 
>
> We don't want the request passed in usb_ep_queue() to be calling
> giveback() IF DONE IN the usb_ep_queue() context only.

right, that's how this should behave.

>> I also don't want to pass another argument to kick_transfer because it
>> should be unnecessary: the current request should *always* be the last
>> one in the list. Therefore we should rely on something like
>> list_last_entry() followed by list_for_each_entry_safe_reverse() to
>> handle this without a special case.
>> 
>> ret = dwc3_send_gadget_ep_cmd();
>> if (ret < 0) {
>> 	current = list_last_entry();
>> 
>> 	unmap(current);
>>         for_each_trb_in(current) {
>>         	clear_HWO(trb);
>>         }
>> 
>> 	list_for_entry_safe_reverse() {
>>         	move_cancelled();
>>         }
>> }
>>
> Nice, thanks for the suggestion and info!  Problem we have is that kick
> transfer is being used elsewhere, for example, during the TRB complete path:
>
> static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep,
> 		const struct dwc3_event_depevt *event, int status)
> {
> ...
> 	else if (dwc3_gadget_ep_should_continue(dep))
> 		if (__dwc3_gadget_kick_transfer(dep) == 0)
> 			no_started_trb = false;
>
> So in these types of calls, we would still want ALL requests to be
> cancelled w/ giveback() called, so that the completion() callbacks can
> cleanup/free those requests accordingly.
>
> If we went and only unmapped the last entry (and removed it from any
> list), then no one would clean it up as it is outside of the
> usb_ep_queue() context, and not within any of the DWC3 lists.

oh, I see what you mean. At the moment we want kick_transfer to behave
in two different manners and that's probably where the bug is
originating from.

It sounds like it's time to split kick_transfer into
kick_queued_transfer() and e.g. continue_pending_transfers()

The thing is that if we continue to sprinkle special cases all over the
place, soon enough it'll be super hard to maintain the driver.

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (512 bytes)