Re: [PATCH 1/9] md: fix error handling in md_alloc

From: Logan Gunthorpe <logang@deltatee.com>
To: Christoph Hellwig <hch@lst.de>, Song Liu <song@kernel.org>
Cc: linux-raid@vger.kernel.org, linux-block@vger.kernel.org
Subject: Re: [PATCH 1/9] md: fix error handling in md_alloc
Date: Wed, 13 Jul 2022 10:31:38 -0600	[thread overview]
Message-ID: <663a32ea-fb8c-2a5e-253e-29582ba99f3d@deltatee.com> (raw)
In-Reply-To: <1f9beba5-3fa0-a0e6-c810-a82cec8e3496@deltatee.com>



On 2022-07-13 09:26, Logan Gunthorpe wrote:
> 
> 
> 
> On 2022-07-13 05:31, Christoph Hellwig wrote:
>> Error handling in md_alloc is a mess.  Untangle it to just free the mddev
>> directly before add_disk is called and thus the gendisk is globally
>> visible.  After that clear the hold flag and let the mddev_put take care
>> of cleaning up the mddev through the usual mechanisms.
>>
>> Fixes: 5e55e2f5fc95 ("[PATCH] md: convert compile time warnings into runtime warnings")
>> Fixes: 9be68dd7ac0e ("md: add error handling support for add_disk()")
>> Fixes: 7ad1069166c0 ("md: properly unwind when failing to add the kobject in md_alloc")
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>> ---
>>
>> Note: the put_disk here is not fully correct on md-next, but will
>> do the right thing once merged with the block tree.
>>
>>  drivers/md/md.c | 45 ++++++++++++++++++++++++++++++++-------------
>>  1 file changed, 32 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index b64de313838f2..7affddade8b6b 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -791,6 +791,15 @@ static struct mddev *mddev_alloc(dev_t unit)
>>  	return ERR_PTR(error);
>>  }
>>  
>> +static void mddev_free(struct mddev *mddev)
>> +{
>> +	spin_lock(&all_mddevs_lock);
>> +	list_del(&mddev->all_mddevs);
>> +	spin_unlock(&all_mddevs_lock);
>> +
>> +	kfree(mddev);
>> +}
> 
> Sadly, I still don't think this is correct. mddev_init() has already
> called kobject_init() and according to the documentation it *must* be
> cleaned up with a call to kobject_put(). That doesn't happen in this
> path -- so I believe this will cause memory leaks.

What about something along these lines:

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 78e2588ed43e..1d13840cec6d 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -5581,6 +5581,15 @@ md_attr_store(struct kobject *kobj, struct
attribute *at>
        return rv;
 }

+static void mddev_free(struct mddev *mddev)
+{
+       percpu_ref_exit(&mddev->writes_pending);
+       bioset_exit(&mddev->bio_set);
+       bioset_exit(&mddev->sync_set);
+
+       kfree(mddev);
+}
+
 static void md_free(struct kobject *ko)
 {
        struct mddev *mddev = container_of(ko, struct mddev, kobj);
@@ -5593,12 +5602,9 @@ static void md_free(struct kobject *ko)
        if (mddev->gendisk) {
                del_gendisk(mddev->gendisk);
                blk_cleanup_disk(mddev->gendisk);
+       } else {
+               mddev_free(mddev);
        }
-       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 = {
@@ -7858,6 +7864,13 @@ static unsigned int md_check_events(struct
gendisk *disk>
        return ret;
 }

+static void md_free_disk(struct gendisk *disk)
+{
+       struct mddev *mddev = disk->private_data;
+
+       mddev_free(mddev);
+}
+
 const struct block_device_operations md_fops =
 {
        .owner          = THIS_MODULE,
@@ -7871,6 +7884,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,
 };
