iio: core: fix a possible circular locking dependency [Linux 3.16.72]

This Linux kernel change "iio: core: fix a possible circular locking dependency" is included in the Linux 3.16.72 release. This change is authored by Fabrice Gasnier <fabrice.gasnier [at] st.com> on Mon Mar 25 14:01:23 2019 +0100. The commit for this change in Linux stable tree is 246ead0 (patch) which is from upstream commit 7f75591. The same Linux upstream change may have been applied to various maintained Linux releases and you can find all Linux releases containing changes from upstream 7f75591.

iio: core: fix a possible circular locking dependency

commit 7f75591fc5a123929a29636834d1bcb8b5c9fee3 upstream.

This fixes a possible circular locking dependency detected warning seen
with:
- CONFIG_PROVE_LOCKING=y
- consumer/provider IIO devices (ex: "voltage-divider" consumer of "adc")

When using the IIO consumer interface, e.g. iio_channel_get(), the consumer
device will likely call iio_read_channel_raw() or similar that rely on
'info_exist_lock' mutex.

typically:
...
    mutex_lock(&chan->indio_dev->info_exist_lock);
    if (chan->indio_dev->info == NULL) {
        ret = -ENODEV;
        goto err_unlock;
    }
    ret = do_some_ops()
err_unlock:
    mutex_unlock(&chan->indio_dev->info_exist_lock);
    return ret;
...

Same mutex is also hold in iio_device_unregister().

The following deadlock warning happens when:
- the consumer device has called an API like iio_read_channel_raw()
  at least once.
- the consumer driver is unregistered, removed (unbind from sysfs)

======================================================
WARNING: possible circular locking dependency detected
4.19.24 #577 Not tainted
------------------------------------------------------
sh/372 is trying to acquire lock:
(kn->count#30){++++}, at: kernfs_remove_by_name_ns+0x3c/0x84

but task is already holding lock:
(&dev->info_exist_lock){+.+.}, at: iio_device_unregister+0x18/0x60

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #1 (&dev->info_exist_lock){+.+.}:
       __mutex_lock+0x70/0xa3c
       mutex_lock_nested+0x1c/0x24
       iio_read_channel_raw+0x1c/0x60
       iio_read_channel_info+0xa8/0xb0
       dev_attr_show+0x1c/0x48
       sysfs_kf_seq_show+0x84/0xec
       seq_read+0x154/0x528
       __vfs_read+0x2c/0x15c
       vfs_read+0x8c/0x110
       ksys_read+0x4c/0xac
       ret_fast_syscall+0x0/0x28
       0xbedefb60

-> #0 (kn->count#30){++++}:
       lock_acquire+0xd8/0x268
       __kernfs_remove+0x288/0x374
       kernfs_remove_by_name_ns+0x3c/0x84
       remove_files+0x34/0x78
       sysfs_remove_group+0x40/0x9c
       sysfs_remove_groups+0x24/0x34
       device_remove_attrs+0x38/0x64
       device_del+0x11c/0x360
       cdev_device_del+0x14/0x2c
       iio_device_unregister+0x24/0x60
       release_nodes+0x1bc/0x200
       device_release_driver_internal+0x1a0/0x230
       unbind_store+0x80/0x130
       kernfs_fop_write+0x100/0x1e4
       __vfs_write+0x2c/0x160
       vfs_write+0xa4/0x17c
       ksys_write+0x4c/0xac
       ret_fast_syscall+0x0/0x28
       0xbe906840

other info that might help us debug this:

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(&dev->info_exist_lock);
                               lock(kn->count#30);
                               lock(&dev->info_exist_lock);
  lock(kn->count#30);

 *** DEADLOCK ***
...

cdev_device_del() can be called without holding the lock. It should be safe
as info_exist_lock prevents kernelspace consumers to use the exported
routines during/after provider removal. cdev_device_del() is for userspace.

Help to reproduce:
See example: Documentation/devicetree/bindings/iio/afe/voltage-divider.txt
sysv {
    compatible = "voltage-divider";
    io-channels = <&adc 0>;
    output-ohms = <22>;
    full-ohms = <222>;
};

First, go to iio:deviceX for the "voltage-divider", do one read:
$ cd /sys/bus/iio/devices/iio:deviceX
$ cat in_voltage0_raw

Then, unbind the consumer driver. It triggers above deadlock warning.
$ cd /sys/bus/platform/drivers/iio-rescale/
$ echo sysv > unbind

Note I don't actually expect stable will pick this up all the
way back into IIO being in staging, but if's probably valid that
far back.

Signed-off-by: Fabrice Gasnier <[email protected]>
Fixes: ac917a81117c ("staging:iio:core set the iio_dev.info pointer to null on unregister")
Signed-off-by: Jonathan Cameron <[email protected]>
[bwh: Backported to 3.16: adjust context]
Signed-off-by: Ben Hutchings <[email protected]>

There are 4 lines of Linux source code added/deleted in this change. Code changes to Linux kernel are as follows.

 drivers/iio/industrialio-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 4d1b400..5ea56ed 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -1195,12 +1195,12 @@ int iio_device_register(struct iio_dev *indio_dev)
  **/
 void iio_device_unregister(struct iio_dev *indio_dev)
 {
-   mutex_lock(&indio_dev->info_exist_lock);
-
    device_del(&indio_dev->dev);

    if (indio_dev->chrdev.dev)
        cdev_del(&indio_dev->chrdev);
+
+   mutex_lock(&indio_dev->info_exist_lock);
    iio_device_unregister_debugfs(indio_dev);

    iio_disable_all_buffers(indio_dev);

Leave a Reply

Your email address will not be published. Required fields are marked *