]>
Commit | Line | Data |
---|---|---|
21cf661e GKH |
1 | From 7f75591fc5a123929a29636834d1bcb8b5c9fee3 Mon Sep 17 00:00:00 2001 |
2 | From: Fabrice Gasnier <fabrice.gasnier@st.com> | |
3 | Date: Mon, 25 Mar 2019 14:01:23 +0100 | |
4 | Subject: iio: core: fix a possible circular locking dependency | |
5 | ||
6 | From: Fabrice Gasnier <fabrice.gasnier@st.com> | |
7 | ||
8 | commit 7f75591fc5a123929a29636834d1bcb8b5c9fee3 upstream. | |
9 | ||
10 | This fixes a possible circular locking dependency detected warning seen | |
11 | with: | |
12 | - CONFIG_PROVE_LOCKING=y | |
13 | - consumer/provider IIO devices (ex: "voltage-divider" consumer of "adc") | |
14 | ||
15 | When using the IIO consumer interface, e.g. iio_channel_get(), the consumer | |
16 | device will likely call iio_read_channel_raw() or similar that rely on | |
17 | 'info_exist_lock' mutex. | |
18 | ||
19 | typically: | |
20 | ... | |
21 | mutex_lock(&chan->indio_dev->info_exist_lock); | |
22 | if (chan->indio_dev->info == NULL) { | |
23 | ret = -ENODEV; | |
24 | goto err_unlock; | |
25 | } | |
26 | ret = do_some_ops() | |
27 | err_unlock: | |
28 | mutex_unlock(&chan->indio_dev->info_exist_lock); | |
29 | return ret; | |
30 | ... | |
31 | ||
32 | Same mutex is also hold in iio_device_unregister(). | |
33 | ||
34 | The following deadlock warning happens when: | |
35 | - the consumer device has called an API like iio_read_channel_raw() | |
36 | at least once. | |
37 | - the consumer driver is unregistered, removed (unbind from sysfs) | |
38 | ||
39 | ====================================================== | |
40 | WARNING: possible circular locking dependency detected | |
41 | 4.19.24 #577 Not tainted | |
42 | ------------------------------------------------------ | |
43 | sh/372 is trying to acquire lock: | |
44 | (kn->count#30){++++}, at: kernfs_remove_by_name_ns+0x3c/0x84 | |
45 | ||
46 | but task is already holding lock: | |
47 | (&dev->info_exist_lock){+.+.}, at: iio_device_unregister+0x18/0x60 | |
48 | ||
49 | which lock already depends on the new lock. | |
50 | ||
51 | the existing dependency chain (in reverse order) is: | |
52 | ||
53 | -> #1 (&dev->info_exist_lock){+.+.}: | |
54 | __mutex_lock+0x70/0xa3c | |
55 | mutex_lock_nested+0x1c/0x24 | |
56 | iio_read_channel_raw+0x1c/0x60 | |
57 | iio_read_channel_info+0xa8/0xb0 | |
58 | dev_attr_show+0x1c/0x48 | |
59 | sysfs_kf_seq_show+0x84/0xec | |
60 | seq_read+0x154/0x528 | |
61 | __vfs_read+0x2c/0x15c | |
62 | vfs_read+0x8c/0x110 | |
63 | ksys_read+0x4c/0xac | |
64 | ret_fast_syscall+0x0/0x28 | |
65 | 0xbedefb60 | |
66 | ||
67 | -> #0 (kn->count#30){++++}: | |
68 | lock_acquire+0xd8/0x268 | |
69 | __kernfs_remove+0x288/0x374 | |
70 | kernfs_remove_by_name_ns+0x3c/0x84 | |
71 | remove_files+0x34/0x78 | |
72 | sysfs_remove_group+0x40/0x9c | |
73 | sysfs_remove_groups+0x24/0x34 | |
74 | device_remove_attrs+0x38/0x64 | |
75 | device_del+0x11c/0x360 | |
76 | cdev_device_del+0x14/0x2c | |
77 | iio_device_unregister+0x24/0x60 | |
78 | release_nodes+0x1bc/0x200 | |
79 | device_release_driver_internal+0x1a0/0x230 | |
80 | unbind_store+0x80/0x130 | |
81 | kernfs_fop_write+0x100/0x1e4 | |
82 | __vfs_write+0x2c/0x160 | |
83 | vfs_write+0xa4/0x17c | |
84 | ksys_write+0x4c/0xac | |
85 | ret_fast_syscall+0x0/0x28 | |
86 | 0xbe906840 | |
87 | ||
88 | other info that might help us debug this: | |
89 | ||
90 | Possible unsafe locking scenario: | |
91 | ||
92 | CPU0 CPU1 | |
93 | ---- ---- | |
94 | lock(&dev->info_exist_lock); | |
95 | lock(kn->count#30); | |
96 | lock(&dev->info_exist_lock); | |
97 | lock(kn->count#30); | |
98 | ||
99 | *** DEADLOCK *** | |
100 | ... | |
101 | ||
102 | cdev_device_del() can be called without holding the lock. It should be safe | |
103 | as info_exist_lock prevents kernelspace consumers to use the exported | |
104 | routines during/after provider removal. cdev_device_del() is for userspace. | |
105 | ||
106 | Help to reproduce: | |
107 | See example: Documentation/devicetree/bindings/iio/afe/voltage-divider.txt | |
108 | sysv { | |
109 | compatible = "voltage-divider"; | |
110 | io-channels = <&adc 0>; | |
111 | output-ohms = <22>; | |
112 | full-ohms = <222>; | |
113 | }; | |
114 | ||
115 | First, go to iio:deviceX for the "voltage-divider", do one read: | |
116 | $ cd /sys/bus/iio/devices/iio:deviceX | |
117 | $ cat in_voltage0_raw | |
118 | ||
119 | Then, unbind the consumer driver. It triggers above deadlock warning. | |
120 | $ cd /sys/bus/platform/drivers/iio-rescale/ | |
121 | $ echo sysv > unbind | |
122 | ||
123 | Note I don't actually expect stable will pick this up all the | |
124 | way back into IIO being in staging, but if's probably valid that | |
125 | far back. | |
126 | ||
127 | Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com> | |
128 | Fixes: ac917a81117c ("staging:iio:core set the iio_dev.info pointer to null on unregister") | |
129 | Cc: <Stable@vger.kernel.org> | |
130 | Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> | |
131 | Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> | |
132 | ||
133 | --- | |
134 | drivers/iio/industrialio-core.c | 4 ++-- | |
135 | 1 file changed, 2 insertions(+), 2 deletions(-) | |
136 | ||
137 | --- a/drivers/iio/industrialio-core.c | |
138 | +++ b/drivers/iio/industrialio-core.c | |
139 | @@ -1741,10 +1741,10 @@ EXPORT_SYMBOL(iio_device_register); | |
140 | **/ | |
141 | void iio_device_unregister(struct iio_dev *indio_dev) | |
142 | { | |
143 | - mutex_lock(&indio_dev->info_exist_lock); | |
144 | - | |
145 | cdev_device_del(&indio_dev->chrdev, &indio_dev->dev); | |
146 | ||
147 | + mutex_lock(&indio_dev->info_exist_lock); | |
148 | + | |
149 | iio_device_unregister_debugfs(indio_dev); | |
150 | ||
151 | iio_disable_all_buffers(indio_dev); |