[PATCH 0/4] mtd: cleanups and small bug fixes for init_msp_flash()

[PATCH 0/4] mtd: cleanups and small bug fixes for init_msp_flash()

[Jesper Juhl]

Hi,

Here's a small series of patches to do a little cleanup + bug fixing
in drivers/mtd/maps/pmcmsp-flash.c::init_msp_flash().

The patches in this series are

 [PATCH 1/4] mtd: Don't cast kmalloc() return value in drivers/mtd/maps/pmcmsp-flash.c
 [PATCH 2/4] mtd: convert some kmalloc()+memset() calls to kcalloc() in drivers/mtd/maps/pmcmsp-flash.c
 [PATCH 3/4] mtd: Fix a potential NULL ptr deref bug and mem leak in init_msp_flash()
 [PATCH 4/4] mtd: Check for allocation failures and bail out appropriately in init_msp_flash()

Some of the patches touch the same line of code multiple times, but 
they are split this way to 
 a) to keep logical changes seperate and 
 b) to make the patch series 'git bisect' friendly 

There's still more that could be done to this function, such as 
rolling back mem allocations for devices already initialized in 
case a subsequent one fails or similar, but that will have to wait.
This is all I have time for at the moment.

Please consider merging :-)


Kind regards,
  Jesper Juhl <jesper.juhl@gmail.com>

[PATCH 1/4] mtd: Don't cast kmalloc() return value in drivers/mtd/maps/pmcmsp-flash.c

[Jesper Juhl]

    mtd: Don't cast kmalloc() return value in drivers/mtd/maps/pmcmsp-flash.c
    
    kmalloc() returns a void pointer.
    No need to cast it.
    
    Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
---
 drivers/mtd/maps/pmcmsp-flash.c |   13 +++++--------
 1 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/maps/pmcmsp-flash.c b/drivers/mtd/maps/pmcmsp-flash.c
index 7e0377e..dfdb120 100644
--- a/drivers/mtd/maps/pmcmsp-flash.c
+++ b/drivers/mtd/maps/pmcmsp-flash.c
@@ -73,12 +73,9 @@ int __init init_msp_flash(void)
 		return -ENXIO;
 
 	printk(KERN_NOTICE "Found %d PMC flash devices\n", fcnt);
-	msp_flash = (struct mtd_info **)kmalloc(
-			fcnt * sizeof(struct map_info *), GFP_KERNEL);
-	msp_parts = (struct mtd_partition **)kmalloc(
-			fcnt * sizeof(struct mtd_partition *), GFP_KERNEL);
-	msp_maps = (struct map_info *)kmalloc(
-			fcnt * sizeof(struct mtd_info), GFP_KERNEL);
+	msp_flash = kmalloc(fcnt * sizeof(struct map_info *), GFP_KERNEL);
+	msp_parts = kmalloc(fcnt * sizeof(struct mtd_partition *), GFP_KERNEL);
+	msp_maps = kmalloc(fcnt * sizeof(struct mtd_info), GFP_KERNEL);
 	memset(msp_maps, 0, fcnt * sizeof(struct mtd_info));
 
 	/* loop over the flash devices, initializing each */
@@ -95,8 +92,8 @@ int __init init_msp_flash(void)
 			continue;
 		}
 
-		msp_parts[i] = (struct mtd_partition *)kmalloc(
-			pcnt * sizeof(struct mtd_partition), GFP_KERNEL);
+		msp_parts[i] = kmalloc(pcnt * sizeof(struct mtd_partition),
+			GFP_KERNEL);
 		memset(msp_parts[i], 0, pcnt * sizeof(struct mtd_partition));
 
 		/* now initialize the devices proper */

[PATCH 2/4] mtd: convert some kmalloc()+memset() calls to kcalloc() in drivers/mtd/maps/pmcmsp-flash.c

[Jesper Juhl]

    mtd: convert some kmalloc()+memset() calls to kcalloc() in drivers/mtd/maps/pmcmsp-flash.c
    
    No point in doing kmalloc() followed by memset() when we have
    kcalloc() at our disposal.
    
    Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
---
 drivers/mtd/maps/pmcmsp-flash.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/maps/pmcmsp-flash.c b/drivers/mtd/maps/pmcmsp-flash.c
index dfdb120..af72cdd 100644
--- a/drivers/mtd/maps/pmcmsp-flash.c
+++ b/drivers/mtd/maps/pmcmsp-flash.c
@@ -75,8 +75,7 @@ int __init init_msp_flash(void)
 	printk(KERN_NOTICE "Found %d PMC flash devices\n", fcnt);
 	msp_flash = kmalloc(fcnt * sizeof(struct map_info *), GFP_KERNEL);
 	msp_parts = kmalloc(fcnt * sizeof(struct mtd_partition *), GFP_KERNEL);
-	msp_maps = kmalloc(fcnt * sizeof(struct mtd_info), GFP_KERNEL);
-	memset(msp_maps, 0, fcnt * sizeof(struct mtd_info));
+	msp_maps = kcalloc(fcnt, sizeof(struct mtd_info), GFP_KERNEL);
 
 	/* loop over the flash devices, initializing each */
 	for (i = 0; i < fcnt; i++) {
@@ -92,9 +91,8 @@ int __init init_msp_flash(void)
 			continue;
 		}
 
-		msp_parts[i] = kmalloc(pcnt * sizeof(struct mtd_partition),
+		msp_parts[i] = kcalloc(pcnt, sizeof(struct mtd_partition),
 			GFP_KERNEL);
-		memset(msp_parts[i], 0, pcnt * sizeof(struct mtd_partition));
 
 		/* now initialize the devices proper */
 		flash_name[5] = '0' + i;

[PATCH 3/4] mtd: Fix a potential NULL ptr deref bug and mem leak in init_msp_flash()

[Jesper Juhl]

    mtd: Fix a potential NULL ptr deref bug and mem leak in init_msp_flash()
    
    In drivers/mtd/maps/pmcmsp-flash.c::init_msp_flash() there is
    currently this funky little piece of code:
    
           msp_maps[i].name = strncpy(kmalloc(7, GFP_KERNEL),
                                   flash_name, 7);
    
    If this (tiny) memory allocation should happen to fail, then
    strncpy() will result in bad things happening - much better to
    simply check the allocation and return a sensible error than crash
    the entire kernel on a NULL deref.
    
    While fixing the above I also reorganized some other lines of code
    in the neighbourhood. There is a nice little check of whether or
    not the ioremap() returns NULL, but the check happens after we have
    already done the kmalloc() described above, so in case it triggers
    it will cause us to leak the 7 bytes that kmalloc() allocated. This
    is easily avoidable by simply moving the check so that if ioremap()
    fails we don't even attempt to do the memory allocation.
    And while I was moving code around I also moved the setting of
    msp_maps[i].bankwidth to 1 below both the ioremap() and kmalloc() so
    that if we bail out due to either of them failing then we don't have
    to spend time doing that assignment - very unlikely to actually
    matter in real life, but it seemed such an obvious
    micro-optimization that I just figured I might as well do it.
    
    Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
---
 drivers/mtd/maps/pmcmsp-flash.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/maps/pmcmsp-flash.c b/drivers/mtd/maps/pmcmsp-flash.c
index af72cdd..b6d382a 100644
--- a/drivers/mtd/maps/pmcmsp-flash.c
+++ b/drivers/mtd/maps/pmcmsp-flash.c
@@ -115,14 +115,18 @@ int __init init_msp_flash(void)
 		 */
 		if (size > CONFIG_MSP_FLASH_MAP_LIMIT)
 			size = CONFIG_MSP_FLASH_MAP_LIMIT;
-		msp_maps[i].virt = ioremap(addr, size);
-		msp_maps[i].bankwidth = 1;
-		msp_maps[i].name = strncpy(kmalloc(7, GFP_KERNEL),
-					flash_name, 7);
 
+		msp_maps[i].virt = ioremap(addr, size);
 		if (msp_maps[i].virt == NULL)
 			return -ENXIO;
 
+		msp_maps[i].name = kmalloc(7, GFP_KERNEL);
+		if (msp_maps[i].name == NULL)
+			return -ENOMEM;
+		strncpy(msp_maps[i].name, flash_name, 7);
+
+		msp_maps[i].bankwidth = 1;
+
 		for (j = 0; j < pcnt; j++) {
 			part_name[5] = '0' + i;
 			part_name[7] = '0' + j;

[PATCH 4/4] mtd: Check for allocation failures and bail out appropriately in init_msp_flash()

[Jesper Juhl]

    mtd: Check for allocation failures and bail out appropriately in init_msp_flash()
    
    Just trusting that a memory allocation succeeds is a bad habbit that
    can lead to null pointer dereferences fairly fast.
    In drivers/mtd/maps/pmcmsp-flash.c::init_msp_flash() there are a few
    allocations where I don't see anything guaranteeing that they will
    never fail, yet they are not checked for success...
    This patch adds checks for these allocations and also cleans up
    previous allocations properly in case one fails.
    
    Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
---
 drivers/mtd/maps/pmcmsp-flash.c |   19 +++++++++++++++++--
 1 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/maps/pmcmsp-flash.c b/drivers/mtd/maps/pmcmsp-flash.c
index b6d382a..5472547 100644
--- a/drivers/mtd/maps/pmcmsp-flash.c
+++ b/drivers/mtd/maps/pmcmsp-flash.c
@@ -57,6 +57,7 @@ int __init init_msp_flash(void)
 	char flash_name[] = "flash0";
 	char part_name[] = "flash0_0";
 	unsigned addr, size;
+	int err = 0;
 
 	/* If ELB is disabled by "ful-mux" mode, we can't get at flash */
 	if ((*DEV_ID_REG & DEV_ID_SINGLE_PC) &&
@@ -74,8 +75,14 @@ int __init init_msp_flash(void)
 
 	printk(KERN_NOTICE "Found %d PMC flash devices\n", fcnt);
 	msp_flash = kmalloc(fcnt * sizeof(struct map_info *), GFP_KERNEL);
+	if (msp_flash == NULL)
+		goto out_mem;
 	msp_parts = kmalloc(fcnt * sizeof(struct mtd_partition *), GFP_KERNEL);
+	if (msp_parts == NULL)
+		goto out_mem_flash;
 	msp_maps = kcalloc(fcnt, sizeof(struct mtd_info), GFP_KERNEL);
+	if (msp_maps == NULL)
+		goto out_mem_parts;
 
 	/* loop over the flash devices, initializing each */
 	for (i = 0; i < fcnt; i++) {
@@ -122,7 +129,7 @@ int __init init_msp_flash(void)
 
 		msp_maps[i].name = kmalloc(7, GFP_KERNEL);
 		if (msp_maps[i].name == NULL)
-			return -ENOMEM;
+			goto out_mem;
 		strncpy(msp_maps[i].name, flash_name, 7);
 
 		msp_maps[i].bankwidth = 1;
@@ -153,7 +160,15 @@ int __init init_msp_flash(void)
 		}
 	}
 
-	return 0;
+out:
+	return err;
+out_mem_parts:
+	kfree(parts);
+out_mem_flash:
+	kfree(flash);
+out_mem:
+	err = -ENOMEM;
+	goto out;
 }
 
 static void __exit cleanup_msp_flash(void)

Re: [PATCH 1/4] mtd: Don't cast kmalloc() return value in drivers/mtd/maps/pmcmsp-flash.c

[Robert P. J. Day]

On Sun, 26 Aug 2007, Jesper Juhl wrote:

> +	msp_maps = kmalloc(fcnt * sizeof(struct mtd_info), GFP_KERNEL);
>  	memset(msp_maps, 0, fcnt * sizeof(struct mtd_info));

...

> +		msp_parts[i] = kmalloc(pcnt * sizeof(struct mtd_partition),
> +			GFP_KERNEL);
>  		memset(msp_parts[i], 0, pcnt * sizeof(struct mtd_partition));

again, both of the above could be done with a single call to kcalloc()
in each case.  why are you so philosophically opposed to doing it that
way?

rday
-- 
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://crashcourse.ca
========================================================================

Re: [PATCH 1/4] mtd: Don't cast kmalloc() return value in drivers/mtd/maps/pmcmsp-flash.c

[Jesper Juhl]

On 26/08/07, Robert P. J. Day <rpjday@mindspring.com> wrote:
> On Sun, 26 Aug 2007, Jesper Juhl wrote:
>
> > +     msp_maps = kmalloc(fcnt * sizeof(struct mtd_info), GFP_KERNEL);
> >       memset(msp_maps, 0, fcnt * sizeof(struct mtd_info));
>
> ...
>
> > +             msp_parts[i] = kmalloc(pcnt * sizeof(struct mtd_partition),
> > +                     GFP_KERNEL);
> >               memset(msp_parts[i], 0, pcnt * sizeof(struct mtd_partition));
>
> again, both of the above could be done with a single call to kcalloc()
> in each case.  why are you so philosophically opposed to doing it that
> way?
>
Check the second patch in the series, please.

-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [PATCH 1/4] mtd: Don't cast kmalloc() return value in drivers/mtd/maps/pmcmsp-flash.c

[Robert P. J. Day]

On Sun, 26 Aug 2007, Jesper Juhl wrote:

> Check the second patch in the series, please.

ah, got it.  sorry for jumping the gun.

rday
-- 
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://crashcourse.ca
========================================================================