usb: gadget: Kernel panic (NULL pointer dereference) when using fsl_udc2_core on i.MX31 PDK

[balbi@kernel.org (Felipe Balbi)]

Hi,

Magnus Lilja <lilja.magnus@gmail.com> writes:
>> Magnus Lilja <lilja.magnus@gmail.com> writes:
>>>> Magnus Lilja <lilja.magnus@gmail.com> writes:
>>>>>>> I tried the fsl_udc_core gadget driver on the i.MX31 PDK board and got a
>>>>>>> kernel panic (NULL pointer dereference) when connecting the USB cable. I
>>>>>>> had the g_serial module loaded as well.
>>>>>>>
>>>>>>> The NULL pointer panic comes from gadget/udc/core.c
>>>>>>> usb_gadget_giveback_request() which calls req->complete() and in some
>>>>>>> cases req->complete is NULL.
>>>>>>>
>>>>>>> Commit 304f7e5e1d08 ("usb: gadget: Refactor request completion") changed
>>>>>>> fsl_udc2_core.c (and several other files) and in fsl_udc2_core.c a check
>>>>>>> that req->complete is non-NULL was removed:
>>>>>>>
>>>>>>> --- a/drivers/usb/gadget/udc/fsl_udc_core.c
>>>>>>> +++ b/drivers/usb/gadget/udc/fsl_udc_core.c
>>>>>>> @@ -197,10 +197,8 @@ __acquires(ep->udc->lock)
>>>>>>>          ep->stopped = 1;
>>>>>>>
>>>>>>>          spin_unlock(&ep->udc->lock);
>>>>>>> -       /* complete() is from gadget layer,
>>>>>>> -        * eg fsg->bulk_in_complete() */
>>>>>>> -       if (req->req.complete)
>>>>>>> -               req->req.complete(&ep->ep, &req->req);
>>>>>>> +
>>>>>>> +       usb_gadget_giveback_request(&ep->ep, &req->req);
>>>>>>>
>>>>>>>          spin_lock(&ep->udc->lock);
>>>>>>>          ep->stopped = stopped;
>>>>>>>
>>>>>>> If I re-introduce the check (either in fsl_udc_core.c or core.c) at
>>>>>>> least USB gadget operation using g_serial seems to work just fine.
>>>>>>>
>>>>>>> I don't know the logic in detail to understand whether this is a proper
>>>>>>> fix or if there is some other more problem with the fls_udc_core driver.
>>>>>>> Does anyone have input in this matter?
>>>>>>>
>>>>>>> I can produce a proper patch that fixes this problem by re-introducing
>>>>>>> the check (in either fsl_udc_core.c or core.c) if that is a proper
>>>>>>> solution and I can also assist in testing other fixes to the problem.
>>>>>>
>>>>>> ->complete() is supposed to be mandatory. Which gadget do you have that
>>>>>> ->doesn't set ->complete() to a valid function pointer?
>>>>>
>>>>> I'm modprobing g_serial so the following modules are loaded (using my patch):
>>>>>
>>>>> ~ # lsmod
>>>>> usb_f_acm
>>>>> u_serial
>>>>> g_serial
>>>>> libcomposite
>>>>> configfs
>>>>> fsl_usb2_udc
>>>>
>>>> okay, can you figure out which request is coming without ->complete()
>>>> set? To which endpoint is this request being queued? It would be nice to
>>>> know these details. Maybe this is an old bug which ought to be fixed.
>>>
>>> Sure, I can try figure that out. Any input to make the debug of the
>>> faster is appreciated if you have any.
>>
>> well, the easiest way is to add something like:
>>
>>         if (!req->complete)
>>                 dump_stack();
>>
>> to fsl udc driver. Then you would know who queued the request without
>> ->complete. A slightly better approach would be to:
>>
>> if (WARN(!req->complete,
>>         "%s: queueing request without ->complete\n", ep->name))
>>         return;
>>
>> Or something like that.
>
> Well, I think I found it.
>
> fsl_udc_core.c:ep0_prime_status() sets req->req.complete = NULL before
> it queues a transfer and my printk()'s indicate that this is indeed
> the offending function.
>
> fsl_udc_core.c:ch9getstatus() also sets complete to NULL but in my
> tests right now I haven't seen that one.
>
> So it's an internal problem in the fsl_udc_core.c file.

seems like it. It's rather odd that fsl_udc doesn't wanna know about
completion of Status stage. Oh well, I guess in this case it doesn't
matter if you add a complete function or reinstate the previous check
for valid complete.

If you decide to reinstate the check, please add a comment above the
check explaining that fsl_udc itself queues requests with NULL
->complete().

I must say, however, that I would suggest adding a complete callback
since that will help us BUG with NULL pointer deref on bad gadget
drivers ;-)

-- 
balbi