[PATCH] Fix kfree usage in various mtd map remove functions

[PATCH] Fix kfree usage in various mtd map remove functions

[Phil Endecott]

The remove() functions in various mtd map drivers were incorrectly kfree()ing the struct 
resource that they had passed to release_resource(), and/or failing to free the *_flash_info 
structure.  This looks like a typo which has gradually propogated by copy&paste into a number 
of these drivers.

Since mtd drivers are rarely or never used as modules (they typically provide root filesystems) 
it is unlikely that these code paths have ever been exercised.  It also means that this patch 
has not been tested by the author.

This patch is against 2.6.21.

Signed-off-by: Phil Endecott <spam_from_linux_mtd_patch@chezphil.org>


diff -Nur linux-arm-2.6.21/drivers/mtd/maps.orig/integrator-flash.c linux-arm-2.6.21/drivers/mtd/maps/integrator-flash.c
--- linux-arm-2.6.21/drivers/mtd/maps.orig/integrator-flash.c	2007-05-17 18:56:10.000000000 +0100
+++ linux-arm-2.6.21/drivers/mtd/maps/integrator-flash.c	2007-05-17 18:58:16.000000000 +0100
@@ -174,7 +174,6 @@
 
 		iounmap(info->map.virt);
 		release_resource(info->res);
-		kfree(info->res);
 
 		if (info->plat && info->plat->exit)
 			info->plat->exit();
diff -Nur linux-arm-2.6.21/drivers/mtd/maps.orig/ixp2000.c linux-arm-2.6.21/drivers/mtd/maps/ixp2000.c
--- linux-arm-2.6.21/drivers/mtd/maps.orig/ixp2000.c	2007-05-17 18:56:10.000000000 +0100
+++ linux-arm-2.6.21/drivers/mtd/maps/ixp2000.c	2007-05-17 18:57:29.000000000 +0100
@@ -129,10 +129,10 @@
 
 	kfree(info->partitions);
 
-	if (info->res) {
+	if (info->res)
 		release_resource(info->res);
-		kfree(info->res);
-	}
+
+	kfree(info);
 
 	if (plat->exit)
 		plat->exit();
diff -Nur linux-arm-2.6.21/drivers/mtd/maps.orig/ixp4xx.c linux-arm-2.6.21/drivers/mtd/maps/ixp4xx.c
--- linux-arm-2.6.21/drivers/mtd/maps.orig/ixp4xx.c	2007-05-17 18:56:10.000000000 +0100
+++ linux-arm-2.6.21/drivers/mtd/maps/ixp4xx.c	2007-05-17 18:57:02.000000000 +0100
@@ -172,10 +172,10 @@
 
 	kfree(info->partitions);
 
-	if (info->res) {
+	if (info->res)
 		release_resource(info->res);
-		kfree(info->res);
-	}
+
+	kfree(info);
 
 	if (plat->exit)
 		plat->exit();
Binary files linux-arm-2.6.21/drivers/mtd/maps.orig/ixp4xx.o and linux-arm-2.6.21/drivers/mtd/maps/ixp4xx.o differ
diff -Nur linux-arm-2.6.21/drivers/mtd/maps.orig/physmap.c linux-arm-2.6.21/drivers/mtd/maps/physmap.c
--- linux-arm-2.6.21/drivers/mtd/maps.orig/physmap.c	2007-05-17 18:56:10.000000000 +0100
+++ linux-arm-2.6.21/drivers/mtd/maps/physmap.c	2007-05-17 18:59:20.000000000 +0100
@@ -64,10 +64,10 @@
 	if (info->map.virt != NULL)
 		iounmap(info->map.virt);
 
-	if (info->res != NULL) {
+	if (info->res)
 		release_resource(info->res);
-		kfree(info->res);
-	}
+
+	kfree(info);
 
 	return 0;
 }
diff -Nur linux-arm-2.6.21/drivers/mtd/maps.orig/physmap_of.c linux-arm-2.6.21/drivers/mtd/maps/physmap_of.c
--- linux-arm-2.6.21/drivers/mtd/maps.orig/physmap_of.c	2007-05-17 18:56:10.000000000 +0100
+++ linux-arm-2.6.21/drivers/mtd/maps/physmap_of.c	2007-05-17 18:59:57.000000000 +0100
@@ -107,10 +107,10 @@
 	if (info->map.virt != NULL)
 		iounmap(info->map.virt);
 
-	if (info->res != NULL) {
+	if (info->res)
 		release_resource(info->res);
-		kfree(info->res);
-	}
+
+	kfree(info);
 
 	return 0;
 }

Re: [PATCH] Fix kfree usage in various mtd map remove functions

[MikeW]

Phil Endecott <spam_from_linux_mtd <at> chezphil.org> writes:

> 
> The remove() functions in various mtd map drivers were incorrectly kfree()ing
the struct 
> resource that they had passed to release_resource(), and/or failing to free
the *_flash_info 
> structure.  This looks like a typo which has gradually propogated by
copy&paste into a number 
> of these drivers.
> 
> Since mtd drivers are rarely or never used as modules (they typically provide
root filesystems) 
> it is unlikely that these code paths have ever been exercised.  It also means
that this patch 
> has not been tested by the author.
> 
> This patch is against 2.6.21.
> 
> Signed-off-by: Phil Endecott <spam_from_linux_mtd_patch <at> chezphil.org>
> 

Good chap !!

They could be tested by building as modules then insmod/rmmod-ing
repeatedly, whilst observing kernel memory usage, before and after fix.

Regards,
MikeW

Re: [PATCH] Fix kfree usage in various mtd map remove functions

[MikeW]

Phil Endecott <spam_from_linux_mtd <at> chezphil.org> writes:

....
> Since mtd drivers are rarely or never used as modules (they typically provide
root filesystems) 
> it is unlikely that these code paths have ever been exercised.  It also means
that this patch 
> has not been tested by the author.
....

PS a little worrying that people are committing patches without
testing them properly. Hope it's just an exception !

MikeW

Re: [PATCH] Fix kfree usage in various mtd map remove functions

[Phil Endecott]

The other issue that I noticed was the use of release_resource() 
instead of release_mem_region().  I'm not at all sure what should be 
going on here, but as far as I can see from kernel/resource.c these 
functions are not equivalent, and the "Linux Device Drivers" book shows 
release_mem_region() used to reverse the effect of 
request_mem_region().  I think that release_resource() should be called 
when (for example) a hotpluggable device is unplugged and the resource 
goes away, whereas release_mem_region() should be called when a driver 
no longer needs use of that area of memory.  It's complex to understand 
the source because the same 'struct resource' is used to describe a 
physical resource and also a reservation/allocation of a portion of a 
physical resource to a driver.

I could submit a patch that replaces the release_resource() calls with 
release_mem_region(), but I would not be happy doing so unless someone 
with a bit more of a clue than me said it was the right thing to do.

Does anyone know of any documentation for any of these 'resource' functions?

See for example:
   drivers/mtd/maps/ixp4xx.c function int ixp4xx_flash_remove


Phil.

Re: [PATCH] Fix kfree usage in various mtd map remove functions

[MikeW]

Phil Endecott <spam_from_linux_mtd <at> chezphil.org> writes:

> 
> The other issue that I noticed was the use of release_resource() 
> instead of release_mem_region().  I'm not at all sure what should be 
> going on here, but as far as I can see from kernel/resource.c these 
> functions are not equivalent, and the "Linux Device Drivers" book shows 
> release_mem_region() used to reverse the effect of 
> request_mem_region().  I think that release_resource() should be called 
> when (for example) a hotpluggable device is unplugged and the resource 
> goes away, whereas release_mem_region() should be called when a driver 
> no longer needs use of that area of memory.  It's complex to understand 
> the source because the same 'struct resource' is used to describe a 
> physical resource and also a reservation/allocation of a portion of a 
> physical resource to a driver.
> 
> I could submit a patch that replaces the release_resource() calls with 
> release_mem_region(), but I would not be happy doing so unless someone 
> with a bit more of a clue than me said it was the right thing to do.
> 
> Does anyone know of any documentation for any of these 'resource' functions?
> 
> See for example:
>    drivers/mtd/maps/ixp4xx.c function int ixp4xx_flash_remove
> 
> Phil.

The API manages device registers in I/O space or I/O memory space.
The driver allocations show up in e.g. /proc/iomem and prevent
drivers from trying to control the same physical device hardware.
If your 'resource' request fails then the driver load fails -
or your driver has to try an alternative address.

The pairings are:
 request_region/request_mem_region,
 release_region/release_mem_region
but release_resource is generic so can be used as release.. or release_mem..

The existing calls you describe above sound OK but perhaps the author
should have commented more clearly.

Best regards,
MikeW