Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753184AbdFMKGw (ORCPT ); Tue, 13 Jun 2017 06:06:52 -0400 Received: from mail-yw0-f176.google.com ([209.85.161.176]:33907 "EHLO mail-yw0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753045AbdFMKGn (ORCPT ); Tue, 13 Jun 2017 06:06:43 -0400 MIME-Version: 1.0 In-Reply-To: References: From: Andrey Konovalov Date: Tue, 13 Jun 2017 12:06:31 +0200 Message-ID: Subject: Re: usb/gadget: potential deadlock in gadgetfs_suspend To: Alan Stern Cc: Felipe Balbi , Greg Kroah-Hartman , Peter Chen , Krzysztof Opasiak , Colin Ian King , =?UTF-8?Q?Felix_H=C3=A4dicke?= , Roger Quadros , USB list , LKML , Dmitry Vyukov , Kostya Serebryany , syzkaller Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8823 Lines: 198 On Mon, Jun 12, 2017 at 10:25 PM, Alan Stern wrote: > > As you surmised, this was caused by a race. The race was between > dummy_udc_stop() and set_link_state(), both in dummy_hcd.c. A symptom > of this race is that the first routine clears dum->driver while the > second dereferences it, and neither access is protected by a lock. > > It turns out this problem affects at least one other UDC driver > (net2280.c). Below is a patch that adds the missing lock (and removes > some unneeded unlocks) from these drivers. It turns out that the > disconnect, reset, suspend, and resume callbacks should all be invoked > while the UDC's lock is held, because they can race with gadget driver > unbinding. > > Adding locked regions can lead to the possibility of deadlock or > lockdep violations. Please let me know if the patch below raises any > problems. Hi Alan, Thanks for the patch! I've been testing with your patch applied and the "bad spinlock magic" crashes seem to be gone. However I got another crash (happened only once over the night), which happens during "spin_lock_irqsave (&dev->lock, flags)": kasan: GPF could be caused by NULL-ptr deref or user memory access general protection fault: 0000 [#1] SMP KASAN Dumping ftrace buffer: (ftrace buffer empty) Modules linked in: CPU: 3 PID: 900 Comm: kworker/3:1 Not tainted 4.12.0-rc4+ #36 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 Workqueue: usb_hub_wq hub_event task: ffff88006c5aadc0 task.stack: ffff88006c620000 RIP: 0010:__lock_acquire+0xbe5/0x3690 kernel/locking/lockdep.c:3246 RSP: 0018:ffff88006c625a80 EFLAGS: 00010006 RAX: dffffc0000000000 RBX: ffff88006c5aadc0 RCX: 0000000000000000 RDX: 0000000000000003 RSI: 0000000000000000 RDI: 1ffff1000d8c4bab RBP: ffff88006c625fc0 R08: 0000000000000001 R09: 0000000000000001 R10: ffff88006c5ab698 R11: ffffffff87dd2e80 R12: dffffc0000000000 R13: 0000000000000001 R14: 0000000000000018 R15: 0000000000000000 FS: 0000000000000000(0000) GS:ffff88006dd00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000000020fdffff CR3: 000000006797e000 CR4: 00000000000006e0 Call Trace: lock_acquire+0x22d/0x560 kernel/locking/lockdep.c:3855 __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline] _raw_spin_lock_irqsave+0xc9/0x110 kernel/locking/spinlock.c:159 gadgetfs_disconnect+0xf1/0x230 drivers/usb/gadget/legacy/inode.c:1664 usb_gadget_udc_reset+0x3b/0xb0 drivers/usb/gadget/udc/core.c:1020 set_link_state+0x648/0x9f0 drivers/usb/gadget/udc/dummy_hcd.c:446 dummy_hub_control+0x11bb/0x1fb0 drivers/usb/gadget/udc/dummy_hcd.c:2243 rh_call_control drivers/usb/core/hcd.c:689 [inline] rh_urb_enqueue drivers/usb/core/hcd.c:846 [inline] usb_hcd_submit_urb+0x92f/0x20b0 drivers/usb/core/hcd.c:1650 usb_submit_urb+0x8b2/0x12c0 drivers/usb/core/urb.c:542 usb_start_wait_urb+0x148/0x5b0 drivers/usb/core/message.c:56 usb_internal_control_msg drivers/usb/core/message.c:100 [inline] usb_control_msg+0x341/0x4d0 drivers/usb/core/message.c:151 set_port_feature+0x73/0x90 drivers/usb/core/hub.c:422 hub_port_reset+0x277/0x1550 drivers/usb/core/hub.c:2772 hub_port_init+0x7dc/0x2940 drivers/usb/core/hub.c:4510 hub_port_connect drivers/usb/core/hub.c:4826 [inline] hub_port_connect_change drivers/usb/core/hub.c:4999 [inline] port_event drivers/usb/core/hub.c:5105 [inline] hub_event+0x1ae1/0x3d40 drivers/usb/core/hub.c:5185 process_one_work+0xc08/0x1bd0 kernel/workqueue.c:2097 process_scheduled_works kernel/workqueue.c:2157 [inline] worker_thread+0xb2b/0x1860 kernel/workqueue.c:2233 kthread+0x363/0x440 kernel/kthread.c:231 ret_from_fork+0x2a/0x40 arch/x86/entry/entry_64.S:424 Code: e9 03 f3 48 ab 48 81 c4 18 05 00 00 44 89 e0 5b 41 5c 41 5d 41 5e 41 5f 5d c3 4c 89 f2 48 b8 00 00 00 00 00 fc ff df 48 c1 ea 03 <80> 3c 02 00 0f 85 51 24 00 00 49 81 3e 40 ea 13 87 41 bd 00 00 RIP: __lock_acquire+0xbe5/0x3690 kernel/locking/lockdep.c:3246 RSP: ffff88006c625a80 ---[ end trace f5a7b971fc1b0546 ]--- Kernel panic - not syncing: Fatal exception Shutting down cpus with NMI Dumping ftrace buffer: (ftrace buffer empty) Kernel Offset: disabled Rebooting in 86400 seconds.. > > Alan Stern > > > > Index: usb-4.x/drivers/usb/gadget/legacy/inode.c > =================================================================== > --- usb-4.x.orig/drivers/usb/gadget/legacy/inode.c > +++ usb-4.x/drivers/usb/gadget/legacy/inode.c > @@ -1679,9 +1679,10 @@ static void > gadgetfs_suspend (struct usb_gadget *gadget) > { > struct dev_data *dev = get_gadget_data (gadget); > + unsigned long flags; > > INFO (dev, "suspended from state %d\n", dev->state); > - spin_lock (&dev->lock); > + spin_lock_irqsave(&dev->lock, flags); > switch (dev->state) { > case STATE_DEV_SETUP: // VERY odd... host died?? > case STATE_DEV_CONNECTED: > @@ -1692,7 +1693,7 @@ gadgetfs_suspend (struct usb_gadget *gad > default: > break; > } > - spin_unlock (&dev->lock); > + spin_unlock_irqrestore(&dev->lock, flags); > } > > static struct usb_gadget_driver gadgetfs_driver = { > Index: usb-4.x/drivers/usb/gadget/udc/dummy_hcd.c > =================================================================== > --- usb-4.x.orig/drivers/usb/gadget/udc/dummy_hcd.c > +++ usb-4.x/drivers/usb/gadget/udc/dummy_hcd.c > @@ -442,23 +442,16 @@ static void set_link_state(struct dummy_ > /* Report reset and disconnect events to the driver */ > if (dum->driver && (disconnect || reset)) { > stop_activity(dum); > - spin_unlock(&dum->lock); > if (reset) > usb_gadget_udc_reset(&dum->gadget, dum->driver); > else > dum->driver->disconnect(&dum->gadget); > - spin_lock(&dum->lock); > } > } else if (dum_hcd->active != dum_hcd->old_active) { > - if (dum_hcd->old_active && dum->driver->suspend) { > - spin_unlock(&dum->lock); > + if (dum_hcd->old_active && dum->driver->suspend) > dum->driver->suspend(&dum->gadget); > - spin_lock(&dum->lock); > - } else if (!dum_hcd->old_active && dum->driver->resume) { > - spin_unlock(&dum->lock); > + else if (!dum_hcd->old_active && dum->driver->resume) > dum->driver->resume(&dum->gadget); > - spin_lock(&dum->lock); > - } > } > > dum_hcd->old_status = dum_hcd->port_status; > @@ -983,7 +976,9 @@ 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); > > return 0; > } > Index: usb-4.x/drivers/usb/gadget/udc/net2280.c > =================================================================== > --- usb-4.x.orig/drivers/usb/gadget/udc/net2280.c > +++ usb-4.x/drivers/usb/gadget/udc/net2280.c > @@ -2470,11 +2470,8 @@ static void stop_activity(struct net2280 > nuke(&dev->ep[i]); > > /* report disconnect; the driver is already quiesced */ > - if (driver) { > - spin_unlock(&dev->lock); > + if (driver) > driver->disconnect(&dev->gadget); > - spin_lock(&dev->lock); > - } > > usb_reinit(dev); > } > @@ -3348,8 +3345,6 @@ next_endpoints: > BIT(PCI_RETRY_ABORT_INTERRUPT)) > > static void handle_stat1_irqs(struct net2280 *dev, u32 stat) > -__releases(dev->lock) > -__acquires(dev->lock) > { > struct net2280_ep *ep; > u32 tmp, num, mask, scratch; > @@ -3390,14 +3385,12 @@ __acquires(dev->lock) > if (disconnect || reset) { > stop_activity(dev, dev->driver); > ep0_start(dev); > - spin_unlock(&dev->lock); > if (reset) > usb_gadget_udc_reset > (&dev->gadget, dev->driver); > else > (dev->driver->disconnect) > (&dev->gadget); > - spin_lock(&dev->lock); > return; > } > } >