* [PATCH] block: break circular locks in blk_request_module @ 2021-06-17 9:20 Desmond Cheong Zhi Xi 2021-06-17 11:51 ` Christoph Hellwig 0 siblings, 1 reply; 4+ messages in thread From: Desmond Cheong Zhi Xi @ 2021-06-17 9:20 UTC (permalink / raw) To: axboe Cc: Desmond Cheong Zhi Xi, linux-block, linux-kernel, skhan, gregkh, linux-kernel-mentees, syzbot+6a8a0d93c91e8fbf2e80 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 saving the reference to probe in blk_request_module, then calling it after releasing major_names_lock. This is safe because even if struct blk_major_name is freed, the reference to the probe function is still valid. Reported-and-tested-by: syzbot+6a8a0d93c91e8fbf2e80@syzkaller.appspotmail.com Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx@gmail.com> --- block/genhd.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/block/genhd.c b/block/genhd.c index 9f8cb7beaad1..ccaa5cf620f5 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -676,12 +676,14 @@ void blk_request_module(dev_t devt) { unsigned int major = MAJOR(devt); struct blk_major_name **n; + void (*probe)(dev_t devt); mutex_lock(&major_names_lock); for (n = &major_names[major_to_index(major)]; *n; n = &(*n)->next) { if ((*n)->major == major && (*n)->probe) { - (*n)->probe(devt); + probe = (*n)->probe; mutex_unlock(&major_names_lock); + probe(devt); return; } } -- 2.25.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] block: break circular locks in blk_request_module 2021-06-17 9:20 [PATCH] block: break circular locks in blk_request_module Desmond Cheong Zhi Xi @ 2021-06-17 11:51 ` Christoph Hellwig 2021-06-17 15:23 ` Desmond Cheong Zhi Xi 0 siblings, 1 reply; 4+ messages in thread From: Christoph Hellwig @ 2021-06-17 11:51 UTC (permalink / raw) To: Desmond Cheong Zhi Xi Cc: axboe, linux-block, linux-kernel, skhan, gregkh, linux-kernel-mentees, syzbot+6a8a0d93c91e8fbf2e80, miquel.raynal, richard, vigneshr, linux-mtd On Thu, Jun 17, 2021 at 05:20:16PM +0800, Desmond Cheong Zhi Xi wrote: > mutex_lock(&major_names_lock); > for (n = &major_names[major_to_index(major)]; *n; n = &(*n)->next) { > if ((*n)->major == major && (*n)->probe) { > - (*n)->probe(devt); > + probe = (*n)->probe; > mutex_unlock(&major_names_lock); > + probe(devt); And now you can all probe after it has been freed and/or the module has been unloaded. The obviously correct fix is to only hold mtd_table_mutex for the actually required critical section: diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c index fb8e12d590a1..065d94f9b1fb 100644 --- a/drivers/mtd/mtd_blkdevs.c +++ b/drivers/mtd/mtd_blkdevs.c @@ -529,13 +529,11 @@ int register_mtd_blktrans(struct mtd_blktrans_ops *tr) 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 +543,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; } ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] block: break circular locks in blk_request_module 2021-06-17 11:51 ` Christoph Hellwig @ 2021-06-17 15:23 ` Desmond Cheong Zhi Xi 2021-06-17 15:27 ` Christoph Hellwig 0 siblings, 1 reply; 4+ messages in thread From: Desmond Cheong Zhi Xi @ 2021-06-17 15:23 UTC (permalink / raw) To: Christoph Hellwig Cc: axboe, linux-block, linux-kernel, skhan, gregkh, linux-kernel-mentees, syzbot+6a8a0d93c91e8fbf2e80, miquel.raynal, richard, vigneshr, linux-mtd On 17/6/21 7:51 pm, Christoph Hellwig wrote: > On Thu, Jun 17, 2021 at 05:20:16PM +0800, Desmond Cheong Zhi Xi wrote: >> mutex_lock(&major_names_lock); >> for (n = &major_names[major_to_index(major)]; *n; n = &(*n)->next) { >> if ((*n)->major == major && (*n)->probe) { >> - (*n)->probe(devt); >> + probe = (*n)->probe; >> mutex_unlock(&major_names_lock); >> + probe(devt); > > And now you can all probe after it has been freed and/or the module has > been unloaded. The obviously correct fix is to only hold mtd_table_mutex > for the actually required critical section: > Thank you for the correction, Christoph. I hadn't thought of the scenario where the module is unloaded. I'll be more conscientious in the future. > diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c > index fb8e12d590a1..065d94f9b1fb 100644 > --- a/drivers/mtd/mtd_blkdevs.c > +++ b/drivers/mtd/mtd_blkdevs.c > @@ -529,13 +529,11 @@ int register_mtd_blktrans(struct mtd_blktrans_ops *tr) > 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 +543,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; > } > This fix passes the Syzkaller repro test on my local machine and on Syzbot. I can prepare a v2 patch for this. May I include you with the Co-developed-by: and Signed-off-by: tags? If another tag would be more appropriate, or if you want to submit the patch yourself, please let me know. Best wishes, Desmond ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] block: break circular locks in blk_request_module 2021-06-17 15:23 ` Desmond Cheong Zhi Xi @ 2021-06-17 15:27 ` Christoph Hellwig 0 siblings, 0 replies; 4+ messages in thread From: Christoph Hellwig @ 2021-06-17 15:27 UTC (permalink / raw) To: Desmond Cheong Zhi Xi Cc: Christoph Hellwig, axboe, linux-block, linux-kernel, skhan, gregkh, linux-kernel-mentees, syzbot+6a8a0d93c91e8fbf2e80, miquel.raynal, richard, vigneshr, linux-mtd On Thu, Jun 17, 2021 at 11:23:36PM +0800, Desmond Cheong Zhi Xi wrote: > This fix passes the Syzkaller repro test on my local machine and on Syzbot. > I can prepare a v2 patch for this. May I include you with the > Co-developed-by: and Signed-off-by: tags? If another tag would be more > appropriate, or if you want to submit the patch yourself, please let me > know. Sounds good to me, thanks! ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-06-17 15:28 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-17 9:20 [PATCH] block: break circular locks in blk_request_module Desmond Cheong Zhi Xi 2021-06-17 11:51 ` Christoph Hellwig 2021-06-17 15:23 ` Desmond Cheong Zhi Xi 2021-06-17 15:27 ` Christoph Hellwig
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).