Re: [PATCH] usb: gadget: udc: udc_stop before gadget unbind

[Felipe Balbi]

Hi,

Danilo Krummrich <danilokrummrich@xxxxxxxxxxxxx> writes:
> thanks for reviewing.

np :-)

> On 2017-08-15 12:03, Felipe Balbi wrote:
>> Hi,
>> 
>> Danilo Krummrich <danilokrummrich@xxxxxxxxxxxxx> writes:
>>> udc_stop needs to be called before gadget driver unbind. Otherwise it
>>> might happen that udc drivers still call into the gadget driver (e.g.
>>> to reset gadget after OTG event). If this happens it is likely to get
>>> panics from gadget driver dereferencing NULL ptr, as gadget's drvdata
>>> is set to NULL on unbind.
>> 
>> seems like the problem here is with the OTG layer, not UDC core.
>> 
> I mentioned this just as example, it can happen whenever a UDC driver 
> calls
> the gadget driver (e.g. by calling usb_gadget_udc_reset() in ISR) after 
> gadget
> drivers unbind() was called already (e.g. by gadget configfs).
> If this happens gadget drivers drvdata was already set to NULL by 
> unbind()
> and reset() could result into a NULL ptr exception.
> Therefore my assumption was that it needs to be prevented that the 
> gadget
> driver is getting called after unbind.

We have a known problem in the design of the gadget API that causes this
races but we couldn't come up with a solution yet :-)

Inverting these two calls is not the correct way to go about this :-)

>>> Signed-off-by: Danilo Krummrich <danilokrummrich@xxxxxxxxxxxxx>
>>> ---
>>> Actually there could still be a race:
>>> (CPU1 code taken from dwc3 drivers dwc3_disconnect_gadget() as 
>>> exsample)
>>> 
>>> CPU0					CPU1
>>> ----					----
>>> usb_gadget_disconnect(udc->gadget);
>>> udc->driver->disconnect(udc->gadget);
>>> 					if (dwc->gadget_driver && dwc->gadget_driver->disconnect)
>>> usb_gadget_udc_stop(udc);
>>> udc->driver->unbind(udc->gadget);
>>> 					dwc->gadget_driver->disconnect(&dwc->gadget);
>>> 
>>> UDC drivers typically set their gadget driver pointer to NULL in 
>>> udc_stop
>>> and check for it before calling into the gadget driver. To fix the 
>>> issue
>>> above every udc driver could apply a lock around this.
>>> 
>>> If you see the need for having this or another solutions I can provide
>>> further patches. This patch could also just serve as a base for 
>>> discussion
>>> if someone knows a smarter solution.
>>> 
>>> I saw this problem causing a panic on hikey960 board and provided a 
>>> quick
>>> workaround for the same problem here:
>>> https://android-review.googlesource.com/#/c/kernel/common/+/457476/
>>> (panic log in the commit message of the linked patch)
>>> ---
>>>  drivers/usb/gadget/udc/core.c | 6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/drivers/usb/gadget/udc/core.c 
>>> b/drivers/usb/gadget/udc/core.c
>>> index efce68e9a8e0..8155468afc0d 100644
>>> --- a/drivers/usb/gadget/udc/core.c
>>> +++ b/drivers/usb/gadget/udc/core.c
>>> @@ -1234,8 +1234,12 @@ static void usb_gadget_remove_driver(struct 
>>> usb_udc *udc)
>>> 
>>>  	usb_gadget_disconnect(udc->gadget);
>>>  	udc->driver->disconnect(udc->gadget);
>>> -	udc->driver->unbind(udc->gadget);
>>> +	/* udc_stop needs to be called before gadget driver unbind to 
>>> prevent
>>> +	 * udc driver calls into gadget driver after unbind which could 
>>> cause
>>> +	 * a nullptr exception.
>>> +	 */
>>>  	usb_gadget_udc_stop(udc);
>>> +	udc->driver->unbind(udc->gadget);
>> 
>> This patch is incorrect, it will prevent us from giving back requests 
>> to
>> gadget driver properly. ->unbind() has to happen before ->udc_stop().
>
> Do you mean after udc_stop the udc driver can not call the gadget driver
> anymore? If not, I did not got your point, sorry for that. Can you 
> please
> help me out? Would the changed order raise another issue I'm not aware 
> of?

right, ->udc_stop() is supposed to completely teardown the USB
controller, including disabling interrupts and all. The only thing it
_can_ do from ->udc_stop() would be giving back any pending requests
that were left (which would cause req->complete() to be called with an
error status). But even that is unlikely in the case you mention since
->unbind() was already called.

> If I understood you correctly, without this patch udc driver can not 
> call
> the gadget driver back as well, because this would result in a NULL ptr
> dereference, as unbind() sets drvdata to NULL.
>
> In any case the race described in my original message can still happen,
> regardless of the order of udc_stop and unbind. But with this patch the
> needed locking could easily done within the udc driver only. Without, 
> the
> lock needs to be acquired before udc->driver->unbind(udc->gadget) and
> released after usb_gadget_udc_stop(). Otherwise an ISR of the udc driver
> trying to call into the gadget driver could do this after gadget driver
> already unbound.

right

-- 
balbi

Attachment: signature.asc
Description: PGP signature