mtd: Add check for kcalloc()

[PATCH] mtd: Add check for kcalloc()

Posted by Jiasheng Jiang 1 year ago

Add a check for kcalloc() and print an error message if it fails.

Fixes: 78c08247b9d3 ("mtd: Support kmsg dumper based on pstore/blk")
Signed-off-by: Jiasheng Jiang <jiashengjiangcool@gmail.com>
---
 drivers/mtd/mtdpstore.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/mtd/mtdpstore.c b/drivers/mtd/mtdpstore.c
index 7ac8ac901306..368997155c07 100644
--- a/drivers/mtd/mtdpstore.c
+++ b/drivers/mtd/mtdpstore.c
@@ -423,6 +423,11 @@ static void mtdpstore_notify_add(struct mtd_info *mtd)
 	longcnt = BITS_TO_LONGS(div_u64(mtd->size, mtd->erasesize));
 	cxt->badmap = kcalloc(longcnt, sizeof(long), GFP_KERNEL);
 
+	if (!cxt->rmmap || !cxt->usedmap || !cxt->badmap) {
+		dev_err(&mtd->dev, "failed to allocate memory for map\n");
+		return;
+	}
+
 	/* just support dmesg right now */
 	cxt->dev.flags = PSTORE_FLAGS_DMESG;
 	cxt->dev.zone.read = mtdpstore_read;
-- 
2.25.1

Re: [PATCH] mtd: Add check for kcalloc()

Posted by Miquel Raynal 1 year ago

On 02/02/2025 at 20:54:56 GMT, Jiasheng Jiang <jiashengjiangcool@gmail.com> wrote:

> Add a check for kcalloc() and print an error message if it fails.

Checking, yes, but logging, no. IIRC the core does it already if required.

> Fixes: 78c08247b9d3 ("mtd: Support kmsg dumper based on pstore/blk")

Cc: stable is missing

> Signed-off-by: Jiasheng Jiang <jiashengjiangcool@gmail.com>
> ---
>  drivers/mtd/mtdpstore.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/mtd/mtdpstore.c b/drivers/mtd/mtdpstore.c
> index 7ac8ac901306..368997155c07 100644
> --- a/drivers/mtd/mtdpstore.c
> +++ b/drivers/mtd/mtdpstore.c
> @@ -423,6 +423,11 @@ static void mtdpstore_notify_add(struct mtd_info *mtd)
>  	longcnt = BITS_TO_LONGS(div_u64(mtd->size, mtd->erasesize));
>  	cxt->badmap = kcalloc(longcnt, sizeof(long), GFP_KERNEL);
>  
> +	if (!cxt->rmmap || !cxt->usedmap || !cxt->badmap) {
> +		dev_err(&mtd->dev, "failed to allocate memory for map\n");
> +		return;
> +	}
> +
>  	/* just support dmesg right now */
>  	cxt->dev.flags = PSTORE_FLAGS_DMESG;
>  	cxt->dev.zone.read = mtdpstore_read;

What if register_pstore_device() fails? Your only partially fixing the
problem if you don't handle the free()s there as well.

Thanks,
Miquèl

Re: [PATCH] mtd: Add check for kcalloc()

Posted by Jiasheng Jiang 1 year ago

Hi Miquel,

On Mon, Feb 3, 2025 at 3:32 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> On 02/02/2025 at 20:54:56 GMT, Jiasheng Jiang <jiashengjiangcool@gmail.com> wrote:
>
> > Add a check for kcalloc() and print an error message if it fails.
>
> Checking, yes, but logging, no. IIRC the core does it already if required.
>
> > Fixes: 78c08247b9d3 ("mtd: Support kmsg dumper based on pstore/blk")
>
> Cc: stable is missing
>
> > Signed-off-by: Jiasheng Jiang <jiashengjiangcool@gmail.com>
> > ---
> >  drivers/mtd/mtdpstore.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/mtd/mtdpstore.c b/drivers/mtd/mtdpstore.c
> > index 7ac8ac901306..368997155c07 100644
> > --- a/drivers/mtd/mtdpstore.c
> > +++ b/drivers/mtd/mtdpstore.c
> > @@ -423,6 +423,11 @@ static void mtdpstore_notify_add(struct mtd_info *mtd)
> >       longcnt = BITS_TO_LONGS(div_u64(mtd->size, mtd->erasesize));
> >       cxt->badmap = kcalloc(longcnt, sizeof(long), GFP_KERNEL);
> >
> > +     if (!cxt->rmmap || !cxt->usedmap || !cxt->badmap) {
> > +             dev_err(&mtd->dev, "failed to allocate memory for map\n");
> > +             return;
> > +     }
> > +
> >       /* just support dmesg right now */
> >       cxt->dev.flags = PSTORE_FLAGS_DMESG;
> >       cxt->dev.zone.read = mtdpstore_read;
>
> What if register_pstore_device() fails? Your only partially fixing the
> problem if you don't handle the free()s there as well.
>
> Thanks,
> Miquèl

Thanks, I have submitted a v2 to fix the problems above.

-Jiasheng

Re: [PATCH] mtd: Add check for kcalloc()

Posted by Christophe JAILLET 1 year ago

Le 03/02/2025 à 09:32, Miquel Raynal a écrit :
> On 02/02/2025 at 20:54:56 GMT, Jiasheng Jiang <jiashengjiangcool@gmail.com> wrote:
> 
>> Add a check for kcalloc() and print an error message if it fails.
> 
> Checking, yes, but logging, no. IIRC the core does it already if required.
> 
>> Fixes: 78c08247b9d3 ("mtd: Support kmsg dumper based on pstore/blk")
> 
> Cc: stable is missing
> 
>> Signed-off-by: Jiasheng Jiang <jiashengjiangcool@gmail.com>
>> ---
>>   drivers/mtd/mtdpstore.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/mtd/mtdpstore.c b/drivers/mtd/mtdpstore.c
>> index 7ac8ac901306..368997155c07 100644
>> --- a/drivers/mtd/mtdpstore.c
>> +++ b/drivers/mtd/mtdpstore.c
>> @@ -423,6 +423,11 @@ static void mtdpstore_notify_add(struct mtd_info *mtd)
>>   	longcnt = BITS_TO_LONGS(div_u64(mtd->size, mtd->erasesize));
>>   	cxt->badmap = kcalloc(longcnt, sizeof(long), GFP_KERNEL);
>>   
>> +	if (!cxt->rmmap || !cxt->usedmap || !cxt->badmap) {
>> +		dev_err(&mtd->dev, "failed to allocate memory for map\n");
>> +		return;
>> +	}
>> +
>>   	/* just support dmesg right now */
>>   	cxt->dev.flags = PSTORE_FLAGS_DMESG;
>>   	cxt->dev.zone.read = mtdpstore_read;
> 
> What if register_pstore_device() fails? Your only partially fixing the
> problem if you don't handle the free()s there as well.

... and if an allocation fails above, then some kfree() also needs to be 
called.

And, unrelated to your patch, these kcalloc()/kfree() could be some 
bitmap_zalloc()/bitmap_free(). (but i'm not sure it really worth the effort)

CJ

> 
> Thanks,
> Miquèl
> 
>

Re: [PATCH] mtd: Add check for kcalloc()

Posted by Jiasheng Jiang 1 year ago

Hi Christophe,

On Mon, Feb 3, 2025 at 1:04 PM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
>
> Le 03/02/2025 à 09:32, Miquel Raynal a écrit :
> > On 02/02/2025 at 20:54:56 GMT, Jiasheng Jiang <jiashengjiangcool@gmail.com> wrote:
> >
> >> Add a check for kcalloc() and print an error message if it fails.
> >
> > Checking, yes, but logging, no. IIRC the core does it already if required.
> >
> >> Fixes: 78c08247b9d3 ("mtd: Support kmsg dumper based on pstore/blk")
> >
> > Cc: stable is missing
> >
> >> Signed-off-by: Jiasheng Jiang <jiashengjiangcool@gmail.com>
> >> ---
> >>   drivers/mtd/mtdpstore.c | 5 +++++
> >>   1 file changed, 5 insertions(+)
> >>
> >> diff --git a/drivers/mtd/mtdpstore.c b/drivers/mtd/mtdpstore.c
> >> index 7ac8ac901306..368997155c07 100644
> >> --- a/drivers/mtd/mtdpstore.c
> >> +++ b/drivers/mtd/mtdpstore.c
> >> @@ -423,6 +423,11 @@ static void mtdpstore_notify_add(struct mtd_info *mtd)
> >>      longcnt = BITS_TO_LONGS(div_u64(mtd->size, mtd->erasesize));
> >>      cxt->badmap = kcalloc(longcnt, sizeof(long), GFP_KERNEL);
> >>
> >> +    if (!cxt->rmmap || !cxt->usedmap || !cxt->badmap) {
> >> +            dev_err(&mtd->dev, "failed to allocate memory for map\n");
> >> +            return;
> >> +    }
> >> +
> >>      /* just support dmesg right now */
> >>      cxt->dev.flags = PSTORE_FLAGS_DMESG;
> >>      cxt->dev.zone.read = mtdpstore_read;
> >
> > What if register_pstore_device() fails? Your only partially fixing the
> > problem if you don't handle the free()s there as well.
>
> ... and if an allocation fails above, then some kfree() also needs to be
> called.
>

Thanks, I have add kfree() to deal with each allocation in my v2.

> And, unrelated to your patch, these kcalloc()/kfree() could be some
> bitmap_zalloc()/bitmap_free(). (but i'm not sure it really worth the effort)

I think it works well currently, so it may not be necessary to do the
replacements.

>
> CJ
>
> >
> > Thanks,
> > Miquèl
> >
> >
>

-Jiasheng

[PATCH v2] mtd: Add check and kfree() for kcalloc()

Posted by Jiasheng Jiang 1 year ago

Add a check for kcalloc() to ensure successful allocation.
Moreover, add kfree() in the error-handling path to prevent memory leaks.

Fixes: 78c08247b9d3 ("mtd: Support kmsg dumper based on pstore/blk")
Cc: <stable@vger.kernel.org> # v5.10+
Signed-off-by: Jiasheng Jiang <jiashengjiangcool@gmail.com>
---
Changelog:

v1 -> v2:

1. Remove redundant logging.
2. Add kfree() in the error-handling path.
---
 drivers/mtd/mtdpstore.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/mtdpstore.c b/drivers/mtd/mtdpstore.c
index 7ac8ac901306..2d8e330dd215 100644
--- a/drivers/mtd/mtdpstore.c
+++ b/drivers/mtd/mtdpstore.c
@@ -418,10 +418,17 @@ static void mtdpstore_notify_add(struct mtd_info *mtd)
 
 	longcnt = BITS_TO_LONGS(div_u64(mtd->size, info->kmsg_size));
 	cxt->rmmap = kcalloc(longcnt, sizeof(long), GFP_KERNEL);
+	if (!cxt->rmmap)
+		goto end;
+
 	cxt->usedmap = kcalloc(longcnt, sizeof(long), GFP_KERNEL);
+	if (!cxt->usedmap)
+		goto free_rmmap;
 
 	longcnt = BITS_TO_LONGS(div_u64(mtd->size, mtd->erasesize));
 	cxt->badmap = kcalloc(longcnt, sizeof(long), GFP_KERNEL);
+	if (!cxt->badmap)
+		goto free_usedmap;
 
 	/* just support dmesg right now */
 	cxt->dev.flags = PSTORE_FLAGS_DMESG;
@@ -435,10 +442,20 @@ static void mtdpstore_notify_add(struct mtd_info *mtd)
 	if (ret) {
 		dev_err(&mtd->dev, "mtd%d register to psblk failed\n",
 				mtd->index);
-		return;
+		goto free_badmap;
 	}
 	cxt->mtd = mtd;
 	dev_info(&mtd->dev, "Attached to MTD device %d\n", mtd->index);
+	goto end;
+
+free_badmap:
+	kfree(cxt->badmap);
+free_usedmap:
+	kfree(cxt->usedmap);
+free_rmmap:
+	kfree(cxt->rmmap);
+end:
+	return;
 }
 
 static int mtdpstore_flush_removed_do(struct mtdpstore_context *cxt,
-- 
2.25.1

Re: [PATCH v2] mtd: Add check and kfree() for kcalloc()

Posted by Miquel Raynal 1 year ago

Hello,

> --- a/drivers/mtd/mtdpstore.c
> +++ b/drivers/mtd/mtdpstore.c
> @@ -418,10 +418,17 @@ static void mtdpstore_notify_add(struct mtd_info *mtd)
>  
>  	longcnt = BITS_TO_LONGS(div_u64(mtd->size, info->kmsg_size));
>  	cxt->rmmap = kcalloc(longcnt, sizeof(long), GFP_KERNEL);
> +	if (!cxt->rmmap)
> +		goto end;

We prefer to return immediately in this case.

Also, any reasons not to use devm_kcalloc()? This would be the correct
approach as of today as long as the lifetime of the device is known.

Thanks,
Miquèl

Re: [PATCH v2] mtd: Add check and kfree() for kcalloc()

Posted by Jiri Slaby 1 year ago

On 04. 02. 25, 3:33, Jiasheng Jiang wrote:
> Add a check for kcalloc() to ensure successful allocation.
> Moreover, add kfree() in the error-handling path to prevent memory leaks.
> 
> Fixes: 78c08247b9d3 ("mtd: Support kmsg dumper based on pstore/blk")
> Cc: <stable@vger.kernel.org> # v5.10+
> Signed-off-by: Jiasheng Jiang <jiashengjiangcool@gmail.com>
> ---
> Changelog:
> 
> v1 -> v2:
> 
> 1. Remove redundant logging.
> 2. Add kfree() in the error-handling path.
> ---
>   drivers/mtd/mtdpstore.c | 19 ++++++++++++++++++-
>   1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/mtdpstore.c b/drivers/mtd/mtdpstore.c
> index 7ac8ac901306..2d8e330dd215 100644
> --- a/drivers/mtd/mtdpstore.c
> +++ b/drivers/mtd/mtdpstore.c
> @@ -418,10 +418,17 @@ static void mtdpstore_notify_add(struct mtd_info *mtd)
>   
>   	longcnt = BITS_TO_LONGS(div_u64(mtd->size, info->kmsg_size));
>   	cxt->rmmap = kcalloc(longcnt, sizeof(long), GFP_KERNEL);
> +	if (!cxt->rmmap)
> +		goto end;
> +
>   	cxt->usedmap = kcalloc(longcnt, sizeof(long), GFP_KERNEL);
> +	if (!cxt->usedmap)
> +		goto free_rmmap;
>   
>   	longcnt = BITS_TO_LONGS(div_u64(mtd->size, mtd->erasesize));
>   	cxt->badmap = kcalloc(longcnt, sizeof(long), GFP_KERNEL);
> +	if (!cxt->badmap)
> +		goto free_usedmap;

Could you add a single 'if' for all of them here instead? And goto 
single free.

>   	/* just support dmesg right now */
>   	cxt->dev.flags = PSTORE_FLAGS_DMESG;
> @@ -435,10 +442,20 @@ static void mtdpstore_notify_add(struct mtd_info *mtd)
>   	if (ret) {
>   		dev_err(&mtd->dev, "mtd%d register to psblk failed\n",
>   				mtd->index);
> -		return;
> +		goto free_badmap;
>   	}
>   	cxt->mtd = mtd;
>   	dev_info(&mtd->dev, "Attached to MTD device %d\n", mtd->index);
> +	goto end;
> +

And:

free:
> +	kfree(cxt->badmap);
> +	kfree(cxt->usedmap);
> +	kfree(cxt->rmmap);

And NULL them as Christophe suggests.

>   }
>   
>   static int mtdpstore_flush_removed_do(struct mtdpstore_context *cxt,

-- 
js
suse labs

Re: [PATCH v2] mtd: Add check and kfree() for kcalloc()

Posted by Christophe JAILLET 1 year ago

Le 04/02/2025 à 03:33, Jiasheng Jiang a écrit :
> Add a check for kcalloc() to ensure successful allocation.
> Moreover, add kfree() in the error-handling path to prevent memory leaks.
> 
> Fixes: 78c08247b9d3 ("mtd: Support kmsg dumper based on pstore/blk")
> Cc: <stable@vger.kernel.org> # v5.10+
> Signed-off-by: Jiasheng Jiang <jiashengjiangcool@gmail.com>
> ---
> Changelog:
> 
> v1 -> v2:
> 
> 1. Remove redundant logging.
> 2. Add kfree() in the error-handling path.
> ---
>   drivers/mtd/mtdpstore.c | 19 ++++++++++++++++++-
>   1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/mtdpstore.c b/drivers/mtd/mtdpstore.c
> index 7ac8ac901306..2d8e330dd215 100644
> --- a/drivers/mtd/mtdpstore.c
> +++ b/drivers/mtd/mtdpstore.c
> @@ -418,10 +418,17 @@ static void mtdpstore_notify_add(struct mtd_info *mtd)
>   
>   	longcnt = BITS_TO_LONGS(div_u64(mtd->size, info->kmsg_size));
>   	cxt->rmmap = kcalloc(longcnt, sizeof(long), GFP_KERNEL);
> +	if (!cxt->rmmap)
> +		goto end;

Nitpick: Could be a direct return.

> +
>   	cxt->usedmap = kcalloc(longcnt, sizeof(long), GFP_KERNEL);
> +	if (!cxt->usedmap)
> +		goto free_rmmap;
>   
>   	longcnt = BITS_TO_LONGS(div_u64(mtd->size, mtd->erasesize));
>   	cxt->badmap = kcalloc(longcnt, sizeof(long), GFP_KERNEL);
> +	if (!cxt->badmap)
> +		goto free_usedmap;
>   
>   	/* just support dmesg right now */
>   	cxt->dev.flags = PSTORE_FLAGS_DMESG;
> @@ -435,10 +442,20 @@ static void mtdpstore_notify_add(struct mtd_info *mtd)
>   	if (ret) {
>   		dev_err(&mtd->dev, "mtd%d register to psblk failed\n",
>   				mtd->index);
> -		return;
> +		goto free_badmap;
>   	}
>   	cxt->mtd = mtd;
>   	dev_info(&mtd->dev, "Attached to MTD device %d\n", mtd->index);
> +	goto end;

Mater of taste, but I think that having an explicit return here would be 
clearer that a goto end;

> +
> +free_badmap:
> +	kfree(cxt->badmap);
> +free_usedmap:
> +	kfree(cxt->usedmap);
> +free_rmmap:
> +	kfree(cxt->rmmap);

I think that in all these paths, you should also have
	cxt->XXXmap = NULL;
after the kfree().

otherwise when mtdpstore_notify_remove() is called, you could have a 
double free.

CJ

> +end:
> +	return;
>   }
>   
>   static int mtdpstore_flush_removed_do(struct mtdpstore_context *cxt,

Re: [PATCH v2] mtd: Add check and kfree() for kcalloc()

Posted by Jiri Slaby 1 year ago

On 04. 02. 25, 7:17, Christophe JAILLET wrote:
> Le 04/02/2025 à 03:33, Jiasheng Jiang a écrit :
>> Add a check for kcalloc() to ensure successful allocation.
>> Moreover, add kfree() in the error-handling path to prevent memory leaks.
>>
>> Fixes: 78c08247b9d3 ("mtd: Support kmsg dumper based on pstore/blk")
>> Cc: <stable@vger.kernel.org> # v5.10+
>> Signed-off-by: Jiasheng Jiang <jiashengjiangcool@gmail.com>
>> ---
>> Changelog:
>>
>> v1 -> v2:
>>
>> 1. Remove redundant logging.
>> 2. Add kfree() in the error-handling path.
>> ---
>>   drivers/mtd/mtdpstore.c | 19 ++++++++++++++++++-
>>   1 file changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/mtdpstore.c b/drivers/mtd/mtdpstore.c
>> index 7ac8ac901306..2d8e330dd215 100644
>> --- a/drivers/mtd/mtdpstore.c
>> +++ b/drivers/mtd/mtdpstore.c
>> @@ -418,10 +418,17 @@ static void mtdpstore_notify_add(struct mtd_info 
>> *mtd)
>>       longcnt = BITS_TO_LONGS(div_u64(mtd->size, info->kmsg_size));
>>       cxt->rmmap = kcalloc(longcnt, sizeof(long), GFP_KERNEL);
>> +    if (!cxt->rmmap)
>> +        goto end;
> 
> Nitpick: Could be a direct return.
> 
>> +
>>       cxt->usedmap = kcalloc(longcnt, sizeof(long), GFP_KERNEL);
>> +    if (!cxt->usedmap)
>> +        goto free_rmmap;
>>       longcnt = BITS_TO_LONGS(div_u64(mtd->size, mtd->erasesize));
>>       cxt->badmap = kcalloc(longcnt, sizeof(long), GFP_KERNEL);
>> +    if (!cxt->badmap)
>> +        goto free_usedmap;
>>       /* just support dmesg right now */
>>       cxt->dev.flags = PSTORE_FLAGS_DMESG;
>> @@ -435,10 +442,20 @@ static void mtdpstore_notify_add(struct mtd_info 
>> *mtd)
>>       if (ret) {
>>           dev_err(&mtd->dev, "mtd%d register to psblk failed\n",
>>                   mtd->index);
>> -        return;
>> +        goto free_badmap;
>>       }
>>       cxt->mtd = mtd;
>>       dev_info(&mtd->dev, "Attached to MTD device %d\n", mtd->index);
>> +    goto end;
> 
> Mater of taste, but I think that having an explicit return here would be 
> clearer that a goto end;

Yes, drop the whole end.

>> +free_badmap:
>> +    kfree(cxt->badmap);
>> +free_usedmap:
>> +    kfree(cxt->usedmap);
>> +free_rmmap:
>> +    kfree(cxt->rmmap);
> 
> I think that in all these paths, you should also have
>      cxt->XXXmap = NULL;
> after the kfree().
> 
> otherwise when mtdpstore_notify_remove() is called, you could have a 
> double free.

Right, and this is already a problem for failing 
register_pstore_device() in _add() -- there is unconditional 
unregister_pstore_device() in _remove(). Should _remove() check cxt->mtd 
first?

thanks,
-- 
js
suse labs

Re: [PATCH v2] mtd: Add check and kfree() for kcalloc()

Posted by Christophe JAILLET 1 year ago

Le 04/02/2025 à 07:36, Jiri Slaby a écrit :
> On 04. 02. 25, 7:17, Christophe JAILLET wrote:
>> Le 04/02/2025 à 03:33, Jiasheng Jiang a écrit :
>>> Add a check for kcalloc() to ensure successful allocation.
>>> Moreover, add kfree() in the error-handling path to prevent memory 
>>> leaks.
>>>
>>> Fixes: 78c08247b9d3 ("mtd: Support kmsg dumper based on pstore/blk")
>>> Cc: <stable@vger.kernel.org> # v5.10+
>>> Signed-off-by: Jiasheng Jiang <jiashengjiangcool@gmail.com>
>>> ---
>>> Changelog:
>>>
>>> v1 -> v2:
>>>
>>> 1. Remove redundant logging.
>>> 2. Add kfree() in the error-handling path.
>>> ---
>>>   drivers/mtd/mtdpstore.c | 19 ++++++++++++++++++-
>>>   1 file changed, 18 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mtd/mtdpstore.c b/drivers/mtd/mtdpstore.c
>>> index 7ac8ac901306..2d8e330dd215 100644
>>> --- a/drivers/mtd/mtdpstore.c
>>> +++ b/drivers/mtd/mtdpstore.c
>>> @@ -418,10 +418,17 @@ static void mtdpstore_notify_add(struct 
>>> mtd_info *mtd)
>>>       longcnt = BITS_TO_LONGS(div_u64(mtd->size, info->kmsg_size));
>>>       cxt->rmmap = kcalloc(longcnt, sizeof(long), GFP_KERNEL);
>>> +    if (!cxt->rmmap)
>>> +        goto end;
>>
>> Nitpick: Could be a direct return.
>>
>>> +
>>>       cxt->usedmap = kcalloc(longcnt, sizeof(long), GFP_KERNEL);
>>> +    if (!cxt->usedmap)
>>> +        goto free_rmmap;
>>>       longcnt = BITS_TO_LONGS(div_u64(mtd->size, mtd->erasesize));
>>>       cxt->badmap = kcalloc(longcnt, sizeof(long), GFP_KERNEL);
>>> +    if (!cxt->badmap)
>>> +        goto free_usedmap;
>>>       /* just support dmesg right now */
>>>       cxt->dev.flags = PSTORE_FLAGS_DMESG;
>>> @@ -435,10 +442,20 @@ static void mtdpstore_notify_add(struct 
>>> mtd_info *mtd)
>>>       if (ret) {
>>>           dev_err(&mtd->dev, "mtd%d register to psblk failed\n",
>>>                   mtd->index);
>>> -        return;
>>> +        goto free_badmap;
>>>       }
>>>       cxt->mtd = mtd;
>>>       dev_info(&mtd->dev, "Attached to MTD device %d\n", mtd->index);
>>> +    goto end;
>>
>> Mater of taste, but I think that having an explicit return here would 
>> be clearer that a goto end;
> 
> Yes, drop the whole end.
> 
>>> +free_badmap:
>>> +    kfree(cxt->badmap);
>>> +free_usedmap:
>>> +    kfree(cxt->usedmap);
>>> +free_rmmap:
>>> +    kfree(cxt->rmmap);
>>
>> I think that in all these paths, you should also have
>>      cxt->XXXmap = NULL;
>> after the kfree().
>>
>> otherwise when mtdpstore_notify_remove() is called, you could have a 
>> double free.
> 
> Right, and this is already a problem for failing 
> register_pstore_device() in _add() -- there is unconditional 
> unregister_pstore_device() in _remove(). Should _remove() check cxt->mtd 
> first?

Not sure it is needed.
IIUC, [1] would not match in this case, because [2] would not have been 
executed. Agreed?

CJ


[1]: https://elixir.bootlin.com/linux/v6.13/source/fs/pstore/blk.c#L169
[2]: https://elixir.bootlin.com/linux/v6.13/source/fs/pstore/blk.c#L141

> 
> thanks,

Re: [PATCH v2] mtd: Add check and kfree() for kcalloc()

Posted by Jiasheng Jiang 1 year ago

Hi Christophe,

On Tue, Feb 4, 2025 at 3:50 PM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
>
> Le 04/02/2025 à 07:36, Jiri Slaby a écrit :
> > On 04. 02. 25, 7:17, Christophe JAILLET wrote:
> >> Le 04/02/2025 à 03:33, Jiasheng Jiang a écrit :
> >>> Add a check for kcalloc() to ensure successful allocation.
> >>> Moreover, add kfree() in the error-handling path to prevent memory
> >>> leaks.
> >>>
> >>> Fixes: 78c08247b9d3 ("mtd: Support kmsg dumper based on pstore/blk")
> >>> Cc: <stable@vger.kernel.org> # v5.10+
> >>> Signed-off-by: Jiasheng Jiang <jiashengjiangcool@gmail.com>
> >>> ---
> >>> Changelog:
> >>>
> >>> v1 -> v2:
> >>>
> >>> 1. Remove redundant logging.
> >>> 2. Add kfree() in the error-handling path.
> >>> ---
> >>>   drivers/mtd/mtdpstore.c | 19 ++++++++++++++++++-
> >>>   1 file changed, 18 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/mtd/mtdpstore.c b/drivers/mtd/mtdpstore.c
> >>> index 7ac8ac901306..2d8e330dd215 100644
> >>> --- a/drivers/mtd/mtdpstore.c
> >>> +++ b/drivers/mtd/mtdpstore.c
> >>> @@ -418,10 +418,17 @@ static void mtdpstore_notify_add(struct
> >>> mtd_info *mtd)
> >>>       longcnt = BITS_TO_LONGS(div_u64(mtd->size, info->kmsg_size));
> >>>       cxt->rmmap = kcalloc(longcnt, sizeof(long), GFP_KERNEL);
> >>> +    if (!cxt->rmmap)
> >>> +        goto end;
> >>
> >> Nitpick: Could be a direct return.
> >>
> >>> +
> >>>       cxt->usedmap = kcalloc(longcnt, sizeof(long), GFP_KERNEL);
> >>> +    if (!cxt->usedmap)
> >>> +        goto free_rmmap;
> >>>       longcnt = BITS_TO_LONGS(div_u64(mtd->size, mtd->erasesize));
> >>>       cxt->badmap = kcalloc(longcnt, sizeof(long), GFP_KERNEL);
> >>> +    if (!cxt->badmap)
> >>> +        goto free_usedmap;
> >>>       /* just support dmesg right now */
> >>>       cxt->dev.flags = PSTORE_FLAGS_DMESG;
> >>> @@ -435,10 +442,20 @@ static void mtdpstore_notify_add(struct
> >>> mtd_info *mtd)
> >>>       if (ret) {
> >>>           dev_err(&mtd->dev, "mtd%d register to psblk failed\n",
> >>>                   mtd->index);
> >>> -        return;
> >>> +        goto free_badmap;
> >>>       }
> >>>       cxt->mtd = mtd;
> >>>       dev_info(&mtd->dev, "Attached to MTD device %d\n", mtd->index);
> >>> +    goto end;
> >>
> >> Mater of taste, but I think that having an explicit return here would
> >> be clearer that a goto end;
> >
> > Yes, drop the whole end.
> >
> >>> +free_badmap:
> >>> +    kfree(cxt->badmap);
> >>> +free_usedmap:
> >>> +    kfree(cxt->usedmap);
> >>> +free_rmmap:
> >>> +    kfree(cxt->rmmap);
> >>
> >> I think that in all these paths, you should also have
> >>      cxt->XXXmap = NULL;
> >> after the kfree().
> >>
> >> otherwise when mtdpstore_notify_remove() is called, you could have a
> >> double free.
> >
> > Right, and this is already a problem for failing
> > register_pstore_device() in _add() -- there is unconditional
> > unregister_pstore_device() in _remove(). Should _remove() check cxt->mtd
> > first?
>
> Not sure it is needed.
> IIUC, [1] would not match in this case, because [2] would not have been
> executed. Agreed?

Thanks for your advice.
I have submitted a v3 to replace kcalloc() with devm_kcalloc() to
prevent memory leaks.
Moreover, I moved the return value check to another patch, so that
each patch does only one thing.

-Jiasheng

>
> CJ
>
>
> [1]: https://elixir.bootlin.com/linux/v6.13/source/fs/pstore/blk.c#L169
> [2]: https://elixir.bootlin.com/linux/v6.13/source/fs/pstore/blk.c#L141
>
> >
> > thanks,
>

[PATCH v3 1/2] mtd: Replace kcalloc() with devm_kcalloc()

Posted by Jiasheng Jiang 1 year ago

Replace kcalloc() with devm_kcalloc() to prevent memory leaks in case of
errors.

Fixes: 78c08247b9d3 ("mtd: Support kmsg dumper based on pstore/blk")
Cc: <stable@vger.kernel.org> # v5.10+
Signed-off-by: Jiasheng Jiang <jiashengjiangcool@gmail.com>
---
Changelog:

v2 -> v3:

1. Replace kcalloc() with devm_kcalloc().
2. Remove kfree().
3. Remove checks.

v1 -> v2:

1. Remove redundant logging.
2. Add kfree() in the error-handling path.
---
 drivers/mtd/mtdpstore.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/mtdpstore.c b/drivers/mtd/mtdpstore.c
index 7ac8ac901306..2d004d41cf75 100644
--- a/drivers/mtd/mtdpstore.c
+++ b/drivers/mtd/mtdpstore.c
@@ -417,11 +417,11 @@ static void mtdpstore_notify_add(struct mtd_info *mtd)
 	}
 
 	longcnt = BITS_TO_LONGS(div_u64(mtd->size, info->kmsg_size));
-	cxt->rmmap = kcalloc(longcnt, sizeof(long), GFP_KERNEL);
-	cxt->usedmap = kcalloc(longcnt, sizeof(long), GFP_KERNEL);
+	cxt->rmmap = devm_kcalloc(&mtd->dev, longcnt, sizeof(long), GFP_KERNEL);
+	cxt->usedmap = devm_kcalloc(&mtd->dev, longcnt, sizeof(long), GFP_KERNEL);
 
 	longcnt = BITS_TO_LONGS(div_u64(mtd->size, mtd->erasesize));
-	cxt->badmap = kcalloc(longcnt, sizeof(long), GFP_KERNEL);
+	cxt->badmap = devm_kcalloc(&mtd->dev, longcnt, sizeof(long), GFP_KERNEL);
 
 	/* just support dmesg right now */
 	cxt->dev.flags = PSTORE_FLAGS_DMESG;
@@ -527,9 +527,6 @@ static void mtdpstore_notify_remove(struct mtd_info *mtd)
 	mtdpstore_flush_removed(cxt);
 
 	unregister_pstore_device(&cxt->dev);
-	kfree(cxt->badmap);
-	kfree(cxt->usedmap);
-	kfree(cxt->rmmap);
 	cxt->mtd = NULL;
 	cxt->index = -1;
 }
-- 
2.25.1

Re: [PATCH v3 1/2] mtd: Replace kcalloc() with devm_kcalloc()

Posted by Miquel Raynal 1 year ago

On Wed, 05 Feb 2025 02:31:40 +0000, Jiasheng Jiang wrote:
> Replace kcalloc() with devm_kcalloc() to prevent memory leaks in case of
> errors.
> 
> 

Applied to mtd/next, thanks!

[1/2] mtd: Replace kcalloc() with devm_kcalloc()
      commit: c76f83f2e834101660090406ba925526d5f27c07
[2/2] mtd: Add check for devm_kcalloc()
      commit: d132814fd6fd2ecb3618577f266611ca10d67611

Patche(s) should be available on mtd/linux.git and will be
part of the next PR (provided that no robot complains by then).

Kind regards,
Miquèl

[PATCH v3 2/2] mtd: Add check for devm_kcalloc()

Posted by Jiasheng Jiang 1 year ago

Add a check for devm_kcalloc() to ensure successful allocation.

Fixes: 78c08247b9d3 ("mtd: Support kmsg dumper based on pstore/blk")
Cc: <stable@vger.kernel.org> # v5.10+
Signed-off-by: Jiasheng Jiang <jiashengjiangcool@gmail.com>
---
Changelog:

v2 -> v3:

1. No change.

v1 -> v2:

1. No change.
---
 drivers/mtd/mtdpstore.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mtd/mtdpstore.c b/drivers/mtd/mtdpstore.c
index 2d004d41cf75..9cf3872e37ae 100644
--- a/drivers/mtd/mtdpstore.c
+++ b/drivers/mtd/mtdpstore.c
@@ -423,6 +423,9 @@ static void mtdpstore_notify_add(struct mtd_info *mtd)
 	longcnt = BITS_TO_LONGS(div_u64(mtd->size, mtd->erasesize));
 	cxt->badmap = devm_kcalloc(&mtd->dev, longcnt, sizeof(long), GFP_KERNEL);
 
+	if (!cxt->rmmap || !cxt->usedmap || !cxt->badmap)
+		return;
+
 	/* just support dmesg right now */
 	cxt->dev.flags = PSTORE_FLAGS_DMESG;
 	cxt->dev.zone.read = mtdpstore_read;
-- 
2.25.1