Syzbot reported a circular locking dependency:
https://syzkaller.appspot.com/bug?id=7bd106c28e846d1023d4ca915718b1a0905444cb
This happens because of the following lock dependencies:
1. loop_ctl_mutex -> bdev->bd_mutex (when loop_control_ioctl calls
loop_remove, which then calls del_gendisk; this also happens in
loop_exit which eventually calls loop_remove)
2. bdev->bd_mutex -> mtd_table_mutex (when blkdev_get_by_dev calls
__blkdev_get, which then calls blktrans_open)
3. mtd_table_mutex -> major_names_lock (when register_mtd_blktrans
calls __register_blkdev)
4. major_names_lock -> loop_ctl_mutex (when blk_request_module calls
loop_probe)
Hence there's an overall dependency of:
loop_ctl_mutex ----------> bdev->bd_mutex
^ |
| |
| v
major_names_lock <--------- mtd_table_mutex
We can break this circular dependency by holding mtd_table_mutex only
for the required critical section in register_mtd_blktrans. This
avoids the mtd_table_mutex -> major_names_lock dependency.
Reported-and-tested-by: [email protected]
Co-developed-by: Christoph Hellwig <[email protected]>
Signed-off-by: Christoph Hellwig <[email protected]>
Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
---
Changes in v1 -> v2:
Break the circular dependency in register_mtd_blktrans instead of blk_request_module, as suggested by Christoph Hellwig.
drivers/mtd/mtd_blkdevs.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
index fb8e12d590a1..7d26cfe24d05 100644
--- a/drivers/mtd/mtd_blkdevs.c
+++ b/drivers/mtd/mtd_blkdevs.c
@@ -528,14 +528,10 @@ int register_mtd_blktrans(struct mtd_blktrans_ops *tr)
if (!blktrans_notifier.list.next)
register_mtd_user(&blktrans_notifier);
-
- mutex_lock(&mtd_table_mutex);
-
ret = register_blkdev(tr->major, tr->name);
if (ret < 0) {
printk(KERN_WARNING "Unable to register %s block device on major %d: %d\n",
tr->name, tr->major, ret);
- mutex_unlock(&mtd_table_mutex);
return ret;
}
@@ -545,12 +541,12 @@ int register_mtd_blktrans(struct mtd_blktrans_ops *tr)
tr->blkshift = ffs(tr->blksize) - 1;
INIT_LIST_HEAD(&tr->devs);
- list_add(&tr->list, &blktrans_majors);
+ mutex_lock(&mtd_table_mutex);
+ list_add(&tr->list, &blktrans_majors);
mtd_for_each_device(mtd)
if (mtd->type != MTD_ABSENT)
tr->add_mtd(tr, mtd);
-
mutex_unlock(&mtd_table_mutex);
return 0;
}
--
2.25.1
On 18/6/21 12:09 am, Desmond Cheong Zhi Xi wrote:
> Syzbot reported a circular locking dependency:
> https://syzkaller.appspot.com/bug?id=7bd106c28e846d1023d4ca915718b1a0905444cb
>
> This happens because of the following lock dependencies:
>
> 1. loop_ctl_mutex -> bdev->bd_mutex (when loop_control_ioctl calls
> loop_remove, which then calls del_gendisk; this also happens in
> loop_exit which eventually calls loop_remove)
>
> 2. bdev->bd_mutex -> mtd_table_mutex (when blkdev_get_by_dev calls
> __blkdev_get, which then calls blktrans_open)
>
> 3. mtd_table_mutex -> major_names_lock (when register_mtd_blktrans
> calls __register_blkdev)
>
> 4. major_names_lock -> loop_ctl_mutex (when blk_request_module calls
> loop_probe)
>
> Hence there's an overall dependency of:
>
> loop_ctl_mutex ----------> bdev->bd_mutex
> ^ |
> | |
> | v
> major_names_lock <--------- mtd_table_mutex
>
> We can break this circular dependency by holding mtd_table_mutex only
> for the required critical section in register_mtd_blktrans. This
> avoids the mtd_table_mutex -> major_names_lock dependency.
>
> Reported-and-tested-by: [email protected]
> Co-developed-by: Christoph Hellwig <[email protected]>
> Signed-off-by: Christoph Hellwig <[email protected]>
> Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
> ---
>
> Changes in v1 -> v2:
>
> Break the circular dependency in register_mtd_blktrans instead of blk_request_module, as suggested by Christoph Hellwig.
>
> drivers/mtd/mtd_blkdevs.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
> index fb8e12d590a1..7d26cfe24d05 100644
> --- a/drivers/mtd/mtd_blkdevs.c
> +++ b/drivers/mtd/mtd_blkdevs.c
> @@ -528,14 +528,10 @@ int register_mtd_blktrans(struct mtd_blktrans_ops *tr)
> if (!blktrans_notifier.list.next)
> register_mtd_user(&blktrans_notifier);
>
> -
> - mutex_lock(&mtd_table_mutex);
> -
> ret = register_blkdev(tr->major, tr->name);
> if (ret < 0) {
> printk(KERN_WARNING "Unable to register %s block device on major %d: %d\n",
> tr->name, tr->major, ret);
> - mutex_unlock(&mtd_table_mutex);
> return ret;
> }
>
> @@ -545,12 +541,12 @@ int register_mtd_blktrans(struct mtd_blktrans_ops *tr)
> tr->blkshift = ffs(tr->blksize) - 1;
>
> INIT_LIST_HEAD(&tr->devs);
> - list_add(&tr->list, &blktrans_majors);
>
> + mutex_lock(&mtd_table_mutex);
> + list_add(&tr->list, &blktrans_majors);
> mtd_for_each_device(mtd)
> if (mtd->type != MTD_ABSENT)
> tr->add_mtd(tr, mtd);
> -
> mutex_unlock(&mtd_table_mutex);
> return 0;
> }
>
Hi maintainers,
Any chance to review this patch?
For additional reference, the mtd_table_mutex --> major_names_lock
hierarchy that can be removed by this patch also appears in a different
lock chain:
https://syzkaller.appspot.com/bug?id=cbf5fe846f14a90f05e10df200b08c57941dc750
Best wishes,
Desmond
Hello,
Desmond Cheong Zhi Xi <[email protected]> wrote on Wed, 30 Jun
2021 16:04:17 +0800:
> On 18/6/21 12:09 am, Desmond Cheong Zhi Xi wrote:
> > Syzbot reported a circular locking dependency:
> > https://syzkaller.appspot.com/bug?id=7bd106c28e846d1023d4ca915718b1a0905444cb
> >
> > This happens because of the following lock dependencies:
> >
> > 1. loop_ctl_mutex -> bdev->bd_mutex (when loop_control_ioctl calls
> > loop_remove, which then calls del_gendisk; this also happens in
> > loop_exit which eventually calls loop_remove)
> >
> > 2. bdev->bd_mutex -> mtd_table_mutex (when blkdev_get_by_dev calls
> > __blkdev_get, which then calls blktrans_open)
> >
> > 3. mtd_table_mutex -> major_names_lock (when register_mtd_blktrans
> > calls __register_blkdev)
> >
> > 4. major_names_lock -> loop_ctl_mutex (when blk_request_module calls
> > loop_probe)
> >
> > Hence there's an overall dependency of:
> >
> > loop_ctl_mutex ----------> bdev->bd_mutex
> > ^ |
> > | |
> > | v
> > major_names_lock <--------- mtd_table_mutex
> >
> > We can break this circular dependency by holding mtd_table_mutex only
> > for the required critical section in register_mtd_blktrans. This
> > avoids the mtd_table_mutex -> major_names_lock dependency.
> >
> > Reported-and-tested-by: [email protected]
> > Co-developed-by: Christoph Hellwig <[email protected]>
> > Signed-off-by: Christoph Hellwig <[email protected]>
> > Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
> > ---
> >
> > Changes in v1 -> v2:
> >
> > Break the circular dependency in register_mtd_blktrans instead of blk_request_module, as suggested by Christoph Hellwig.
> >
> > drivers/mtd/mtd_blkdevs.c | 8 ++------
> > 1 file changed, 2 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
> > index fb8e12d590a1..7d26cfe24d05 100644
> > --- a/drivers/mtd/mtd_blkdevs.c
> > +++ b/drivers/mtd/mtd_blkdevs.c
> > @@ -528,14 +528,10 @@ int register_mtd_blktrans(struct mtd_blktrans_ops *tr)
> > if (!blktrans_notifier.list.next)
> > register_mtd_user(&blktrans_notifier);
> > > -
> > - mutex_lock(&mtd_table_mutex);
> > -
> > ret = register_blkdev(tr->major, tr->name);
> > if (ret < 0) {
> > printk(KERN_WARNING "Unable to register %s block device on major %d: %d\n",
> > tr->name, tr->major, ret);
> > - mutex_unlock(&mtd_table_mutex);
> > return ret;
> > }
> > > @@ -545,12 +541,12 @@ int register_mtd_blktrans(struct mtd_blktrans_ops *tr)
> > tr->blkshift = ffs(tr->blksize) - 1;
> > > INIT_LIST_HEAD(&tr->devs);
> > - list_add(&tr->list, &blktrans_majors);
> > > + mutex_lock(&mtd_table_mutex);
> > + list_add(&tr->list, &blktrans_majors);
> > mtd_for_each_device(mtd)
> > if (mtd->type != MTD_ABSENT)
> > tr->add_mtd(tr, mtd);
> > -
> > mutex_unlock(&mtd_table_mutex);
> > return 0;
> > }
> >
>
> Hi maintainers,
>
> Any chance to review this patch?
>
> For additional reference, the mtd_table_mutex --> major_names_lock hierarchy that can be removed by this patch also appears in a different lock chain:
> https://syzkaller.appspot.com/bug?id=cbf5fe846f14a90f05e10df200b08c57941dc750
I'm fine with the patch, but it came too late in the release cycle so
now I'm waiting -rc1 to apply it.
Thanks,
Miquèl
On 30/6/21 4:43 pm, Miquel Raynal wrote:
> Hello,
>
> Desmond Cheong Zhi Xi <[email protected]> wrote on Wed, 30 Jun
> 2021 16:04:17 +0800:
>
>> On 18/6/21 12:09 am, Desmond Cheong Zhi Xi wrote:
>>> Syzbot reported a circular locking dependency:
>>> https://syzkaller.appspot.com/bug?id=7bd106c28e846d1023d4ca915718b1a0905444cb
>>>
>>> This happens because of the following lock dependencies:
>>>
>>> 1. loop_ctl_mutex -> bdev->bd_mutex (when loop_control_ioctl calls
>>> loop_remove, which then calls del_gendisk; this also happens in
>>> loop_exit which eventually calls loop_remove)
>>>
>>> 2. bdev->bd_mutex -> mtd_table_mutex (when blkdev_get_by_dev calls
>>> __blkdev_get, which then calls blktrans_open)
>>>
>>> 3. mtd_table_mutex -> major_names_lock (when register_mtd_blktrans
>>> calls __register_blkdev)
>>>
>>> 4. major_names_lock -> loop_ctl_mutex (when blk_request_module calls
>>> loop_probe)
>>>
>>> Hence there's an overall dependency of:
>>>
>>> loop_ctl_mutex ----------> bdev->bd_mutex
>>> ^ |
>>> | |
>>> | v
>>> major_names_lock <--------- mtd_table_mutex
>>>
>>> We can break this circular dependency by holding mtd_table_mutex only
>>> for the required critical section in register_mtd_blktrans. This
>>> avoids the mtd_table_mutex -> major_names_lock dependency.
>>>
>>> Reported-and-tested-by: [email protected]
>>> Co-developed-by: Christoph Hellwig <[email protected]>
>>> Signed-off-by: Christoph Hellwig <[email protected]>
>>> Signed-off-by: Desmond Cheong Zhi Xi <[email protected]>
>>> ---
>>>
>>> Changes in v1 -> v2:
>>>
>>> Break the circular dependency in register_mtd_blktrans instead of blk_request_module, as suggested by Christoph Hellwig.
>>>
>>> drivers/mtd/mtd_blkdevs.c | 8 ++------
>>> 1 file changed, 2 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c
>>> index fb8e12d590a1..7d26cfe24d05 100644
>>> --- a/drivers/mtd/mtd_blkdevs.c
>>> +++ b/drivers/mtd/mtd_blkdevs.c
>>> @@ -528,14 +528,10 @@ int register_mtd_blktrans(struct mtd_blktrans_ops *tr)
>>> if (!blktrans_notifier.list.next)
>>> register_mtd_user(&blktrans_notifier);
>>> > -
>>> - mutex_lock(&mtd_table_mutex);
>>> -
>>> ret = register_blkdev(tr->major, tr->name);
>>> if (ret < 0) {
>>> printk(KERN_WARNING "Unable to register %s block device on major %d: %d\n",
>>> tr->name, tr->major, ret);
>>> - mutex_unlock(&mtd_table_mutex);
>>> return ret;
>>> }
>>> > @@ -545,12 +541,12 @@ int register_mtd_blktrans(struct mtd_blktrans_ops *tr)
>>> tr->blkshift = ffs(tr->blksize) - 1;
>>> > INIT_LIST_HEAD(&tr->devs);
>>> - list_add(&tr->list, &blktrans_majors);
>>> > + mutex_lock(&mtd_table_mutex);
>>> + list_add(&tr->list, &blktrans_majors);
>>> mtd_for_each_device(mtd)
>>> if (mtd->type != MTD_ABSENT)
>>> tr->add_mtd(tr, mtd);
>>> -
>>> mutex_unlock(&mtd_table_mutex);
>>> return 0;
>>> }
>>>
>>
>> Hi maintainers,
>>
>> Any chance to review this patch?
>>
>> For additional reference, the mtd_table_mutex --> major_names_lock hierarchy that can be removed by this patch also appears in a different lock chain:
>> https://syzkaller.appspot.com/bug?id=cbf5fe846f14a90f05e10df200b08c57941dc750
>
> I'm fine with the patch, but it came too late in the release cycle so
> now I'm waiting -rc1 to apply it.
>
> Thanks,
> Miquèl
>
Got it. Thanks for the review, Miquèl.
On Thu, 2021-06-17 at 16:09:04 UTC, Desmond Cheong Zhi Xi wrote:
> Syzbot reported a circular locking dependency:
> https://syzkaller.appspot.com/bug?id=7bd106c28e846d1023d4ca915718b1a0905444cb
>
> This happens because of the following lock dependencies:
>
> 1. loop_ctl_mutex -> bdev->bd_mutex (when loop_control_ioctl calls
> loop_remove, which then calls del_gendisk; this also happens in
> loop_exit which eventually calls loop_remove)
>
> 2. bdev->bd_mutex -> mtd_table_mutex (when blkdev_get_by_dev calls
> __blkdev_get, which then calls blktrans_open)
>
> 3. mtd_table_mutex -> major_names_lock (when register_mtd_blktrans
> calls __register_blkdev)
>
> 4. major_names_lock -> loop_ctl_mutex (when blk_request_module calls
> loop_probe)
>
> Hence there's an overall dependency of:
>
> loop_ctl_mutex ----------> bdev->bd_mutex
> ^ |
> | |
> | v
> major_names_lock <--------- mtd_table_mutex
>
> We can break this circular dependency by holding mtd_table_mutex only
> for the required critical section in register_mtd_blktrans. This
> avoids the mtd_table_mutex -> major_names_lock dependency.
>
> Reported-and-tested-by: [email protected]
> Co-developed-by: Christoph Hellwig <[email protected]>
> Signed-off-by: Christoph Hellwig <[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