linux-kernel - Re: [PATCH v2 1/2] usb: dwc3: gadget: Add disconnect checking when changing function dynamically

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: <CAMz4kuKLCAhT4qTD_EDWPt22X-VC5wYZgfr3P=-2y90=KmcemA@mail.gmail.com>
Date:   Sun, 18 Sep 2016 12:48:05 +0800
From:   Baolin Wang <baolin.wang@...aro.org>
To:     Felipe Balbi <balbi@...nel.org>
Cc:     Greg KH <gregkh@...uxfoundation.org>,
        Mark Brown <broonie@...nel.org>,
        USB <linux-usb@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/2] usb: dwc3: gadget: Add disconnect checking when
 changing function dynamically

Hi Felipe,

Sorry for late reply due to my holiday.

On 9 September 2016 at 18:47, Felipe Balbi <balbi@...nel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@...aro.org> writes:
>> When system has stpped the gadget, we should avoid queuing any requests
>> which will cause tranfer failed. Thus adding some disconnect checking to
>> avoid this situation.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@...aro.org>
>
> do you mind if we discuss this for a little longer?

Sure.

>
>> ---
>> Changes since v1:
>>  - Split into 2 separate ptaches.
>>  - Choose complete mechanism instead of polling.
>> ---
>>  drivers/usb/dwc3/ep0.c    |    8 ++++++++
>>  drivers/usb/dwc3/gadget.c |   12 +++++++++---
>>  2 files changed, 17 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>> index fe79d77..632e5a4 100644
>> --- a/drivers/usb/dwc3/ep0.c
>> +++ b/drivers/usb/dwc3/ep0.c
>> @@ -228,6 +228,14 @@ int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct usb_request *request,
>>       int                             ret;
>>
>>       spin_lock_irqsave(&dwc->lock, flags);
>> +     if (!dwc->pullups_connected) {
>> +             dwc3_trace(trace_dwc3_ep0,
>> +                     "queuing request %p to %s when gadget is disconnected",
>> +                     request, dep->name);
>> +             ret = -ESHUTDOWN;
>> +             goto out;
>> +     }
>
> I have been thinking about this branch here. It's not a problem to queue
> a request with pullups disconnected. It's only a problem to issue
> START_TRANSFER without RUN_STOP bit set.
>
> So maybe this check should be done in dwc3_send_gadget_ep_cmd(). By
> doing that we also make sure to do the check in one place and one place
> only because all endpoints rely dwc3_send_gadget_ep_cmd().

OK, that makes sense to me.

>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 1783406..1a33308 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -1040,6 +1040,13 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
>>       struct dwc3             *dwc = dep->dwc;
>>       int                     ret;
>>
>> +     if (!dwc->pullups_connected) {
>> +             dwc3_trace(trace_dwc3_gadget,
>> +                     "queuing request %p to %s when gadget is disconnected",
>> +                     &req->request, dep->endpoint.name);
>> +             return -ESHUTDOWN;
>> +     }
>> +
>>       if (!dep->endpoint.desc) {
>>               dwc3_trace(trace_dwc3_gadget,
>>                               "trying to queue request %p to disabled %s",
>> @@ -1984,13 +1991,12 @@ static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, struct dwc3_ep *dep,
>>               if (ret)
>>                       break;
>>       }
>> -
>
> trailing change.

Ah, sorry. Will remove it.

>
>>       /*
>>        * Our endpoint might get disabled by another thread during
>>        * dwc3_gadget_giveback(). If that happens, we're just gonna return 1
>>        * early on so DWC3_EP_BUSY flag gets cleared
>>        */
>> -     if (!dep->endpoint.desc)
>> +     if (!dep->endpoint.desc || !dwc->pullups_connected)
>
> I'm still considering this as well. Sure, we kill pullups before the
> descriptor is set to NULL, but that shouldn't be a problem. What will
> happen is:
>
> usb_gadget_disconnect();
> udc->driver->disconnect();
>  for_each_ep() {
>   for_each_request() {
>    usb_ep_dequeue();
>   }
>   usb_ep_disable();
>    dep->endpoint.desc = NULL;
>  }
> udc->driver->unbind();
> usb_gadget_udc_stop();
>
> I don't see a problem here. Did you manage to trigger any failure when
> you didn't have this check? Care to show some logs? We might have a bug
> elsewhere which we don't want to mask by adding this check here.

If we did not add this 'pullups_connected' checking here, it maybe
will kick one transfer to cause a problem of TART_TRANSFER, but it
also can be removed if we check in dwc3_send_gadget_ep_cmd() as you
suggested. Thanks for your comments and I will send out the new patch.


-- 
Baolin.wang
Best Regards