[PATCH 1/1] Don't set mddev private to NULL in raid0 pers->free

[PATCH 1/1] Don't set mddev private to NULL in raid0 pers->free

[Xiao Ni]

It panics when reshaping from raid0 to other raid levels. raid0 sets
mddev->private to NULL. It's the reason that causes the problem.
Function level_store finds new pers and create new conf, then it
calls oldpers->free. In oldpers->free, raid0 sets mddev->private
to NULL again. And __md_stop is the right position to set
mddev->private to NULL.

And this patch also deletes double free memory codes. io_acct_set
is free in pers->free.

Fixes: 0c031fd37f69 (md: Move alloc/free acct bioset in to personality)
Reported-by: Fine Fan <ffan@redhat.com>
Signed-off-by: Xiao Ni <xni@redhat.com>
---
 drivers/md/md.c    | 4 ----
 drivers/md/raid0.c | 1 -
 2 files changed, 5 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 707e802..55b6412e 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -5598,8 +5598,6 @@ static void md_free(struct kobject *ko)
 
 	bioset_exit(&mddev->bio_set);
 	bioset_exit(&mddev->sync_set);
-	if (mddev->level != 1 && mddev->level != 10)
-		bioset_exit(&mddev->io_acct_set);
 	kfree(mddev);
 }
 
@@ -6285,8 +6283,6 @@ void md_stop(struct mddev *mddev)
 	__md_stop(mddev);
 	bioset_exit(&mddev->bio_set);
 	bioset_exit(&mddev->sync_set);
-	if (mddev->level != 1 && mddev->level != 10)
-		bioset_exit(&mddev->io_acct_set);
 }
 
 EXPORT_SYMBOL_GPL(md_stop);
diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
index e11701e..5fa0d40 100644
--- a/drivers/md/raid0.c
+++ b/drivers/md/raid0.c
@@ -362,7 +362,6 @@ static void free_conf(struct mddev *mddev, struct r0conf *conf)
 	kfree(conf->strip_zone);
 	kfree(conf->devlist);
 	kfree(conf);
-	mddev->private = NULL;
 }
 
 static void raid0_free(struct mddev *mddev, void *priv)
-- 
2.7.5

Re: [PATCH 1/1] Don't set mddev private to NULL in raid0 pers->free

[Song Liu]

Hi Xiao,

Thanks for the patch!

On Thu, Apr 28, 2022 at 10:09 PM Xiao Ni <xni@redhat.com> wrote:
>

prefix the subject with "md:", and provide more details.

> It panics when reshaping from raid0 to other raid levels. raid0 sets
> mddev->private to NULL. It's the reason that causes the problem.
> Function level_store finds new pers and create new conf, then it
> calls oldpers->free. In oldpers->free, raid0 sets mddev->private
> to NULL again. And __md_stop is the right position to set
> mddev->private to NULL.

We need more details here: the panic backtrace, and why it panics.
raid5_free also sets private to NULL. Does it has the same problem?

>
> And this patch also deletes double free memory codes. io_acct_set
> is free in pers->free.

Please put this part in a separate patch.

Thanks,
Song

>
> Fixes: 0c031fd37f69 (md: Move alloc/free acct bioset in to personality)
> Reported-by: Fine Fan <ffan@redhat.com>
> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
>  drivers/md/md.c    | 4 ----
>  drivers/md/raid0.c | 1 -
>  2 files changed, 5 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 707e802..55b6412e 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -5598,8 +5598,6 @@ static void md_free(struct kobject *ko)
>
>         bioset_exit(&mddev->bio_set);
>         bioset_exit(&mddev->sync_set);
> -       if (mddev->level != 1 && mddev->level != 10)
> -               bioset_exit(&mddev->io_acct_set);
>         kfree(mddev);
>  }
>
> @@ -6285,8 +6283,6 @@ void md_stop(struct mddev *mddev)
>         __md_stop(mddev);
>         bioset_exit(&mddev->bio_set);
>         bioset_exit(&mddev->sync_set);
> -       if (mddev->level != 1 && mddev->level != 10)
> -               bioset_exit(&mddev->io_acct_set);
>  }
>
>  EXPORT_SYMBOL_GPL(md_stop);
> diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
> index e11701e..5fa0d40 100644
> --- a/drivers/md/raid0.c
> +++ b/drivers/md/raid0.c
> @@ -362,7 +362,6 @@ static void free_conf(struct mddev *mddev, struct r0conf *conf)
>         kfree(conf->strip_zone);
>         kfree(conf->devlist);
>         kfree(conf);
> -       mddev->private = NULL;
>  }
>
>  static void raid0_free(struct mddev *mddev, void *priv)
> --
> 2.7.5
>

Re: [PATCH 1/1] Don't set mddev private to NULL in raid0 pers->free

[Xiao Ni]

On Mon, May 9, 2022 at 2:24 PM Song Liu <song@kernel.org> wrote:
>
> Hi Xiao,
>
> Thanks for the patch!
>
> On Thu, Apr 28, 2022 at 10:09 PM Xiao Ni <xni@redhat.com> wrote:
> >
>
> prefix the subject with "md:", and provide more details.

Hi Song

Thanks for pointing about this and I will add "md:" in v2.
>
> > It panics when reshaping from raid0 to other raid levels. raid0 sets
> > mddev->private to NULL. It's the reason that causes the problem.
> > Function level_store finds new pers and create new conf, then it
> > calls oldpers->free. In oldpers->free, raid0 sets mddev->private
> > to NULL again. And __md_stop is the right position to set
> > mddev->private to NULL.
>
> We need more details here: the panic backtrace, and why it panics.
> raid5_free also sets private to NULL. Does it has the same problem?

The backtrace will be added in v2.

I don't find raid5_free sets mddev->private to NULL.
The normal process of stopping a raid should be like this:

do_md_stop
       |
__md_stop  (pers->free(); mddev->private=NULL)
       |
md_free (free biosets of mddev; free mddev)

personality raid doesn't set mddev->private to NULL. __md_stop does this job.
Reshaping from raid0 to raid other raid level doesn't stop the raid.
The new raid
will go on working. Now raid0_free->free_conf sets mddev->private to NULL during
reshape. The new raid can't work anymore. It will panic when dereference
mddev->private because of NULL pointer dereference.


>
> >
> > And this patch also deletes double free memory codes. io_acct_set
> > is free in pers->free.
>
> Please put this part in a separate patch.

will do this in v2

Regards
Xiao

>
> Thanks,
> Song
>
> >
> > Fixes: 0c031fd37f69 (md: Move alloc/free acct bioset in to personality)
> > Reported-by: Fine Fan <ffan@redhat.com>
> > Signed-off-by: Xiao Ni <xni@redhat.com>
> > ---
> >  drivers/md/md.c    | 4 ----
> >  drivers/md/raid0.c | 1 -
> >  2 files changed, 5 deletions(-)
> >
> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > index 707e802..55b6412e 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -5598,8 +5598,6 @@ static void md_free(struct kobject *ko)
> >
> >         bioset_exit(&mddev->bio_set);
> >         bioset_exit(&mddev->sync_set);
> > -       if (mddev->level != 1 && mddev->level != 10)
> > -               bioset_exit(&mddev->io_acct_set);
> >         kfree(mddev);
> >  }
> >
> > @@ -6285,8 +6283,6 @@ void md_stop(struct mddev *mddev)
> >         __md_stop(mddev);
> >         bioset_exit(&mddev->bio_set);
> >         bioset_exit(&mddev->sync_set);
> > -       if (mddev->level != 1 && mddev->level != 10)
> > -               bioset_exit(&mddev->io_acct_set);
> >  }
> >
> >  EXPORT_SYMBOL_GPL(md_stop);
> > diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
> > index e11701e..5fa0d40 100644
> > --- a/drivers/md/raid0.c
> > +++ b/drivers/md/raid0.c
> > @@ -362,7 +362,6 @@ static void free_conf(struct mddev *mddev, struct r0conf *conf)
> >         kfree(conf->strip_zone);
> >         kfree(conf->devlist);
> >         kfree(conf);
> > -       mddev->private = NULL;
> >  }
> >
> >  static void raid0_free(struct mddev *mddev, void *priv)
> > --
> > 2.7.5
> >
>

Re: [PATCH 1/1] Don't set mddev private to NULL in raid0 pers->free

[Song Liu]

On Wed, May 11, 2022 at 9:14 AM Xiao Ni <xni@redhat.com> wrote:
>
> On Mon, May 9, 2022 at 2:24 PM Song Liu <song@kernel.org> wrote:
> >
> > Hi Xiao,
> >
> > Thanks for the patch!
> >
> > On Thu, Apr 28, 2022 at 10:09 PM Xiao Ni <xni@redhat.com> wrote:
> > >
> >
> > prefix the subject with "md:", and provide more details.
>
> Hi Song
>
> Thanks for pointing about this and I will add "md:" in v2.
> >
> > > It panics when reshaping from raid0 to other raid levels. raid0 sets
> > > mddev->private to NULL. It's the reason that causes the problem.
> > > Function level_store finds new pers and create new conf, then it
> > > calls oldpers->free. In oldpers->free, raid0 sets mddev->private
> > > to NULL again. And __md_stop is the right position to set
> > > mddev->private to NULL.
> >
> > We need more details here: the panic backtrace, and why it panics.
> > raid5_free also sets private to NULL. Does it has the same problem?
>
> The backtrace will be added in v2.
>
> I don't find raid5_free sets mddev->private to NULL.
> The normal process of stopping a raid should be like this:
>
> do_md_stop
>        |
> __md_stop  (pers->free(); mddev->private=NULL)
>        |
> md_free (free biosets of mddev; free mddev)
>
> personality raid doesn't set mddev->private to NULL. __md_stop does this job.
> Reshaping from raid0 to raid other raid level doesn't stop the raid.
> The new raid
> will go on working. Now raid0_free->free_conf sets mddev->private to NULL during
> reshape. The new raid can't work anymore. It will panic when dereference
> mddev->private because of NULL pointer dereference.

Got it. Thanks for the explanation.

Thanks,
Song