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@xxxxxx>
> ---
>
> 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.
I have also noticed this code base already makes that same mistake in
mddev_alloc(): freeing the mddev on the error path after kobject_init().
But that would be easy to fix up.
Logan
