Messages in this thread Patch in this message | | | Subject | Re: [PATCH] mtd_blkdevs: add mtd_table_mutex lock back to blktrans_{open, release} to avoid race condition | From | Liwei Song <> | Date | Tue, 13 Sep 2022 13:52:52 +0800 |
| |
On 9/9/22 22:36, ChristophHellwig wrote: > Can you try this patch (it'll need to be split up into a few if it > works):
Hi Christoph,
Thanks for your help, With this patch, the race condition issue I met can be fixed, but there will be a deadlock issue as below when boot the board:
[ 10.277872] ====================================================== [ 10.282765] WARNING: possible circular locking dependency detected [ 10.287661] 5.18.0-yocto-standard+ #4 Not tainted [ 10.291078] ------------------------------------------------------ [ 10.295967] systemd-udevd/173 is trying to acquire lock: [ 10.299994] ffff800009999150 (mtd_table_mutex){+.+.}-{3:3}, at: blktrans_open+0x60/0x128 [ 10.306854] but task is already holding lock: [ 10.310097] ffff000180832718 (&disk->open_mutex){+.+.}-{3:3}, at: blkdev_get_by_dev+0xf8/0x2f8 [ 10.317464] which lock already depends on the new lock.
[ 10.321749] the existing dependency chain (in reverse order) is: [ 10.326659] -> #1 (&disk->open_mutex){+.+.}-{3:3}: [ 10.330373] __mutex_lock+0x90/0x8f8 [ 10.333202] mutex_lock_nested+0x54/0x70 [ 10.336365] bd_register_pending_holders+0x30/0x120 [ 10.340489] device_add_disk+0x1f4/0x370 [ 10.343669] add_mtd_blktrans_dev+0x304/0x468 [ 10.343691] mtdblock_add_mtd+0x7c/0xd0 [ 10.343702] blktrans_notify_add+0x50/0x70 [ 10.343713] add_mtd_device+0x40c/0x460 [ 10.343724] add_mtd_partitions+0xa0/0x1a8 [ 10.343735] parse_mtd_partitions+0x1b4/0x400 [ 10.343746] mtd_device_parse_register+0x94/0x2a0 [ 10.343757] spi_nor_probe+0x220/0x2e8 [ 10.343767] spi_mem_probe+0x74/0xb0 [ 10.343777] spi_probe+0x88/0xe0 [ 10.343787] really_probe+0xb0/0x268 [ 10.343801] __driver_probe_device+0x7c/0xe0 [ 10.343811] driver_probe_device+0x50/0x100 [ 10.343823] __device_attach_driver+0x98/0xd0 [ 10.343833] bus_for_each_drv+0x74/0xd8 [ 10.343844] __device_attach+0xf0/0x150 [ 10.343854] device_initial_probe+0x24/0x30 [ 10.343865] bus_probe_device+0xa4/0xb0 [ 10.343875] device_add+0x424/0x8b8 [ 10.343884] __spi_add_device+0x7c/0x120 [ 10.343898] spi_add_device+0x4c/0x80 [ 10.343909] of_register_spi_device+0x228/0x380 [ 10.343920] spi_register_controller+0x3c4/0x718 [ 10.343931] devm_spi_register_controller+0x28/0x80 [ 10.343943] cqspi_probe+0x714/0x9f8 [ 10.343952] platform_probe+0x6c/0xd8 [ 10.343962] really_probe+0xb0/0x268 [ 10.343972] __driver_probe_device+0x7c/0xe0 [ 10.343982] driver_probe_device+0x50/0x100 [ 10.343994] __driver_attach+0xa4/0x100 [ 10.344004] bus_for_each_dev+0x84/0xd8 [ 10.344014] driver_attach+0x30/0x40 [ 10.344024] bus_add_driver+0x160/0x208 [ 10.344034] driver_register+0x64/0x110 [ 10.344045] __platform_driver_register+0x34/0x40 [ 10.344054] cqspi_platform_driver_init+0x20/0x28 [ 10.344068] do_one_initcall+0xa4/0x440 [ 10.344080] kernel_init_freeable+0x320/0x378 [ 10.344092] kernel_init+0x2c/0x138 [ 10.344104] ret_from_fork+0x10/0x20 [ 10.344115] -> #0 (mtd_table_mutex){+.+.}-{3:3}: [ 10.344137] __lock_acquire+0x1118/0x15d8 [ 10.344148] lock_acquire+0x2b8/0x3f8 [ 10.344156] __mutex_lock+0x90/0x8f8 [ 10.344166] mutex_lock_nested+0x54/0x70 [ 10.344177] blktrans_open+0x60/0x128 [ 10.344189] blkdev_get_whole+0x3c/0xb8 [ 10.344203] blkdev_get_by_dev+0x1ac/0x2f8 [ 10.344211] blkdev_open+0x64/0xb8 [ 10.344219] do_dentry_open+0x1b0/0x3b8 [ 10.344234] vfs_open+0x38/0x48 [ 10.344243] path_openat+0x758/0x938 [ 10.344255] do_filp_open+0x94/0x118 [ 10.344267] do_sys_openat2+0x234/0x308 [ 10.344278] do_sys_open+0x84/0xc0 [ 10.344286] __arm64_sys_openat+0x2c/0x38 [ 10.344295] invoke_syscall+0x64/0x138 [ 10.344309] el0_svc_common.constprop.4+0xf8/0x118 [ 10.344320] do_el0_svc+0x80/0xa0 [ 10.344330] el0_svc+0x68/0x1c8 [ 10.344339] el0t_64_sync_handler+0x88/0xb0 [ 10.344349] el0t_64_sync+0x15c/0x160 [ 10.344358] other info that might help us debug this:
[ 10.344362] Possible unsafe locking scenario:
[ 10.344366] CPU0 CPU1 [ 10.344370] ---- ---- [ 10.344373] lock(&disk->open_mutex); [ 10.344384] lock(mtd_table_mutex); [ 10.344395] lock(&disk->open_mutex); [ 10.344406] lock(mtd_table_mutex); [ 10.344416] *** DEADLOCK ***
[ 10.344420] 1 lock held by systemd-udevd/173: [ 10.344427] #0: ffff000180832718 (&disk->open_mutex){+.+.}-{3:3}, at: blkdev_get_by_dev+0xf8/0x2f8 [ 10.344457] stack backtrace: [ 10.344465] CPU: 3 PID: 173 Comm: systemd-udevd Not tainted 5.18.0-yocto-standard+ #4 [ 10.344476] Hardware name: SoCFPGA Stratix 10 SoCDK (DT) [ 10.344483] Call trace: [ 10.344488] dump_backtrace.part.6+0xf4/0x100 [ 10.344498] show_stack+0x2c/0x48 [ 10.344508] dump_stack_lvl+0x9c/0xcc [ 10.344522] dump_stack+0x14/0x2c [ 10.344532] print_circular_bug.isra.49+0x1a8/0x200 [ 10.344546] check_noncircular+0x124/0x138 [ 10.344557] __lock_acquire+0x1118/0x15d8 [ 10.344566] lock_acquire+0x2b8/0x3f8 [ 10.344574] __mutex_lock+0x90/0x8f8 [ 10.344585] mutex_lock_nested+0x54/0x70 [ 10.344595] blktrans_open+0x60/0x128 [ 10.344607] blkdev_get_whole+0x3c/0xb8 [ 10.344618] blkdev_get_by_dev+0x1ac/0x2f8 [ 10.344627] blkdev_open+0x64/0xb8 [ 10.344636] do_dentry_open+0x1b0/0x3b8 [ 10.344647] vfs_open+0x38/0x48 [ 10.344656] path_openat+0x758/0x938 [ 10.344666] do_filp_open+0x94/0x118 [ 10.344676] do_sys_openat2+0x234/0x308 [ 10.344687] do_sys_open+0x84/0xc0 [ 10.344695] __arm64_sys_openat+0x2c/0x38 [ 10.344704] invoke_syscall+0x64/0x138 [ 10.344715] el0_svc_common.constprop.4+0xf8/0x118 [ 10.344726] do_el0_svc+0x80/0xa0 [ 10.344736] el0_svc+0x68/0x1c8 [ 10.344745] el0t_64_sync_handler+0x88/0xb0 [ 10.344754] el0t_64_sync+0x15c/0x160
Thanks, Liwei.
> > diff --git a/drivers/mtd/mtd_blkdevs.c b/drivers/mtd/mtd_blkdevs.c > index 60b222799871e..9eda1dd98a406 100644 > --- a/drivers/mtd/mtd_blkdevs.c > +++ b/drivers/mtd/mtd_blkdevs.c > @@ -24,24 +24,16 @@ > > static LIST_HEAD(blktrans_majors); > > -static void blktrans_dev_release(struct kref *kref) > +static void blktrans_free_disk(struct gendisk *disk) > { > - struct mtd_blktrans_dev *dev = > - container_of(kref, struct mtd_blktrans_dev, ref); > + struct mtd_blktrans_dev *dev = disk->private_data; > > - put_disk(dev->disk); > blk_mq_free_tag_set(dev->tag_set); > kfree(dev->tag_set); > list_del(&dev->list); > kfree(dev); > } > > -static void blktrans_dev_put(struct mtd_blktrans_dev *dev) > -{ > - kref_put(&dev->ref, blktrans_dev_release); > -} > - > - > static blk_status_t do_blktrans_request(struct mtd_blktrans_ops *tr, > struct mtd_blktrans_dev *dev, > struct request *req) > @@ -187,63 +179,58 @@ static int blktrans_open(struct block_device *bdev, fmode_t mode) > struct mtd_blktrans_dev *dev = bdev->bd_disk->private_data; > int ret = 0; > > - kref_get(&dev->ref); > + if (disk_openers(bdev->bd_disk) > 0) > + return 0; > > - mutex_lock(&dev->lock); > - > - if (dev->open) > - goto unlock; > + mutex_lock(&mtd_table_mutex); > + if (!dev->mtd) { > + mutex_lock(&mtd_table_mutex); > + return -EINVAL; > + } > + ret = __get_mtd_device(dev->mtd); > + mutex_unlock(&mtd_table_mutex); > + if (ret) > + return ret; > > + mutex_lock(&dev->lock); > __module_get(dev->tr->owner); > - > - if (!dev->mtd) > - goto unlock; > - > if (dev->tr->open) { > ret = dev->tr->open(dev); > if (ret) > goto error_put; > } > - > - ret = __get_mtd_device(dev->mtd); > - if (ret) > - goto error_release; > dev->file_mode = mode; > - > -unlock: > dev->open++; > mutex_unlock(&dev->lock); > - return ret; > > -error_release: > - if (dev->tr->release) > - dev->tr->release(dev); > + return 0; > + > error_put: > module_put(dev->tr->owner); > mutex_unlock(&dev->lock); > - blktrans_dev_put(dev); > + > + put_mtd_device(dev->mtd); > return ret; > } > > static void blktrans_release(struct gendisk *disk, fmode_t mode) > { > struct mtd_blktrans_dev *dev = disk->private_data; > + struct mtd_info *mtd = NULL; > > - mutex_lock(&dev->lock); > - > - if (--dev->open) > - goto unlock; > + if (disk_openers(disk) > 0) > + return; > > + mutex_lock(&dev->lock); > + dev->open--; > module_put(dev->tr->owner); > - > - if (dev->mtd) { > - if (dev->tr->release) > - dev->tr->release(dev); > - __put_mtd_device(dev->mtd); > - } > -unlock: > + mtd = dev->mtd; > + if (mtd && dev->tr->release) > + dev->tr->release(dev); > mutex_unlock(&dev->lock); > - blktrans_dev_put(dev); > + > + if (mtd) > + put_mtd_device(dev->mtd); > } > > static int blktrans_getgeo(struct block_device *bdev, struct hd_geometry *geo) > @@ -266,6 +253,7 @@ static const struct block_device_operations mtd_block_ops = { > .owner = THIS_MODULE, > .open = blktrans_open, > .release = blktrans_release, > + .free_disk = blktrans_free_disk, > .getgeo = blktrans_getgeo, > }; > > @@ -318,7 +306,6 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new) > added: > > mutex_init(&new->lock); > - kref_init(&new->ref); > if (!tr->writesect) > new->readonly = 1; > > @@ -410,6 +397,7 @@ int add_mtd_blktrans_dev(struct mtd_blktrans_dev *new) > > int del_mtd_blktrans_dev(struct mtd_blktrans_dev *old) > { > + struct mtd_info *old_mtd = NULL; > unsigned long flags; > > lockdep_assert_held(&mtd_table_mutex); > @@ -438,13 +426,14 @@ int del_mtd_blktrans_dev(struct mtd_blktrans_dev *old) > if (old->open) { > if (old->tr->release) > old->tr->release(old); > - __put_mtd_device(old->mtd); > + old_mtd = old->mtd; > } > - > old->mtd = NULL; > - > mutex_unlock(&old->lock); > - blktrans_dev_put(old); > + put_disk(old->disk); > + > + if (old->mtd) > + put_mtd_device(old_mtd); > return 0; > } > > diff --git a/include/linux/mtd/blktrans.h b/include/linux/mtd/blktrans.h > index 15cc9b95e32b5..41a81fc9f0462 100644 > --- a/include/linux/mtd/blktrans.h > +++ b/include/linux/mtd/blktrans.h > @@ -7,7 +7,6 @@ > #define __MTD_TRANS_H__ > > #include <linux/mutex.h> > -#include <linux/kref.h> > #include <linux/sysfs.h> > > struct hd_geometry; > @@ -26,7 +25,6 @@ struct mtd_blktrans_dev { > unsigned long size; > int readonly; > int open; > - struct kref ref; > struct gendisk *disk; > struct attribute_group *disk_attributes; > struct request_queue *rq; >
| |