* [PATCH v2] mtd: Add check and kfree() for kcalloc() [not found] <30ad77af-4a7b-4a15-9c0b-b0c70d9e1643@wanadoo.fr> @ 2025-02-04 2:33 ` Jiasheng Jiang 2025-02-04 6:17 ` Christophe JAILLET ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Jiasheng Jiang @ 2025-02-04 2:33 UTC (permalink / raw) To: christophe.jaillet Cc: gmpy.liaowx, jiashengjiangcool, kees, linux-kernel, linux-mtd, miquel.raynal, richard, vigneshr, stable 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] mtd: Add check and kfree() for kcalloc() 2025-02-04 2:33 ` [PATCH v2] mtd: Add check and kfree() for kcalloc() Jiasheng Jiang @ 2025-02-04 6:17 ` Christophe JAILLET 2025-02-04 6:36 ` Jiri Slaby 2025-02-04 6:32 ` Jiri Slaby 2025-02-04 9:17 ` Miquel Raynal 2 siblings, 1 reply; 10+ messages in thread From: Christophe JAILLET @ 2025-02-04 6:17 UTC (permalink / raw) To: Jiasheng Jiang Cc: gmpy.liaowx, kees, linux-kernel, linux-mtd, miquel.raynal, richard, vigneshr, stable 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, ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] mtd: Add check and kfree() for kcalloc() 2025-02-04 6:17 ` Christophe JAILLET @ 2025-02-04 6:36 ` Jiri Slaby 2025-02-04 20:50 ` Christophe JAILLET 0 siblings, 1 reply; 10+ messages in thread From: Jiri Slaby @ 2025-02-04 6:36 UTC (permalink / raw) To: Christophe JAILLET, Jiasheng Jiang Cc: gmpy.liaowx, kees, linux-kernel, linux-mtd, miquel.raynal, richard, vigneshr, stable 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] mtd: Add check and kfree() for kcalloc() 2025-02-04 6:36 ` Jiri Slaby @ 2025-02-04 20:50 ` Christophe JAILLET 2025-02-05 2:31 ` [PATCH v3 1/2] mtd: Replace kcalloc() with devm_kcalloc() Jiasheng Jiang 2025-02-05 2:35 ` [PATCH v2] mtd: Add check and kfree() for kcalloc() Jiasheng Jiang 0 siblings, 2 replies; 10+ messages in thread From: Christophe JAILLET @ 2025-02-04 20:50 UTC (permalink / raw) To: Jiri Slaby, Jiasheng Jiang Cc: gmpy.liaowx, kees, linux-kernel, linux-mtd, miquel.raynal, richard, vigneshr, stable 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, ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 1/2] mtd: Replace kcalloc() with devm_kcalloc() 2025-02-04 20:50 ` Christophe JAILLET @ 2025-02-05 2:31 ` Jiasheng Jiang 2025-02-05 2:31 ` [PATCH v3 2/2] mtd: Add check for devm_kcalloc() Jiasheng Jiang 2025-02-07 14:46 ` [PATCH v3 1/2] mtd: Replace kcalloc() with devm_kcalloc() Miquel Raynal 2025-02-05 2:35 ` [PATCH v2] mtd: Add check and kfree() for kcalloc() Jiasheng Jiang 1 sibling, 2 replies; 10+ messages in thread From: Jiasheng Jiang @ 2025-02-05 2:31 UTC (permalink / raw) To: christophe.jaillet Cc: gmpy.liaowx, jiashengjiangcool, jirislaby, kees, linux-kernel, linux-mtd, miquel.raynal, richard, stable, vigneshr 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v3 2/2] mtd: Add check for devm_kcalloc() 2025-02-05 2:31 ` [PATCH v3 1/2] mtd: Replace kcalloc() with devm_kcalloc() Jiasheng Jiang @ 2025-02-05 2:31 ` Jiasheng Jiang 2025-02-07 14:46 ` [PATCH v3 1/2] mtd: Replace kcalloc() with devm_kcalloc() Miquel Raynal 1 sibling, 0 replies; 10+ messages in thread From: Jiasheng Jiang @ 2025-02-05 2:31 UTC (permalink / raw) To: christophe.jaillet Cc: gmpy.liaowx, jiashengjiangcool, jirislaby, kees, linux-kernel, linux-mtd, miquel.raynal, richard, stable, vigneshr 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/2] mtd: Replace kcalloc() with devm_kcalloc() 2025-02-05 2:31 ` [PATCH v3 1/2] mtd: Replace kcalloc() with devm_kcalloc() Jiasheng Jiang 2025-02-05 2:31 ` [PATCH v3 2/2] mtd: Add check for devm_kcalloc() Jiasheng Jiang @ 2025-02-07 14:46 ` Miquel Raynal 1 sibling, 0 replies; 10+ messages in thread From: Miquel Raynal @ 2025-02-07 14:46 UTC (permalink / raw) To: christophe.jaillet, Jiasheng Jiang Cc: gmpy.liaowx, jirislaby, kees, linux-kernel, linux-mtd, richard, stable, vigneshr 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] mtd: Add check and kfree() for kcalloc() 2025-02-04 20:50 ` Christophe JAILLET 2025-02-05 2:31 ` [PATCH v3 1/2] mtd: Replace kcalloc() with devm_kcalloc() Jiasheng Jiang @ 2025-02-05 2:35 ` Jiasheng Jiang 1 sibling, 0 replies; 10+ messages in thread From: Jiasheng Jiang @ 2025-02-05 2:35 UTC (permalink / raw) To: Christophe JAILLET Cc: Jiri Slaby, gmpy.liaowx, kees, linux-kernel, linux-mtd, miquel.raynal, richard, vigneshr, stable 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, > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] mtd: Add check and kfree() for kcalloc() 2025-02-04 2:33 ` [PATCH v2] mtd: Add check and kfree() for kcalloc() Jiasheng Jiang 2025-02-04 6:17 ` Christophe JAILLET @ 2025-02-04 6:32 ` Jiri Slaby 2025-02-04 9:17 ` Miquel Raynal 2 siblings, 0 replies; 10+ messages in thread From: Jiri Slaby @ 2025-02-04 6:32 UTC (permalink / raw) To: Jiasheng Jiang, christophe.jaillet Cc: gmpy.liaowx, kees, linux-kernel, linux-mtd, miquel.raynal, richard, vigneshr, stable 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] mtd: Add check and kfree() for kcalloc() 2025-02-04 2:33 ` [PATCH v2] mtd: Add check and kfree() for kcalloc() Jiasheng Jiang 2025-02-04 6:17 ` Christophe JAILLET 2025-02-04 6:32 ` Jiri Slaby @ 2025-02-04 9:17 ` Miquel Raynal 2 siblings, 0 replies; 10+ messages in thread From: Miquel Raynal @ 2025-02-04 9:17 UTC (permalink / raw) To: Jiasheng Jiang Cc: christophe.jaillet, gmpy.liaowx, kees, linux-kernel, linux-mtd, richard, vigneshr, stable 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2025-02-07 14:46 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <30ad77af-4a7b-4a15-9c0b-b0c70d9e1643@wanadoo.fr>
2025-02-04 2:33 ` [PATCH v2] mtd: Add check and kfree() for kcalloc() Jiasheng Jiang
2025-02-04 6:17 ` Christophe JAILLET
2025-02-04 6:36 ` Jiri Slaby
2025-02-04 20:50 ` Christophe JAILLET
2025-02-05 2:31 ` [PATCH v3 1/2] mtd: Replace kcalloc() with devm_kcalloc() Jiasheng Jiang
2025-02-05 2:31 ` [PATCH v3 2/2] mtd: Add check for devm_kcalloc() Jiasheng Jiang
2025-02-07 14:46 ` [PATCH v3 1/2] mtd: Replace kcalloc() with devm_kcalloc() Miquel Raynal
2025-02-05 2:35 ` [PATCH v2] mtd: Add check and kfree() for kcalloc() Jiasheng Jiang
2025-02-04 6:32 ` Jiri Slaby
2025-02-04 9:17 ` Miquel Raynal
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).