4.9-stable review patch. If anyone has any objections, please let me know.
------------------
From: Bradley Bolen bradleybolen@gmail.com
commit 7f29ae9f977bcdc3654e68bc36d170223c52fd48 upstream.
This fixes a race with idr_alloc where gd->first_minor can be set to the same value for two simultaneous calls to ubiblock_create. Each instance calls device_add_disk with the same first_minor. device_add_disk calls bdi_register_owner which generates several warnings.
WARNING: CPU: 1 PID: 179 at kernel-source/fs/sysfs/dir.c:31 sysfs_warn_dup+0x68/0x88 sysfs: cannot create duplicate filename '/devices/virtual/bdi/252:2'
WARNING: CPU: 1 PID: 179 at kernel-source/lib/kobject.c:240 kobject_add_internal+0x1ec/0x2f8 kobject_add_internal failed for 252:2 with -EEXIST, don't try to register things with the same name in the same directory
WARNING: CPU: 1 PID: 179 at kernel-source/fs/sysfs/dir.c:31 sysfs_warn_dup+0x68/0x88 sysfs: cannot create duplicate filename '/dev/block/252:2'
However, device_add_disk does not error out when bdi_register_owner returns an error. Control continues until reaching blk_register_queue. It then BUGs.
kernel BUG at kernel-source/fs/sysfs/group.c:113! [<c01e26cc>] (internal_create_group) from [<c01e2950>] (sysfs_create_group+0x20/0x24) [<c01e2950>] (sysfs_create_group) from [<c00e3d38>] (blk_trace_init_sysfs+0x18/0x20) [<c00e3d38>] (blk_trace_init_sysfs) from [<c02bdfbc>] (blk_register_queue+0xd8/0x154) [<c02bdfbc>] (blk_register_queue) from [<c02cec84>] (device_add_disk+0x194/0x44c) [<c02cec84>] (device_add_disk) from [<c0436ec8>] (ubiblock_create+0x284/0x2e0) [<c0436ec8>] (ubiblock_create) from [<c0427bb8>] (vol_cdev_ioctl+0x450/0x554) [<c0427bb8>] (vol_cdev_ioctl) from [<c0189110>] (vfs_ioctl+0x30/0x44) [<c0189110>] (vfs_ioctl) from [<c01892e0>] (do_vfs_ioctl+0xa0/0x790) [<c01892e0>] (do_vfs_ioctl) from [<c0189a14>] (SyS_ioctl+0x44/0x68) [<c0189a14>] (SyS_ioctl) from [<c0010640>] (ret_fast_syscall+0x0/0x34)
Locking idr_alloc/idr_remove removes the race and keeps gd->first_minor unique.
Fixes: 2bf50d42f3a4 ("UBI: block: Dynamically allocate minor numbers") Signed-off-by: Bradley Bolen bradleybolen@gmail.com Reviewed-by: Boris Brezillon boris.brezillon@free-electrons.com Signed-off-by: Richard Weinberger richard@nod.at Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
--- drivers/mtd/ubi/block.c | 42 ++++++++++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 16 deletions(-)
--- a/drivers/mtd/ubi/block.c +++ b/drivers/mtd/ubi/block.c @@ -99,6 +99,8 @@ struct ubiblock {
/* Linked list of all ubiblock instances */ static LIST_HEAD(ubiblock_devices); +static DEFINE_IDR(ubiblock_minor_idr); +/* Protects ubiblock_devices and ubiblock_minor_idr */ static DEFINE_MUTEX(devices_mutex); static int ubiblock_major;
@@ -353,8 +355,6 @@ static struct blk_mq_ops ubiblock_mq_ops .init_request = ubiblock_init_request, };
-static DEFINE_IDR(ubiblock_minor_idr); - int ubiblock_create(struct ubi_volume_info *vi) { struct ubiblock *dev; @@ -367,14 +367,15 @@ int ubiblock_create(struct ubi_volume_in /* Check that the volume isn't already handled */ mutex_lock(&devices_mutex); if (find_dev_nolock(vi->ubi_num, vi->vol_id)) { - mutex_unlock(&devices_mutex); - return -EEXIST; + ret = -EEXIST; + goto out_unlock; } - mutex_unlock(&devices_mutex);
dev = kzalloc(sizeof(struct ubiblock), GFP_KERNEL); - if (!dev) - return -ENOMEM; + if (!dev) { + ret = -ENOMEM; + goto out_unlock; + }
mutex_init(&dev->dev_mutex);
@@ -439,14 +440,13 @@ int ubiblock_create(struct ubi_volume_in goto out_free_queue; }
- mutex_lock(&devices_mutex); list_add_tail(&dev->list, &ubiblock_devices); - mutex_unlock(&devices_mutex);
/* Must be the last step: anyone can call file ops from now on */ add_disk(dev->gd); dev_info(disk_to_dev(dev->gd), "created from ubi%d:%d(%s)", dev->ubi_num, dev->vol_id, vi->name); + mutex_unlock(&devices_mutex); return 0;
out_free_queue: @@ -459,6 +459,8 @@ out_put_disk: put_disk(dev->gd); out_free_dev: kfree(dev); +out_unlock: + mutex_unlock(&devices_mutex);
return ret; } @@ -480,30 +482,36 @@ static void ubiblock_cleanup(struct ubib int ubiblock_remove(struct ubi_volume_info *vi) { struct ubiblock *dev; + int ret;
mutex_lock(&devices_mutex); dev = find_dev_nolock(vi->ubi_num, vi->vol_id); if (!dev) { - mutex_unlock(&devices_mutex); - return -ENODEV; + ret = -ENODEV; + goto out_unlock; }
/* Found a device, let's lock it so we can check if it's busy */ mutex_lock(&dev->dev_mutex); if (dev->refcnt > 0) { - mutex_unlock(&dev->dev_mutex); - mutex_unlock(&devices_mutex); - return -EBUSY; + ret = -EBUSY; + goto out_unlock_dev; }
/* Remove from device list */ list_del(&dev->list); - mutex_unlock(&devices_mutex); - ubiblock_cleanup(dev); mutex_unlock(&dev->dev_mutex); + mutex_unlock(&devices_mutex); + kfree(dev); return 0; + +out_unlock_dev: + mutex_unlock(&dev->dev_mutex); +out_unlock: + mutex_unlock(&devices_mutex); + return ret; }
static int ubiblock_resize(struct ubi_volume_info *vi) @@ -632,6 +640,7 @@ static void ubiblock_remove_all(void) struct ubiblock *next; struct ubiblock *dev;
+ mutex_lock(&devices_mutex); list_for_each_entry_safe(dev, next, &ubiblock_devices, list) { /* The module is being forcefully removed */ WARN_ON(dev->desc); @@ -640,6 +649,7 @@ static void ubiblock_remove_all(void) ubiblock_cleanup(dev); kfree(dev); } + mutex_unlock(&devices_mutex); }
int __init ubiblock_init(void)