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

[Magnus Lilja]

Hi

On 24 January 2017 at 11:54, Felipe Balbi <balbi at kernel.org> wrote:
>
> Hi,
>
> Magnus Lilja <lilja.magnus at gmail.com> writes:
>>> Magnus Lilja <lilja.magnus at 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.

Regards, Magnus¨