2:47 a.m.
FYI, we noticed the following commit:
commit: f16443a034c7aa359ddf6f0f9bc40d01ca31faea ("USB: gadgetfs, dummy-hcd, net2280:
fix locking for callbacks")
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master
in testcase: trinity
with following parameters:
runtime: 300s
test-description: Trinity is a linux system call fuzz tester.
test-url: http://codemonkey.org.uk/projects/trinity/
on test machine: qemu-system-x86_64 -enable-kvm -m 420M
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
+-----------------------------------------+------------+------------+
| | d2f48f05cd | f16443a034 |
+-----------------------------------------+------------+------------+
| boot_successes | 53 | 0 |
| boot_failures | 1 | 58 |
| INFO:task_blocked_for_more_than#seconds | 1 | |
| calltrace:rtnl_lock | 1 | |
| calltrace:debug_show_all_locks | 1 | |
| BUG:kernel_hang_in_test_stage | 1 | |
| EIP:arch_local_irq_restore | 0 | 34 |
| EIP:ftrace_likely_update | 0 | 5 |
| EIP:__rb_reserve_next | 0 | 3 |
| EIP:native_safe_halt | 0 | 5 |
| EIP:arch_local_irq_enable | 0 | 1 |
| EIP:ring_buffer_unlock_commit | 0 | 1 |
| EIP:ring_buffer_lock_reserve | 0 | 3 |
| EIP:rb_calculate_event_length | 0 | 2 |
| EIP:rb_commit | 0 | 2 |
| EIP:paravirt_sched_clock | 0 | 1 |
| EIP:ring_buffer_producer | 0 | 1 |
+-----------------------------------------+------------+------------+
[ 2.748828] kernel_init+0x8/0x150
[ 2.748828] kernel_init+0x8/0x150
[ 2.748830] ret_from_fork+0x19/0x24
[ 2.748830] ret_from_fork+0x19/0x24
[ 2.748830]
[ 2.748830] other info that might help us debug this:
[ 2.748830]
[ 2.748830]
[ 2.748830] other info that might help us debug this:
[ 2.748830]
[ 2.748837] Possible unsafe locking scenario:
[ 2.748837]
[ 2.748837] Possible unsafe locking scenario:
[ 2.748837]
[ 2.748839] CPU0 CPU1
[ 2.748839] CPU0 CPU1
[ 2.748840] ---- ----
[ 2.748840] ---- ----
[ 2.748840] lock(&(&cdev->lock)->rlock);
[ 2.748840] lock(&(&cdev->lock)->rlock);
[ 2.748843]
lock(&(&dum_hcd->dum->lock)->rlock);
[ 2.748843]
lock(&(&dum_hcd->dum->lock)->rlock);
[ 2.748845] lock(&(&cdev->lock)->rlock);
[ 2.748845] lock(&(&cdev->lock)->rlock);
[ 2.748848] lock(&(&dum_hcd->dum->lock)->rlock);
[ 2.748848] lock(&(&dum_hcd->dum->lock)->rlock);
[ 2.748850]
[ 2.748850] *** DEADLOCK ***
[ 2.748850]
[ 2.748850]
[ 2.748850] *** DEADLOCK ***
[ 2.748850]
[ 2.748852] 3 locks held by swapper/1:
[ 2.748852] 3 locks held by swapper/1:
[ 2.748853] #0: (console_lock){+.+...}, at: [<790d2ca8>]
vprintk_emit+0x363/0x38e
[ 2.748853] #0: (console_lock){+.+...}, at: [<790d2ca8>]
vprintk_emit+0x363/0x38e
[ 2.748859] #1: ((&dum_hcd->timer)){+.-...}, at: [<790ec25b>]
call_timer_fn+0x0/0x38c
[ 2.748859] #1: ((&dum_hcd->timer)){+.-...}, at: [<790ec25b>]
call_timer_fn+0x0/0x38c
[ 2.748865] #2: (&(&cdev->lock)->rlock){..-...}, at: [<799eab67>]
composite_setup+0xaa9/0x1a55
[ 2.748865] #2: (&(&cdev->lock)->rlock){..-...}, at: [<799eab67>]
composite_setup+0xaa9/0x1a55
[ 2.748871]
[ 2.748871] stack backtrace:
[ 2.748871]
[ 2.748871] stack backtrace:
[ 2.748874] CPU: 0 PID: 1 Comm: swapper Not tainted 4.12.0-rc5-00006-gf16443a #1
[ 2.748874] CPU: 0 PID: 1 Comm: swapper Not tainted 4.12.0-rc5-00006-gf16443a #1
[ 2.748876] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
1.9.3-20161025_171302-gandalf 04/01/2014
[ 2.748876] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
1.9.3-20161025_171302-gandalf 04/01/2014
[ 2.748878] Call Trace:
[ 2.748878] Call Trace:
[ 2.748879] <SOFTIRQ>
[ 2.748879] <SOFTIRQ>
[ 2.748882] ? show_stack+0x56/0x5e
[ 2.748882] ? show_stack+0x56/0x5e
[ 2.748886] dump_stack+0x16/0x18
[ 2.748886] dump_stack+0x16/0x18
[ 2.748889] print_circular_bug+0x18b/0x198
[ 2.748889] print_circular_bug+0x18b/0x198
[ 2.748892] validate_chain+0x668/0x829
[ 2.748892] validate_chain+0x668/0x829
[ 2.748895] ? ret_from_fork+0x19/0x24
[ 2.748895] ? ret_from_fork+0x19/0x24
[ 2.748897] ? mark_lock+0x1e/0x1cc
[ 2.748897] ? mark_lock+0x1e/0x1cc
[ 2.748900] __lock_acquire+0x543/0x5eb
[ 2.748900] __lock_acquire+0x543/0x5eb
[ 2.748902] lock_acquire+0xe1/0x157
[ 2.748902] lock_acquire+0xe1/0x157
[ 2.748905] ? dummy_queue+0xd6/0x1fb
[ 2.748905] ? dummy_queue+0xd6/0x1fb
[ 2.748907] _raw_spin_lock_irqsave+0x37/0x47
[ 2.748907] _raw_spin_lock_irqsave+0x37/0x47
[ 2.748910] ? dummy_queue+0xd6/0x1fb
[ 2.748910] ? dummy_queue+0xd6/0x1fb
[ 2.748911] dummy_queue+0xd6/0x1fb
[ 2.748911] dummy_queue+0xd6/0x1fb
[ 2.748913] usb_ep_queue+0x80/0x1b5
[ 2.748913] usb_ep_queue+0x80/0x1b5
[ 2.748915] ncm_do_notify+0x155/0x187
[ 2.748915] ncm_do_notify+0x155/0x187
[ 2.748917] ncm_set_alt+0x267/0x276
[ 2.748917] ncm_set_alt+0x267/0x276
[ 2.748919] composite_setup+0xc75/0x1a55
[ 2.748919] composite_setup+0xc75/0x1a55
[ 2.748920] ? check_chain_key+0x8c/0xe1
[ 2.748920] ? check_chain_key+0x8c/0xe1
[ 2.748923] ? do_raw_spin_unlock+0xea/0x126
[ 2.748923] ? do_raw_spin_unlock+0xea/0x126
[ 2.748925] dummy_timer+0x8ba/0x1083
[ 2.748925] dummy_timer+0x8ba/0x1083
[ 2.748926] ? dummy_timer+0x8ba/0x1083
[ 2.748926] ? dummy_timer+0x8ba/0x1083
[ 2.748928] ? __lock_is_held+0x35/0x65
[ 2.748928] ? __lock_is_held+0x35/0x65
[ 2.748930] ? dummy_udc_probe+0x1b5/0x1b5
[ 2.748930] ? dummy_udc_probe+0x1b5/0x1b5
[ 2.748932] call_timer_fn+0x179/0x38c
[ 2.748932] call_timer_fn+0x179/0x38c
[ 2.748933] ? dummy_udc_probe+0x1b5/0x1b5
[ 2.748933] ? dummy_udc_probe+0x1b5/0x1b5
[ 2.748935] ? dummy_udc_probe+0x1b5/0x1b5
[ 2.748935] ? dummy_udc_probe+0x1b5/0x1b5
[ 2.748936] run_timer_softirq+0x1a1/0x1c9
[ 2.748936] run_timer_softirq+0x1a1/0x1c9
[ 2.748939] ? rcu_read_unlock_sched_notrace+0x21/0x41
[ 2.748939] ? rcu_read_unlock_sched_notrace+0x21/0x41
[ 2.748941] __do_softirq+0x1f6/0x4c7
[ 2.748941] __do_softirq+0x1f6/0x4c7
[ 2.748946] ? univ8250_console_setup+0x7a/0x7a
[ 2.748946] ? univ8250_console_setup+0x7a/0x7a
[ 2.748948] ? _local_bh_enable+0x5f/0x5f
[ 2.748948] ? _local_bh_enable+0x5f/0x5f
[ 2.748950] do_softirq_own_stack+0x1e/0x24
[ 2.748950] do_softirq_own_stack+0x1e/0x24
[ 2.748951] </SOFTIRQ>
[ 2.748951] </SOFTIRQ>
[ 2.748953] irq_exit+0x92/0xab
[ 2.748953] irq_exit+0x92/0xab
[ 2.748954] smp_apic_timer_interrupt+0x23/0x2c
[ 2.748954] smp_apic_timer_interrupt+0x23/0x2c
[ 2.748956] apic_timer_interrupt+0x34/0x3c
[ 2.748956] apic_timer_interrupt+0x34/0x3c
[ 2.748958] EIP: arch_local_irq_restore+0x5/0xb
[ 2.748958] EIP: arch_local_irq_restore+0x5/0xb
[ 2.748959] EFLAGS: 00200202 CPU: 0
[ 2.748959] EFLAGS: 00200202 CPU: 0
[ 2.748960] EAX: 00200202 EBX: 00000000 ECX: 00000006 EDX: 00000007
[ 2.748960] EAX: 00200202 EBX: 00000000 ECX: 00000006 EDX: 00000007
[ 2.748961] ESI: 796eb13b EDI: 00000000 EBP: 919efe00 ESP: 919efe00
[ 2.748961] ESI: 796eb13b EDI: 00000000 EBP: 919efe00 ESP: 919efe00
[ 2.748962] DS: 007b ES: 007b FS: 0000 GS: 0000 SS: 0068
[ 2.748962] DS: 007b ES: 007b FS: 0000 GS: 0000 SS: 0068
[ 2.748964] ? univ8250_console_setup+0x7a/0x7a
[ 2.748964] ? univ8250_console_setup+0x7a/0x7a
[ 2.748966] ? trace_raw_output_console+0x2d/0x59
[ 2.748966] ? trace_raw_output_console+0x2d/0x59
[ 2.748968] console_unlock+0x514/0x667
[ 2.748968] console_unlock+0x514/0x667
[ 2.748970] vprintk_emit+0x384/0x38e
[ 2.748970] vprintk_emit+0x384/0x38e
[ 2.748971] vprintk_default+0x12/0x14
[ 2.748971] vprintk_default+0x12/0x14
[ 2.748973] vprintk_func+0x63/0x67
[ 2.748973] vprintk_func+0x63/0x67
[ 2.748975] printk+0xe/0x10
[ 2.748975] printk+0xe/0x10
[ 2.748977] test+0x2c7/0x314
[ 2.748977] test+0x2c7/0x314
[ 2.748979] ? disk_type+0x4e/0x4e
[ 2.748979] ? disk_type+0x4e/0x4e
[ 2.748981] raid6_test+0xb8/0x116
[ 2.748981] raid6_test+0xb8/0x116
[ 2.748983] ? test+0x314/0x314
[ 2.748983] ? test+0x314/0x314
[ 2.748984] do_one_initcall+0x8f/0x18f
[ 2.748984] do_one_initcall+0x8f/0x18f
[ 2.748986] ? parse_args+0x19f/0x24d
[ 2.748986] ? parse_args+0x19f/0x24d
[ 2.748988] ? kernel_init_freeable+0xdb/0x1bc
[ 2.748988] ? kernel_init_freeable+0xdb/0x1bc
[ 2.748989] kernel_init_freeable+0xfe/0x1bc
[ 2.748989] kernel_init_freeable+0xfe/0x1bc
[ 2.748991] ? rest_init+0x12a/0x12a
[ 2.748991] ? rest_init+0x12a/0x12a
[ 2.748992] kernel_init+0x8/0x150
[ 2.748992] kernel_init+0x8/0x150
[ 2.748994] ret_from_fork+0x19/0x24
[ 2.748994] ret_from_fork+0x19/0x24
[ 2.821984] cdc_ncm 1-1:1.0: MAC-Address: 4e:8c:d2:7b:df:4e
[ 2.821984] cdc_ncm 1-1:1.0: MAC-Address: 4e:8c:d2:7b:df:4e
[ 2.822334] cdc_ncm 1-1:1.0 usb1: register 'cdc_ncm' at usb-dummy_hcd.0-1, CDC
NCM, 4e:8c:d2:7b:df:4e
[ 2.822334] cdc_ncm 1-1:1.0 usb1: register 'cdc_ncm' at usb-dummy_hcd.0-1, CDC
NCM, 4e:8c:d2:7b:df:4e
[ 2.827981] raid6test: test_disks(12, 15): faila= 12(D) failb= 15(D) OK
To reproduce:
git clone https://github.com/01org/lkp-tests.git
cd lkp-tests
bin/lkp qemu -k <bzImage> job-script # job-script is attached in this
email
Thanks,
Kernel Test Robot
2:51 p.m.
Felipe:
On Thu, 29 Jun 2017, kernel test robot wrote:
FYI, we noticed the following commit:
commit: f16443a034c7aa359ddf6f0f9bc40d01ca31faea ("USB: gadgetfs, dummy-hcd,
net2280: fix locking for callbacks")
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master
in testcase: trinity
with following parameters:
runtime: 300s
test-description: Trinity is a linux system call fuzz tester.
test-url: http://codemonkey.org.uk/projects/trinity/
on test machine: qemu-system-x86_64 -enable-kvm -m 420M
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
...
I won't include the entire report. The gist is that we have a problem
with lock ordering. The report is about dummy-hcd, but this could
affect any UDC driver.
1. When a SETUP request arrives, composite_setup() acquires
cdev->lock before calling the function driver's callback.
When that callback submits a reply, it causes the UDC driver
to acquire its private lock.
2. When a bus reset occurs, the UDC's interrupt handler acquires
its private lock before calling usb_gadget_udc_reset(), which
calls composite_disconnect(), which acquires cdev->lock.
So there's an ABBA ordering problem between the UDC's private lock and
the composite core's cdev->lock.
Use of the UDC's private lock in 1 seems unavoidable. Perhaps it can
be avoided in 2, but wouldn't that leave us open to a race between
reset handling and gadget driver unregistration? In fact, that was the
very reason for creating the commit cited at the top of this bug
report.
I don't know enough of the details of the composite core to say whether
its lock usage can be reduced.
Do you have any suggestions?
Alan Stern
7:44 p.m.
On Thu, 29 Jun 2017, Alan Stern wrote:
Felipe:
On Thu, 29 Jun 2017, kernel test robot wrote:
> FYI, we noticed the following commit:
>
> commit: f16443a034c7aa359ddf6f0f9bc40d01ca31faea ("USB: gadgetfs, dummy-hcd,
net2280: fix locking for callbacks")
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master
>
> in testcase: trinity
> with following parameters:
>
> runtime: 300s
>
> test-description: Trinity is a linux system call fuzz tester.
> test-url: http://codemonkey.org.uk/projects/trinity/
>
>
> on test machine: qemu-system-x86_64 -enable-kvm -m 420M
>
> caused below changes (please refer to attached dmesg/kmsg for entire
log/backtrace):
...
I won't include the entire report. The gist is that we have a problem
with lock ordering. The report is about dummy-hcd, but this could
affect any UDC driver.
1. When a SETUP request arrives, composite_setup() acquires
cdev->lock before calling the function driver's callback.
When that callback submits a reply, it causes the UDC driver
to acquire its private lock.
2. When a bus reset occurs, the UDC's interrupt handler acquires
its private lock before calling usb_gadget_udc_reset(), which
calls composite_disconnect(), which acquires cdev->lock.
So there's an ABBA ordering problem between the UDC's private lock and
the composite core's cdev->lock.
Use of the UDC's private lock in 1 seems unavoidable. Perhaps it can
be avoided in 2, but wouldn't that leave us open to a race between
reset handling and gadget driver unregistration? In fact, that was the
very reason for creating the commit cited at the top of this bug
report.
I don't know enough of the details of the composite core to say whether
its lock usage can be reduced.
Do you have any suggestions?
Actually, I had an idea this morning.
The UDC driver certainly cannot retain its private lock across ->setup
callbacks, because the handler will submit a response request which
will cause the UDC driver to reacquire the lock. Therefore the setup
callback is already subject to a race with unregistration.
This strongly suggests that the UDC driver should not keep its private
lock during the other callbacks either. Which means we need some way
to prevent the race from occurring. To be more explicit, the UDC
driver's udc_stop routine needs to wait until no callbacks are running.
Here's a sample patch for dummy-hcd to illustrate the idea:
--- usb-4.x.orig/drivers/usb/gadget/udc/dummy_hcd.c
+++ usb-4.x/drivers/usb/gadget/udc/dummy_hcd.c
@@ -253,6 +253,7 @@ struct dummy {
*/
struct dummy_ep ep[DUMMY_ENDPOINTS];
int address;
+ int active_callbacks;
struct usb_gadget gadget;
struct usb_gadget_driver *driver;
struct dummy_request fifo_req;
@@ -442,16 +443,24 @@ static void set_link_state(struct dummy_
/* Report reset and disconnect events to the driver */
if (dum->driver && (disconnect || reset)) {
stop_activity(dum);
+ ++dum->active_callbacks;
+ spin_unlock(&dum->lock);
if (reset)
usb_gadget_udc_reset(&dum->gadget, dum->driver);
else
dum->driver->disconnect(&dum->gadget);
+ spin_lock(&dum->lock);
+ --dum->active_callbacks;
}
- } else if (dum_hcd->active != dum_hcd->old_active) {
+ } else if (dum->driver && dum_hcd->active != dum_hcd->old_active) {
+ ++dum->active_callbacks;
+ spin_unlock(&dum->lock);
if (dum_hcd->old_active && dum->driver->suspend)
dum->driver->suspend(&dum->gadget);
- else if (!dum_hcd->old_active && dum->driver->resume)
+ else if (!dum_hcd->old_active && dum->driver->resume)
dum->driver->resume(&dum->gadget);
+ spin_lock(&dum->lock);
+ --dum->active_callbacks;
}
dum_hcd->old_status = dum_hcd->port_status;
@@ -976,10 +985,22 @@ static int dummy_udc_stop(struct usb_gad
struct dummy_hcd *dum_hcd = gadget_to_dummy_hcd(g);
struct dummy *dum = dum_hcd->dum;
- spin_lock_irq(&dum->lock);
- dum->driver = NULL;
- spin_unlock_irq(&dum->lock);
+ /* Wait until no callbacks are running, then unbind the driver */
+ for (;;) {
+ int c;
+
+ spin_lock_irq(&dum->lock);
+ c = dum->active_callbacks;
+ if (c == 0) {
+ dum->driver = NULL;
+ stop_activity(dum);
+ }
+ spin_unlock_irq(&dum->lock);
+ if (c == 0)
+ break;
+ usleep_range(1000, 2000);
+ }
return 0;
}
@@ -1850,10 +1871,12 @@ restart:
* until setup() returns; no reentrancy issues etc.
*/
if (value > 0) {
+ ++dum->active_callbacks;
spin_unlock(&dum->lock);
value = dum->driver->setup(&dum->gadget,
&setup);
spin_lock(&dum->lock);
+ --dum->active_callbacks;
if (value >= 0) {
/* no delays (max 64KB data stage) */
It's a little clunky, especially that second-to-last hunk, but I don't
see any way around it. What do you think of this approach?
Alan Stern
6:47 a.m.
Hi,
Alan Stern <stern(a)rowland.harvard.edu> writes:
Felipe:
On Thu, 29 Jun 2017, kernel test robot wrote:
> FYI, we noticed the following commit:
>
> commit: f16443a034c7aa359ddf6f0f9bc40d01ca31faea ("USB: gadgetfs, dummy-hcd,
net2280: fix locking for callbacks")
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master
>
> in testcase: trinity
> with following parameters:
>
> runtime: 300s
>
> test-description: Trinity is a linux system call fuzz tester.
> test-url: http://codemonkey.org.uk/projects/trinity/
>
>
> on test machine: qemu-system-x86_64 -enable-kvm -m 420M
>
> caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
...
I won't include the entire report. The gist is that we have a problem
with lock ordering. The report is about dummy-hcd, but this could
affect any UDC driver.
1. When a SETUP request arrives, composite_setup() acquires
cdev->lock before calling the function driver's callback.
When that callback submits a reply, it causes the UDC driver
to acquire its private lock.
this only seems to happen before calling set_config():
case USB_REQ_SET_CONFIGURATION:
if (ctrl->bRequestType != 0)
goto unknown;
if (gadget_is_otg(gadget)) {
if (gadget->a_hnp_support)
DBG(cdev, "HNP available\n");
else if (gadget->a_alt_hnp_support)
DBG(cdev, "HNP on another port\n");
else
VDBG(cdev, "HNP inactive\n");
}
spin_lock(&cdev->lock);
value = set_config(cdev, ctrl, w_value);
spin_unlock(&cdev->lock);
break;
The "reply" here is a usb_ep_queue(gadget->ep0, req) call which should
hold UDC driver's private lock and disable interrupts for a short period
of time. Here's how it looks on dwc3:
int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct usb_request *request,
gfp_t gfp_flags)
{
struct dwc3_request *req = to_dwc3_request(request);
struct dwc3_ep *dep = to_dwc3_ep(ep);
struct dwc3 *dwc = dep->dwc;
unsigned long flags;
int ret;
spin_lock_irqsave(&dwc->lock, flags);
if (!dep->endpoint.desc) {
dev_err(dwc->dev, "%s: can't queue to disabled endpoint\n",
dep->name);
ret = -ESHUTDOWN;
goto out;
}
/* we share one TRB for ep0/1 */
if (!list_empty(&dep->pending_list)) {
ret = -EBUSY;
goto out;
}
ret = __dwc3_gadget_ep0_queue(dep, req);
out:
spin_unlock_irqrestore(&dwc->lock, flags);
return ret;
}
2. When a bus reset occurs, the UDC's interrupt handler
acquires
its private lock before calling usb_gadget_udc_reset(), which
calls composite_disconnect(), which acquires cdev->lock.
The reset handler will only run after B has been released though, no?
Also, UDC should *release* its private lock before calling UDC reset:
static void dwc3_reset_gadget(struct dwc3 *dwc)
{
if (!dwc->gadget_driver)
return;
if (dwc->gadget.speed != USB_SPEED_UNKNOWN) {
spin_unlock(&dwc->lock);
usb_gadget_udc_reset(&dwc->gadget, dwc->gadget_driver);
spin_lock(&dwc->lock);
}
}
Are you sure this isn't a bug in dummy only?
--
balbi
2:40 p.m.
On Thu, 13 Jul 2017, Felipe Balbi wrote:
Hi,
Alan Stern <stern(a)rowland.harvard.edu> writes:
> Felipe:
>
> On Thu, 29 Jun 2017, kernel test robot wrote:
>
>> FYI, we noticed the following commit:
>>
>> commit: f16443a034c7aa359ddf6f0f9bc40d01ca31faea ("USB: gadgetfs,
dummy-hcd, net2280: fix locking for callbacks")
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master
>>
>> in testcase: trinity
>> with following parameters:
>>
>> runtime: 300s
>>
>> test-description: Trinity is a linux system call fuzz tester.
>> test-url: http://codemonkey.org.uk/projects/trinity/
>>
>>
>> on test machine: qemu-system-x86_64 -enable-kvm -m 420M
>>
>> caused below changes (please refer to attached dmesg/kmsg for entire
log/backtrace):
> ...
>
> I won't include the entire report. The gist is that we have a problem
> with lock ordering. The report is about dummy-hcd, but this could
> affect any UDC driver.
>
> 1. When a SETUP request arrives, composite_setup() acquires
> cdev->lock before calling the function driver's callback.
> When that callback submits a reply, it causes the UDC driver
> to acquire its private lock.
this only seems to happen before calling set_config():
case USB_REQ_SET_CONFIGURATION:
if (ctrl->bRequestType != 0)
goto unknown;
if (gadget_is_otg(gadget)) {
if (gadget->a_hnp_support)
DBG(cdev, "HNP available\n");
else if (gadget->a_alt_hnp_support)
DBG(cdev, "HNP on another port\n");
else
VDBG(cdev, "HNP inactive\n");
}
spin_lock(&cdev->lock);
value = set_config(cdev, ctrl, w_value);
spin_unlock(&cdev->lock);
break;
That's true. Why is the lock held for set_config() but not for any of
the other callbacks?
Doesn't that mean the other callbacks can race with function
unregistration?
The "reply" here is a usb_ep_queue(gadget->ep0, req)
call which should
hold UDC driver's private lock and disable interrupts for a short period
of time. Here's how it looks on dwc3:
int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct usb_request *request,
gfp_t gfp_flags)
{
struct dwc3_request *req = to_dwc3_request(request);
struct dwc3_ep *dep = to_dwc3_ep(ep);
struct dwc3 *dwc = dep->dwc;
unsigned long flags;
int ret;
spin_lock_irqsave(&dwc->lock, flags);
if (!dep->endpoint.desc) {
dev_err(dwc->dev, "%s: can't queue to disabled endpoint\n",
dep->name);
ret = -ESHUTDOWN;
goto out;
}
/* we share one TRB for ep0/1 */
if (!list_empty(&dep->pending_list)) {
ret = -EBUSY;
goto out;
}
ret = __dwc3_gadget_ep0_queue(dep, req);
out:
spin_unlock_irqrestore(&dwc->lock, flags);
return ret;
}
> 2. When a bus reset occurs, the UDC's interrupt handler acquires
> its private lock before calling usb_gadget_udc_reset(), which
> calls composite_disconnect(), which acquires cdev->lock.
The reset handler will only run after B has been released though, no?
B? Do you mean the UDC's private lock?
Also, UDC should *release* its private lock before calling UDC
reset:
static void dwc3_reset_gadget(struct dwc3 *dwc)
{
if (!dwc->gadget_driver)
return;
if (dwc->gadget.speed != USB_SPEED_UNKNOWN) {
spin_unlock(&dwc->lock);
usb_gadget_udc_reset(&dwc->gadget, dwc->gadget_driver);
spin_lock(&dwc->lock);
}
}
Are you sure this isn't a bug in dummy only?
That's the issue. The commit in $SUBJECT changed dummy-hcd precisely
to _avoid_ releasing the private lock before calling the reset routine.
The reason is explained in the commit's changelog: If the lock isn't
held then it is possible for the user to cause a use-after-free BUG by
closing the gadgetfs control file when a reset is about to occur. The
bug report showed this happening with dummy-hcd, but the same bug ought
to affect any UDC driver that isn't careful about races between
callbacks and unregistration.
Please read the follow-up email in this thread, the one sent on June 30.
Alan Stern
7 a.m.
Hi,
Alan Stern <stern(a)rowland.harvard.edu> writes:
> > Felipe:
> >
> > On Thu, 29 Jun 2017, kernel test robot wrote:
> >
> >> FYI, we noticed the following commit:
> >>
> >> commit: f16443a034c7aa359ddf6f0f9bc40d01ca31faea ("USB: gadgetfs,
dummy-hcd, net2280: fix locking for callbacks")
> >> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master
> >>
> >> in testcase: trinity
> >> with following parameters:
> >>
> >> runtime: 300s
> >>
> >> test-description: Trinity is a linux system call fuzz tester.
> >> test-url: http://codemonkey.org.uk/projects/trinity/
> >>
> >>
> >> on test machine: qemu-system-x86_64 -enable-kvm -m 420M
> >>
> >> caused below changes (please refer to attached dmesg/kmsg for entire
log/backtrace):
> > ...
> >
> > I won't include the entire report. The gist is that we have a problem
> > with lock ordering. The report is about dummy-hcd, but this could
> > affect any UDC driver.
> >
> > 1. When a SETUP request arrives, composite_setup() acquires
> > cdev->lock before calling the function driver's callback.
> > When that callback submits a reply, it causes the UDC driver
> > to acquire its private lock.
>
> this only seems to happen before calling set_config():
>
> case USB_REQ_SET_CONFIGURATION:
> if (ctrl->bRequestType != 0)
> goto unknown;
> if (gadget_is_otg(gadget)) {
> if (gadget->a_hnp_support)
> DBG(cdev, "HNP available\n");
> else if (gadget->a_alt_hnp_support)
> DBG(cdev, "HNP on another port\n");
> else
> VDBG(cdev, "HNP inactive\n");
> }
> spin_lock(&cdev->lock);
> value = set_config(cdev, ctrl, w_value);
> spin_unlock(&cdev->lock);
> break;
That's true. Why is the lock held for set_config() but not for any of
the other callbacks?
this is really old code from Dave. Your guess is as good as mine :-(
Doesn't that mean the other callbacks can race with function
unregistration?
Probably not as UDCs are required to cancel transfers and kill all
endpoints before unregistering. We would probably just giveback a few
requests with -ESHUTDOWN and prevent new ones from being queued to HW,
no?
> The "reply" here is a usb_ep_queue(gadget->ep0, req)
call which should
> hold UDC driver's private lock and disable interrupts for a short period
> of time. Here's how it looks on dwc3:
>
> int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct usb_request *request,
> gfp_t gfp_flags)
> {
> struct dwc3_request *req = to_dwc3_request(request);
> struct dwc3_ep *dep = to_dwc3_ep(ep);
> struct dwc3 *dwc = dep->dwc;
>
> unsigned long flags;
>
> int ret;
>
> spin_lock_irqsave(&dwc->lock, flags);
> if (!dep->endpoint.desc) {
> dev_err(dwc->dev, "%s: can't queue to disabled endpoint\n",
> dep->name);
> ret = -ESHUTDOWN;
> goto out;
> }
>
> /* we share one TRB for ep0/1 */
> if (!list_empty(&dep->pending_list)) {
> ret = -EBUSY;
> goto out;
> }
>
> ret = __dwc3_gadget_ep0_queue(dep, req);
>
> out:
> spin_unlock_irqrestore(&dwc->lock, flags);
>
> return ret;
> }
>
> > 2. When a bus reset occurs, the UDC's interrupt handler acquires
> > its private lock before calling usb_gadget_udc_reset(), which
> > calls composite_disconnect(), which acquires cdev->lock.
>
> The reset handler will only run after B has been released though, no?
B? Do you mean the UDC's private lock?
yes :-)
> Also, UDC should *release* its private lock before calling UDC
reset:
>
> static void dwc3_reset_gadget(struct dwc3 *dwc)
> {
> if (!dwc->gadget_driver)
> return;
>
> if (dwc->gadget.speed != USB_SPEED_UNKNOWN) {
> spin_unlock(&dwc->lock);
> usb_gadget_udc_reset(&dwc->gadget, dwc->gadget_driver);
> spin_lock(&dwc->lock);
> }
> }
>
> Are you sure this isn't a bug in dummy only?
That's the issue. The commit in $SUBJECT changed dummy-hcd precisely
to _avoid_ releasing the private lock before calling the reset routine.
The reason is explained in the commit's changelog: If the lock isn't
held then it is possible for the user to cause a use-after-free BUG by
closing the gadgetfs control file when a reset is about to occur. The
bug report showed this happening with dummy-hcd, but the same bug ought
to affect any UDC driver that isn't careful about races between
callbacks and unregistration.
understood. Then seems like this should be handled in a more generic way.
Please read the follow-up email in this thread, the one sent on June
30.
Will do
--
balbi
2:17 p.m.
On Fri, 14 Jul 2017, Felipe Balbi wrote:
Hi,
Alan Stern <stern(a)rowland.harvard.edu> writes:
>> > Felipe:
>> >
>> > On Thu, 29 Jun 2017, kernel test robot wrote:
>> >
>> >> FYI, we noticed the following commit:
>> >>
>> >> commit: f16443a034c7aa359ddf6f0f9bc40d01ca31faea ("USB: gadgetfs,
dummy-hcd, net2280: fix locking for callbacks")
>> >> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master
>> >>
>> >> in testcase: trinity
>> >> with following parameters:
>> >>
>> >> runtime: 300s
>> >>
>> >> test-description: Trinity is a linux system call fuzz tester.
>> >> test-url: http://codemonkey.org.uk/projects/trinity/
>> >>
>> >>
>> >> on test machine: qemu-system-x86_64 -enable-kvm -m 420M
>> >>
>> >> caused below changes (please refer to attached dmesg/kmsg for entire
log/backtrace):
>> > ...
>> >
>> > I won't include the entire report. The gist is that we have a problem
>> > with lock ordering. The report is about dummy-hcd, but this could
>> > affect any UDC driver.
>> >
>> > 1. When a SETUP request arrives, composite_setup() acquires
>> > cdev->lock before calling the function driver's callback.
>> > When that callback submits a reply, it causes the UDC driver
>> > to acquire its private lock.
>>
>> this only seems to happen before calling set_config():
>>
>> case USB_REQ_SET_CONFIGURATION:
>> if (ctrl->bRequestType != 0)
>> goto unknown;
>> if (gadget_is_otg(gadget)) {
>> if (gadget->a_hnp_support)
>> DBG(cdev, "HNP available\n");
>> else if (gadget->a_alt_hnp_support)
>> DBG(cdev, "HNP on another port\n");
>> else
>> VDBG(cdev, "HNP inactive\n");
>> }
>> spin_lock(&cdev->lock);
>> value = set_config(cdev, ctrl, w_value);
>> spin_unlock(&cdev->lock);
>> break;
>
> That's true. Why is the lock held for set_config() but not for any of
> the other callbacks?
this is really old code from Dave. Your guess is as good as mine :-(
> Doesn't that mean the other callbacks can race with function
> unregistration?
Probably not as UDCs are required to cancel transfers and kill all
endpoints before unregistering. We would probably just giveback a few
requests with -ESHUTDOWN and prevent new ones from being queued to HW,
no?
But SETUP callbacks aren't associated with pending requests. They get
generated whenever a SETUP packet is received, even if the gadget
driver has no requests queued. Cancelling transfers won't prevent
them.
Killing all endpoints might or might not do the trick. Does killing
ep0 prevent the UDC driver from receiving SETUP packets? This may vary
between UDC drivers.
There are also the other callbacks (reset, disconnect, bus suspend, and
bus resume), which aren't associated with endpoints or requests at all.
Probably drivers really should have something like synchronize_irq() in
the udc_stop routine. That would solve a lot of problems (although it
wouldn't help dummy_hcd, which doesn't rely on interrupts).
Alan Stern
7:58 a.m.
Hi,
Alan Stern <stern(a)rowland.harvard.edu> writes:
> >> > On Thu, 29 Jun 2017, kernel test robot wrote:
> >> >
> >> >> FYI, we noticed the following commit:
> >> >>
> >> >> commit: f16443a034c7aa359ddf6f0f9bc40d01ca31faea ("USB:
gadgetfs, dummy-hcd, net2280: fix locking for callbacks")
> >> >> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git
master
> >> >>
> >> >> in testcase: trinity
> >> >> with following parameters:
> >> >>
> >> >> runtime: 300s
> >> >>
> >> >> test-description: Trinity is a linux system call fuzz tester.
> >> >> test-url: http://codemonkey.org.uk/projects/trinity/
> >> >>
> >> >>
> >> >> on test machine: qemu-system-x86_64 -enable-kvm -m 420M
> >> >>
> >> >> caused below changes (please refer to attached dmesg/kmsg for
entire log/backtrace):
> >> > ...
> >> >
> >> > I won't include the entire report. The gist is that we have a
problem
> >> > with lock ordering. The report is about dummy-hcd, but this could
> >> > affect any UDC driver.
> >> >
> >> > 1. When a SETUP request arrives, composite_setup() acquires
> >> > cdev->lock before calling the function driver's callback.
> >> > When that callback submits a reply, it causes the UDC driver
> >> > to acquire its private lock.
> >>
> >> this only seems to happen before calling set_config():
> >>
> >> case USB_REQ_SET_CONFIGURATION:
> >> if (ctrl->bRequestType != 0)
> >> goto unknown;
> >> if (gadget_is_otg(gadget)) {
> >> if (gadget->a_hnp_support)
> >> DBG(cdev, "HNP available\n");
> >> else if (gadget->a_alt_hnp_support)
> >> DBG(cdev, "HNP on another port\n");
> >> else
> >> VDBG(cdev, "HNP inactive\n");
> >> }
> >> spin_lock(&cdev->lock);
> >> value = set_config(cdev, ctrl, w_value);
> >> spin_unlock(&cdev->lock);
> >> break;
> >
> > That's true. Why is the lock held for set_config() but not for any of
> > the other callbacks?
>
> this is really old code from Dave. Your guess is as good as mine :-(
>
> > Doesn't that mean the other callbacks can race with function
> > unregistration?
>
> Probably not as UDCs are required to cancel transfers and kill all
> endpoints before unregistering. We would probably just giveback a few
> requests with -ESHUTDOWN and prevent new ones from being queued to HW,
> no?
But SETUP callbacks aren't associated with pending requests. They get
they are if ->setup() returns DELAYED_STATUS.
generated whenever a SETUP packet is received, even if the gadget
driver has no requests queued. Cancelling transfers won't prevent
them.
Killing all endpoints might or might not do the trick. Does killing
ep0 prevent the UDC driver from receiving SETUP packets? This may vary
between UDC drivers.
no, it does not. If ->setup() fails, we are required to Stall and
restart ep0. Restarting ep0 involves preparing it to receive a new
SETUP request.
There are also the other callbacks (reset, disconnect, bus suspend,
and
bus resume), which aren't associated with endpoints or requests at all.
Probably drivers really should have something like synchronize_irq() in
the udc_stop routine. That would solve a lot of problems (although it
wouldn't help dummy_hcd, which doesn't rely on interrupts).
Hmm, free_irq() calls synchronize_irq(). UDCs should just request_irq()
on udc_start() and free_irq() on udc_stop(). That's what dwc3 is doing.
--
balbi
2:27 p.m.
On Mon, 17 Jul 2017, Felipe Balbi wrote:
Hi,
Alan Stern <stern(a)rowland.harvard.edu> writes:
>> >> > On Thu, 29 Jun 2017, kernel test robot wrote:
>> >> >
>> >> >> FYI, we noticed the following commit:
>> >> >>
>> >> >> commit: f16443a034c7aa359ddf6f0f9bc40d01ca31faea ("USB:
gadgetfs, dummy-hcd, net2280: fix locking for callbacks")
>> >> >>
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master
>> >> >>
>> >> >> in testcase: trinity
>> >> >> with following parameters:
>> >> >>
>> >> >> runtime: 300s
>> >> >>
>> >> >> test-description: Trinity is a linux system call fuzz tester.
>> >> >> test-url: http://codemonkey.org.uk/projects/trinity/
>> >> >>
>> >> >>
>> >> >> on test machine: qemu-system-x86_64 -enable-kvm -m 420M
>> >> >>
>> >> >> caused below changes (please refer to attached dmesg/kmsg for
entire log/backtrace):
>> >> > ...
>> >> >
>> >> > I won't include the entire report. The gist is that we have a
problem
>> >> > with lock ordering. The report is about dummy-hcd, but this could
>> >> > affect any UDC driver.
>> >> >
>> >> > 1. When a SETUP request arrives, composite_setup() acquires
>> >> > cdev->lock before calling the function driver's callback.
>> >> > When that callback submits a reply, it causes the UDC driver
>> >> > to acquire its private lock.
>> >>
>> >> this only seems to happen before calling set_config():
>> >>
>> >> case USB_REQ_SET_CONFIGURATION:
>> >> if (ctrl->bRequestType != 0)
>> >> goto unknown;
>> >> if (gadget_is_otg(gadget)) {
>> >> if (gadget->a_hnp_support)
>> >> DBG(cdev, "HNP available\n");
>> >> else if (gadget->a_alt_hnp_support)
>> >> DBG(cdev, "HNP on another port\n");
>> >> else
>> >> VDBG(cdev, "HNP inactive\n");
>> >> }
>> >> spin_lock(&cdev->lock);
>> >> value = set_config(cdev, ctrl, w_value);
>> >> spin_unlock(&cdev->lock);
>> >> break;
>> >
>> > That's true. Why is the lock held for set_config() but not for any of
>> > the other callbacks?
>>
>> this is really old code from Dave. Your guess is as good as mine :-(
>>
>> > Doesn't that mean the other callbacks can race with function
>> > unregistration?
>>
>> Probably not as UDCs are required to cancel transfers and kill all
>> endpoints before unregistering. We would probably just giveback a few
>> requests with -ESHUTDOWN and prevent new ones from being queued to HW,
>> no?
>
> But SETUP callbacks aren't associated with pending requests. They get
they are if ->setup() returns DELAYED_STATUS.
No. In the DELAYED_STATUS case, the gadget driver submits a request
_after_ the SETUP packet is received.
In no case is there a pending request that an incoming SETUP packet is
associated with.
> generated whenever a SETUP packet is received, even if the
gadget
> driver has no requests queued. Cancelling transfers won't prevent
> them.
>
> Killing all endpoints might or might not do the trick. Does killing
> ep0 prevent the UDC driver from receiving SETUP packets? This may vary
> between UDC drivers.
no, it does not. If ->setup() fails, we are required to Stall and
restart ep0. Restarting ep0 involves preparing it to receive a new
SETUP request.
I wasn't talking about stalling or restarting ep0; I was talking about
killing it.
> There are also the other callbacks (reset, disconnect, bus
suspend, and
> bus resume), which aren't associated with endpoints or requests at all.
>
> Probably drivers really should have something like synchronize_irq() in
> the udc_stop routine. That would solve a lot of problems (although it
> wouldn't help dummy_hcd, which doesn't rely on interrupts).
Hmm, free_irq() calls synchronize_irq(). UDCs should just request_irq()
on udc_start() and free_irq() on udc_stop(). That's what dwc3 is doing.
In general, I agree. But free_irq() isn't enough; you also have to
tell the UDC hardware to stop generating interrupt requests. Otherwise
you'll end up with a "nobody cared!" error.
Alan Stern
6:02 a.m.
Hi,
Alan Stern <stern(a)rowland.harvard.edu> writes:
> Alan Stern <stern(a)rowland.harvard.edu> writes:
> >> >> > On Thu, 29 Jun 2017, kernel test robot wrote:
> >> >> >
> >> >> >> FYI, we noticed the following commit:
> >> >> >>
> >> >> >> commit: f16443a034c7aa359ddf6f0f9bc40d01ca31faea
("USB: gadgetfs, dummy-hcd, net2280: fix locking for callbacks")
> >> >> >>
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master
> >> >> >>
> >> >> >> in testcase: trinity
> >> >> >> with following parameters:
> >> >> >>
> >> >> >> runtime: 300s
> >> >> >>
> >> >> >> test-description: Trinity is a linux system call fuzz
tester.
> >> >> >> test-url: http://codemonkey.org.uk/projects/trinity/
> >> >> >>
> >> >> >>
> >> >> >> on test machine: qemu-system-x86_64 -enable-kvm -m 420M
> >> >> >>
> >> >> >> caused below changes (please refer to attached dmesg/kmsg
for entire log/backtrace):
> >> >> > ...
> >> >> >
> >> >> > I won't include the entire report. The gist is that we
have a problem
> >> >> > with lock ordering. The report is about dummy-hcd, but this
could
> >> >> > affect any UDC driver.
> >> >> >
> >> >> > 1. When a SETUP request arrives, composite_setup()
acquires
> >> >> > cdev->lock before calling the function driver's
callback.
> >> >> > When that callback submits a reply, it causes the UDC driver
> >> >> > to acquire its private lock.
> >> >>
> >> >> this only seems to happen before calling set_config():
> >> >>
> >> >> case USB_REQ_SET_CONFIGURATION:
> >> >> if (ctrl->bRequestType != 0)
> >> >> goto unknown;
> >> >> if (gadget_is_otg(gadget)) {
> >> >> if (gadget->a_hnp_support)
> >> >> DBG(cdev, "HNP available\n");
> >> >> else if (gadget->a_alt_hnp_support)
> >> >> DBG(cdev, "HNP on another port\n");
> >> >> else
> >> >> VDBG(cdev, "HNP inactive\n");
> >> >> }
> >> >> spin_lock(&cdev->lock);
> >> >> value = set_config(cdev, ctrl, w_value);
> >> >> spin_unlock(&cdev->lock);
> >> >> break;
> >> >
> >> > That's true. Why is the lock held for set_config() but not for any
of
> >> > the other callbacks?
> >>
> >> this is really old code from Dave. Your guess is as good as mine :-(
> >>
> >> > Doesn't that mean the other callbacks can race with function
> >> > unregistration?
> >>
> >> Probably not as UDCs are required to cancel transfers and kill all
> >> endpoints before unregistering. We would probably just giveback a few
> >> requests with -ESHUTDOWN and prevent new ones from being queued to HW,
> >> no?
> >
> > But SETUP callbacks aren't associated with pending requests. They get
>
> they are if ->setup() returns DELAYED_STATUS.
No. In the DELAYED_STATUS case, the gadget driver submits a request
_after_ the SETUP packet is received.
In no case is there a pending request that an incoming SETUP packet is
associated with.
Oh, I see now what you mean. You're talking *specifically* the SETUP
stage :-) Yeah, I agree. Currently, we don't have a request associated
with it.
> > generated whenever a SETUP packet is received, even if the
gadget
> > driver has no requests queued. Cancelling transfers won't prevent
> > them.
> >
> > Killing all endpoints might or might not do the trick. Does killing
> > ep0 prevent the UDC driver from receiving SETUP packets? This may vary
> > between UDC drivers.
>
> no, it does not. If ->setup() fails, we are required to Stall and
> restart ep0. Restarting ep0 involves preparing it to receive a new
> SETUP request.
I wasn't talking about stalling or restarting ep0; I was talking about
killing it.
Right, there's no killing of ep0 unless we're talking about module
removal. In case of ->setup() returning an error, we *must* stall and
restart ep0 (reprogram DMA, prepare 8-byte buffer, etc).
If we don't receive SETUP packet, then we don't even get a completion
interrupt on ep0.
> > There are also the other callbacks (reset, disconnect, bus
suspend, and
> > bus resume), which aren't associated with endpoints or requests at all.
> >
> > Probably drivers really should have something like synchronize_irq() in
> > the udc_stop routine. That would solve a lot of problems (although it
> > wouldn't help dummy_hcd, which doesn't rely on interrupts).
>
> Hmm, free_irq() calls synchronize_irq(). UDCs should just request_irq()
> on udc_start() and free_irq() on udc_stop(). That's what dwc3 is doing.
In general, I agree. But free_irq() isn't enough; you also have to
tell the UDC hardware to stop generating interrupt requests. Otherwise
you'll end up with a "nobody cared!" error.
Yeah, but that's also up to the UDC driver. It must guarantee that
interrupts are masked before freeing the handler. This is not
USB-specific in any way, right?: -)
--
balbi
1195
days inactive
1214
days old
9 comments
3 participants
participants (3)
-
Alan Stern -
Felipe Balbi -
kernel test robot