[PATCH] release_resource() check for NULL resource

[PATCH] release_resource() check for NULL resource

[Ben Dooks]

[-- Attachment #1: Type: text/plain, Size: 508 bytes --]

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

Signed-off-by: Ben Dooks <ben-linux@fluff.org>

diff -urN -X ../dontdiff linux-2.6.14-rc3/kernel/resource.c linux-2.6.14-rc3-bjd1/kernel/resource.c
--- linux-2.6.14-rc3/kernel/resource.c	2005-10-02 12:58:03.000000000 +0100
+++ linux-2.6.14-rc3-bjd1/kernel/resource.c	2005-10-02 17:58:09.000000000 +0100
@@ -181,6 +181,9 @@
 {
 	struct resource *tmp, **p;
 
+	if (!old)
+		return 0;
+
 	p = &old->parent->child;
 	for (;;) {
 		tmp = *p;

[-- Attachment #2: resource-release-null.patch --]
[-- Type: text/plain, Size: 389 bytes --]

diff -urN -X ../dontdiff linux-2.6.14-rc3/kernel/resource.c linux-2.6.14-rc3-bjd1/kernel/resource.c
--- linux-2.6.14-rc3/kernel/resource.c	2005-10-02 12:58:03.000000000 +0100
+++ linux-2.6.14-rc3-bjd1/kernel/resource.c	2005-10-02 17:58:09.000000000 +0100
@@ -181,6 +181,9 @@
 {
 	struct resource *tmp, **p;
 
+	if (!old)
+		return 0;
+
 	p = &old->parent->child;
 	for (;;) {
 		tmp = *p;

Re: [PATCH] release_resource() check for NULL resource

[Randy.Dunlap]

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?

btw, please use diff -p also (as in Documentation/SubmittingPatches)


> Signed-off-by: Ben Dooks <ben-linux@fluff.org>
> 
> diff -urN -X ../dontdiff linux-2.6.14-rc3/kernel/resource.c linux-2.6.14-rc3-bjd1/kernel/resource.c
> --- linux-2.6.14-rc3/kernel/resource.c	2005-10-02 12:58:03.000000000 +0100
> +++ linux-2.6.14-rc3-bjd1/kernel/resource.c	2005-10-02 17:58:09.000000000 +0100
> @@ -181,6 +181,9 @@
>  {
>  	struct resource *tmp, **p;
>  
> +	if (!old)
> +		return 0;
> +
>  	p = &old->parent->child;
>  	for (;;) {
>  		tmp = *p;
> 


---
~Randy

Re: [PATCH] release_resource() check for NULL resource

[Ben Dooks]

On Sun, Oct 02, 2005 at 10:39:22AM -0700, Randy.Dunlap wrote:
> 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().
 
> btw, please use diff -p also (as in Documentation/SubmittingPatches)
> 
> 
> > Signed-off-by: Ben Dooks <ben-linux@fluff.org>
> > 
> > diff -urN -X ../dontdiff linux-2.6.14-rc3/kernel/resource.c linux-2.6.14-rc3-bjd1/kernel/resource.c
> > --- linux-2.6.14-rc3/kernel/resource.c	2005-10-02 12:58:03.000000000 +0100
> > +++ linux-2.6.14-rc3-bjd1/kernel/resource.c	2005-10-02 17:58:09.000000000 +0100
> > @@ -181,6 +181,9 @@
> >  {
> >  	struct resource *tmp, **p;
> >  
> > +	if (!old)
> > +		return 0;
> > +
> >  	p = &old->parent->child;
> >  	for (;;) {
> >  		tmp = *p;

-- 
Ben (ben@fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: [PATCH] release_resource() check for NULL resource

[Jesper Juhl]

On 10/3/05, Ben Dooks <ben-linux@fluff.org> wrote:
> On Sun, Oct 02, 2005 at 10:39:22AM -0700, Randy.Dunlap wrote:
> > 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().
>
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 <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] release_resource() check for NULL resource

[Russell King]

On Mon, Oct 03, 2005 at 11:59:01AM +0200, Jesper Juhl wrote:
> On 10/3/05, Ben Dooks <ben-linux@fluff.org> wrote:
> > On Sun, Oct 02, 2005 at 10:39:22AM -0700, Randy.Dunlap wrote:
> > > 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().
> >
> 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?

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

Re: [PATCH] release_resource() check for NULL resource

[Pekka Enberg]

On 10/3/05, Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> 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

Re: [PATCH] release_resource() check for NULL resource

[Russell King]

On Mon, Oct 03, 2005 at 03:43:50PM +0300, Pekka Enberg wrote:
> 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.

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().

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

Re: [PATCH] release_resource() check for NULL resource

[Pekka J Enberg]

On Mon, 3 Oct 2005, Russell King wrote:
> 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.

On Mon, 3 Oct 2005, Russell King wrote:
> 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

Re: [PATCH] release_resource() check for NULL resource

[Russell King]

On Mon, Oct 03, 2005 at 03:54:31PM +0300, Pekka J Enberg wrote:
> On Mon, 3 Oct 2005, Russell King wrote:
> > 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.

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

Re: [PATCH] release_resource() check for NULL resource

[Pekka J Enberg]

On Mon, 3 Oct 2005, Russell King wrote:
> 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

Re: [PATCH] release_resource() check for NULL resource

[Russell King]

On Mon, Oct 03, 2005 at 04:12:41PM +0300, Pekka J Enberg wrote:
> On Mon, 3 Oct 2005, Russell King wrote:
> > 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.

> 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

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

Re: [PATCH] release_resource() check for NULL resource

[Al Viro]

On Mon, Oct 03, 2005 at 03:43:50PM +0300, Pekka Enberg wrote:
> On 10/3/05, Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> > 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.

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.

Re: [PATCH] release_resource() check for NULL resource

[Pekka Enberg]

Hi,

On Mon, Oct 03, 2005 at 03:43:50PM +0300, Pekka Enberg wrote:
> > 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.

On Mon, 2005-10-03 at 14:42 +0100, Al Viro wrote:
> 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

Re: [PATCH] release_resource() check for NULL resource

[Randy.Dunlap]

On Mon, 3 Oct 2005 11:04:31 +0100 Russell King wrote:

> On Mon, Oct 03, 2005 at 11:59:01AM +0200, Jesper Juhl wrote:
> > On 10/3/05, Ben Dooks <ben-linux@fluff.org> wrote:
> > > On Sun, Oct 02, 2005 at 10:39:22AM -0700, Randy.Dunlap wrote:
> > > > 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().
> > >
> > 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?

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

Re: [PATCH] release_resource() check for NULL resource

[Diego de Estrada]

On 10/2/05, Randy.Dunlap <rdunlap@xenotime.net> wrote:
> 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.

Re: [PATCH] release_resource() check for NULL resource

[Randy.Dunlap]

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

> On 10/2/05, Randy.Dunlap <rdunlap@xenotime.net> wrote:
> > 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

Re: [PATCH] release_resource() check for NULL resource

[Bodo Eggert]

Pekka Enberg <penberg@cs.helsinki.fi> wrote:

> static int driver_init(void)
> {
>      dev->resource1 = request_region(...);
>      if (!dev->resource1)
>              goto failed;

> failed:
>      driver_release(dev);

> static void driver_release(struct device * dev)
> {
>      release_resource(dev->resource1);
>      release_resource(dev->resource2);

If the dev struct* isn't properly initialized, it will try to free a random
resource.
-- 
Ich danke GMX dafür, die Verwendung meiner Adressen mittels per SPF
verbreiteten Lügen zu sabotieren.

Re: [PATCH] release_resource() check for NULL resource

[Russell King]

On Wed, Oct 05, 2005 at 12:31:07AM +0200, Bodo Eggert wrote:
> Pekka Enberg <penberg@cs.helsinki.fi> wrote:
> > static int driver_init(void)
> > {
> >      dev->resource1 = request_region(...);
> >      if (!dev->resource1)
> >              goto failed;
> 
> > failed:
> >      driver_release(dev);
> 
> > static void driver_release(struct device * dev)
> > {
> >      release_resource(dev->resource1);
> >      release_resource(dev->resource2);
> 
> If the dev struct* isn't properly initialized, it will try to free a random
> resource.

Fur christ sake, I've made this point several times in this thread and
it still seems to be missed.  (or maybe it's just folks sloppy use of
English.)

release_resource does *not* free anything.  It unregisters it from the
resource tree _only_.

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