release_resource() check for NULL resource

PATCH In review Ben Dooks 7 participants 16 messages Oct 2, 2005 – Oct 4, 2005 1 message not in the archive

lore

Share & tools

lore

Ben Dooks

author

2005-10-02 17:03 UTC #0 lore raw

PATCH 1/1 release_resource() check for NULL resource

If release_resource() is passed a NULL resource
the kernel will OOPS.

Signed-off-by Ben Dooks <[email protected]>

kernel/resource.c +3 -0

diff -urN -X ../dontdiff linux-2.6.14-rc3/kernel/resource.c linux-2.6.14-rc3-bjd1/kernel/resource.c
@@ -181,6 +181,9 @@
 {
 	struct resource *tmp, **p;
 
+	if (!old)
+		return 0;
+
 	p = &old->parent->child;
 	for (;;) {
 		tmp = *p;

kernel/resource.c +3 -0

diff -urN -X ../dontdiff linux-2.6.14-rc3/kernel/resource.c linux-2.6.14-rc3-bjd1/kernel/resource.c
@@ -181,6 +181,9 @@
 {
 	struct resource *tmp, **p;
 
+	if (!old)
+		return 0;
+
 	p = &old->parent->child;
 	for (;;) {
 		tmp = *p;

Ben Dooks

author

2005-10-03 09:48 UTC #2 lore raw

Quoted 7 lines · Randy.Dunlap

Quoted 2 lines · On Sun, 2 Oct 2005 18:03:18 +0100 Ben Dooks wrote:
If release_resource() is passed a NULL resource
the kernel will OOPS.
does this actually happen? you are fixing a real oops? if so, what driver caused it?

I was developing a couple of new drivers, and found that this does not behave like kfree() which does check for NULL paramemters. I belive it would be helpful if functions like this followed the example of kfree().

Quoted 18 lines

btw, please use diff -p also (as in Documentation/SubmittingPatches)
Quoted 15 lines
Signed-off-by Ben Dooks <[email protected]>
diff -urN -X ../dontdiff linux-2.6.14-rc3/kernel/resource.c linux-2.6.14-rc3-bjd1/kernel/resource.c
kernel/resource.c +3 -0
@@ -181,6 +181,9 @@
 {
 	struct resource *tmp, **p;
 
+	if (!old)
+		return 0;
+
 	p = &old->parent->child;
 	for (;;) {
 		tmp = *p;

Signature · 3 lines

Ben ([email protected], http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Jesper Juhl

2005-10-03 09:59 UTC #3 lore raw

Quoted 6 lines · Ben Dooks

Quoted 5 lines · Randy.Dunlap
Quoted 2 lines · On Sun, 2 Oct 2005 18:03:18 +0100 Ben Dooks wrote:
If release_resource() is passed a NULL resource
the kernel will OOPS.

Quoted 8 lines

Quoted 2 lines
does this actually happen? you are fixing a real oops? if so, what driver caused it?
I was developing a couple of new drivers, and found that this does not behave like kfree() which does check for NULL paramemters. I belive it would be helpful if functions like this followed the example of kfree().

I would agree that it makes sense for resource release functions to be written defensively and be able to cope with being passed a NULL resource, just like kfree(), vfree(), crypto_free_tfm() and others are already doing.
Seems safer and allows us to get rid of checks for NULL before calling such functions thus making code simpler, more readable and in some cases smaller.

Just my 0.02euro

--
Jesper Juhl <[email protected]>
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

Russell King

2005-10-03 10:04 UTC #4 lore raw

Quoted 15 lines · Jesper Juhl

Quoted 6 lines · Ben Dooks
Quoted 5 lines · Randy.Dunlap
Quoted 2 lines · On Sun, 2 Oct 2005 18:03:18 +0100 Ben Dooks wrote:
If release_resource() is passed a NULL resource
the kernel will OOPS.
Quoted 8 lines
Quoted 2 lines
does this actually happen? you are fixing a real oops? if so, what driver caused it?
I was developing a couple of new drivers, and found that this does not behave like kfree() which does check for NULL paramemters. I belive it would be helpful if functions like this followed the example of kfree().

Quoted 7 lines

I would agree that it makes sense for resource release functions to be written defensively and be able to cope with being passed a NULL resource, just like kfree(), vfree(), crypto_free_tfm() and others are already doing.
Seems safer and allows us to get rid of checks for NULL before calling such functions thus making code simpler, more readable and in some cases smaller.

I'm not convinced - release_resource() isn't like kfree() - it's more like device_unregister().

It makes sense for kfree() to ignore NULL pointers, but does it really make sense for *_unregister() to do so too? Surely you want to only unregister things which you know have previously been registered?

Signature · 3 lines

Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Pekka Enberg

2005-10-03 12:43 UTC #5 lore raw

Quoted 3 lines · Russell King

It makes sense for kfree() to ignore NULL pointers, but does it really make sense for *_unregister() to do so too? Surely you want to only unregister things which you know have previously been registered?

Usually yes but it makes releasing partial initialization much simpler because you can reuse the normal release counterpart. For example,

static int driver_init(void)

{
     dev->resource1 = request_region(...);
     if (!dev->resource1)
             goto failed;

     dev->resource2 = request_region(...);
     if (!dev->resource2)
             goto failed;

     return 0;

failed:

     driver_release(dev);
     return -1;
}

static void driver_release(struct device * dev)

{
     release_resource(dev->resource1);
     release_resource(dev->resource2);
     kfree(dev);
}

Many drivers have the release function copy-pasted to init with lots of goto labels exactly because release_region, iounmap, and friends aren't NULL safe.

Pekka

Russell King

2005-10-03 12:50 UTC #6 lore raw

Quoted 27 lines · Pekka Enberg

Usually yes but it makes releasing partial initialization much simpler because you can reuse the normal release counterpart. For example,
static int driver_init(void)
{
     dev->resource1 = request_region(...);
     if (!dev->resource1)
             goto failed;

     dev->resource2 = request_region(...);
     if (!dev->resource2)
             goto failed;

     return 0;
failed:
     driver_release(dev);
     return -1;
}
static void driver_release(struct device * dev)
{
     release_resource(dev->resource1);
     release_resource(dev->resource2);
     kfree(dev);
}

Quoted 3 lines

Many drivers have the release function copy-pasted to init with lots of goto labels exactly because release_region, iounmap, and friends aren't NULL safe.

And the above is buggy. request_region() allocates memory. release_resource() unregisters the resource but does _not_ free the allocated memory.

On the other hand, release_region() is the counter-part of request_region() and should be used to release resources created by request_region().

Signature · 3 lines

Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Pekka J Enberg

2005-10-03 12:54 UTC #7 lore raw

Quoted 3 lines · Russell King

And the above is buggy. request_region() allocates memory. release_resource() unregisters the resource but does _not_ free the allocated memory.

Indeed but that's besides the point as kfree() deals with NULLs fine.

Quoted 3 lines · Russell King

On the other hand, release_region() is the counter-part of request_region() and should be used to release resources created by request_region().

Right. To me, it makes sense for the release counter-part accept NULL if the allocation/initialization function can return it.

Pekka

Russell King

2005-10-03 13:00 UTC #8 lore raw

Quoted 7 lines · Pekka J Enberg

Quoted 3 lines · Russell King
On the other hand, release_region() is the counter-part of request_region() and should be used to release resources created by request_region().
Right. To me, it makes sense for the release counter-part accept NULL if the allocation/initialization function can return it.

Right. Which it already can (with a complaint).

However, I think you missed my point though. release_resource() is _not_ the counterpart of request_region(). It's the counter-part of request_resource() which does not allocate any memory itself.

Signature · 3 lines

Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Pekka J Enberg

2005-10-03 13:12 UTC #9 lore raw

Quoted 3 lines · Russell King

However, I think you missed my point though. release_resource() is _not_ the counterpart of request_region(). It's the counter-part of request_resource() which does not allocate any memory itself.

So what is the counter-part for request_region if it's not release_resource? As far as I can tell, release_region is marked as compatability cruft.

But anyway, my point is that dealing with NULL in many release functions is beneficial for releasing a partially initialized state.

Pekka

Russell King

2005-10-03 13:42 UTC #10 lore raw

Quoted 8 lines · Pekka J Enberg

Quoted 3 lines · Russell King
However, I think you missed my point though. release_resource() is _not_ the counterpart of request_region(). It's the counter-part of request_resource() which does not allocate any memory itself.
So what is the counter-part for request_region if it's not release_resource? As far as I can tell, release_region is marked as compatability cruft.

As I stated in previous mails, release_region() undoes all the effects of request_region(). If you take a look at kernel/resource.c, you'll notice that request_region() is also compatibility cruft as well.

Quoted 2 lines

But anyway, my point is that dealing with NULL in many release functions is beneficial for releasing a partially initialized state.

If that's what you think, thanks for volunteering to fix all the sysfs and driver model interfaces as well. The following examples below functionally correspond to the request_resource()/ release_resource() and would also require fixing if what you think were true.

request_resource | release_resource
------------------------+---------------------------
device_register | device_unregister
bus_register | bus_unregister
driver_register | driver_unregister

Signature · 3 lines

Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Al Viro

2005-10-03 13:42 UTC #11 lore raw

In reply to Pekka Enberg

Quoted 32 lines · Pekka Enberg

Quoted 3 lines · Russell King
It makes sense for kfree() to ignore NULL pointers, but does it really make sense for *_unregister() to do so too? Surely you want to only unregister things which you know have previously been registered?
Usually yes but it makes releasing partial initialization much simpler because you can reuse the normal release counterpart. For example,
static int driver_init(void)
{
     dev->resource1 = request_region(...);
     if (!dev->resource1)
             goto failed;

     dev->resource2 = request_region(...);
     if (!dev->resource2)
             goto failed;

     return 0;
failed:
     driver_release(dev);
     return -1;
}
static void driver_release(struct device * dev)
{
     release_resource(dev->resource1);
     release_resource(dev->resource2);
     kfree(dev);
}

Quoted 3 lines

Many drivers have the release function copy-pasted to init with lots of goto labels exactly because release_region, iounmap, and friends aren't NULL safe.

Bullshit. I have waded through many, *many* initialization sequences like that. "Lots of goto labels" is _less_ prone to breakage when properly done; your variant begs for trouble upon the driver changes.

Note that "lots of goto" is actually a cleaner control structure than what you propose - the amount of instances of offending statement is far from being the only metrics. The only things to verify with it are
* releasing is done in the opposite order to claiming
* you always exit to the releasing the last thing you've claimed.
Both are easy to maintain when you change the code _and_ do not break when you need to introduce something like "keep the number of objects"; I've seen too many cases when release had done the things that can _not_ be made idempotent and still had been used in that manner. With obvious breakage resulting from it.

And such breakage is easily introduced when changing the code - it's more common than forgetting to propagate change between release and failure exits in initialization.

Pekka Enberg

2005-10-03 14:57 UTC #12 lore raw

Hi,

Quoted 3 lines · Pekka Enberg

Quoted 3 lines
Many drivers have the release function copy-pasted to init with lots of goto labels exactly because release_region, iounmap, and friends aren't NULL safe.

Quoted 7 lines · Al Viro

Bullshit. I have waded through many, *many* initialization sequences like that. "Lots of goto labels" is _less_ prone to breakage when properly done; your variant begs for trouble upon the driver changes.
Note that "lots of goto" is actually a cleaner control structure than what you propose - the amount of instances of offending statement is far from being the only metrics. The only things to verify with it are

Fair enough. Bad example from my part.

My main argument still remains. It is useful to handle NULL in some release functions (iounmap and release_resource come to mind) because it simplifies releasing a partially initialized state. Grep for NULL checks around iounmap() and release_resource() calls for existing usage in the drivers...

Pekka

Randy.Dunlap

2005-10-04 02:16 UTC #13 lore raw

In reply to Russell King

Quoted 24 lines · On Mon, 3 Oct 2005 11:04:31 +0100 Russell King wrote:

Quoted 15 lines · Jesper Juhl
Quoted 6 lines · Ben Dooks
Quoted 5 lines · Randy.Dunlap
Quoted 2 lines · On Sun, 2 Oct 2005 18:03:18 +0100 Ben Dooks wrote:
If release_resource() is passed a NULL resource
the kernel will OOPS.
Quoted 8 lines
Quoted 2 lines
does this actually happen? you are fixing a real oops? if so, what driver caused it?
I was developing a couple of new drivers, and found that this does not behave like kfree() which does check for NULL paramemters. I belive it would be helpful if functions like this followed the example of kfree().
Quoted 7 lines
I would agree that it makes sense for resource release functions to be written defensively and be able to cope with being passed a NULL resource, just like kfree(), vfree(), crypto_free_tfm() and others are already doing.
Seems safer and allows us to get rid of checks for NULL before calling such functions thus making code simpler, more readable and in some cases smaller.

Quoted 6 lines

I'm not convinced - release_resource() isn't like kfree() - it's more like device_unregister().
It makes sense for kfree() to ignore NULL pointers, but does it really make sense for *_unregister() to do so too? Surely you want to only unregister things which you know have previously been registered?

I agree. The driver should know the state of such registrations or allocations and act accordingly. Having foo_release() check its parameter before acting on it just encourages sloppy programming. kfree() is an exception IMO, not the rule.

---
~Randy
You can't do anything without having to do something else first. -- Belefant's Law

A message that is not in the archive (sent off-list or not yet received): [email protected]

Diego de Estrada

2005-10-04 03:00 UTC #15 lore raw

Quoted 7 lines · Randy.Dunlap

Quoted 2 lines · On Sun, 2 Oct 2005 18:03:18 +0100 Ben Dooks wrote:
If release_resource() is passed a NULL resource
the kernel will OOPS.
does this actually happen? you are fixing a real oops? if so, what driver caused it?

The point is: no driver should make the kernel OOPS. Thanks Ben.

Randy.Dunlap

2005-10-04 03:28 UTC #16 lore raw

In reply to a message not in the archive

Quoted 10 lines · On Mon, 3 Oct 2005 23:59:16 -0300 Diego de Estrada wrote:

Quoted 7 lines · Randy.Dunlap
Quoted 2 lines · On Sun, 2 Oct 2005 18:03:18 +0100 Ben Dooks wrote:
If release_resource() is passed a NULL resource
the kernel will OOPS.
does this actually happen? you are fixing a real oops? if so, what driver caused it?
The point is: no driver should make the kernel OOPS. Thanks Ben.

I understand that sentiment, but if a driver is bad, we generally want to know about that rather than paste (or paper) over it on a continuous basis.

---
~Randy
You can't do anything without having to do something else first. -- Belefant's Law