| Message ID | 20240604172607.3185916-2-hch@lst.de (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [1/2] md/raid0: don't free conf on raid0_run failure | expand |
| Context | Check | Description |
|---|---|---|
| mdraidci/vmtest-md-6_11-VM_Test-1 | success | Logs for ShellCheck |
| mdraidci/vmtest-md-6_11-VM_Test-2 | success | Logs for Unittests |
| mdraidci/vmtest-md-6_11-VM_Test-3 | success | Logs for Validate matrix.py |
| mdraidci/vmtest-md-6_11-VM_Test-4 | success | Logs for set-matrix |
| mdraidci/vmtest-md-6_11-VM_Test-5 | pending | Logs for x86_64-gcc / build / build for x86_64 with gcc |
| mdraidci/vmtest-md-6_11-VM_Test-9 | pending | Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18 |
| mdraidci/vmtest-md-6_11-VM_Test-8 | pending | Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17 and -O2 optimization |
| mdraidci/vmtest-md-6_11-VM_Test-6 | success | Logs for x86_64-gcc / build-release |
| mdraidci/vmtest-md-6_11-VM_Test-10 | pending | Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization |
| mdraidci/vmtest-md-6_11-VM_Test-7 | pending | Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17 |
| mdraidci/vmtest-md-6_11-PR | success | PR summary |
| mdraidci/vmtest-md-6_11-VM_Test-0 | success | Logs for build-kernel |
在 2024/06/05 1:25, Christoph Hellwig 写道: > The core md code calls the ->free method which already frees conf. > > Fixes: 0c031fd37f69 ("md: Move alloc/free acct bioset in to personality") > Signed-off-by: Christoph Hellwig <hch@lst.de> LGTM Reviewed-by: Yu Kuai <yukuai3@huawei.com> > --- > drivers/md/raid0.c | 21 +++++---------------- > 1 file changed, 5 insertions(+), 16 deletions(-) > > diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c > index c5d4aeb68404c9..81c01347cd24e6 100644 > --- a/drivers/md/raid0.c > +++ b/drivers/md/raid0.c > @@ -365,18 +365,13 @@ static sector_t raid0_size(struct mddev *mddev, sector_t sectors, int raid_disks > return array_sectors; > } > > -static void free_conf(struct mddev *mddev, struct r0conf *conf) > -{ > - kfree(conf->strip_zone); > - kfree(conf->devlist); > - kfree(conf); > -} > - > static void raid0_free(struct mddev *mddev, void *priv) > { > struct r0conf *conf = priv; > > - free_conf(mddev, conf); > + kfree(conf->strip_zone); > + kfree(conf->devlist); > + kfree(conf); > } > > static int raid0_set_limits(struct mddev *mddev) > @@ -415,7 +410,7 @@ static int raid0_run(struct mddev *mddev) > if (!mddev_is_dm(mddev)) { > ret = raid0_set_limits(mddev); > if (ret) > - goto out_free_conf; > + return ret; > } > > /* calculate array device size */ > @@ -427,13 +422,7 @@ static int raid0_run(struct mddev *mddev) > > dump_zones(mddev); > > - ret = md_integrity_register(mddev); > - if (ret) > - goto out_free_conf; > - return 0; > -out_free_conf: > - free_conf(mddev, conf); > - return ret; > + return md_integrity_register(mddev); > } > > /* >
Hi, 在 2024/06/07 14:08, Yu Kuai 写道: > 在 2024/06/05 1:25, Christoph Hellwig 写道: >> The core md code calls the ->free method which already frees conf. >> >> Fixes: 0c031fd37f69 ("md: Move alloc/free acct bioset in to personality") >> Signed-off-by: Christoph Hellwig <hch@lst.de> > > LGTM > Reviewed-by: Yu Kuai <yukuai3@huawei.com> Sorry that I just noticed that the sysfs api level_store() calls the pers->run() as well, and it didn't handle failure by pers->free(). It's weried that the api will return success in this case, and after this patch, it will require __md_stop() to free the conf. Can you also fix the level_store()? By checking pers->run() and if it failed, call pers->free() and return error. Thanks, Kuai > >> --- >> drivers/md/raid0.c | 21 +++++---------------- >> 1 file changed, 5 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c >> index c5d4aeb68404c9..81c01347cd24e6 100644 >> --- a/drivers/md/raid0.c >> +++ b/drivers/md/raid0.c >> @@ -365,18 +365,13 @@ static sector_t raid0_size(struct mddev *mddev, >> sector_t sectors, int raid_disks >> return array_sectors; >> } >> -static void free_conf(struct mddev *mddev, struct r0conf *conf) >> -{ >> - kfree(conf->strip_zone); >> - kfree(conf->devlist); >> - kfree(conf); >> -} >> - >> static void raid0_free(struct mddev *mddev, void *priv) >> { >> struct r0conf *conf = priv; >> - free_conf(mddev, conf); >> + kfree(conf->strip_zone); >> + kfree(conf->devlist); >> + kfree(conf); >> } >> static int raid0_set_limits(struct mddev *mddev) >> @@ -415,7 +410,7 @@ static int raid0_run(struct mddev *mddev) >> if (!mddev_is_dm(mddev)) { >> ret = raid0_set_limits(mddev); >> if (ret) >> - goto out_free_conf; >> + return ret; >> } >> /* calculate array device size */ >> @@ -427,13 +422,7 @@ static int raid0_run(struct mddev *mddev) >> dump_zones(mddev); >> - ret = md_integrity_register(mddev); >> - if (ret) >> - goto out_free_conf; >> - return 0; >> -out_free_conf: >> - free_conf(mddev, conf); >> - return ret; >> + return md_integrity_register(mddev); >> } >> /* >> > > . >
On Fri, Jun 07, 2024 at 02:17:14PM +0800, Yu Kuai wrote: > Sorry that I just noticed that the sysfs api level_store() calls the > pers->run() as well, and it didn't handle failure by pers->free(). > It's weried that the api will return success in this case, and after > this patch, it will require __md_stop() to free the conf. > > Can you also fix the level_store()? By checking pers->run() and if it > failed, call pers->free() and return error. I can look into it. But if we don't have consistent callers anyway I'd be tempted to not call ->free on ->run failure, as that is a rather unusual convention.
diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c index c5d4aeb68404c9..81c01347cd24e6 100644 --- a/drivers/md/raid0.c +++ b/drivers/md/raid0.c @@ -365,18 +365,13 @@ static sector_t raid0_size(struct mddev *mddev, sector_t sectors, int raid_disks return array_sectors; } -static void free_conf(struct mddev *mddev, struct r0conf *conf) -{ - kfree(conf->strip_zone); - kfree(conf->devlist); - kfree(conf); -} - static void raid0_free(struct mddev *mddev, void *priv) { struct r0conf *conf = priv; - free_conf(mddev, conf); + kfree(conf->strip_zone); + kfree(conf->devlist); + kfree(conf); } static int raid0_set_limits(struct mddev *mddev) @@ -415,7 +410,7 @@ static int raid0_run(struct mddev *mddev) if (!mddev_is_dm(mddev)) { ret = raid0_set_limits(mddev); if (ret) - goto out_free_conf; + return ret; } /* calculate array device size */ @@ -427,13 +422,7 @@ static int raid0_run(struct mddev *mddev) dump_zones(mddev); - ret = md_integrity_register(mddev); - if (ret) - goto out_free_conf; - return 0; -out_free_conf: - free_conf(mddev, conf); - return ret; + return md_integrity_register(mddev); } /*
The core md code calls the ->free method which already frees conf. Fixes: 0c031fd37f69 ("md: Move alloc/free acct bioset in to personality") Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/md/raid0.c | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-)