* [PATCH] md-cluster: Fix potential error pointer dereference in resize_bitmaps() @ 2020-08-04 10:16 Dan Carpenter 2020-08-04 10:40 ` Guoqing Jiang 0 siblings, 1 reply; 5+ messages in thread From: Dan Carpenter @ 2020-08-04 10:16 UTC (permalink / raw) To: Song Liu, Guoqing Jiang Cc: Shaohua Li, NeilBrown, linux-raid, kernel-janitors The error handling calls md_bitmap_free(bitmap) which checks for NULL but will Oops if we pass an error pointer. Let's set "bitmap" to NULL on this error path. Fixes: afd756286083 ("md-cluster/raid10: resize all the bitmaps before start reshape") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- drivers/md/md-cluster.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c index 73fd50e77975..d50737ec4039 100644 --- a/drivers/md/md-cluster.c +++ b/drivers/md/md-cluster.c @@ -1139,6 +1139,7 @@ static int resize_bitmaps(struct mddev *mddev, sector_t newsize, sector_t oldsiz bitmap = get_bitmap_from_slot(mddev, i); if (IS_ERR(bitmap)) { pr_err("can't get bitmap from slot %d\n", i); + bitmap = NULL; goto out; } counts = &bitmap->counts; -- 2.27.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] md-cluster: Fix potential error pointer dereference in resize_bitmaps() 2020-08-04 10:16 [PATCH] md-cluster: Fix potential error pointer dereference in resize_bitmaps() Dan Carpenter @ 2020-08-04 10:40 ` Guoqing Jiang 2020-08-04 11:15 ` Dan Carpenter 0 siblings, 1 reply; 5+ messages in thread From: Guoqing Jiang @ 2020-08-04 10:40 UTC (permalink / raw) To: Dan Carpenter, Song Liu Cc: Shaohua Li, NeilBrown, linux-raid, kernel-janitors On 8/4/20 12:16 PM, Dan Carpenter wrote: > The error handling calls md_bitmap_free(bitmap) which checks for NULL > but will Oops if we pass an error pointer. Let's set "bitmap" to NULL > on this error path. > > Fixes: afd756286083 ("md-cluster/raid10: resize all the bitmaps before start reshape") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > drivers/md/md-cluster.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c > index 73fd50e77975..d50737ec4039 100644 > --- a/drivers/md/md-cluster.c > +++ b/drivers/md/md-cluster.c > @@ -1139,6 +1139,7 @@ static int resize_bitmaps(struct mddev *mddev, sector_t newsize, sector_t oldsiz > bitmap = get_bitmap_from_slot(mddev, i); > if (IS_ERR(bitmap)) { > pr_err("can't get bitmap from slot %d\n", i); > + bitmap = NULL; > goto out; > } > counts = &bitmap->counts; Thanks for the catch, Reviewed-by: Guoqing Jiang <guoqing.jiang@cloud.ionos.com> BTW, seems there could be memory leak in the function since it keeps allocate bitmap in the loop ..., will send a format patch. diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c index 73fd50e77975..89d7b32489d8 100644 --- a/drivers/md/md-cluster.c +++ b/drivers/md/md-cluster.c @@ -1165,6 +1165,8 @@ static int resize_bitmaps(struct mddev *mddev, sector_t newsize, sector_t oldsiz * can't resize bitmap */ goto out; + + md_bitmap_free(bitmap); } return 0; Thanks, Guoqing ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] md-cluster: Fix potential error pointer dereference in resize_bitmaps() 2020-08-04 10:40 ` Guoqing Jiang @ 2020-08-04 11:15 ` Dan Carpenter 2020-08-06 0:20 ` Song Liu 0 siblings, 1 reply; 5+ messages in thread From: Dan Carpenter @ 2020-08-04 11:15 UTC (permalink / raw) To: Guoqing Jiang Cc: Song Liu, Shaohua Li, NeilBrown, linux-raid, kernel-janitors On Tue, Aug 04, 2020 at 12:40:18PM +0200, Guoqing Jiang wrote: > > > On 8/4/20 12:16 PM, Dan Carpenter wrote: > > The error handling calls md_bitmap_free(bitmap) which checks for NULL > > but will Oops if we pass an error pointer. Let's set "bitmap" to NULL > > on this error path. > > > > Fixes: afd756286083 ("md-cluster/raid10: resize all the bitmaps before start reshape") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > --- > > drivers/md/md-cluster.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c > > index 73fd50e77975..d50737ec4039 100644 > > --- a/drivers/md/md-cluster.c > > +++ b/drivers/md/md-cluster.c > > @@ -1139,6 +1139,7 @@ static int resize_bitmaps(struct mddev *mddev, sector_t newsize, sector_t oldsiz > > bitmap = get_bitmap_from_slot(mddev, i); > > if (IS_ERR(bitmap)) { > > pr_err("can't get bitmap from slot %d\n", i); > > + bitmap = NULL; > > goto out; > > } > > counts = &bitmap->counts; > > Thanks for the catch, Reviewed-by: Guoqing Jiang > <guoqing.jiang@cloud.ionos.com> > > BTW, seems there could be memory leak in the function since it keeps > allocate bitmap > in the loop ..., will send a format patch. > > > diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c > index 73fd50e77975..89d7b32489d8 100644 > --- a/drivers/md/md-cluster.c > +++ b/drivers/md/md-cluster.c > @@ -1165,6 +1165,8 @@ static int resize_bitmaps(struct mddev *mddev, > sector_t newsize, sector_t oldsiz > * can't resize bitmap > */ > goto out; > + > + md_bitmap_free(bitmap); Hm... I'm now not at all certain my patch is correct. Although it's obviously harmless and fixes an Oops. I had thought that that the call to update_bitmap_size(mddev, oldsize) would free the rest of the loop. I really suspect adding a free like you're suggesting will break the success path. I'm not familiar with this code at all. regards, dan carpenter ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] md-cluster: Fix potential error pointer dereference in resize_bitmaps() 2020-08-04 11:15 ` Dan Carpenter @ 2020-08-06 0:20 ` Song Liu 2020-10-03 15:57 ` heming.zhao 0 siblings, 1 reply; 5+ messages in thread From: Song Liu @ 2020-08-06 0:20 UTC (permalink / raw) To: Dan Carpenter Cc: Guoqing Jiang, Shaohua Li, NeilBrown, linux-raid, kernel-janitors On Tue, Aug 4, 2020 at 4:16 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > On Tue, Aug 04, 2020 at 12:40:18PM +0200, Guoqing Jiang wrote: > > > > > > On 8/4/20 12:16 PM, Dan Carpenter wrote: > > > The error handling calls md_bitmap_free(bitmap) which checks for NULL > > > but will Oops if we pass an error pointer. Let's set "bitmap" to NULL > > > on this error path. > > > > > > Fixes: afd756286083 ("md-cluster/raid10: resize all the bitmaps before start reshape") > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > --- > > > drivers/md/md-cluster.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c > > > index 73fd50e77975..d50737ec4039 100644 > > > --- a/drivers/md/md-cluster.c > > > +++ b/drivers/md/md-cluster.c > > > @@ -1139,6 +1139,7 @@ static int resize_bitmaps(struct mddev *mddev, sector_t newsize, sector_t oldsiz > > > bitmap = get_bitmap_from_slot(mddev, i); > > > if (IS_ERR(bitmap)) { > > > pr_err("can't get bitmap from slot %d\n", i); > > > + bitmap = NULL; > > > goto out; > > > } > > > counts = &bitmap->counts; > > > > Thanks for the catch, Reviewed-by: Guoqing Jiang > > <guoqing.jiang@cloud.ionos.com> > > > > BTW, seems there could be memory leak in the function since it keeps > > allocate bitmap > > in the loop ..., will send a format patch. > > > > > > diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c > > index 73fd50e77975..89d7b32489d8 100644 > > --- a/drivers/md/md-cluster.c > > +++ b/drivers/md/md-cluster.c > > @@ -1165,6 +1165,8 @@ static int resize_bitmaps(struct mddev *mddev, > > sector_t newsize, sector_t oldsiz > > * can't resize bitmap > > */ > > goto out; > > + > > + md_bitmap_free(bitmap); > > Hm... I'm now not at all certain my patch is correct. Although it's > obviously harmless and fixes an Oops. I had thought that that the call > to update_bitmap_size(mddev, oldsize) would free the rest of the loop. > > I really suspect adding a free like you're suggesting will break the > success path. > > I'm not familiar with this code at all. Thanks Dan and Guoqing. I applied Dans' patch to md-next. I think we are leaking bitmap in resize_bitmaps(). But we gonna need more complex fix than the md_bitmap_free() above. Thanks, Song ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] md-cluster: Fix potential error pointer dereference in resize_bitmaps() 2020-08-06 0:20 ` Song Liu @ 2020-10-03 15:57 ` heming.zhao 0 siblings, 0 replies; 5+ messages in thread From: heming.zhao @ 2020-10-03 15:57 UTC (permalink / raw) To: Song Liu, Dan Carpenter Cc: Guoqing Jiang, Shaohua Li, NeilBrown, linux-raid, kernel-janitors Hello Song, I just found this mail, and I send a similar patch on 2020-9-27. From my thinking, Guoqing's patch for fixing memory leak is enough. the resize_bitmaps() work flow: - first use update_bitmap_size() to broadcast BITMAP_RESIZE to all nodes, and waiting for the other nodes ack. - then in for loop, by holding bitmap00x lock to update not booted slot's bitmap info. the bitmap in for() is temporary, it should be safely freed on every time of end for(). Thanks. On 8/6/20 8:20 AM, Song Liu wrote: > On Tue, Aug 4, 2020 at 4:16 AM Dan Carpenter <dan.carpenter@oracle.com> wrote: >> >> On Tue, Aug 04, 2020 at 12:40:18PM +0200, Guoqing Jiang wrote: >>> >>> >>> On 8/4/20 12:16 PM, Dan Carpenter wrote: >>>> The error handling calls md_bitmap_free(bitmap) which checks for NULL >>>> but will Oops if we pass an error pointer. Let's set "bitmap" to NULL >>>> on this error path. >>>> >>>> Fixes: afd756286083 ("md-cluster/raid10: resize all the bitmaps before start reshape") >>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> >>>> --- >>>> drivers/md/md-cluster.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c >>>> index 73fd50e77975..d50737ec4039 100644 >>>> --- a/drivers/md/md-cluster.c >>>> +++ b/drivers/md/md-cluster.c >>>> @@ -1139,6 +1139,7 @@ static int resize_bitmaps(struct mddev *mddev, sector_t newsize, sector_t oldsiz >>>> bitmap = get_bitmap_from_slot(mddev, i); >>>> if (IS_ERR(bitmap)) { >>>> pr_err("can't get bitmap from slot %d\n", i); >>>> + bitmap = NULL; >>>> goto out; >>>> } >>>> counts = &bitmap->counts; >>> >>> Thanks for the catch, Reviewed-by: Guoqing Jiang >>> <guoqing.jiang@cloud.ionos.com> >>> >>> BTW, seems there could be memory leak in the function since it keeps >>> allocate bitmap >>> in the loop ..., will send a format patch. >>> >>> >>> diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c >>> index 73fd50e77975..89d7b32489d8 100644 >>> --- a/drivers/md/md-cluster.c >>> +++ b/drivers/md/md-cluster.c >>> @@ -1165,6 +1165,8 @@ static int resize_bitmaps(struct mddev *mddev, >>> sector_t newsize, sector_t oldsiz >>> * can't resize bitmap >>> */ >>> goto out; >>> + >>> + md_bitmap_free(bitmap); >> >> Hm... I'm now not at all certain my patch is correct. Although it's >> obviously harmless and fixes an Oops. I had thought that that the call >> to update_bitmap_size(mddev, oldsize) would free the rest of the loop. >> >> I really suspect adding a free like you're suggesting will break the >> success path. >> >> I'm not familiar with this code at all. > > Thanks Dan and Guoqing. I applied Dans' patch to md-next. > > I think we are leaking bitmap in resize_bitmaps(). But we gonna need > more complex fix than the md_bitmap_free() above. > > Thanks, > Song > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-10-03 15:59 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-08-04 10:16 [PATCH] md-cluster: Fix potential error pointer dereference in resize_bitmaps() Dan Carpenter 2020-08-04 10:40 ` Guoqing Jiang 2020-08-04 11:15 ` Dan Carpenter 2020-08-06 0:20 ` Song Liu 2020-10-03 15:57 ` heming.zhao
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).