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

[Jiri Slaby]

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@xxxxxxxxxxxxxxx> # v5.10+
> > Signed-off-by: Jiasheng Jiang <jiashengjiangcool@xxxxxxxxx>
> > ---
> > 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