Re: [PATCH 2/8] md: implement ->free_disk

[Christoph Hellwig <hch@lst.de>]

On Tue, Jul 12, 2022 at 05:13:48PM -0600, Logan Gunthorpe wrote:
> > +
> > +	percpu_ref_exit(&mddev->writes_pending);
> > +	bioset_exit(&mddev->bio_set);
> > +	bioset_exit(&mddev->sync_set);
> > +
> > +	kfree(mddev);
> > +}
> 
> I still don't think this is entirely correct. There are error paths that
> will put the kobject before the disk is created and if they get hit then
> the kfree(mddev) will never be called and the memory will be leaked.

True.

> Instead of creating an ugly special path for that, I came up with a solution 
> that I think  makes a bit more sense: the kobject is still freed in it's 
> own free  function, but the disk holds a reference to the kobject and drops
> it in its free function. The sysfs puts and del_gendisk are then moved
> into mddev_delayed_delete() so they happen earlier.

I'm not sure this is a good idea.  The mddev kobject hangs off the
disk, so I don't think that it should in any way control the life
time of the disk, as that just creates potential problems down the
road.

Moreover we actually need the special kfree path anyway for the
add_disk failure, something that I had missed.  Something like
the untested patch below on top of my series.  I'll look into folding
and testing it.

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 53a92b306b1fc..62f40c49344e4 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -5641,7 +5641,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 out_unlock_disks_mutex;
+				goto out_free_mddev;
 			}
 		spin_unlock(&all_mddevs_lock);
 	}
@@ -5654,7 +5654,7 @@ static int md_alloc(dev_t dev, char *name)
 	error = -ENOMEM;
 	disk = blk_alloc_disk(NUMA_NO_NODE);
 	if (!disk)
-		goto out_unlock_disks_mutex;
+		goto out_free_mddev;
 
 	disk->major = MAJOR(mddev->unit);
 	disk->first_minor = unit << shift;
@@ -5674,26 +5674,35 @@ static int md_alloc(dev_t dev, char *name)
 	disk->events |= DISK_EVENT_MEDIA_CHANGE;
 	mddev->gendisk = disk;
 	error = add_disk(disk);
-	if (error) {
-		mddev->gendisk = NULL;
-		put_disk(disk);
-		goto out_unlock_disks_mutex;
-	}
+	if (error)
+		goto out_put_disk;
 
 	error = kobject_add(&mddev->kobj, &disk_to_dev(disk)->kobj, "%s", "md");
-	if (error)
+	if (error) {
+		/*
+		 * The disk is already live at this point.  Clear the hold flag
+		 * and let mddev_put take care of the deletion, as it isn't any
+		 * different from a normal close on last release now.
+		 */
+		mddev->hold_active = 0;
 		goto out_unlock_disks_mutex;
+	}
 
 	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");
 
 out_unlock_disks_mutex:
-	if (error)
-		mddev->hold_active = 0;
 	mutex_unlock(&disks_mutex);
 	mddev_put(mddev);
 	return error;
+
+out_put_disk:
+	put_disk(disk);
+out_free_mddev:
+	mutex_unlock(&disks_mutex);
+	kfree(mddev);
+	return error;
 }
 
 static void md_probe(dev_t dev)