Linux-Raid Archives on lore.kernel.org
 help / color / Atom feed
* [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	[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	[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, back to index

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

Linux-Raid Archives on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-raid/0 linux-raid/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-raid linux-raid/ https://lore.kernel.org/linux-raid \
		linux-raid@vger.kernel.org
	public-inbox-index linux-raid

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-raid


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git