On Mon, May 9, 2022 at 2:24 PM Song Liu <song@xxxxxxxxxx> wrote:
>
> Hi Xiao,
>
> Thanks for the patch!
>
> On Thu, Apr 28, 2022 at 10:09 PM Xiao Ni <xni@xxxxxxxxxx> 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@xxxxxxxxxx>
> > Signed-off-by: Xiao Ni <xni@xxxxxxxxxx>
> > ---
> > 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
> >
>
