2021-07-17 10:11:57

by Desmond Cheong Zhi Xi

Subject: [PATCH] mtd: fix lock hierarchy in deregister_mtd_blktrans

There is a lock hierarchy of major_names_lock --> mtd_table_mutex. One
existing chain is as follows:

1. major_names_lock --> loop_ctl_mutex (when blk_request_module calls
loop_probe)

2. loop_ctl_mutex --> bdev->bd_mutex (when loop_control_ioctl calls
loop_remove, which then calls del_gendisk)

3. bdev->bd_mutex --> mtd_table_mutex (when blkdev_get_by_dev calls
__blkdev_get, which then calls blktrans_open)

Since unregister_blkdev grabs the major_names_lock, we need to call it
outside the critical section for mtd_table_mutex, otherwise we invert
the lock hierarchy.

Reported-by: Hillf Danton <[email protected]>
Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
---
drivers/mtd/mtd_blkdevs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
index 7d26cfe24d05..87e26788ef60 100644
--- a/drivers/mtd/mtd_blkdevs.c
+++ b/drivers/mtd/mtd_blkdevs.c
@@ -563,8 +563,8 @@ int deregister_mtd_blktrans(struct mtd_blktrans_ops *tr)
list_for_each_entry_safe(dev, next, &tr->devs, list)
tr->remove_dev(dev);

- unregister_blkdev(tr->major, tr->name);
mutex_unlock(&mtd_table_mutex);
+ unregister_blkdev(tr->major, tr->name);

BUG_ON(!list_empty(&tr->devs));
return 0;
--
2.25.1

2021-08-02 11:17:56

by Desmond Cheong Zhi Xi

Subject: Re: [PATCH] mtd: fix lock hierarchy in deregister_mtd_blktrans

On 17/7/21 6:07 pm, Desmond Cheong Zhi Xi wrote:
> There is a lock hierarchy of major_names_lock --> mtd_table_mutex. One
> existing chain is as follows:
>
> 1. major_names_lock --> loop_ctl_mutex (when blk_request_module calls
> loop_probe)
>
> 2. loop_ctl_mutex --> bdev->bd_mutex (when loop_control_ioctl calls
> loop_remove, which then calls del_gendisk)
>
> 3. bdev->bd_mutex --> mtd_table_mutex (when blkdev_get_by_dev calls
> __blkdev_get, which then calls blktrans_open)
>
> Since unregister_blkdev grabs the major_names_lock, we need to call it
> outside the critical section for mtd_table_mutex, otherwise we invert
> the lock hierarchy.
>
> Reported-by: Hillf Danton <[email protected]>
> Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
> ---
> drivers/mtd/mtd_blkdevs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
> index 7d26cfe24d05..87e26788ef60 100644
> --- a/drivers/mtd/mtd_blkdevs.c
> +++ b/drivers/mtd/mtd_blkdevs.c
> @@ -563,8 +563,8 @@ int deregister_mtd_blktrans(struct mtd_blktrans_ops *tr)
> list_for_each_entry_safe(dev, next, &tr->devs, list)
> tr->remove_dev(dev);
>
> - unregister_blkdev(tr->major, tr->name);
> mutex_unlock(&mtd_table_mutex);
> + unregister_blkdev(tr->major, tr->name);
>
> BUG_ON(!list_empty(&tr->devs));
> return 0;
>

Hi Miqu?l,

Just a friendly ping, this patch is part 2 of fixing the lock hierarchy
inversion between major_names_lock and mtd_table_mutex that was
identified by Hillf Danton.

Best wishes,
Desmond

2021-08-07 00:08:56

by Miquel Raynal

Subject: Re: [PATCH] mtd: fix lock hierarchy in deregister_mtd_blktrans

On Sat, 2021-07-17 at 10:07:19 UTC, Desmond Cheong Zhi Xi wrote:
> There is a lock hierarchy of major_names_lock --> mtd_table_mutex. One
> existing chain is as follows:
>
> 1. major_names_lock --> loop_ctl_mutex (when blk_request_module calls
> loop_probe)
>
> 2. loop_ctl_mutex --> bdev->bd_mutex (when loop_control_ioctl calls
> loop_remove, which then calls del_gendisk)
>
> 3. bdev->bd_mutex --> mtd_table_mutex (when blkdev_get_by_dev calls
> __blkdev_get, which then calls blktrans_open)
>
> Since unregister_blkdev grabs the major_names_lock, we need to call it
> outside the critical section for mtd_table_mutex, otherwise we invert
> the lock hierarchy.
>
> Reported-by: Hillf Danton <[email protected]>
> Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/fixes, thanks.

Miquel