[4.2.y-ckt stable] Patch "mtd: blkdevs: fix potential deadlock + lockdep warnings" has been added to staging queue
Kamal Mostafa
kamal at canonical.com
Mon Jan 4 23:21:57 UTC 2016
This is a note to let you know that I have just added a patch titled
mtd: blkdevs: fix potential deadlock + lockdep warnings
to the linux-4.2.y-queue branch of the 4.2.y-ckt extended stable tree
which can be found at:
http://kernel.ubuntu.com/git/ubuntu/linux.git/log/?h=linux-4.2.y-queue
This patch is scheduled to be released in version 4.2.8-ckt1.
If you, or anyone else, feels it should not be added to this tree, please
reply to this email.
For more information about the 4.2.y-ckt tree, see
https://wiki.ubuntu.com/Kernel/Dev/ExtendedStable
Thanks.
-Kamal
------
>From 6cca233e8d03235060d0c5c58f2b656d8bba35b1 Mon Sep 17 00:00:00 2001
From: Brian Norris <computersforpeace at gmail.com>
Date: Mon, 26 Oct 2015 10:20:23 -0700
Subject: mtd: blkdevs: fix potential deadlock + lockdep warnings
commit f3c63795e90f0c6238306883b6c72f14d5355721 upstream.
Commit 073db4a51ee4 ("mtd: fix: avoid race condition when accessing
mtd->usecount") fixed a race condition but due to poor ordering of the
mutex acquisition, introduced a potential deadlock.
The deadlock can occur, for example, when rmmod'ing the m25p80 module, which
will delete one or more MTDs, along with any corresponding mtdblock
devices. This could potentially race with an acquisition of the block
device as follows.
-> blktrans_open()
-> mutex_lock(&dev->lock);
-> mutex_lock(&mtd_table_mutex);
-> del_mtd_device()
-> mutex_lock(&mtd_table_mutex);
-> blktrans_notify_remove() -> del_mtd_blktrans_dev()
-> mutex_lock(&dev->lock);
This is a classic (potential) ABBA deadlock, which can be fixed by
making the A->B ordering consistent everywhere. There was no real
purpose to the ordering in the original patch, AFAIR, so this shouldn't
be a problem. This ordering was actually already present in
del_mtd_blktrans_dev(), for one, where the function tried to ensure that
its caller already held mtd_table_mutex before it acquired &dev->lock:
if (mutex_trylock(&mtd_table_mutex)) {
mutex_unlock(&mtd_table_mutex);
BUG();
}
So, reverse the ordering of acquisition of &dev->lock and &mtd_table_mutex so
we always acquire mtd_table_mutex first.
Snippets of the lockdep output follow:
# modprobe -r m25p80
[ 53.419251]
[ 53.420838] ======================================================
[ 53.427300] [ INFO: possible circular locking dependency detected ]
[ 53.433865] 4.3.0-rc6 #96 Not tainted
[ 53.437686] -------------------------------------------------------
[ 53.444220] modprobe/372 is trying to acquire lock:
[ 53.449320] (&new->lock){+.+...}, at: [<c043fe4c>] del_mtd_blktrans_dev+0x80/0xdc
[ 53.457271]
[ 53.457271] but task is already holding lock:
[ 53.463372] (mtd_table_mutex){+.+.+.}, at: [<c0439994>] del_mtd_device+0x18/0x100
[ 53.471321]
[ 53.471321] which lock already depends on the new lock.
[ 53.471321]
[ 53.479856]
[ 53.479856] the existing dependency chain (in reverse order) is:
[ 53.487660]
-> #1 (mtd_table_mutex){+.+.+.}:
[ 53.492331] [<c043fc5c>] blktrans_open+0x34/0x1a4
[ 53.497879] [<c01afce0>] __blkdev_get+0xc4/0x3b0
[ 53.503364] [<c01b0bb8>] blkdev_get+0x108/0x320
[ 53.508743] [<c01713c0>] do_dentry_open+0x218/0x314
[ 53.514496] [<c0180454>] path_openat+0x4c0/0xf9c
[ 53.519959] [<c0182044>] do_filp_open+0x5c/0xc0
[ 53.525336] [<c0172758>] do_sys_open+0xfc/0x1cc
[ 53.530716] [<c000f740>] ret_fast_syscall+0x0/0x1c
[ 53.536375]
-> #0 (&new->lock){+.+...}:
[ 53.540587] [<c063f124>] mutex_lock_nested+0x38/0x3cc
[ 53.546504] [<c043fe4c>] del_mtd_blktrans_dev+0x80/0xdc
[ 53.552606] [<c043f164>] blktrans_notify_remove+0x7c/0x84
[ 53.558891] [<c04399f0>] del_mtd_device+0x74/0x100
[ 53.564544] [<c043c670>] del_mtd_partitions+0x80/0xc8
[ 53.570451] [<c0439aa0>] mtd_device_unregister+0x24/0x48
[ 53.576637] [<c046ce6c>] spi_drv_remove+0x1c/0x34
[ 53.582207] [<c03de0f0>] __device_release_driver+0x88/0x114
[ 53.588663] [<c03de19c>] device_release_driver+0x20/0x2c
[ 53.594843] [<c03dd9e8>] bus_remove_device+0xd8/0x108
[ 53.600748] [<c03dacc0>] device_del+0x10c/0x210
[ 53.606127] [<c03dadd0>] device_unregister+0xc/0x20
[ 53.611849] [<c046d878>] __unregister+0x10/0x20
[ 53.617211] [<c03da868>] device_for_each_child+0x50/0x7c
[ 53.623387] [<c046eae8>] spi_unregister_master+0x58/0x8c
[ 53.629578] [<c03e12f0>] release_nodes+0x15c/0x1c8
[ 53.635223] [<c03de0f8>] __device_release_driver+0x90/0x114
[ 53.641689] [<c03de900>] driver_detach+0xb4/0xb8
[ 53.647147] [<c03ddc78>] bus_remove_driver+0x4c/0xa0
[ 53.652970] [<c00cab50>] SyS_delete_module+0x11c/0x1e4
[ 53.658976] [<c000f740>] ret_fast_syscall+0x0/0x1c
[ 53.664621]
[ 53.664621] other info that might help us debug this:
[ 53.664621]
[ 53.672979] Possible unsafe locking scenario:
[ 53.672979]
[ 53.679169] CPU0 CPU1
[ 53.683900] ---- ----
[ 53.688633] lock(mtd_table_mutex);
[ 53.692383] lock(&new->lock);
[ 53.698306] lock(mtd_table_mutex);
[ 53.704658] lock(&new->lock);
[ 53.707946]
[ 53.707946] *** DEADLOCK ***
Fixes: 073db4a51ee4 ("mtd: fix: avoid race condition when accessing mtd->usecount")
Reported-by: Felipe Balbi <balbi at ti.com>
Tested-by: Felipe Balbi <balbi at ti.com>
Signed-off-by: Brian Norris <computersforpeace at gmail.com>
Signed-off-by: Kamal Mostafa <kamal at canonical.com>
---
drivers/mtd/mtd_blkdevs.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
index 41acc50..6b5b83d 100644
--- a/drivers/mtd/mtd_blkdevs.c
+++ b/drivers/mtd/mtd_blkdevs.c
@@ -196,8 +196,8 @@ static int blktrans_open(struct block_device *bdev, fmode_t mode)
if (!dev)
return -ERESTARTSYS; /* FIXME: busy loop! -arnd*/
- mutex_lock(&dev->lock);
mutex_lock(&mtd_table_mutex);
+ mutex_lock(&dev->lock);
if (dev->open)
goto unlock;
@@ -221,8 +221,8 @@ static int blktrans_open(struct block_device *bdev, fmode_t mode)
unlock:
dev->open++;
- mutex_unlock(&mtd_table_mutex);
mutex_unlock(&dev->lock);
+ mutex_unlock(&mtd_table_mutex);
blktrans_dev_put(dev);
return ret;
@@ -232,8 +232,8 @@ error_release:
error_put:
module_put(dev->tr->owner);
kref_put(&dev->ref, blktrans_dev_release);
- mutex_unlock(&mtd_table_mutex);
mutex_unlock(&dev->lock);
+ mutex_unlock(&mtd_table_mutex);
blktrans_dev_put(dev);
return ret;
}
@@ -245,8 +245,8 @@ static void blktrans_release(struct gendisk *disk, fmode_t mode)
if (!dev)
return;
- mutex_lock(&dev->lock);
mutex_lock(&mtd_table_mutex);
+ mutex_lock(&dev->lock);
if (--dev->open)
goto unlock;
@@ -260,8 +260,8 @@ static void blktrans_release(struct gendisk *disk, fmode_t mode)
__put_mtd_device(dev->mtd);
}
unlock:
- mutex_unlock(&mtd_table_mutex);
mutex_unlock(&dev->lock);
+ mutex_unlock(&mtd_table_mutex);
blktrans_dev_put(dev);
}
--
1.9.1
More information about the kernel-team
mailing list