ep0 request dequeue for function drivers

ep0 request dequeue for function drivers

[Pratham Pratap]

Hi,

Let's say a function driver queues a request on ep0 and before the 
completion handler could run composition switch/physical disconnect 
happens. This request will be in pending list since gadget_giveback is 
not done but the composite driver will free the request from 
composite_dev_cleanup. Now, once the next connect happens, another ep0 
request is queued and while handling the completion of that request, 
gadget driver might end up accessing the old/stale request leading to 
list_poison since pending list is corrupted.

To fix this, the function drivers might want to use setup_pending(mark 
it to true) flag so that when composite_dev_cleanup is run the requests 
are given back from usb_ep_dequeue; clear the setup pending flag in 
function driver when completion handler is run successfully. I can see 
this issue in almost all the function drivers.

Looking for suggestions and comments.

--Pratham

Re: ep0 request dequeue for function drivers

[Greg KH]

On Mon, Apr 26, 2021 at 01:42:50PM +0530, Pratham Pratap wrote:
> Hi,
> 
> Let's say a function driver queues a request on ep0 and before the
> completion handler could run composition switch/physical disconnect happens.
> This request will be in pending list since gadget_giveback is not done but
> the composite driver will free the request from composite_dev_cleanup. Now,
> once the next connect happens, another ep0 request is queued and while
> handling the completion of that request, gadget driver might end up
> accessing the old/stale request leading to list_poison since pending list is
> corrupted.
> 
> To fix this, the function drivers might want to use setup_pending(mark it to
> true) flag so that when composite_dev_cleanup is run the requests are given
> back from usb_ep_dequeue; clear the setup pending flag in function driver
> when completion handler is run successfully. I can see this issue in almost
> all the function drivers.
> 
> Looking for suggestions and comments.

Have you tried making this change and determined that it works or not?
If so, please submit a patch.

thanks,

greg k-h

Re: ep0 request dequeue for function drivers

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2281 bytes --]


Hi,

Pratham Pratap <prathampratap@codeaurora.org> writes:
> Hi,
>
> Let's say a function driver queues a request on ep0 and before the 
> completion handler could run composition switch/physical disconnect 
> happens. This request will be in pending list since gadget_giveback is 
> not done but the composite driver will free the request from 
> composite_dev_cleanup. Now, once the next connect happens, another ep0 
> request is queued and while handling the completion of that request, 
> gadget driver might end up accessing the old/stale request leading to 
> list_poison since pending list is corrupted.

argh, I'm assuming you're using dwc3. It's always a good idea to Cc
maintainers for the drivers or subsystems in question. You can rely on
scripts/get_maintainer.pl to get an idea who you should Cc.

Anyway, assuming dwc3.

Have you tried to actually reproduce this? Did you collect tracepoints?
Did you read dwc3's documentation, specially in regards to reporting
bugs?

Try to consider what happens when the cable is yanked and you'll quickly
realize what you suggest can't happen. How does USB know that cable is
disconnected? What happens to dwc3 when the cable is disconnected? What
does the driver do about it?

If you really found a bug, please report it correctly, following the
Reporting Bugs section of dwc3 documentation, Cc the relevant people and
make sure to reproduce the problem with *mainline*; downstream kernel is
not acceptable ;-)

Also, please be clear about the setup you're using. The only thing I can
infer is that you're using dwc3 with one of the QC platforms and I can
only infer that due to your email domain. Please be clear, how to
reproduce? Which QC platform are you using? Which kernel version?

> To fix this, the function drivers might want to use setup_pending(mark 
> it to true) flag so that when composite_dev_cleanup is run the requests 
> are given back from usb_ep_dequeue; clear the setup pending flag in 
> function driver when completion handler is run successfully. I can see 
> this issue in almost all the function drivers.

Nah, we don't need that. Please answer the questions above about
handling for cable disconnect and you'll see this is unnecessary.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

Re: ep0 request dequeue for function drivers

[Pratham Pratap]

Thanks for the explanation. Now it is clear why we don't need this in 
upstream.

--Pratham

On 4/26/2021 3:47 PM, Felipe Balbi wrote:
> Hi,
>
> Pratham Pratap <prathampratap@codeaurora.org> writes:
>> Hi,
>>
>> Let's say a function driver queues a request on ep0 and before the
>> completion handler could run composition switch/physical disconnect
>> happens. This request will be in pending list since gadget_giveback is
>> not done but the composite driver will free the request from
>> composite_dev_cleanup. Now, once the next connect happens, another ep0
>> request is queued and while handling the completion of that request,
>> gadget driver might end up accessing the old/stale request leading to
>> list_poison since pending list is corrupted.
> argh, I'm assuming you're using dwc3. It's always a good idea to Cc
> maintainers for the drivers or subsystems in question. You can rely on
> scripts/get_maintainer.pl to get an idea who you should Cc.
>
> Anyway, assuming dwc3.
>
> Have you tried to actually reproduce this? Did you collect tracepoints?
> Did you read dwc3's documentation, specially in regards to reporting
> bugs?
>
> Try to consider what happens when the cable is yanked and you'll quickly
> realize what you suggest can't happen. How does USB know that cable is
> disconnected? What happens to dwc3 when the cable is disconnected? What
> does the driver do about it?
>
> If you really found a bug, please report it correctly, following the
> Reporting Bugs section of dwc3 documentation, Cc the relevant people and
> make sure to reproduce the problem with *mainline*; downstream kernel is
> not acceptable ;-)
>
> Also, please be clear about the setup you're using. The only thing I can
> infer is that you're using dwc3 with one of the QC platforms and I can
> only infer that due to your email domain. Please be clear, how to
> reproduce? Which QC platform are you using? Which kernel version?
>
>> To fix this, the function drivers might want to use setup_pending(mark
>> it to true) flag so that when composite_dev_cleanup is run the requests
>> are given back from usb_ep_dequeue; clear the setup pending flag in
>> function driver when completion handler is run successfully. I can see
>> this issue in almost all the function drivers.
> Nah, we don't need that. Please answer the questions above about
> handling for cable disconnect and you'll see this is unnecessary.
>