[PATCH] md-cluster: Fix potential error pointer dereference in resize_bitmaps()

[PATCH] md-cluster: Fix potential error pointer dereference in resize_bitmaps()

[Dan Carpenter]

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

Re: [PATCH] md-cluster: Fix potential error pointer dereference in resize_bitmaps()

[Guoqing Jiang]

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

Re: [PATCH] md-cluster: Fix potential error pointer dereference in resize_bitmaps()

[Dan Carpenter]

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

Re: [PATCH] md-cluster: Fix potential error pointer dereference in resize_bitmaps()

[Song Liu]

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

Re: [PATCH] md-cluster: Fix potential error pointer dereference in resize_bitmaps()

[heming.zhao]

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
>