* [PATCH] UBI: block: Fix locking for idr_alloc/idr_remove
@ 2018-01-18 13:55 Bradley Bolen
2018-01-18 15:13 ` Boris Brezillon
0 siblings, 1 reply; 6+ messages in thread
From: Bradley Bolen @ 2018-01-18 13:55 UTC (permalink / raw)
To: linux-mtd
Cc: dedekind1, richard, dwmw2, computersforpeace, boris.brezillon,
marek.vasut, cyrille.pitchen, ezequiel, Bradley Bolen, stable
From: Bradley Bolen <bradleybolen@gmail.com>
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")
Cc: stable@vger.kernel.org
Signed-off-by: Bradley Bolen <bradleybolen@gmail.com>
---
drivers/mtd/ubi/block.c | 42 ++++++++++++++++++++++++++----------------
1 file changed, 26 insertions(+), 16 deletions(-)
diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
index b210fdb..b1fc28f 100644
--- 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;
@@ -351,8 +353,6 @@ static int ubiblock_init_request(struct blk_mq_tag_set *set,
.init_request = ubiblock_init_request,
};
-static DEFINE_IDR(ubiblock_minor_idr);
-
int ubiblock_create(struct ubi_volume_info *vi)
{
struct ubiblock *dev;
@@ -365,14 +365,15 @@ int ubiblock_create(struct ubi_volume_info *vi)
/* 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);
@@ -437,14 +438,13 @@ int ubiblock_create(struct ubi_volume_info *vi)
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:
@@ -457,6 +457,8 @@ int ubiblock_create(struct ubi_volume_info *vi)
put_disk(dev->gd);
out_free_dev:
kfree(dev);
+out_unlock:
+ mutex_unlock(&devices_mutex);
return ret;
}
@@ -478,30 +480,36 @@ static void ubiblock_cleanup(struct ubiblock *dev)
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)
@@ -630,6 +638,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);
@@ -638,6 +647,7 @@ static void ubiblock_remove_all(void)
ubiblock_cleanup(dev);
kfree(dev);
}
+ mutex_unlock(&devices_mutex);
}
int __init ubiblock_init(void)
--
1.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] UBI: block: Fix locking for idr_alloc/idr_remove
2018-01-18 13:55 [PATCH] UBI: block: Fix locking for idr_alloc/idr_remove Bradley Bolen
@ 2018-01-18 15:13 ` Boris Brezillon
2018-01-18 15:49 ` Richard Weinberger
0 siblings, 1 reply; 6+ messages in thread
From: Boris Brezillon @ 2018-01-18 15:13 UTC (permalink / raw)
To: Bradley Bolen
Cc: linux-mtd, dedekind1, richard, dwmw2, computersforpeace,
marek.vasut, cyrille.pitchen, ezequiel, stable
On Thu, 18 Jan 2018 08:55:20 -0500
Bradley Bolen <bradleybolen@gmail.com> wrote:
> From: Bradley Bolen <bradleybolen@gmail.com>
>
> 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")
> Cc: stable@vger.kernel.org
> Signed-off-by: Bradley Bolen <bradleybolen@gmail.com>
Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
> drivers/mtd/ubi/block.c | 42 ++++++++++++++++++++++++++----------------
> 1 file changed, 26 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
> index b210fdb..b1fc28f 100644
> --- 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;
>
> @@ -351,8 +353,6 @@ static int ubiblock_init_request(struct blk_mq_tag_set *set,
> .init_request = ubiblock_init_request,
> };
>
> -static DEFINE_IDR(ubiblock_minor_idr);
> -
> int ubiblock_create(struct ubi_volume_info *vi)
> {
> struct ubiblock *dev;
> @@ -365,14 +365,15 @@ int ubiblock_create(struct ubi_volume_info *vi)
> /* 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);
>
> @@ -437,14 +438,13 @@ int ubiblock_create(struct ubi_volume_info *vi)
> 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:
> @@ -457,6 +457,8 @@ int ubiblock_create(struct ubi_volume_info *vi)
> put_disk(dev->gd);
> out_free_dev:
> kfree(dev);
> +out_unlock:
> + mutex_unlock(&devices_mutex);
>
> return ret;
> }
> @@ -478,30 +480,36 @@ static void ubiblock_cleanup(struct ubiblock *dev)
> 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)
> @@ -630,6 +638,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);
> @@ -638,6 +647,7 @@ static void ubiblock_remove_all(void)
> ubiblock_cleanup(dev);
> kfree(dev);
> }
> + mutex_unlock(&devices_mutex);
> }
>
> int __init ubiblock_init(void)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] UBI: block: Fix locking for idr_alloc/idr_remove
2018-01-18 15:13 ` Boris Brezillon
@ 2018-01-18 15:49 ` Richard Weinberger
0 siblings, 0 replies; 6+ messages in thread
From: Richard Weinberger @ 2018-01-18 15:49 UTC (permalink / raw)
To: Boris Brezillon
Cc: Bradley Bolen, linux-mtd, dedekind1, dwmw2, computersforpeace,
marek.vasut, cyrille.pitchen, ezequiel, stable
Boris, Bradley,
Am Donnerstag, 18. Januar 2018, 16:13:10 CET schrieb Boris Brezillon:
> On Thu, 18 Jan 2018 08:55:20 -0500
>
> Bradley Bolen <bradleybolen@gmail.com> wrote:
> > From: Bradley Bolen <bradleybolen@gmail.com>
> >
> > 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")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Bradley Bolen <bradleybolen@gmail.com>
>
> Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Thanks!
Thanks,
//richard
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] UBI: block: Fix locking for idr_alloc/idr_remove
2018-01-18 13:55 ` Greg KH
@ 2018-01-18 14:20 ` Brad Bolen
0 siblings, 0 replies; 6+ messages in thread
From: Brad Bolen @ 2018-01-18 14:20 UTC (permalink / raw)
To: Greg KH; +Cc: stable
My apologies. I was testing sending to myself and forgot about the
stable tag in the commit message.
On Thu, Jan 18, 2018 at 8:55 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Thu, Jan 18, 2018 at 08:47:41AM -0500, Bradley Bolen wrote:
>> From: Bradley Bolen <bradleybolen@gmail.com>
>>
>> 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")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Bradley Bolen <bradleybolen@gmail.com>
>> ---
>> drivers/mtd/ubi/block.c | 42 ++++++++++++++++++++++++++----------------
>> 1 file changed, 26 insertions(+), 16 deletions(-)
>
> <formletter>
>
> This is not the correct way to submit patches for inclusion in the
> stable kernel tree. Please read:
> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> for how to do this properly.
>
> </formletter>
>
> It's also not how you submit patches upstream, you have read
> Documentation/SubmittingPatches, right?
>
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] UBI: block: Fix locking for idr_alloc/idr_remove
2018-01-18 13:47 Bradley Bolen
@ 2018-01-18 13:55 ` Greg KH
2018-01-18 14:20 ` Brad Bolen
0 siblings, 1 reply; 6+ messages in thread
From: Greg KH @ 2018-01-18 13:55 UTC (permalink / raw)
To: Bradley Bolen; +Cc: stable
On Thu, Jan 18, 2018 at 08:47:41AM -0500, Bradley Bolen wrote:
> From: Bradley Bolen <bradleybolen@gmail.com>
>
> 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")
> Cc: stable@vger.kernel.org
> Signed-off-by: Bradley Bolen <bradleybolen@gmail.com>
> ---
> drivers/mtd/ubi/block.c | 42 ++++++++++++++++++++++++++----------------
> 1 file changed, 26 insertions(+), 16 deletions(-)
<formletter>
This is not the correct way to submit patches for inclusion in the
stable kernel tree. Please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.
</formletter>
It's also not how you submit patches upstream, you have read
Documentation/SubmittingPatches, right?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] UBI: block: Fix locking for idr_alloc/idr_remove
@ 2018-01-18 13:47 Bradley Bolen
2018-01-18 13:55 ` Greg KH
0 siblings, 1 reply; 6+ messages in thread
From: Bradley Bolen @ 2018-01-18 13:47 UTC (permalink / raw)
To: bradleybolen; +Cc: stable
From: Bradley Bolen <bradleybolen@gmail.com>
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")
Cc: stable@vger.kernel.org
Signed-off-by: Bradley Bolen <bradleybolen@gmail.com>
---
drivers/mtd/ubi/block.c | 42 ++++++++++++++++++++++++++----------------
1 file changed, 26 insertions(+), 16 deletions(-)
diff --git a/drivers/mtd/ubi/block.c b/drivers/mtd/ubi/block.c
index b210fdb..b1fc28f 100644
--- 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;
@@ -351,8 +353,6 @@ static int ubiblock_init_request(struct blk_mq_tag_set *set,
.init_request = ubiblock_init_request,
};
-static DEFINE_IDR(ubiblock_minor_idr);
-
int ubiblock_create(struct ubi_volume_info *vi)
{
struct ubiblock *dev;
@@ -365,14 +365,15 @@ int ubiblock_create(struct ubi_volume_info *vi)
/* 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);
@@ -437,14 +438,13 @@ int ubiblock_create(struct ubi_volume_info *vi)
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:
@@ -457,6 +457,8 @@ int ubiblock_create(struct ubi_volume_info *vi)
put_disk(dev->gd);
out_free_dev:
kfree(dev);
+out_unlock:
+ mutex_unlock(&devices_mutex);
return ret;
}
@@ -478,30 +480,36 @@ static void ubiblock_cleanup(struct ubiblock *dev)
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)
@@ -630,6 +638,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);
@@ -638,6 +647,7 @@ static void ubiblock_remove_all(void)
ubiblock_cleanup(dev);
kfree(dev);
}
+ mutex_unlock(&devices_mutex);
}
int __init ubiblock_init(void)
--
1.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-01-18 15:57 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-18 13:55 [PATCH] UBI: block: Fix locking for idr_alloc/idr_remove Bradley Bolen
2018-01-18 15:13 ` Boris Brezillon
2018-01-18 15:49 ` Richard Weinberger
-- strict thread matches above, loose matches on Subject: below --
2018-01-18 13:47 Bradley Bolen
2018-01-18 13:55 ` Greg KH
2018-01-18 14:20 ` Brad Bolen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.