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

[Logan Gunthorpe <logang@xxxxxxxxxxxx>]

On 2022-07-12 01:03, Christoph Hellwig wrote:
> Ensure that all private data is only freed once all accesses are done.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
>  drivers/md/md.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 861d6a9481b2e..ae076a7a87796 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -5581,11 +5581,6 @@ static void md_free(struct kobject *ko)
>  		del_gendisk(mddev->gendisk);
>  		put_disk(mddev->gendisk);
>  	}
> -	percpu_ref_exit(&mddev->writes_pending);
> -
> -	bioset_exit(&mddev->bio_set);
> -	bioset_exit(&mddev->sync_set);
> -	kfree(mddev);
>  }
>  
>  static const struct sysfs_ops md_sysfs_ops = {
> @@ -7844,6 +7839,17 @@ static unsigned int md_check_events(struct gendisk *disk, unsigned int clearing)
>  	return ret;
>  }
>  
> +static void md_free_disk(struct gendisk *disk)
> +{
> +	struct mddev *mddev = disk->private_data;
> +
> +	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.

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.

Thoughts?

Logan

--

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 78e2588ed43e..330db78a5b38 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -5585,15 +5585,6 @@ static void md_free(struct kobject *ko)
 {
        struct mddev *mddev = container_of(ko, struct mddev, kobj);
 
-       if (mddev->sysfs_state)
-               sysfs_put(mddev->sysfs_state);
-       if (mddev->sysfs_level)
-               sysfs_put(mddev->sysfs_level);
-
-       if (mddev->gendisk) {
-               del_gendisk(mddev->gendisk);
-               blk_cleanup_disk(mddev->gendisk);
-       }
        percpu_ref_exit(&mddev->writes_pending);
 
        bioset_exit(&mddev->bio_set);
@@ -5618,6 +5609,17 @@ static void mddev_delayed_delete(struct work_struct *ws)
        struct mddev *mddev = container_of(ws, struct mddev, del_work);
 
        kobject_del(&mddev->kobj);
+
+       if (mddev->sysfs_state)
+               sysfs_put(mddev->sysfs_state);
+       if (mddev->sysfs_level)
+               sysfs_put(mddev->sysfs_level);
+
+       if (mddev->gendisk) {
+               del_gendisk(mddev->gendisk);
+               blk_cleanup_disk(mddev->gendisk);
+       }
+
        kobject_put(&mddev->kobj);
 }
 
@@ -5708,6 +5710,7 @@ int md_alloc(dev_t dev, char *name)
        else
                sprintf(disk->disk_name, "md%d", unit);
        disk->fops = &md_fops;
+       kobject_get(&mddev->kobj);
        disk->private_data = mddev;
 
        mddev->queue = disk->queue;
@@ -7858,6 +7861,13 @@ static unsigned int md_check_events(struct gendisk *disk, unsign>
        return ret;
 }
 
+static void md_free_disk(struct gendisk *disk)
+{
+       struct mddev *mddev = disk->private_data;
+
+       kobject_put(&mddev->kobj);
+}
+
 const struct block_device_operations md_fops =
 {
        .owner          = THIS_MODULE,
@@ -7871,6 +7881,7 @@ const struct block_device_operations md_fops =
        .getgeo         = md_getgeo,
        .check_events   = md_check_events,
        .set_read_only  = md_set_read_only,
+       .free_disk      = md_free_disk,
 };