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

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

[Magnus Lilja]

Hi

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.

Thanks, Magnus

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

[Felipe Balbi]

Hi,

Magnus Lilja <lilja.magnus@gmail.com> writes:
> Hi
>
> 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?

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170123/6f63a215/attachment.sig>

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

[Magnus Lilja]

Hi

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


Regards, Magnus

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

[Felipe Balbi]

Hi,

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.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170124/0826a8cd/attachment.sig>

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

[Magnus Lilja]

On 24 January 2017 at 09:52, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> 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.

Regards, Magnus

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

[Felipe Balbi]

Hi,

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.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170124/82b156a5/attachment.sig>

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@kernel.org> wrote:
>
> Hi,
>
> 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.

Regards, Magnus?

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

[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

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

[Magnus Lilja]

On 24 January 2017 at 19:34, Felipe Balbi <balbi@kernel.org> wrote:
>
> 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 ;-)

I can do that. Such a complete() callback function would be a no-op
then I assume (with a comment in it why it is a no-op).

Regards, Magnus

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

[Felipe Balbi]

Hi,

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 ;-)
>
> I can do that. Such a complete() callback function would be a no-op
> then I assume (with a comment in it why it is a no-op).

that's right :-)

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170125/908fb890/attachment.sig>