# fix a lock order reversal in md\_alloc - List: linux-raid - 11 messages, 5 authors - Page: https://ratatoskr.run/linux-raid/2021/09/6640639/t ## fix a lock order reversal in md\_alloc - From: Christoph Hellwig \ - Date: 2021-09-01 11:39:11 - Message: https://ratatoskr.run/linux-raid/2021/09/6640639.md ```text Hi Song, the first patch in this series fixed the reported lock order reversal in md_alloc. The rest sort out the error handling in that function, starting with the patch from Luis to handle add_disk errors, which would otherwise conflict with the first patch. Note that I have had a hard time verifying this all works fine as the testsuite in mdadm already keeps failing a lot for me with the baseline kernel. Some of thos reproducibly and others randomly. Is there a good document somehow describing what to expect from the mdadm test suite? Diffstat: drivers/md/md.c | 56 +++++++++++++++++++++++++++++--------------------------- 1 file changed, 29 insertions(+), 27 deletions(-) ``` ## \[PATCH 1/5\] md: fix a lock order reversal in md\_alloc - From: Christoph Hellwig \ - Date: 2021-09-01 11:40:11 - Message: https://ratatoskr.run/linux-raid/2021/09/6640640.md ```text Commit b0140891a8cea3 ("md: Fix race when creating a new md device.") not only moved assigning mddev->gendisk before calling add_disk, which fixes the races described in the commit log, but also added a mddev->open_mutex critical section over add_disk and creation of the md kobj. Adding a kobject after add_disk is racy vs deleting the gendisk right after adding it, but md already prevents against that by holding a mddev->active reference. On the other hand taking this lock added a lock order reversal with what is not disk->open_mutex (used to be bdev->bd_mutex when the commit was added) for partition devices, which need that lock for the internal open for the partition scan, and a recent commit also takes it for non-partitioned devices, leading to further lockdep splatter. Fixes: b0140891a8ce ("md: Fix race when creating a new md device.") Fixes: d62633873590 ("block: support delayed holder registration") Reported-by: Signed-off-by: Christoph Hellwig Tested-by: --- drivers/md/md.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/drivers/md/md.c b/drivers/md/md.c index ae8fe54ea3581..6c0c3d0d905aa 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -5700,10 +5700,6 @@ static int md_alloc(dev_t dev, char *name) disk->flags |= GENHD_FL_EXT_DEVT; disk->events |= DISK_EVENT_MEDIA_CHANGE; mddev->gendisk = disk; - /* As soon as we call add_disk(), another thread could get - * through to md_open, so make sure it doesn't get too far - */ - mutex_lock(&mddev->open_mutex); add_disk(disk); error = kobject_add(&mddev->kobj, &disk_to_dev(disk)->kobj, "%s", "md"); @@ -5718,7 +5714,6 @@ static int md_alloc(dev_t dev, char *name) if (mddev->kobj.sd && sysfs_create_group(&mddev->kobj, &md_bitmap_group)) pr_debug("pointless warning\n"); - mutex_unlock(&mddev->open_mutex); abort: mutex_unlock(&disks_mutex); if (!error && mddev->kobj.sd) { -- 2.30.2 ``` ## \[PATCH 2/5\] md: add error handling support for add\_disk() - From: Christoph Hellwig \ - Date: 2021-09-01 11:41:15 - Message: https://ratatoskr.run/linux-raid/2021/09/6640641.md ```text From: Luis Chamberlain We never checked for errors on add_disk() as this function returned void. Now that this is fixed, use the shiny new error handling. We just do the unwinding of what was not done before, and are sure to unlock prior to bailing. Signed-off-by: Luis Chamberlain Signed-off-by: Christoph Hellwig --- drivers/md/md.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/md/md.c b/drivers/md/md.c index 6c0c3d0d905aa..6bd5ad3c30b41 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -5700,7 +5700,11 @@ static int md_alloc(dev_t dev, char *name) disk->flags |= GENHD_FL_EXT_DEVT; disk->events |= DISK_EVENT_MEDIA_CHANGE; mddev->gendisk = disk; - add_disk(disk); + error = add_disk(disk); + if (error) { + blk_cleanup_disk(disk); + goto abort; + } error = kobject_add(&mddev->kobj, &disk_to_dev(disk)->kobj, "%s", "md"); if (error) { -- 2.30.2 ``` ## \[PATCH 3/5\] md: add the bitmap group to the default groups for the md kobject - From: Christoph Hellwig \ - Date: 2021-09-01 11:42:12 - Message: https://ratatoskr.run/linux-raid/2021/09/6640642.md ```text Replace the deprecated default_attrs with the default_groups mechanism, and add the always visible bitmap group to the groups created add kobject_add time. Signed-off-by: Christoph Hellwig --- drivers/md/md.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/drivers/md/md.c b/drivers/md/md.c index 6bd5ad3c30b41..2971dae388ef2 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -5490,6 +5490,10 @@ static struct attribute *md_default_attrs[] = { NULL, }; +static const struct attribute_group md_default_group = { + .attrs = md_default_attrs, +}; + static struct attribute *md_redundancy_attrs[] = { &md_scan_mode.attr, &md_last_scan_mode.attr, @@ -5512,6 +5516,12 @@ static const struct attribute_group md_redundancy_group = { .attrs = md_redundancy_attrs, }; +static const struct attribute_group *md_attr_groups[] = { + &md_default_group, + &md_bitmap_group, + NULL, +}; + static ssize_t md_attr_show(struct kobject *kobj, struct attribute *attr, char *page) { @@ -5587,7 +5597,7 @@ static const struct sysfs_ops md_sysfs_ops = { static struct kobj_type md_ktype = { .release = md_free, .sysfs_ops = &md_sysfs_ops, - .default_attrs = md_default_attrs, + .default_groups = md_attr_groups, }; int mdp_major = 0; @@ -5596,7 +5606,6 @@ static void mddev_delayed_delete(struct work_struct *ws) { struct mddev *mddev = container_of(ws, struct mddev, del_work); - sysfs_remove_group(&mddev->kobj, &md_bitmap_group); kobject_del(&mddev->kobj); kobject_put(&mddev->kobj); } @@ -5715,9 +5724,6 @@ static int md_alloc(dev_t dev, char *name) disk->disk_name); error = 0; } - if (mddev->kobj.sd && - sysfs_create_group(&mddev->kobj, &md_bitmap_group)) - pr_debug("pointless warning\n"); abort: mutex_unlock(&disks_mutex); if (!error && mddev->kobj.sd) { -- 2.30.2 ``` ## \[PATCH 4/5\] md: extend disks\_mutex coverage - From: Christoph Hellwig \ - Date: 2021-09-01 11:43:29 - Message: https://ratatoskr.run/linux-raid/2021/09/6640643.md ```text disks_mutex is intended to serialize md_alloc. Extended it to also cover the kobject_uevent call and getting the sysfs dirent to help reducing error handling complexity. Signed-off-by: Christoph Hellwig --- drivers/md/md.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/md/md.c b/drivers/md/md.c index 2971dae388ef2..b90dbf7cc2455 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -5725,12 +5725,12 @@ static int md_alloc(dev_t dev, char *name) error = 0; } abort: - mutex_unlock(&disks_mutex); if (!error && mddev->kobj.sd) { kobject_uevent(&mddev->kobj, KOBJ_ADD); mddev->sysfs_state = sysfs_get_dirent_safe(mddev->kobj.sd, "array_state"); mddev->sysfs_level = sysfs_get_dirent_safe(mddev->kobj.sd, "level"); } + mutex_unlock(&disks_mutex); mddev_put(mddev); return error; } -- 2.30.2 ``` ## \[PATCH 5/5\] md: properly unwind when failing to add the kobject in md\_alloc - From: Christoph Hellwig \ - Date: 2021-09-01 11:44:28 - Message: https://ratatoskr.run/linux-raid/2021/09/6640644.md ```text Add proper error handling to delete the gendisk when failing to add the md kobject and clean up the error unwinding in general. Signed-off-by: Christoph Hellwig --- drivers/md/md.c | 37 +++++++++++++++++-------------------- 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/drivers/md/md.c b/drivers/md/md.c index b90dbf7cc2455..c322841d4edc3 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -5672,7 +5672,7 @@ static int md_alloc(dev_t dev, char *name) strcmp(mddev2->gendisk->disk_name, name) == 0) { spin_unlock(&all_mddevs_lock); error = -EEXIST; - goto abort; + goto out_unlock_disks_mutex; } spin_unlock(&all_mddevs_lock); } @@ -5685,7 +5685,7 @@ static int md_alloc(dev_t dev, char *name) error = -ENOMEM; disk = blk_alloc_disk(NUMA_NO_NODE); if (!disk) - goto abort; + goto out_unlock_disks_mutex; disk->major = MAJOR(mddev->unit); disk->first_minor = unit << shift; @@ -5710,26 +5710,23 @@ static int md_alloc(dev_t dev, char *name) disk->events |= DISK_EVENT_MEDIA_CHANGE; mddev->gendisk = disk; error = add_disk(disk); - if (error) { - blk_cleanup_disk(disk); - goto abort; - } + if (error) + goto out_cleanup_disk; error = kobject_add(&mddev->kobj, &disk_to_dev(disk)->kobj, "%s", "md"); - if (error) { - /* This isn't possible, but as kobject_init_and_add is marked - * __must_check, we must do something with the result - */ - pr_debug("md: cannot register %s/md - name in use\n", - disk->disk_name); - error = 0; - } - abort: - if (!error && mddev->kobj.sd) { - kobject_uevent(&mddev->kobj, KOBJ_ADD); - mddev->sysfs_state = sysfs_get_dirent_safe(mddev->kobj.sd, "array_state"); - mddev->sysfs_level = sysfs_get_dirent_safe(mddev->kobj.sd, "level"); - } + if (error) + goto out_del_gendisk; + + kobject_uevent(&mddev->kobj, KOBJ_ADD); + mddev->sysfs_state = sysfs_get_dirent_safe(mddev->kobj.sd, "array_state"); + mddev->sysfs_level = sysfs_get_dirent_safe(mddev->kobj.sd, "level"); + goto out_unlock_disks_mutex; + +out_del_gendisk: + del_gendisk(disk); +out_cleanup_disk: + blk_cleanup_disk(disk); +out_unlock_disks_mutex: mutex_unlock(&disks_mutex); mddev_put(mddev); return error; -- 2.30.2 ``` ## Re: fix a lock order reversal in md\_alloc - From: Song Liu \ - Date: 2021-09-02 05:06:54 - Message: https://ratatoskr.run/linux-raid/2021/09/6640647.md ```text Hi Christoph, On Wed, Sep 1, 2021 at 4:39 AM Christoph Hellwig [ref] wrote: > > Hi Song, > > the first patch in this series fixed the reported lock order reversal > in md_alloc. The rest sort out the error handling in that function, > starting with the patch from Luis to handle add_disk errors, which > would otherwise conflict with the first patch. Thanks for the fixes! I have noticed the issue, but haven't got a good solution for it. I will test and apply the set (maybe after the labor day long weekend). > > Note that I have had a hard time verifying this all works fine as the > testsuite in mdadm already keeps failing a lot for me with the baseline > kernel. Some of thos reproducibly and others randomly. Is there a > good document somehow describing what to expect from the mdadm test > suite? Unfortunately, mdadm test needs quite some work to be really useful again. I will work with Jes and other folks to come up with a plan for this. Thanks again, Song ``` ## Re: \[PATCH 1/5\] md: fix a lock order reversal in md\_alloc - From: Guoqing Jiang \ - Date: 2021-09-03 06:08:39 - Message: https://ratatoskr.run/linux-raid/2021/09/6640654.md ```text On 9/1/21 7:38 PM, Christoph Hellwig wrote: > Commit b0140891a8cea3 ("md: Fix race when creating a new md device.") > not only moved assigning mddev->gendisk before calling add_disk, which > fixes the races described in the commit log, but also added a > mddev->open_mutex critical section over add_disk and creation of the > md kobj. Adding a kobject after add_disk is racy vs deleting the gendisk > right after adding it, but md already prevents against that by holding > a mddev->active reference. Assuming you mean md_open calls mddev_find -> mddev_get  -> atomic_inc(&mddev->active), but the path had already existed before b0140891a8c,  and md_alloc also called mddev_find at that time, not sure how it prevents the race though I probably missed something. Cc Neil. > On the other hand taking this lock added a lock order reversal with what > is not disk->open_mutex (used to be bdev->bd_mutex when the commit was > added) for partition devices, which need that lock for the internal open > for the partition scan, and a recent commit also takes it for > non-partitioned devices, leading to further lockdep splatter. > > Fixes: b0140891a8ce ("md: Fix race when creating a new md device.") > Fixes: d62633873590 ("block: support delayed holder registration") IIUC, the issue appeared after d6263387359 (which was for dm issue), perhaps stable maintainer should not apply this to any stable kernel if it only includes b0140891a8ce. Thanks, Guoqing ``` ## Re: \[PATCH 1/5\] md: fix a lock order reversal in md\_alloc - From: NeilBrown \ - Date: 2021-09-03 07:48:44 - Message: https://ratatoskr.run/linux-raid/2021/09/6640655.md ```text On Fri, 03 Sep 2021, Guoqing Jiang wrote: > > On 9/1/21 7:38 PM, Christoph Hellwig wrote: > > Commit b0140891a8cea3 ("md: Fix race when creating a new md device.") > > not only moved assigning mddev->gendisk before calling add_disk, which > > fixes the races described in the commit log, but also added a > > mddev->open_mutex critical section over add_disk and creation of the > > md kobj. Adding a kobject after add_disk is racy vs deleting the gendisk > > right after adding it, but md already prevents against that by holding > > a mddev->active reference. > > Assuming you mean md_open calls mddev_find -> mddev_get >  -> atomic_inc(&mddev->active), but the path had already existed > before b0140891a8c,  and md_alloc also called mddev_find at > that time, not sure how it prevents the race though I probably missed > something. Cc Neil. It is a long time since I've looked much at this code, but I *think* that the reason calling kobject_add() after add_disk() is safe is because that gendisk is *only* deleted by md_free() which happens when that kobject is 'put' - in mddev_delayed_delete(). That only gets called when mddev->active is zero (from mddev_put(). So as Christoph says, the fact that md_alloc() hold a count on mddev->active (until it calls mddeV_put() at the very end) makes this safe. As for the ->open_mutex locking - I have a vague memory that it was to protect something that might happen *after* md_open() from running before md_alloc() had completed. add_disk() itself adds sufficient exclusion via disk->open_mutex, so it could only be the kobject/sysfs manipulations. All I can think if is that add_disk() would send an event to udev and udev might do something before the 'md' directory had been created. But the udev rules seem safe against that. In particular they check if md/array_state exists before checking if it is clear or inactive. I did struggle with some races like that, but if that was the problem, I'm surprised that I didn't explain it in the commit message. So in short: I cannot see why I added that locking, and I cannot see that removing it would break anything. Reviewed-by: NeilBrown NeilBrown > > > On the other hand taking this lock added a lock order reversal with what > > is not disk->open_mutex (used to be bdev->bd_mutex when the commit was > > added) for partition devices, which need that lock for the internal open > > for the partition scan, and a recent commit also takes it for > > non-partitioned devices, leading to further lockdep splatter. > > > > Fixes: b0140891a8ce ("md: Fix race when creating a new md device.") > > Fixes: d62633873590 ("block: support delayed holder registration") > > IIUC, the issue appeared after d6263387359 (which was for dm issue), > perhaps stable maintainer should not apply this to any stable kernel > if it only includes b0140891a8ce. > > Thanks, > Guoqing > > ``` ## Re: fix a lock order reversal in md\_alloc - From: Luis Chamberlain \ - Date: 2021-09-04 01:48:43 - Message: https://ratatoskr.run/linux-raid/2021/09/6640656.md ```text I'll drop my stand alone patch then, or can queue these in with my set if they are determined already to be fine. Let me know. Luis ``` ## Re: fix a lock order reversal in md\_alloc - From: Song Liu \ - Date: 2021-09-09 06:14:26 - Message: https://ratatoskr.run/linux-raid/2021/09/6640665.md ```text On Wed, Sep 1, 2021 at 4:39 AM Christoph Hellwig [ref] wrote: > > Hi Song, > > the first patch in this series fixed the reported lock order reversal > in md_alloc. The rest sort out the error handling in that function, > starting with the patch from Luis to handle add_disk errors, which > would otherwise conflict with the first patch. > > Note that I have had a hard time verifying this all works fine as the > testsuite in mdadm already keeps failing a lot for me with the baseline > kernel. Some of thos reproducibly and others randomly. Is there a > good document somehow describing what to expect from the mdadm test > suite? > > Diffstat: > drivers/md/md.c | 56 +++++++++++++++++++++++++++++--------------------------- > 1 file changed, 29 insertions(+), 27 deletions(-) Applied to md-fixes. Thanks! Song ```