| Message ID | 20210617092016.522985-1-desmondcheongzx@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: break circular locks in blk_request_module | expand |
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; }
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
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!
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; } }
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(-)