Re: [PATCH] USB: serial: fix sysfs-attribute removal deadlock

[Tejun Heo <tj@kernel.org>]

cc'ing Li Zhong who's working on a simliar issue in the following
thread and quoting whole body.

  http://thread.gmane.org/gmane.linux.kernel/1680706

Li, this is another variation of the same problem.  Maybe this can be
covered by your work too?

Thanks.

On Wed, Apr 23, 2014 at 11:32:19AM +0200, Johan Hovold wrote:
> 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>
> ---
> 
> I got this new lockdep warning with 3.15-rc, but this dates back to at
> least 0daeed381c6a ("USB-BKL: Remove BKL use for usb serial driver
> probing").
> 
> As far as I can tell, moving driver deregistration out of the table lock
> should be fine. Another option would be to get a module reference in
> new_id store, although that would still trigger the lockdep warning.
> 
> Thanks,
> Johan
> 
> 
>  drivers/usb/serial/usb-serial.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
> index 81fc0dfcfdcf..6d40d56378d7 100644
> --- a/drivers/usb/serial/usb-serial.c
> +++ b/drivers/usb/serial/usb-serial.c
> @@ -1347,10 +1347,12 @@ static int usb_serial_register(struct usb_serial_driver *driver)
>  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);
>  }
>  
>  /**
> -- 
> 1.8.3.2
> 

-- 
tejun