fix a lock order reversal in md_alloc

fix a lock order reversal in md_alloc

[Christoph Hellwig]

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

[Christoph Hellwig]

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: syzbot+fadc0aaf497e6a493b9f@syzkaller.appspotmail.com
Signed-off-by: Christoph Hellwig <hch@lst.de>
Tested-by: syzbot+fadc0aaf497e6a493b9f@syzkaller.appspotmail.com
---
 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

Re: [PATCH 1/5] md: fix a lock order reversal in md_alloc

[Guoqing Jiang]

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

[NeilBrown]

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 <neilb@suse.de>

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
> 
> 

[PATCH 2/5] md: add error handling support for add_disk()

[Christoph Hellwig]

From: Luis Chamberlain <mcgrof@kernel.org>

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 <mcgrof@kernel.org>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 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

[Christoph Hellwig]

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 <hch@lst.de>
---
 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

[Christoph Hellwig]

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 <hch@lst.de>
---
 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

[Christoph Hellwig]

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 <hch@lst.de>
---
 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

[Song Liu]

Hi Christoph,

On Wed, Sep 1, 2021 at 4:39 AM Christoph Hellwig <hch@lst.de> 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: fix a lock order reversal in md_alloc

[Luis Chamberlain]

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

[Song Liu]

On Wed, Sep 1, 2021 at 4:39 AM Christoph Hellwig <hch@lst.de> 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