From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 21 Mar 2023 20:13:51 +0100

The label “setup_err” was used to jump to another pointer check despite of
the detail in the implementation of the function “cfi_intelext_setup”
that it was determined already that a corresponding variable contained
a null pointer because of a failed memory allocation.

* Thus use more appropriate labels instead.

* Delete a redundant check.


This issue was detected by using the Coccinelle software.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/mtd/chips/cfi_cmdset_0001.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0001.c b/drivers/mtd/chips/cfi_cmdset_0001.c
index 54f92d09d9cf..a06318cd5ea4 100644
--- a/drivers/mtd/chips/cfi_cmdset_0001.c
+++ b/drivers/mtd/chips/cfi_cmdset_0001.c
@@ -614,7 +614,7 @@ static struct mtd_info *cfi_intelext_setup(struct mtd_info *mtd)
 				    sizeof(struct mtd_erase_region_info),
 				    GFP_KERNEL);
 	if (!mtd->eraseregions)
-		goto setup_err;
+		goto free_mtd;

 	for (i=0; i<cfi->cfiq->NumEraseRegions; i++) {
 		unsigned long ernum, ersize;
@@ -630,7 +630,7 @@ static struct mtd_info *cfi_intelext_setup(struct mtd_info *mtd)
 			mtd->eraseregions[(j*cfi->cfiq->NumEraseRegions)+i].numblocks = ernum;
 			mtd->eraseregions[(j*cfi->cfiq->NumEraseRegions)+i].lockmap = kmalloc(ernum / 8 + 1, GFP_KERNEL);
 			if (!mtd->eraseregions[(j*cfi->cfiq->NumEraseRegions)+i].lockmap)
-				goto setup_err;
+				goto release_loop;
 		}
 		offset += (ersize * ernum);
 	}
@@ -638,7 +638,7 @@ static struct mtd_info *cfi_intelext_setup(struct mtd_info *mtd)
 	if (offset != devsize) {
 		/* Argh */
 		printk(KERN_WARNING "Sum of regions (%lx) != total size of set of interleaved chips (%lx)\n", offset, devsize);
-		goto setup_err;
+		goto release_loop;
 	}

 	for (i=0; i<mtd->numeraseregions;i++){
@@ -660,18 +660,18 @@ static struct mtd_info *cfi_intelext_setup(struct mtd_info *mtd)
 	/* This function has the potential to distort the reality
 	   a bit and therefore should be called last. */
 	if (cfi_intelext_partition_fixup(mtd, &cfi) != 0)
-		goto setup_err;
+		goto release_loop;

 	__module_get(THIS_MODULE);
 	register_reboot_notifier(&mtd->reboot_notifier);
 	return mtd;

- setup_err:
-	if (mtd->eraseregions)
-		for (i=0; i<cfi->cfiq->NumEraseRegions; i++)
-			for (j=0; j<cfi->numchips; j++)
-				kfree(mtd->eraseregions[(j*cfi->cfiq->NumEraseRegions)+i].lockmap);
+release_loop:
+	for (i=0; i<cfi->cfiq->NumEraseRegions; i++)
+		for (j=0; j<cfi->numchips; j++)
+			kfree(mtd->eraseregions[(j*cfi->cfiq->NumEraseRegions)+i].lockmap);
 	kfree(mtd->eraseregions);
+free_mtd:
 	kfree(mtd);
 	kfree(cfi->cmdset_priv);
 	return NULL;
--
2.40.0

On Tue, Mar 04, 2025 at 08:21:53PM +0100, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 21 Mar 2023 20:13:51 +0100
> 
> The label “setup_err” was used to jump to another pointer check despite of
> the detail in the implementation of the function “cfi_intelext_setup”
> that it was determined already that a corresponding variable contained
> a null pointer because of a failed memory allocation.
> 
> * Thus use more appropriate labels instead.
> 
> * Delete a redundant check.
> 
> 
> This issue was detected by using the Coccinelle software.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Don't use a Fixes tag for cleanups.

regards,
dan carpenter

Hi,

On 04/03/2025 at 20:21:53 +01, Markus Elfring <Markus.Elfring@web.de> wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 21 Mar 2023 20:13:51 +0100
>
> The label “setup_err” was used to jump to another pointer check despite of
> the detail in the implementation of the function “cfi_intelext_setup”
> that it was determined already that a corresponding variable contained
> a null pointer because of a failed memory allocation.

Can you please rephrase the commit log? It is super hard to understand.

Thanks,
Miquèl

>> The label “setup_err” was used to jump to another pointer check despite of
>> the detail in the implementation of the function “cfi_intelext_setup”
>> that it was determined already that a corresponding variable contained
>> a null pointer because of a failed memory allocation.
>
> Can you please rephrase the commit log? It is super hard to understand.
Which wording variant would you find nicer for provided information?

* used jump targets

* null pointer assignments


Regards,
Markus