| From 10164c2ad6d2c16809f6c09e278f946e47801b3a Mon Sep 17 00:00:00 2001 |
| From: Johan Hovold <jhovold@gmail.com> |
| Date: Wed, 23 Apr 2014 11:32:19 +0200 |
| Subject: USB: serial: fix sysfs-attribute removal deadlock |
| |
| From: Johan Hovold <jhovold@gmail.com> |
| |
| commit 10164c2ad6d2c16809f6c09e278f946e47801b3a upstream. |
| |
| Fix driver new_id sysfs-attribute removal deadlock by making sure to |
| not hold any locks that the attribute operations grab when removing the |
| attribute. |
| |
| Specifically, usb_serial_deregister holds the table mutex when |
| deregistering the driver, which includes removing the new_id attribute. |
| This can lead to a deadlock as writing to new_id increments the |
| attribute's active count before trying to grab the same mutex in |
| usb_serial_probe. |
| |
| The deadlock can easily be triggered by inserting a sleep in |
| usb_serial_deregister and writing the id of an unbound device to new_id |
| during module unload. |
| |
| As the table mutex (in this case) is used to prevent subdriver unload |
| during probe, it should be sufficient to only hold the lock while |
| manipulating the usb-serial driver list during deregister. A racing |
| probe will then either fail to find a matching subdriver or fail to get |
| the corresponding module reference. |
| |
| Since v3.15-rc1 this also triggers the following lockdep warning: |
| |
| ====================================================== |
| [ INFO: possible circular locking dependency detected ] |
| 3.15.0-rc2 #123 Tainted: G W |
| ------------------------------------------------------- |
| modprobe/190 is trying to acquire lock: |
| (s_active#4){++++.+}, at: [<c0167aa0>] kernfs_remove_by_name_ns+0x4c/0x94 |
| |
| but task is already holding lock: |
| (table_lock){+.+.+.}, at: [<bf004d84>] usb_serial_deregister+0x3c/0x78 [usbserial] |
| |
| which lock already depends on the new lock. |
| |
| the existing dependency chain (in reverse order) is: |
| |
| -> #1 (table_lock){+.+.+.}: |
| [<c0075f84>] __lock_acquire+0x1694/0x1ce4 |
| [<c0076de8>] lock_acquire+0xb4/0x154 |
| [<c03af3cc>] _raw_spin_lock+0x4c/0x5c |
| [<c02bbc24>] usb_store_new_id+0x14c/0x1ac |
| [<bf007eb4>] new_id_store+0x68/0x70 [usbserial] |
| [<c025f568>] drv_attr_store+0x30/0x3c |
| [<c01690e0>] sysfs_kf_write+0x5c/0x60 |
| [<c01682c0>] kernfs_fop_write+0xd4/0x194 |
| [<c010881c>] vfs_write+0xbc/0x198 |
| [<c0108e4c>] SyS_write+0x4c/0xa0 |
| [<c000f880>] ret_fast_syscall+0x0/0x48 |
| |
| -> #0 (s_active#4){++++.+}: |
| [<c03a7a28>] print_circular_bug+0x68/0x2f8 |
| [<c0076218>] __lock_acquire+0x1928/0x1ce4 |
| [<c0076de8>] lock_acquire+0xb4/0x154 |
| [<c0166b70>] __kernfs_remove+0x254/0x310 |
| [<c0167aa0>] kernfs_remove_by_name_ns+0x4c/0x94 |
| [<c0169fb8>] remove_files.isra.1+0x48/0x84 |
| [<c016a2fc>] sysfs_remove_group+0x58/0xac |
| [<c016a414>] sysfs_remove_groups+0x34/0x44 |
| [<c02623b8>] driver_remove_groups+0x1c/0x20 |
| [<c0260e9c>] bus_remove_driver+0x3c/0xe4 |
| [<c026235c>] driver_unregister+0x38/0x58 |
| [<bf007fb4>] usb_serial_bus_deregister+0x84/0x88 [usbserial] |
| [<bf004db4>] usb_serial_deregister+0x6c/0x78 [usbserial] |
| [<bf005330>] usb_serial_deregister_drivers+0x2c/0x4c [usbserial] |
| [<bf016618>] usb_serial_module_exit+0x14/0x1c [sierra] |
| [<c009d6cc>] SyS_delete_module+0x184/0x210 |
| [<c000f880>] ret_fast_syscall+0x0/0x48 |
| |
| other info that might help us debug this: |
| |
| Possible unsafe locking scenario: |
| |
| CPU0 CPU1 |
| ---- ---- |
| lock(table_lock); |
| lock(s_active#4); |
| lock(table_lock); |
| lock(s_active#4); |
| |
| *** DEADLOCK *** |
| |
| 1 lock held by modprobe/190: |
| #0: (table_lock){+.+.+.}, at: [<bf004d84>] usb_serial_deregister+0x3c/0x78 [usbserial] |
| |
| stack backtrace: |
| CPU: 0 PID: 190 Comm: modprobe Tainted: G W 3.15.0-rc2 #123 |
| [<c0015e10>] (unwind_backtrace) from [<c0013728>] (show_stack+0x20/0x24) |
| [<c0013728>] (show_stack) from [<c03a9a54>] (dump_stack+0x24/0x28) |
| [<c03a9a54>] (dump_stack) from [<c03a7cac>] (print_circular_bug+0x2ec/0x2f8) |
| [<c03a7cac>] (print_circular_bug) from [<c0076218>] (__lock_acquire+0x1928/0x1ce4) |
| [<c0076218>] (__lock_acquire) from [<c0076de8>] (lock_acquire+0xb4/0x154) |
| [<c0076de8>] (lock_acquire) from [<c0166b70>] (__kernfs_remove+0x254/0x310) |
| [<c0166b70>] (__kernfs_remove) from [<c0167aa0>] (kernfs_remove_by_name_ns+0x4c/0x94) |
| [<c0167aa0>] (kernfs_remove_by_name_ns) from [<c0169fb8>] (remove_files.isra.1+0x48/0x84) |
| [<c0169fb8>] (remove_files.isra.1) from [<c016a2fc>] (sysfs_remove_group+0x58/0xac) |
| [<c016a2fc>] (sysfs_remove_group) from [<c016a414>] (sysfs_remove_groups+0x34/0x44) |
| [<c016a414>] (sysfs_remove_groups) from [<c02623b8>] (driver_remove_groups+0x1c/0x20) |
| [<c02623b8>] (driver_remove_groups) from [<c0260e9c>] (bus_remove_driver+0x3c/0xe4) |
| [<c0260e9c>] (bus_remove_driver) from [<c026235c>] (driver_unregister+0x38/0x58) |
| [<c026235c>] (driver_unregister) from [<bf007fb4>] (usb_serial_bus_deregister+0x84/0x88 [usbserial]) |
| [<bf007fb4>] (usb_serial_bus_deregister [usbserial]) from [<bf004db4>] (usb_serial_deregister+0x6c/0x78 [usbserial]) |
| [<bf004db4>] (usb_serial_deregister [usbserial]) from [<bf005330>] (usb_serial_deregister_drivers+0x2c/0x4c [usbserial]) |
| [<bf005330>] (usb_serial_deregister_drivers [usbserial]) from [<bf016618>] (usb_serial_module_exit+0x14/0x1c [sierra]) |
| [<bf016618>] (usb_serial_module_exit [sierra]) from [<c009d6cc>] (SyS_delete_module+0x184/0x210) |
| [<c009d6cc>] (SyS_delete_module) from [<c000f880>] (ret_fast_syscall+0x0/0x48) |
| |
| Signed-off-by: Johan Hovold <jhovold@gmail.com> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| |
| --- |
| drivers/usb/serial/usb-serial.c | 4 +++- |
| 1 file changed, 3 insertions(+), 1 deletion(-) |
| |
| --- a/drivers/usb/serial/usb-serial.c |
| +++ b/drivers/usb/serial/usb-serial.c |
| @@ -1348,10 +1348,12 @@ static int usb_serial_register(struct us |
| static void usb_serial_deregister(struct usb_serial_driver *device) |
| { |
| pr_info("USB Serial deregistering driver %s\n", device->description); |
| + |
| mutex_lock(&table_lock); |
| list_del(&device->driver_list); |
| - usb_serial_bus_deregister(device); |
| mutex_unlock(&table_lock); |
| + |
| + usb_serial_bus_deregister(device); |
| } |
| |
| /** |