Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

thunderbolt: Take domain lock in switch sysfs attribute callbacks

switch_lock was introduced because it allowed serialization of device
authorization requests from userspace without need to take the big
domain lock (tb->lock). This was fine because device authorization with
ICM is just one command that is sent to the firmware. Now that we start
to handle all tunneling in the driver switch_lock is not enough because
we need to walk over the topology to establish paths.

For this reason drop switch_lock from the driver completely in favour of
big domain lock.

There is one complication, though. If userspace is waiting for the lock
in tb_switch_set_authorized(), it keeps the device_del() from removing
the sysfs attribute because it waits for active users to release the
attribute first which leads into following splat:

INFO: task kworker/u8:3:73 blocked for more than 61 seconds.
Tainted: G W 5.1.0-rc1+ #244
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
kworker/u8:3 D12976 73 2 0x80000000
Workqueue: thunderbolt0 tb_handle_hotplug [thunderbolt]
Call Trace:
? __schedule+0x2e5/0x740
? _raw_spin_lock_irqsave+0x12/0x40
? prepare_to_wait_event+0xc5/0x160
schedule+0x2d/0x80
__kernfs_remove.part.17+0x183/0x1f0
? finish_wait+0x80/0x80
kernfs_remove_by_name_ns+0x4a/0x90
remove_files.isra.1+0x2b/0x60
sysfs_remove_group+0x38/0x80
sysfs_remove_groups+0x24/0x40
device_remove_attrs+0x3d/0x70
device_del+0x14c/0x360
device_unregister+0x15/0x50
tb_switch_remove+0x9e/0x1d0 [thunderbolt]
tb_handle_hotplug+0x119/0x5a0 [thunderbolt]
? process_one_work+0x1b7/0x420
process_one_work+0x1b7/0x420
worker_thread+0x37/0x380
? _raw_spin_unlock_irqrestore+0xf/0x30
? process_one_work+0x420/0x420
kthread+0x118/0x130
? kthread_create_on_node+0x60/0x60
ret_from_fork+0x35/0x40

We deal this by following what network stack did for some of their
attributes and use mutex_trylock() with restart_syscall(). This makes
userspace release the attribute allowing sysfs attribute removal to
progress before the write is restarted and eventually fail when the
attribute is removed.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

+20 -28
+19 -26
drivers/thunderbolt/switch.c
··· 10 10 #include <linux/idr.h> 11 11 #include <linux/nvmem-provider.h> 12 12 #include <linux/pm_runtime.h> 13 + #include <linux/sched/signal.h> 13 14 #include <linux/sizes.h> 14 15 #include <linux/slab.h> 15 16 #include <linux/vmalloc.h> 16 17 17 18 #include "tb.h" 18 - 19 - /* Switch authorization from userspace is serialized by this lock */ 20 - static DEFINE_MUTEX(switch_lock); 21 19 22 20 /* Switch NVM support */ 23 21 ··· 252 254 struct tb_switch *sw = priv; 253 255 int ret = 0; 254 256 255 - if (mutex_lock_interruptible(&switch_lock)) 256 - return -ERESTARTSYS; 257 + if (!mutex_trylock(&sw->tb->lock)) 258 + return restart_syscall(); 257 259 258 260 /* 259 261 * Since writing the NVM image might require some special steps, ··· 273 275 memcpy(sw->nvm->buf + offset, val, bytes); 274 276 275 277 unlock: 276 - mutex_unlock(&switch_lock); 278 + mutex_unlock(&sw->tb->lock); 277 279 278 280 return ret; 279 281 } ··· 362 364 } 363 365 nvm->non_active = nvm_dev; 364 366 365 - mutex_lock(&switch_lock); 366 367 sw->nvm = nvm; 367 - mutex_unlock(&switch_lock); 368 - 369 368 return 0; 370 369 371 370 err_nvm_active: ··· 379 384 { 380 385 struct tb_switch_nvm *nvm; 381 386 382 - mutex_lock(&switch_lock); 383 387 nvm = sw->nvm; 384 388 sw->nvm = NULL; 385 - mutex_unlock(&switch_lock); 386 389 387 390 if (!nvm) 388 391 return; ··· 691 698 { 692 699 int ret = -EINVAL; 693 700 694 - if (mutex_lock_interruptible(&switch_lock)) 695 - return -ERESTARTSYS; 701 + if (!mutex_trylock(&sw->tb->lock)) 702 + return restart_syscall(); 696 703 697 704 if (sw->authorized) 698 705 goto unlock; ··· 735 742 } 736 743 737 744 unlock: 738 - mutex_unlock(&switch_lock); 745 + mutex_unlock(&sw->tb->lock); 739 746 return ret; 740 747 } 741 748 ··· 792 799 struct tb_switch *sw = tb_to_switch(dev); 793 800 ssize_t ret; 794 801 795 - if (mutex_lock_interruptible(&switch_lock)) 796 - return -ERESTARTSYS; 802 + if (!mutex_trylock(&sw->tb->lock)) 803 + return restart_syscall(); 797 804 798 805 if (sw->key) 799 806 ret = sprintf(buf, "%*phN\n", TB_SWITCH_KEY_SIZE, sw->key); 800 807 else 801 808 ret = sprintf(buf, "\n"); 802 809 803 - mutex_unlock(&switch_lock); 810 + mutex_unlock(&sw->tb->lock); 804 811 return ret; 805 812 } 806 813 ··· 817 824 else if (hex2bin(key, buf, sizeof(key))) 818 825 return -EINVAL; 819 826 820 - if (mutex_lock_interruptible(&switch_lock)) 821 - return -ERESTARTSYS; 827 + if (!mutex_trylock(&sw->tb->lock)) 828 + return restart_syscall(); 822 829 823 830 if (sw->authorized) { 824 831 ret = -EBUSY; ··· 833 840 } 834 841 } 835 842 836 - mutex_unlock(&switch_lock); 843 + mutex_unlock(&sw->tb->lock); 837 844 return ret; 838 845 } 839 846 static DEVICE_ATTR(key, 0600, key_show, key_store); ··· 879 886 bool val; 880 887 int ret; 881 888 882 - if (mutex_lock_interruptible(&switch_lock)) 883 - return -ERESTARTSYS; 889 + if (!mutex_trylock(&sw->tb->lock)) 890 + return restart_syscall(); 884 891 885 892 /* If NVMem devices are not yet added */ 886 893 if (!sw->nvm) { ··· 928 935 } 929 936 930 937 exit_unlock: 931 - mutex_unlock(&switch_lock); 938 + mutex_unlock(&sw->tb->lock); 932 939 933 940 if (ret) 934 941 return ret; ··· 942 949 struct tb_switch *sw = tb_to_switch(dev); 943 950 int ret; 944 951 945 - if (mutex_lock_interruptible(&switch_lock)) 946 - return -ERESTARTSYS; 952 + if (!mutex_trylock(&sw->tb->lock)) 953 + return restart_syscall(); 947 954 948 955 if (sw->safe_mode) 949 956 ret = -ENODATA; ··· 952 959 else 953 960 ret = sprintf(buf, "%x.%x\n", sw->nvm->major, sw->nvm->minor); 954 961 955 - mutex_unlock(&switch_lock); 962 + mutex_unlock(&sw->tb->lock); 956 963 957 964 return ret; 958 965 }
+1 -2
drivers/thunderbolt/tb.h
··· 79 79 * @depth: Depth in the chain this switch is connected (ICM only) 80 80 * 81 81 * When the switch is being added or removed to the domain (other 82 - * switches) you need to have domain lock held. For switch authorization 83 - * internal switch_lock is enough. 82 + * switches) you need to have domain lock held. 84 83 */ 85 84 struct tb_switch { 86 85 struct device dev;