gadgetfs regression (NULL ptr deref) since v4.4-rc7

gadgetfs regression (NULL ptr deref) since v4.4-rc7

[Vegard Nossum]

Hi,

Using gadgetfs on latest mainline, I get the following NULL pointer 
dereference:

BUG: unable to handle kernel NULL pointer dereference at           (null)
IP: [<ffffffff81138e89>] __list_del_entry+0x29/0xc0
PGD 34f067 PUD 355067 PMD 0
Oops: 0000 [#1] DEBUG_PAGEALLOC
CPU: 0 PID: 35 Comm: afl-fuzz Not tainted 4.5.0-rc2 #1
task: ffff8800002b1ec0 ti: ffff88000033c000 task.ti: ffff88000033c000
RIP: 0010:[<ffffffff81138e89>]  [<ffffffff81138e89>] 
__list_del_entry+0x29/0xc0
RSP: 0018:ffff88000033fe30  EFLAGS: 00010207
RAX: 0000000000000000 RBX: ffffffff8138c108 RCX: dead000000000200
RDX: 0000000000000000 RSI: 0000000000000061 RDI: ffffffff8138c108
RBP: ffff88000033fe30 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: ffffffff8138b928
R13: ffffffff8138c040 R14: ffff88000ec0da20 R15: ffff88000033ff58
FS:  00007ffff7ff2740(0000) GS:ffffffff8135d000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 0000000000335000 CR4: 00000000001406b0
Stack:
  ffff88000033fe48 ffffffff81138f2d ffffffff8138bbd0 ffff88000033fe70
  ffffffff811c880b ffff8800002f3000 ffff88000ec109a0 ffff880000298aa0
  ffff88000033fe88 ffffffff811ce040 ffff88000034cdc0 ffff88000033feb8
Call Trace:
  [<ffffffff81138f2d>] list_del+0xd/0x30
  [<ffffffff811c880b>] usb_gadget_unregister_driver+0xdb/0xf0
  [<ffffffff811ce040>] dev_release+0x20/0x60
  [<ffffffff810b464c>] __fput+0x4c/0x130
  [<ffffffff810b4769>] ____fput+0x9/0x10
  [<ffffffff81048577>] task_work_run+0x67/0xa0
  [<ffffffff81000dcf>] exit_to_usermode_loop+0x8f/0xa0
  [<ffffffff8100105d>] syscall_return_slowpath+0x3d/0x40
  [<ffffffff81278b89>] int_ret_from_sys_call+0x25/0x8f
Code: ff ff 55 48 8b 07 48 b9 00 01 00 00 00 00 ad de 48 8b 57 08 48 89 
e5 48 39 c8 74 29 48 b9 00 02 00 00 00 00 ad de 48 39 ca 74 3a <4c> 8b 
02 4c 39 c7 75 52 4c 8b 40 08 4c 39 c7 75 66 48 89 50 08
RIP  [<ffffffff81138e89>] __list_del_entry+0x29/0xc0
  RSP <ffff88000033fe30>
CR2: 0000000000000000
---[ end trace e6cfe1de661dcffe ]---

Reverting this commit fixes the problem for me:

commit 855ed04a3758b205e84b269f92d26ab36ed8e2f7
Author: Ruslan Bilovol <ruslan.bilovol@gmail.com>
Date:   Mon Nov 23 09:56:38 2015 +0100

     usb: gadget: udc-core: independent registration of gadgets and 
gadget drivers

Though I am still seeing some occasional lockups. Thanks,


Vegard

Re: gadgetfs regression (NULL ptr deref) since v4.4-rc7

[Ruslan Bilovol]

Hi Vegard,

On Mon, Feb 8, 2016 at 1:27 AM, Vegard Nossum <vegard.nossum@oracle.com> wrote:
> Hi,
>
> Using gadgetfs on latest mainline, I get the following NULL pointer
> dereference:
>
> BUG: unable to handle kernel NULL pointer dereference at           (null)
> IP: [<ffffffff81138e89>] __list_del_entry+0x29/0xc0
> PGD 34f067 PUD 355067 PMD 0
> Oops: 0000 [#1] DEBUG_PAGEALLOC
> CPU: 0 PID: 35 Comm: afl-fuzz Not tainted 4.5.0-rc2 #1
> task: ffff8800002b1ec0 ti: ffff88000033c000 task.ti: ffff88000033c000
> RIP: 0010:[<ffffffff81138e89>]  [<ffffffff81138e89>]
> __list_del_entry+0x29/0xc0
> RSP: 0018:ffff88000033fe30  EFLAGS: 00010207
> RAX: 0000000000000000 RBX: ffffffff8138c108 RCX: dead000000000200
> RDX: 0000000000000000 RSI: 0000000000000061 RDI: ffffffff8138c108
> RBP: ffff88000033fe30 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: ffffffff8138b928
> R13: ffffffff8138c040 R14: ffff88000ec0da20 R15: ffff88000033ff58
> FS:  00007ffff7ff2740(0000) GS:ffffffff8135d000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000000 CR3: 0000000000335000 CR4: 00000000001406b0
> Stack:
>  ffff88000033fe48 ffffffff81138f2d ffffffff8138bbd0 ffff88000033fe70
>  ffffffff811c880b ffff8800002f3000 ffff88000ec109a0 ffff880000298aa0
>  ffff88000033fe88 ffffffff811ce040 ffff88000034cdc0 ffff88000033feb8
> Call Trace:
>  [<ffffffff81138f2d>] list_del+0xd/0x30
>  [<ffffffff811c880b>] usb_gadget_unregister_driver+0xdb/0xf0
>  [<ffffffff811ce040>] dev_release+0x20/0x60
>  [<ffffffff810b464c>] __fput+0x4c/0x130
>  [<ffffffff810b4769>] ____fput+0x9/0x10
>  [<ffffffff81048577>] task_work_run+0x67/0xa0
>  [<ffffffff81000dcf>] exit_to_usermode_loop+0x8f/0xa0
>  [<ffffffff8100105d>] syscall_return_slowpath+0x3d/0x40
>  [<ffffffff81278b89>] int_ret_from_sys_call+0x25/0x8f
> Code: ff ff 55 48 8b 07 48 b9 00 01 00 00 00 00 ad de 48 8b 57 08 48 89 e5
> 48 39 c8 74 29 48 b9 00 02 00 00 00 00 ad de 48 39 ca 74 3a <4c> 8b 02 4c 39
> c7 75 52 4c 8b 40 08 4c 39 c7 75 66 48 89 50 08
> RIP  [<ffffffff81138e89>] __list_del_entry+0x29/0xc0
>  RSP <ffff88000033fe30>
> CR2: 0000000000000000
> ---[ end trace e6cfe1de661dcffe ]---
>
> Reverting this commit fixes the problem for me:
>
> commit 855ed04a3758b205e84b269f92d26ab36ed8e2f7
> Author: Ruslan Bilovol <ruslan.bilovol@gmail.com>
> Date:   Mon Nov 23 09:56:38 2015 +0100
>
>     usb: gadget: udc-core: independent registration of gadgets and gadget
> drivers
>
> Though I am still seeing some occasional lockups. Thanks,
>

Original version of this patch had checking of input parameters
(see change in usb_gadget_unregister_driver at
https://lkml.org/lkml/2015/6/22/559) but this approach was
rejected by Alan Stern many times so final version pushed by
Marek Szyprowski doesn't have it, and we have this NULL pointer
dereference.
But this means there is a bug in GadgetFS that was hidden
previously: it tries to deregister gadget driver that was not previously
registered. Previous implementation was returning -ENODEV
retval, current one just does NULL pointer dereference.

At least need to fix GadgetFS that seems to by buggy in
gadget driver unregistering, and I still say we need to nave
input parameters checking in externally visible functions like
usb_gadget_unregister_driver().

Meanwhile you can try following patch that returns checking of input
parameters back and see if it helps.

Best regards,
Ruslan

--------

diff --git a/drivers/usb/gadget/udc/udc-core.c
b/drivers/usb/gadget/udc/udc-core.c
index fd73a3e..f1d375f 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -597,9 +597,16 @@ int usb_gadget_unregister_driver(struct
usb_gadget_driver *driver)
                }

        if (ret) {
-               list_del(&driver->pending);
-               ret = 0;
+               struct usb_gadget_driver *tmp;
+
+               list_for_each_entry(tmp, &gadget_driver_pending_list, pending)
+                       if (tmp == driver) {
+                               list_del(&driver->pending);
+                               ret = 0;
+                               break;
+                       }
        }
+
        mutex_unlock(&udc_lock);
        return ret;
 }

Re: gadgetfs regression (NULL ptr deref) since v4.4-rc7

[Marek Szyprowski]

Hello,

On 2016-02-08 10:46, Ruslan Bilovol wrote:
> Hi Vegard,
>
> On Mon, Feb 8, 2016 at 1:27 AM, Vegard Nossum <vegard.nossum@oracle.com> wrote:
>> Hi,
>>
>> Using gadgetfs on latest mainline, I get the following NULL pointer
>> dereference:
>>
>> BUG: unable to handle kernel NULL pointer dereference at           (null)
>> IP: [<ffffffff81138e89>] __list_del_entry+0x29/0xc0
>> PGD 34f067 PUD 355067 PMD 0
>> Oops: 0000 [#1] DEBUG_PAGEALLOC
>> CPU: 0 PID: 35 Comm: afl-fuzz Not tainted 4.5.0-rc2 #1
>> task: ffff8800002b1ec0 ti: ffff88000033c000 task.ti: ffff88000033c000
>> RIP: 0010:[<ffffffff81138e89>]  [<ffffffff81138e89>]
>> __list_del_entry+0x29/0xc0
>> RSP: 0018:ffff88000033fe30  EFLAGS: 00010207
>> RAX: 0000000000000000 RBX: ffffffff8138c108 RCX: dead000000000200
>> RDX: 0000000000000000 RSI: 0000000000000061 RDI: ffffffff8138c108
>> RBP: ffff88000033fe30 R08: 0000000000000000 R09: 0000000000000000
>> R10: 0000000000000000 R11: 0000000000000246 R12: ffffffff8138b928
>> R13: ffffffff8138c040 R14: ffff88000ec0da20 R15: ffff88000033ff58
>> FS:  00007ffff7ff2740(0000) GS:ffffffff8135d000(0000) knlGS:0000000000000000
>> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> CR2: 0000000000000000 CR3: 0000000000335000 CR4: 00000000001406b0
>> Stack:
>>   ffff88000033fe48 ffffffff81138f2d ffffffff8138bbd0 ffff88000033fe70
>>   ffffffff811c880b ffff8800002f3000 ffff88000ec109a0 ffff880000298aa0
>>   ffff88000033fe88 ffffffff811ce040 ffff88000034cdc0 ffff88000033feb8
>> Call Trace:
>>   [<ffffffff81138f2d>] list_del+0xd/0x30
>>   [<ffffffff811c880b>] usb_gadget_unregister_driver+0xdb/0xf0
>>   [<ffffffff811ce040>] dev_release+0x20/0x60
>>   [<ffffffff810b464c>] __fput+0x4c/0x130
>>   [<ffffffff810b4769>] ____fput+0x9/0x10
>>   [<ffffffff81048577>] task_work_run+0x67/0xa0
>>   [<ffffffff81000dcf>] exit_to_usermode_loop+0x8f/0xa0
>>   [<ffffffff8100105d>] syscall_return_slowpath+0x3d/0x40
>>   [<ffffffff81278b89>] int_ret_from_sys_call+0x25/0x8f
>> Code: ff ff 55 48 8b 07 48 b9 00 01 00 00 00 00 ad de 48 8b 57 08 48 89 e5
>> 48 39 c8 74 29 48 b9 00 02 00 00 00 00 ad de 48 39 ca 74 3a <4c> 8b 02 4c 39
>> c7 75 52 4c 8b 40 08 4c 39 c7 75 66 48 89 50 08
>> RIP  [<ffffffff81138e89>] __list_del_entry+0x29/0xc0
>>   RSP <ffff88000033fe30>
>> CR2: 0000000000000000
>> ---[ end trace e6cfe1de661dcffe ]---
>>
>> Reverting this commit fixes the problem for me:
>>
>> commit 855ed04a3758b205e84b269f92d26ab36ed8e2f7
>> Author: Ruslan Bilovol <ruslan.bilovol@gmail.com>
>> Date:   Mon Nov 23 09:56:38 2015 +0100
>>
>>      usb: gadget: udc-core: independent registration of gadgets and gadget
>> drivers
>>
>> Though I am still seeing some occasional lockups. Thanks,
>>
> Original version of this patch had checking of input parameters
> (see change in usb_gadget_unregister_driver at
> https://lkml.org/lkml/2015/6/22/559) but this approach was
> rejected by Alan Stern many times so final version pushed by
> Marek Szyprowski doesn't have it, and we have this NULL pointer
> dereference.
> But this means there is a bug in GadgetFS that was hidden
> previously: it tries to deregister gadget driver that was not previously
> registered. Previous implementation was returning -ENODEV
> retval, current one just does NULL pointer dereference.
>
> At least need to fix GadgetFS that seems to by buggy in
> gadget driver unregistering, and I still say we need to nave
> input parameters checking in externally visible functions like
> usb_gadget_unregister_driver().
>
> Meanwhile you can try following patch that returns checking of input
> parameters back and see if it helps.
>
> Best regards,
> Ruslan
>
> --------
>
> diff --git a/drivers/usb/gadget/udc/udc-core.c
> b/drivers/usb/gadget/udc/udc-core.c
> index fd73a3e..f1d375f 100644
> --- a/drivers/usb/gadget/udc/udc-core.c
> +++ b/drivers/usb/gadget/udc/udc-core.c
> @@ -597,9 +597,16 @@ int usb_gadget_unregister_driver(struct
> usb_gadget_driver *driver)
>                  }
>
>          if (ret) {
> -               list_del(&driver->pending);
> -               ret = 0;
> +               struct usb_gadget_driver *tmp;
> +
> +               list_for_each_entry(tmp, &gadget_driver_pending_list, pending)
> +                       if (tmp == driver) {
> +                               list_del(&driver->pending);
> +                               ret = 0;
> +                               break;
> +                       }
>          }
> +
>          mutex_unlock(&udc_lock);
>          return ret;
>   }

Sorry, but this is not the solution. If gadgetfs is broken, then it
should be fixed. There is no point in adding workarounds in core just
because there are drivers that can be broken.

I was still not able to reproduce this issue (tried with dwc2, gadgetfs
and ptp gadget daemon). Vegard could you prepare a step-by-step
instruction how to reproduce this problem?

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: gadgetfs regression (NULL ptr deref) since v4.4-rc7

[Vegard Nossum]

(fixed Felipe Balbi e-mail address)

On 02/08/2016 11:19 AM, Marek Szyprowski wrote:
> Hello,
>
> On 2016-02-08 10:46, Ruslan Bilovol wrote:
>> Hi Vegard,
>>
>> On Mon, Feb 8, 2016 at 1:27 AM, Vegard Nossum
>> <vegard.nossum@oracle.com> wrote:
>>> Hi,
>>>
>>> Using gadgetfs on latest mainline, I get the following NULL pointer
>>> dereference:
>>>
>>> BUG: unable to handle kernel NULL pointer dereference at
>>> (null)
>>> IP: [<ffffffff81138e89>] __list_del_entry+0x29/0xc0
>>> PGD 34f067 PUD 355067 PMD 0
>>> Oops: 0000 [#1] DEBUG_PAGEALLOC
>>> CPU: 0 PID: 35 Comm: afl-fuzz Not tainted 4.5.0-rc2 #1
>>> task: ffff8800002b1ec0 ti: ffff88000033c000 task.ti: ffff88000033c000
>>> RIP: 0010:[<ffffffff81138e89>]  [<ffffffff81138e89>]
>>> __list_del_entry+0x29/0xc0
>>> RSP: 0018:ffff88000033fe30  EFLAGS: 00010207
>>> RAX: 0000000000000000 RBX: ffffffff8138c108 RCX: dead000000000200
>>> RDX: 0000000000000000 RSI: 0000000000000061 RDI: ffffffff8138c108
>>> RBP: ffff88000033fe30 R08: 0000000000000000 R09: 0000000000000000
>>> R10: 0000000000000000 R11: 0000000000000246 R12: ffffffff8138b928
>>> R13: ffffffff8138c040 R14: ffff88000ec0da20 R15: ffff88000033ff58
>>> FS:  00007ffff7ff2740(0000) GS:ffffffff8135d000(0000)
>>> knlGS:0000000000000000
>>> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>> CR2: 0000000000000000 CR3: 0000000000335000 CR4: 00000000001406b0
>>> Stack:
>>>   ffff88000033fe48 ffffffff81138f2d ffffffff8138bbd0 ffff88000033fe70
>>>   ffffffff811c880b ffff8800002f3000 ffff88000ec109a0 ffff880000298aa0
>>>   ffff88000033fe88 ffffffff811ce040 ffff88000034cdc0 ffff88000033feb8
>>> Call Trace:
>>>   [<ffffffff81138f2d>] list_del+0xd/0x30
>>>   [<ffffffff811c880b>] usb_gadget_unregister_driver+0xdb/0xf0
>>>   [<ffffffff811ce040>] dev_release+0x20/0x60
>>>   [<ffffffff810b464c>] __fput+0x4c/0x130
>>>   [<ffffffff810b4769>] ____fput+0x9/0x10
>>>   [<ffffffff81048577>] task_work_run+0x67/0xa0
>>>   [<ffffffff81000dcf>] exit_to_usermode_loop+0x8f/0xa0
>>>   [<ffffffff8100105d>] syscall_return_slowpath+0x3d/0x40
>>>   [<ffffffff81278b89>] int_ret_from_sys_call+0x25/0x8f
>>> Code: ff ff 55 48 8b 07 48 b9 00 01 00 00 00 00 ad de 48 8b 57 08 48
>>> 89 e5
>>> 48 39 c8 74 29 48 b9 00 02 00 00 00 00 ad de 48 39 ca 74 3a <4c> 8b
>>> 02 4c 39
>>> c7 75 52 4c 8b 40 08 4c 39 c7 75 66 48 89 50 08
>>> RIP  [<ffffffff81138e89>] __list_del_entry+0x29/0xc0
>>>   RSP <ffff88000033fe30>
>>> CR2: 0000000000000000
>>> ---[ end trace e6cfe1de661dcffe ]---
>>>
>>> Reverting this commit fixes the problem for me:
>>>
>>> commit 855ed04a3758b205e84b269f92d26ab36ed8e2f7
>>> Author: Ruslan Bilovol <ruslan.bilovol@gmail.com>
>>> Date:   Mon Nov 23 09:56:38 2015 +0100
>>>
>>>      usb: gadget: udc-core: independent registration of gadgets and
>>> gadget
>>> drivers
>>>
>>> Though I am still seeing some occasional lockups. Thanks,
>>>
>> Original version of this patch had checking of input parameters
>> (see change in usb_gadget_unregister_driver at
>> https://lkml.org/lkml/2015/6/22/559) but this approach was
>> rejected by Alan Stern many times so final version pushed by
>> Marek Szyprowski doesn't have it, and we have this NULL pointer
>> dereference.
>> But this means there is a bug in GadgetFS that was hidden
>> previously: it tries to deregister gadget driver that was not previously
>> registered. Previous implementation was returning -ENODEV
>> retval, current one just does NULL pointer dereference.
>>
>> At least need to fix GadgetFS that seems to by buggy in
>> gadget driver unregistering, and I still say we need to nave
>> input parameters checking in externally visible functions like
>> usb_gadget_unregister_driver().
>>
>> Meanwhile you can try following patch that returns checking of input
>> parameters back and see if it helps.
>>
>> Best regards,
>> Ruslan
>>
>> --------
>>
>> diff --git a/drivers/usb/gadget/udc/udc-core.c
>> b/drivers/usb/gadget/udc/udc-core.c
>> index fd73a3e..f1d375f 100644
>> --- a/drivers/usb/gadget/udc/udc-core.c
>> +++ b/drivers/usb/gadget/udc/udc-core.c
>> @@ -597,9 +597,16 @@ int usb_gadget_unregister_driver(struct
>> usb_gadget_driver *driver)
>>                  }
>>
>>          if (ret) {
>> -               list_del(&driver->pending);
>> -               ret = 0;
>> +               struct usb_gadget_driver *tmp;
>> +
>> +               list_for_each_entry(tmp, &gadget_driver_pending_list,
>> pending)
>> +                       if (tmp == driver) {
>> +                               list_del(&driver->pending);
>> +                               ret = 0;
>> +                               break;
>> +                       }
>>          }
>> +
>>          mutex_unlock(&udc_lock);
>>          return ret;
>>   }
>

So the patch *seems* to fix the problem I was seeing. However...

> Sorry, but this is not the solution. If gadgetfs is broken, then it
> should be fixed. There is no point in adding workarounds in core just
> because there are drivers that can be broken.

Even if it's not a fix per se, why not modify it to throw a warning
instead of crashing? If I understand correctly, something like

+WARN_ON_ONCE(ret);

after the loop in Ruslan's patch should be enough. I added a BUG_ON(ret)
(instead of WARN_ON_ONCE) and I got this:

------------[ cut here ]------------
kernel BUG at drivers/usb/gadget/udc/udc-core.c:622!
invalid opcode: 0000 [#1] DEBUG_PAGEALLOC KASAN
CPU: 0 PID: 36 Comm: afl-fuzz Not tainted 4.5.0-rc2 #1
task: ffff8800003b6900 ti: ffff88000c840000 task.ti: ffff88000c840000
RIP: 0010:[<ffffffff8152395b>]  [<ffffffff8152395b>] 
usb_gadget_unregister_driver+0x18b/0x2d0
RSP: 0018:ffff88000c847de8  EFLAGS: 00010202
RAX: 0000000000000000 RBX: ffffffff81a117d8 RCX: 1ffff10001908fa6
RDX: 0000000000000001 RSI: 0000000000000061 RDI: ffff88000039c418
RBP: ffff88000c847e10 R08: 0000000000000246 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: ffffffff81a118e0
R13: ffffffff81a13f40 R14: dffffc0000000000 R15: ffff88000c83ca00
FS:  00007ffff7ff2740(0000) GS:ffffffff8193f000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007ffff78c53a0 CR3: 000000000c850000 CR4: 00000000001406b0
Stack:
  ffffffff817f62a0 ffff880000277a40 ffff88000c83ca10 ffff88000c83ca20
  ffff88000c83ca18 ffff88000c847e30 ffffffff81533bb4 ffff88000cc25758
  ffff88000c83ca10 ffff88000c847e88 ffffffff811f0ccb 00007ffff780f0c0
Call Trace:
  [<ffffffff81533bb4>] dev_release+0x44/0x110
  [<ffffffff811f0ccb>] __fput+0x11b/0x490
  [<ffffffff811f10a9>] ____fput+0x9/0x10
  [<ffffffff810c8881>] task_work_run+0xf1/0x190
  [<ffffffff811ea9ea>] ? filp_close+0x8a/0xe0
  [<ffffffff81001c3c>] exit_to_usermode_loop+0xec/0x100
  [<ffffffff81002531>] syscall_return_slowpath+0x91/0xc0
  [<ffffffff817a4309>] int_ret_from_sys_call+0x25/0x8f
Code: f8 48 c1 e8 03 42 80 3c 20 00 0f 85 1b 01 00 00 48 8b 83 c8 00 00 
00 48 3d a0 18 a1 81 48 8d 98 38 ff ff ff 75 be e8 45 b6 e6 ff <0f> 0b 
e8 3e b6 e6 ff 48 89 df e8 26 d0 ff ff 48 8d 7b 08 48 b8
RIP  [<ffffffff8152395b>] usb_gadget_unregister_driver+0x18b/0x2d0
  RSP <ffff88000c847de8>
---[ end trace 875139a9f2b43c09 ]---

> I was still not able to reproduce this issue (tried with dwc2, gadgetfs
> and ptp gadget daemon). Vegard could you prepare a step-by-step
> instruction how to reproduce this problem?

It's a bit complicated to reproduce (and not 100% deterministic) since I
am running into this using a fuzzer.

But I am just using dummy_hcd in the following way:

  - mount gadgetfs
  - open dummy_hcd file
  - read/write some data
  - close dummy_hcd file
  - unmount gadgetfs

I will write back if I find an easy way to reproduce it. In the meantime
I can test patches easily.


Vegard

[PATCH] usb: gadget: provide interface for legacy gadgets to get UDC name

[Marek Szyprowski]

Since commit 855ed04a3758b205e84b269f92d26ab36ed8e2f7 ("usb: gadget:
udc-core: independent registration of gadgets and gadget drivers") gadget
drivers can not assume that UDC drivers are already available on their
initialization. This broke the HACK, which was used in gadgetfs driver,
to get UDC controller name. This patch removes this hack and replaces it
by additional function in the UDC core (which is usefully only for legacy
drivers, please don't use it in the new code).

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
Vegard: Could you check if this patch fixes your issue with gadgetfs and NULL
pointer dereference?

Best regards,
Marek Szyprowski
---
 drivers/usb/gadget/legacy/inode.c | 29 ++---------------------------
 drivers/usb/gadget/udc/udc-core.c | 33 +++++++++++++++++++++++++++++++++
 include/linux/usb/gadget.h        |  1 +
 3 files changed, 36 insertions(+), 27 deletions(-)

diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c
index 7e179f81d05c..7a62a2f7bc18 100644
--- a/drivers/usb/gadget/legacy/inode.c
+++ b/drivers/usb/gadget/legacy/inode.c
@@ -227,7 +227,7 @@ static void put_ep (struct ep_data *data)
  * implicitly, starting with the driver name and then endpoint names.
  */
 
-static const char *CHIP;
+static char CHIP[32];
 
 /*----------------------------------------------------------------------*/
 
@@ -1697,28 +1697,6 @@ static struct usb_gadget_driver gadgetfs_driver = {
 };
 
 /*----------------------------------------------------------------------*/
-
-static void gadgetfs_nop(struct usb_gadget *arg) { }
-
-static int gadgetfs_probe(struct usb_gadget *gadget,
-		struct usb_gadget_driver *driver)
-{
-	CHIP = gadget->name;
-	return -EISNAM;
-}
-
-static struct usb_gadget_driver probe_driver = {
-	.max_speed	= USB_SPEED_HIGH,
-	.bind		= gadgetfs_probe,
-	.unbind		= gadgetfs_nop,
-	.setup		= (void *)gadgetfs_nop,
-	.disconnect	= gadgetfs_nop,
-	.driver	= {
-		.name		= "nop",
-	},
-};
-
-
 /* DEVICE INITIALIZATION
  *
  *     fd = open ("/dev/gadget/$CHIP", O_RDWR)
@@ -1968,10 +1946,7 @@ gadgetfs_fill_super (struct super_block *sb, void *opts, int silent)
 	if (the_device)
 		return -ESRCH;
 
-	/* fake probe to determine $CHIP */
-	CHIP = NULL;
-	usb_gadget_probe_driver(&probe_driver);
-	if (!CHIP)
+	if (usb_get_gadget_udc_name(CHIP, sizeof(CHIP)) != 0)
 		return -ENODEV;
 
 	/* superblock */
diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
index fd73a3ea07c2..eb70051d5949 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -442,6 +442,39 @@ err1:
 EXPORT_SYMBOL_GPL(usb_add_gadget_udc_release);
 
 /**
+ * usb_get_gadget_udc_name - get the name of the first UDC controller
+ * @dst_name
+ * @len
+ * This functions returns the name of the first UDC controller in the system.
+ * Please note that this interface is usefull only for legacy drivers which
+ * assume that there is only one UDC controller in the system and they need to
+ * get its name before initialization. There is no guarantee that the UDC
+ * of the returned name will be still available, when gadget driver registers
+ * itself.
+ *
+ * Returns zero on success, negative errno otherwise.
+ */
+int usb_get_gadget_udc_name(char *dst_name, int len)
+{
+	struct usb_udc *udc;
+	int ret = -ENODEV;
+
+	mutex_lock(&udc_lock);
+	list_for_each_entry(udc, &udc_list, list) {
+		const char *name = dev_name(&udc->dev);
+		/* For now we take the first one */
+		if (!udc->driver && strlen(name) + 1 <= len) {
+			strcpy(dst_name, name);
+			ret = 0;
+			break;
+		}
+	}
+	mutex_unlock(&udc_lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(usb_get_gadget_udc_name);
+
+/**
  * usb_add_gadget_udc - adds a new gadget to the udc class driver list
  * @parent: the parent device to this udc. Usually the controller
  * driver's device.
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index d82d0068872b..0a8f08302a34 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -1126,6 +1126,7 @@ extern int usb_add_gadget_udc_release(struct device *parent,
 		struct usb_gadget *gadget, void (*release)(struct device *dev));
 extern int usb_add_gadget_udc(struct device *parent, struct usb_gadget *gadget);
 extern void usb_del_gadget_udc(struct usb_gadget *gadget);
+extern int usb_get_gadget_udc_name(char *dst_name, int len);
 
 /*-------------------------------------------------------------------------*/
 
-- 
1.9.2

Re: [PATCH] usb: gadget: provide interface for legacy gadgets to get UDC name

[Vegard Nossum]

On 02/08/2016 12:12 PM, Marek Szyprowski wrote:
> Since commit 855ed04a3758b205e84b269f92d26ab36ed8e2f7 ("usb: gadget:
> udc-core: independent registration of gadgets and gadget drivers") gadget
> drivers can not assume that UDC drivers are already available on their
> initialization. This broke the HACK, which was used in gadgetfs driver,
> to get UDC controller name. This patch removes this hack and replaces it
> by additional function in the UDC core (which is usefully only for legacy
> drivers, please don't use it in the new code).
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> Vegard: Could you check if this patch fixes your issue with gadgetfs and NULL
> pointer dereference?
>
> Best regards,
> Marek Szyprowski
> ---

[snip patch]

Thanks for the patch, I gave it a try.

Firstly, it changes /dev/gadget/dummy_udc into /dev/gadget/dummy_udc.0
so it may break some userspace expectations (I don't really know).

Secondly, I still get this crash which looks a lot like what I
originally reported:

kasan: GPF could be caused by NULL-ptr deref or user memory 
accessgeneral protection fault: 0000 [#1] DEBUG_PAGEALLOC KASAN
CPU: 0 PID: 35 Comm: afl-fuzz Not tainted 4.5.0-rc2 #1
task: ffff8800003b6900 ti: ffff88000c840000 task.ti: ffff88000c840000
RIP: 0010:[<ffffffff81388536>]  [<ffffffff81388536>] 
__list_del_entry+0x86/0x1d0
RSP: 0018:ffff88000c847da8  EFLAGS: 00010246
RAX: dffffc0000000000 RBX: 0000000000000000 RCX: ffffffff81a13f08
RDX: 0000000000000000 RSI: 0000000000000061 RDI: ffffffff81a13f10
RBP: ffff88000c847dc0 R08: 0000000000000246 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
R13: dffffc0000000000 R14: ffffffff81a13e40 R15: ffff88000c83c500
FS:  00007ffff7ff2740(0000) GS:ffffffff8193f000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007ffff78c53a0 CR3: 000000000c850000 CR4: 00000000001406b0
Stack:
  ffffffff81a13e40 ffffffff81a13f08 ffffffff81a118e0 ffff88000c847dd8
  ffffffff8138868d ffffffff81a11638 ffff88000c847e10 ffffffff81523a5d
  ffffffff817f62a0 ffff880000277a40 ffff88000c83c510 ffff88000c83c520
Call Trace:
  [<ffffffff8138868d>] list_del+0xd/0x70
  [<ffffffff81523a5d>] usb_gadget_unregister_driver+0x11d/0x240
  [<ffffffff81533c34>] dev_release+0x44/0x110
  [<ffffffff811f0ccb>] __fput+0x11b/0x490
  [<ffffffff811f10a9>] ____fput+0x9/0x10
  [<ffffffff810c8881>] task_work_run+0xf1/0x190
  [<ffffffff811ea9ea>] ? filp_close+0x8a/0xe0
  [<ffffffff81001c3c>] exit_to_usermode_loop+0xec/0x100
  [<ffffffff81002531>] syscall_return_slowpath+0x91/0xc0
  [<ffffffff817a4389>] int_ret_from_sys_call+0x25/0x8f
Code: c4 0f 84 94 00 00 00 48 b8 00 02 00 00 00 00 ad de 48 39 c3 0f 84 
a5 00 00 00 48 89 da 48 b8 00 00 00 00 00 fc ff df 48 c1 ea 03 <80> 3c 
02 00 0f 85 e8 00 00 00 4c 8b 03 4c 39 c1 0f 85 9b 00 00
RIP  [<ffffffff81388536>] __list_del_entry+0x86/0x1d0
  RSP <ffff88000c847da8>
---[ end trace 9a6416535ca1ec01 ]---

I am more than happy to try other patches :-) Thanks,


Vegard

[PATCH] usb: gadget: gadgetfs: unregister gadget only if it got successfully registered

[Marek Szyprowski]

Gadgetfs driver called usb_gadget_unregister_driver unconditionally, even
if it didn't register it earlier due to other failures. This patch fixes
this.

Reported-by: Vegard Nossum <vegard.nossum@oracle.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/usb/gadget/legacy/inode.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c
index 7a62a2f7bc18..cde6a2133c90 100644
--- a/drivers/usb/gadget/legacy/inode.c
+++ b/drivers/usb/gadget/legacy/inode.c
@@ -130,7 +130,8 @@ struct dev_data {
 					setup_can_stall : 1,
 					setup_out_ready : 1,
 					setup_out_error : 1,
-					setup_abort : 1;
+					setup_abort : 1,
+					gadget_registered : 1;
 	unsigned			setup_wLength;
 
 	/* the rest is basically write-once */
@@ -1179,7 +1180,8 @@ dev_release (struct inode *inode, struct file *fd)
 
 	/* closing ep0 === shutdown all */
 
-	usb_gadget_unregister_driver (&gadgetfs_driver);
+	if (dev->gadget_registered)
+		usb_gadget_unregister_driver (&gadgetfs_driver);
 
 	/* at this point "good" hardware has disconnected the
 	 * device from USB; the host won't see it any more.
@@ -1825,6 +1827,7 @@ dev_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
 		 * kick in after the ep0 descriptor is closed.
 		 */
 		value = len;
+		dev->gadget_registered = true;
 	}
 	return value;
 
-- 
1.9.2

[PATCH v2] usb: gadget: provide interface for legacy gadgets to get UDC name

[Marek Szyprowski]

Since commit 855ed04a3758b205e84b269f92d26ab36ed8e2f7 ("usb: gadget:
udc-core: independent registration of gadgets and gadget drivers") gadget
drivers can not assume that UDC drivers are already available on their
initialization. This broke the HACK, which was used in gadgetfs driver,
to get UDC controller name. This patch removes this hack and replaces it
by additional function in the UDC core (which is usefully only for legacy
drivers, please don't use it in the new code).

Reported-by: Vegard Nossum <vegard.nossum@oracle.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
changelog:
v2:
- properly report udc gagdet driver name instead of udc device name

v1: https://lkml.org/lkml/2016/2/8/176
- initial version
---
 drivers/usb/gadget/legacy/inode.c | 29 ++---------------------------
 drivers/usb/gadget/udc/udc-core.c | 33 +++++++++++++++++++++++++++++++++
 include/linux/usb/gadget.h        |  1 +
 3 files changed, 36 insertions(+), 27 deletions(-)

diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c
index 7e179f81d05c..7a62a2f7bc18 100644
--- a/drivers/usb/gadget/legacy/inode.c
+++ b/drivers/usb/gadget/legacy/inode.c
@@ -227,7 +227,7 @@ static void put_ep (struct ep_data *data)
  * implicitly, starting with the driver name and then endpoint names.
  */
 
-static const char *CHIP;
+static char CHIP[32];
 
 /*----------------------------------------------------------------------*/
 
@@ -1697,28 +1697,6 @@ static struct usb_gadget_driver gadgetfs_driver = {
 };
 
 /*----------------------------------------------------------------------*/
-
-static void gadgetfs_nop(struct usb_gadget *arg) { }
-
-static int gadgetfs_probe(struct usb_gadget *gadget,
-		struct usb_gadget_driver *driver)
-{
-	CHIP = gadget->name;
-	return -EISNAM;
-}
-
-static struct usb_gadget_driver probe_driver = {
-	.max_speed	= USB_SPEED_HIGH,
-	.bind		= gadgetfs_probe,
-	.unbind		= gadgetfs_nop,
-	.setup		= (void *)gadgetfs_nop,
-	.disconnect	= gadgetfs_nop,
-	.driver	= {
-		.name		= "nop",
-	},
-};
-
-
 /* DEVICE INITIALIZATION
  *
  *     fd = open ("/dev/gadget/$CHIP", O_RDWR)
@@ -1968,10 +1946,7 @@ gadgetfs_fill_super (struct super_block *sb, void *opts, int silent)
 	if (the_device)
 		return -ESRCH;
 
-	/* fake probe to determine $CHIP */
-	CHIP = NULL;
-	usb_gadget_probe_driver(&probe_driver);
-	if (!CHIP)
+	if (usb_get_gadget_udc_name(CHIP, sizeof(CHIP)) != 0)
 		return -ENODEV;
 
 	/* superblock */
diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
index fd73a3ea07c2..4bde2e110e44 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -442,6 +442,39 @@ err1:
 EXPORT_SYMBOL_GPL(usb_add_gadget_udc_release);
 
 /**
+ * usb_get_gadget_udc_name - get the name of the first UDC controller
+ * @dst_name
+ * @len
+ * This functions returns the name of the first UDC controller in the system.
+ * Please note that this interface is usefull only for legacy drivers which
+ * assume that there is only one UDC controller in the system and they need to
+ * get its name before initialization. There is no guarantee that the UDC
+ * of the returned name will be still available, when gadget driver registers
+ * itself.
+ *
+ * Returns zero on success, negative errno otherwise.
+ */
+int usb_get_gadget_udc_name(char *dst_name, int len)
+{
+	struct usb_udc *udc;
+	int ret = -ENODEV;
+
+	mutex_lock(&udc_lock);
+	list_for_each_entry(udc, &udc_list, list) {
+		const char *name = udc->gadget->name;
+		/* For now we take the first one */
+		if (!udc->driver && strlen(name) + 1 <= len) {
+			strcpy(dst_name, name);
+			ret = 0;
+			break;
+		}
+	}
+	mutex_unlock(&udc_lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(usb_get_gadget_udc_name);
+
+/**
  * usb_add_gadget_udc - adds a new gadget to the udc class driver list
  * @parent: the parent device to this udc. Usually the controller
  * driver's device.
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index d82d0068872b..0a8f08302a34 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -1126,6 +1126,7 @@ extern int usb_add_gadget_udc_release(struct device *parent,
 		struct usb_gadget *gadget, void (*release)(struct device *dev));
 extern int usb_add_gadget_udc(struct device *parent, struct usb_gadget *gadget);
 extern void usb_del_gadget_udc(struct usb_gadget *gadget);
+extern int usb_get_gadget_udc_name(char *dst_name, int len);
 
 /*-------------------------------------------------------------------------*/
 
-- 
1.9.2

Re: [PATCH] usb: gadget: provide interface for legacy gadgets to get UDC name

[Marek Szyprowski]

Hello,

On 2016-02-08 12:31, Vegard Nossum wrote:
> On 02/08/2016 12:12 PM, Marek Szyprowski wrote:
>> Since commit 855ed04a3758b205e84b269f92d26ab36ed8e2f7 ("usb: gadget:
>> udc-core: independent registration of gadgets and gadget drivers") 
>> gadget
>> drivers can not assume that UDC drivers are already available on their
>> initialization. This broke the HACK, which was used in gadgetfs driver,
>> to get UDC controller name. This patch removes this hack and replaces it
>> by additional function in the UDC core (which is usefully only for 
>> legacy
>> drivers, please don't use it in the new code).
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>> Vegard: Could you check if this patch fixes your issue with gadgetfs 
>> and NULL
>> pointer dereference?
>>
>> Best regards,
>> Marek Szyprowski
>> ---
>
> [snip patch]
>
> Thanks for the patch, I gave it a try.
>
> Firstly, it changes /dev/gadget/dummy_udc into /dev/gadget/dummy_udc.0
> so it may break some userspace expectations (I don't really know).

Right, thanks for pointing this issue.

>
> Secondly, I still get this crash which looks a lot like what I
> originally reported:
>
> kasan: GPF could be caused by NULL-ptr deref or user memory 
> accessgeneral protection fault: 0000 [#1] DEBUG_PAGEALLOC KASAN
> CPU: 0 PID: 35 Comm: afl-fuzz Not tainted 4.5.0-rc2 #1
> task: ffff8800003b6900 ti: ffff88000c840000 task.ti: ffff88000c840000
> RIP: 0010:[<ffffffff81388536>]  [<ffffffff81388536>] 
> __list_del_entry+0x86/0x1d0
> RSP: 0018:ffff88000c847da8  EFLAGS: 00010246
> RAX: dffffc0000000000 RBX: 0000000000000000 RCX: ffffffff81a13f08
> RDX: 0000000000000000 RSI: 0000000000000061 RDI: ffffffff81a13f10
> RBP: ffff88000c847dc0 R08: 0000000000000246 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
> R13: dffffc0000000000 R14: ffffffff81a13e40 R15: ffff88000c83c500
> FS:  00007ffff7ff2740(0000) GS:ffffffff8193f000(0000) 
> knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007ffff78c53a0 CR3: 000000000c850000 CR4: 00000000001406b0
> Stack:
>  ffffffff81a13e40 ffffffff81a13f08 ffffffff81a118e0 ffff88000c847dd8
>  ffffffff8138868d ffffffff81a11638 ffff88000c847e10 ffffffff81523a5d
>  ffffffff817f62a0 ffff880000277a40 ffff88000c83c510 ffff88000c83c520
> Call Trace:
>  [<ffffffff8138868d>] list_del+0xd/0x70
>  [<ffffffff81523a5d>] usb_gadget_unregister_driver+0x11d/0x240
>  [<ffffffff81533c34>] dev_release+0x44/0x110
>  [<ffffffff811f0ccb>] __fput+0x11b/0x490
>  [<ffffffff811f10a9>] ____fput+0x9/0x10
>  [<ffffffff810c8881>] task_work_run+0xf1/0x190
>  [<ffffffff811ea9ea>] ? filp_close+0x8a/0xe0
>  [<ffffffff81001c3c>] exit_to_usermode_loop+0xec/0x100
>  [<ffffffff81002531>] syscall_return_slowpath+0x91/0xc0
>  [<ffffffff817a4389>] int_ret_from_sys_call+0x25/0x8f
> Code: c4 0f 84 94 00 00 00 48 b8 00 02 00 00 00 00 ad de 48 39 c3 0f 
> 84 a5 00 00 00 48 89 da 48 b8 00 00 00 00 00 fc ff df 48 c1 ea 03 <80> 
> 3c 02 00 0f 85 e8 00 00 00 4c 8b 03 4c 39 c1 0f 85 9b 00 00
> RIP  [<ffffffff81388536>] __list_del_entry+0x86/0x1d0
>  RSP <ffff88000c847da8>
> ---[ end trace 9a6416535ca1ec01 ]---
>
> I am more than happy to try other patches :-) Thanks,

Okay, now I managed to reproduce it and I've sent a fix for gadgetfs driver
a few minutes ago. When both patches are applied, no more issue should 
be observed.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] usb: gadget: gadgetfs: unregister gadget only if it got successfully registered

[Vegard Nossum]

On 02/08/2016 01:15 PM, Marek Szyprowski wrote:
> Gadgetfs driver called usb_gadget_unregister_driver unconditionally, even
> if it didn't register it earlier due to other failures. This patch fixes
> this.
>
> Reported-by: Vegard Nossum <vegard.nossum@oracle.com>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>   drivers/usb/gadget/legacy/inode.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c
> index 7a62a2f7bc18..cde6a2133c90 100644
> --- a/drivers/usb/gadget/legacy/inode.c
> +++ b/drivers/usb/gadget/legacy/inode.c
> @@ -130,7 +130,8 @@ struct dev_data {
>   					setup_can_stall : 1,
>   					setup_out_ready : 1,
>   					setup_out_error : 1,
> -					setup_abort : 1;
> +					setup_abort : 1,
> +					gadget_registered : 1;
>   	unsigned			setup_wLength;
>
>   	/* the rest is basically write-once */
> @@ -1179,7 +1180,8 @@ dev_release (struct inode *inode, struct file *fd)
>
>   	/* closing ep0 === shutdown all */
>
> -	usb_gadget_unregister_driver (&gadgetfs_driver);
> +	if (dev->gadget_registered)
> +		usb_gadget_unregister_driver (&gadgetfs_driver);
>
>   	/* at this point "good" hardware has disconnected the
>   	 * device from USB; the host won't see it any more.
> @@ -1825,6 +1827,7 @@ dev_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
>   		 * kick in after the ep0 descriptor is closed.
>   		 */
>   		value = len;
> +		dev->gadget_registered = true;
>   	}
>   	return value;
>
>

That (along with the first patch) fixes the problem! Thanks.

It also seems to have fixed another crash I was seeing with
req->complete == NULL in usb_gadget_giveback_request().


Vegard

Re: [PATCH] usb: gadget: gadgetfs: unregister gadget only if it got successfully registered

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1999 bytes --]


Hi,

Vegard Nossum <vegard.nossum@oracle.com> writes:
> On 02/08/2016 01:15 PM, Marek Szyprowski wrote:
>> Gadgetfs driver called usb_gadget_unregister_driver unconditionally, even
>> if it didn't register it earlier due to other failures. This patch fixes
>> this.
>>
>> Reported-by: Vegard Nossum <vegard.nossum@oracle.com>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>   drivers/usb/gadget/legacy/inode.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c
>> index 7a62a2f7bc18..cde6a2133c90 100644
>> --- a/drivers/usb/gadget/legacy/inode.c
>> +++ b/drivers/usb/gadget/legacy/inode.c
>> @@ -130,7 +130,8 @@ struct dev_data {
>>   					setup_can_stall : 1,
>>   					setup_out_ready : 1,
>>   					setup_out_error : 1,
>> -					setup_abort : 1;
>> +					setup_abort : 1,
>> +					gadget_registered : 1;
>>   	unsigned			setup_wLength;
>>
>>   	/* the rest is basically write-once */
>> @@ -1179,7 +1180,8 @@ dev_release (struct inode *inode, struct file *fd)
>>
>>   	/* closing ep0 === shutdown all */
>>
>> -	usb_gadget_unregister_driver (&gadgetfs_driver);
>> +	if (dev->gadget_registered)
>> +		usb_gadget_unregister_driver (&gadgetfs_driver);
>>
>>   	/* at this point "good" hardware has disconnected the
>>   	 * device from USB; the host won't see it any more.
>> @@ -1825,6 +1827,7 @@ dev_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
>>   		 * kick in after the ep0 descriptor is closed.
>>   		 */
>>   		value = len;
>> +		dev->gadget_registered = true;
>>   	}
>>   	return value;
>>
>>
>
> That (along with the first patch) fixes the problem! Thanks.
>
> It also seems to have fixed another crash I was seeing with
> req->complete == NULL in usb_gadget_giveback_request().

please resend both patches with proper Acks and Tested-bys so I can
apply them.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

[PATCH v2 RESEND] usb: gadget: provide interface for legacy gadgets to get UDC name

[Marek Szyprowski]

Since commit 855ed04a3758b205e84b269f92d26ab36ed8e2f7 ("usb: gadget:
udc-core: independent registration of gadgets and gadget drivers") gadget
drivers can not assume that UDC drivers are already available on their
initialization. This broke the HACK, which was used in gadgetfs driver,
to get UDC controller name. This patch removes this hack and replaces it
by additional function in the UDC core (which is usefully only for legacy
drivers, please don't use it in the new code).

Reported-by: Vegard Nossum <vegard.nossum@oracle.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Tested-by: Vegard Nossum <vegard.nossum@oracle.com>
---
changelog:
v2:
- properly report udc gagdet driver name instead of udc device name

v1: https://lkml.org/lkml/2016/2/8/176
- initial version
---
 drivers/usb/gadget/legacy/inode.c | 29 ++---------------------------
 drivers/usb/gadget/udc/udc-core.c | 33 +++++++++++++++++++++++++++++++++
 include/linux/usb/gadget.h        |  1 +
 3 files changed, 36 insertions(+), 27 deletions(-)

diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c
index 7e179f81d05c..7a62a2f7bc18 100644
--- a/drivers/usb/gadget/legacy/inode.c
+++ b/drivers/usb/gadget/legacy/inode.c
@@ -227,7 +227,7 @@ static void put_ep (struct ep_data *data)
  * implicitly, starting with the driver name and then endpoint names.
  */
 
-static const char *CHIP;
+static char CHIP[32];
 
 /*----------------------------------------------------------------------*/
 
@@ -1697,28 +1697,6 @@ static struct usb_gadget_driver gadgetfs_driver = {
 };
 
 /*----------------------------------------------------------------------*/
-
-static void gadgetfs_nop(struct usb_gadget *arg) { }
-
-static int gadgetfs_probe(struct usb_gadget *gadget,
-		struct usb_gadget_driver *driver)
-{
-	CHIP = gadget->name;
-	return -EISNAM;
-}
-
-static struct usb_gadget_driver probe_driver = {
-	.max_speed	= USB_SPEED_HIGH,
-	.bind		= gadgetfs_probe,
-	.unbind		= gadgetfs_nop,
-	.setup		= (void *)gadgetfs_nop,
-	.disconnect	= gadgetfs_nop,
-	.driver	= {
-		.name		= "nop",
-	},
-};
-
-
 /* DEVICE INITIALIZATION
  *
  *     fd = open ("/dev/gadget/$CHIP", O_RDWR)
@@ -1968,10 +1946,7 @@ gadgetfs_fill_super (struct super_block *sb, void *opts, int silent)
 	if (the_device)
 		return -ESRCH;
 
-	/* fake probe to determine $CHIP */
-	CHIP = NULL;
-	usb_gadget_probe_driver(&probe_driver);
-	if (!CHIP)
+	if (usb_get_gadget_udc_name(CHIP, sizeof(CHIP)) != 0)
 		return -ENODEV;
 
 	/* superblock */
diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
index fd73a3ea07c2..4bde2e110e44 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -442,6 +442,39 @@ err1:
 EXPORT_SYMBOL_GPL(usb_add_gadget_udc_release);
 
 /**
+ * usb_get_gadget_udc_name - get the name of the first UDC controller
+ * @dst_name
+ * @len
+ * This functions returns the name of the first UDC controller in the system.
+ * Please note that this interface is usefull only for legacy drivers which
+ * assume that there is only one UDC controller in the system and they need to
+ * get its name before initialization. There is no guarantee that the UDC
+ * of the returned name will be still available, when gadget driver registers
+ * itself.
+ *
+ * Returns zero on success, negative errno otherwise.
+ */
+int usb_get_gadget_udc_name(char *dst_name, int len)
+{
+	struct usb_udc *udc;
+	int ret = -ENODEV;
+
+	mutex_lock(&udc_lock);
+	list_for_each_entry(udc, &udc_list, list) {
+		const char *name = udc->gadget->name;
+		/* For now we take the first one */
+		if (!udc->driver && strlen(name) + 1 <= len) {
+			strcpy(dst_name, name);
+			ret = 0;
+			break;
+		}
+	}
+	mutex_unlock(&udc_lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(usb_get_gadget_udc_name);
+
+/**
  * usb_add_gadget_udc - adds a new gadget to the udc class driver list
  * @parent: the parent device to this udc. Usually the controller
  * driver's device.
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index d82d0068872b..0a8f08302a34 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -1126,6 +1126,7 @@ extern int usb_add_gadget_udc_release(struct device *parent,
 		struct usb_gadget *gadget, void (*release)(struct device *dev));
 extern int usb_add_gadget_udc(struct device *parent, struct usb_gadget *gadget);
 extern void usb_del_gadget_udc(struct usb_gadget *gadget);
+extern int usb_get_gadget_udc_name(char *dst_name, int len);
 
 /*-------------------------------------------------------------------------*/
 
-- 
1.9.2

[PATCH RESEND] usb: gadget: gadgetfs: unregister gadget only if it got successfully registered

[Marek Szyprowski]

Gadgetfs driver called usb_gadget_unregister_driver unconditionally, even
if it didn't register it earlier due to other failures. This patch fixes
this.

Reported-by: Vegard Nossum <vegard.nossum@oracle.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Tested-by: Vegard Nossum <vegard.nossum@oracle.com>
---
 drivers/usb/gadget/legacy/inode.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c
index 7a62a2f7bc18..cde6a2133c90 100644
--- a/drivers/usb/gadget/legacy/inode.c
+++ b/drivers/usb/gadget/legacy/inode.c
@@ -130,7 +130,8 @@ struct dev_data {
 					setup_can_stall : 1,
 					setup_out_ready : 1,
 					setup_out_error : 1,
-					setup_abort : 1;
+					setup_abort : 1,
+					gadget_registered : 1;
 	unsigned			setup_wLength;
 
 	/* the rest is basically write-once */
@@ -1179,7 +1180,8 @@ dev_release (struct inode *inode, struct file *fd)
 
 	/* closing ep0 === shutdown all */
 
-	usb_gadget_unregister_driver (&gadgetfs_driver);
+	if (dev->gadget_registered)
+		usb_gadget_unregister_driver (&gadgetfs_driver);
 
 	/* at this point "good" hardware has disconnected the
 	 * device from USB; the host won't see it any more.
@@ -1825,6 +1827,7 @@ dev_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
 		 * kick in after the ep0 descriptor is closed.
 		 */
 		value = len;
+		dev->gadget_registered = true;
 	}
 	return value;
 
-- 
1.9.2

Re: [PATCH v2 RESEND] usb: gadget: provide interface for legacy gadgets to get UDC name

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3343 bytes --]


Hi,

Marek Szyprowski <m.szyprowski@samsung.com> writes:
> Since commit 855ed04a3758b205e84b269f92d26ab36ed8e2f7 ("usb: gadget:
> udc-core: independent registration of gadgets and gadget drivers") gadget
> drivers can not assume that UDC drivers are already available on their
> initialization. This broke the HACK, which was used in gadgetfs driver,
> to get UDC controller name. This patch removes this hack and replaces it
> by additional function in the UDC core (which is usefully only for legacy
> drivers, please don't use it in the new code).
>
> Reported-by: Vegard Nossum <vegard.nossum@oracle.com>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Tested-by: Vegard Nossum <vegard.nossum@oracle.com>
> ---
> changelog:
> v2:
> - properly report udc gagdet driver name instead of udc device name
>
> v1: https://lkml.org/lkml/2016/2/8/176
> - initial version
> ---
>  drivers/usb/gadget/legacy/inode.c | 29 ++---------------------------
>  drivers/usb/gadget/udc/udc-core.c | 33 +++++++++++++++++++++++++++++++++
>  include/linux/usb/gadget.h        |  1 +
>  3 files changed, 36 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c
> index 7e179f81d05c..7a62a2f7bc18 100644
> --- a/drivers/usb/gadget/legacy/inode.c
> +++ b/drivers/usb/gadget/legacy/inode.c
> @@ -227,7 +227,7 @@ static void put_ep (struct ep_data *data)
>   * implicitly, starting with the driver name and then endpoint names.
>   */
>  
> -static const char *CHIP;
> +static char CHIP[32];
>  
>  /*----------------------------------------------------------------------*/
>  
> @@ -1697,28 +1697,6 @@ static struct usb_gadget_driver gadgetfs_driver = {
>  };
>  
>  /*----------------------------------------------------------------------*/
> -
> -static void gadgetfs_nop(struct usb_gadget *arg) { }
> -
> -static int gadgetfs_probe(struct usb_gadget *gadget,
> -		struct usb_gadget_driver *driver)
> -{
> -	CHIP = gadget->name;
> -	return -EISNAM;
> -}
> -
> -static struct usb_gadget_driver probe_driver = {
> -	.max_speed	= USB_SPEED_HIGH,
> -	.bind		= gadgetfs_probe,
> -	.unbind		= gadgetfs_nop,
> -	.setup		= (void *)gadgetfs_nop,
> -	.disconnect	= gadgetfs_nop,
> -	.driver	= {
> -		.name		= "nop",
> -	},
> -};
> -
> -
>  /* DEVICE INITIALIZATION
>   *
>   *     fd = open ("/dev/gadget/$CHIP", O_RDWR)
> @@ -1968,10 +1946,7 @@ gadgetfs_fill_super (struct super_block *sb, void *opts, int silent)
>  	if (the_device)
>  		return -ESRCH;
>  
> -	/* fake probe to determine $CHIP */
> -	CHIP = NULL;
> -	usb_gadget_probe_driver(&probe_driver);
> -	if (!CHIP)
> +	if (usb_get_gadget_udc_name(CHIP, sizeof(CHIP)) != 0)
>  		return -ENODEV;
>  
>  	/* superblock */
> diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
> index fd73a3ea07c2..4bde2e110e44 100644
> --- a/drivers/usb/gadget/udc/udc-core.c
> +++ b/drivers/usb/gadget/udc/udc-core.c
> @@ -442,6 +442,39 @@ err1:
>  EXPORT_SYMBOL_GPL(usb_add_gadget_udc_release);
>  
>  /**
> + * usb_get_gadget_udc_name - get the name of the first UDC controller
> + * @dst_name
> + * @len

gotta describe these and add a blank line after @len too.

This also seems a little too big for -rc.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

[PATCH v3] usb: gadget: provide interface for legacy gadgets to get UDC name

[Marek Szyprowski]

Since commit 855ed04a3758b205e84b269f92d26ab36ed8e2f7 ("usb: gadget:
udc-core: independent registration of gadgets and gadget drivers") gadget
drivers can not assume that UDC drivers are already available on their
initialization. This broke the HACK, which was used in gadgetfs driver,
to get UDC controller name. This patch removes this hack and replaces it
by additional function in the UDC core (which is usefully only for legacy
drivers, please don't use it in the new code).

Reported-by: Vegard Nossum <vegard.nossum@oracle.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Tested-by: Vegard Nossum <vegard.nossum@oracle.com>
---
changelog:
v3:
- simplified code a bit

v2: https://lkml.org/lkml/2016/2/8/234
- properly report udc gagdet driver name instead of udc device name

v1: https://lkml.org/lkml/2016/2/8/176
- initial version
---
 drivers/usb/gadget/legacy/inode.c | 28 +++-------------------------
 drivers/usb/gadget/udc/udc-core.c | 30 ++++++++++++++++++++++++++++++
 include/linux/usb/gadget.h        |  1 +
 3 files changed, 34 insertions(+), 25 deletions(-)

diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c
index 87fb0fd..5cdaf01 100644
--- a/drivers/usb/gadget/legacy/inode.c
+++ b/drivers/usb/gadget/legacy/inode.c
@@ -1699,28 +1699,6 @@ static struct usb_gadget_driver gadgetfs_driver = {
 };
 
 /*----------------------------------------------------------------------*/
-
-static void gadgetfs_nop(struct usb_gadget *arg) { }
-
-static int gadgetfs_probe(struct usb_gadget *gadget,
-		struct usb_gadget_driver *driver)
-{
-	CHIP = gadget->name;
-	return -EISNAM;
-}
-
-static struct usb_gadget_driver probe_driver = {
-	.max_speed	= USB_SPEED_HIGH,
-	.bind		= gadgetfs_probe,
-	.unbind		= gadgetfs_nop,
-	.setup		= (void *)gadgetfs_nop,
-	.disconnect	= gadgetfs_nop,
-	.driver	= {
-		.name		= "nop",
-	},
-};
-
-
 /* DEVICE INITIALIZATION
  *
  *     fd = open ("/dev/gadget/$CHIP", O_RDWR)
@@ -1971,9 +1949,7 @@ gadgetfs_fill_super (struct super_block *sb, void *opts, int silent)
 	if (the_device)
 		return -ESRCH;
 
-	/* fake probe to determine $CHIP */
-	CHIP = NULL;
-	usb_gadget_probe_driver(&probe_driver);
+	CHIP = usb_get_gadget_udc_name();
 	if (!CHIP)
 		return -ENODEV;
 
@@ -2034,6 +2010,8 @@ gadgetfs_kill_sb (struct super_block *sb)
 		put_dev (the_device);
 		the_device = NULL;
 	}
+	kfree(CHIP);
+	CHIP = NULL;
 }
 
 /*----------------------------------------------------------------------*/
diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
index fd73a3e..fd30242 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -442,6 +442,36 @@ err1:
 EXPORT_SYMBOL_GPL(usb_add_gadget_udc_release);
 
 /**
+ * usb_get_gadget_udc_name - get the name of the first UDC controller
+ * This functions returns the name of the first UDC controller in the system.
+ * Please note that this interface is usefull only for legacy drivers which
+ * assume that there is only one UDC controller in the system and they need to
+ * get its name before initialization. There is no guarantee that the UDC
+ * of the returned name will be still available, when gadget driver registers
+ * itself.
+ *
+ * Returns pointer to string with UDC controller name on success, NULL
+ * otherwise. Caller should kfree() returned string.
+ */
+char *usb_get_gadget_udc_name(void)
+{
+	struct usb_udc *udc;
+	char *name = NULL;
+
+	/* For now we take the first available UDC */
+	mutex_lock(&udc_lock);
+	list_for_each_entry(udc, &udc_list, list) {
+		if (!udc->driver) {
+			name = kstrdup(udc->gadget->name, GFP_KERNEL);
+			break;
+		}
+	}
+	mutex_unlock(&udc_lock);
+	return name;
+}
+EXPORT_SYMBOL_GPL(usb_get_gadget_udc_name);
+
+/**
  * usb_add_gadget_udc - adds a new gadget to the udc class driver list
  * @parent: the parent device to this udc. Usually the controller
  * driver's device.
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index d82d006..547d86d 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -1126,6 +1126,7 @@ extern int usb_add_gadget_udc_release(struct device *parent,
 		struct usb_gadget *gadget, void (*release)(struct device *dev));
 extern int usb_add_gadget_udc(struct device *parent, struct usb_gadget *gadget);
 extern void usb_del_gadget_udc(struct usb_gadget *gadget);
+extern char *usb_get_gadget_udc_name(void);
 
 /*-------------------------------------------------------------------------*/
 
-- 
1.9.2