transport_class: BUG if we can't release the attribute container

transport_class: BUG if we can't release the attribute container

[James Bottomley]

Every current transport class calls transport_container_release but
ignores the return value.  This is catastrophic if it returns an error
because the containers are part of a global list and the next action of
almost every transport class is to free the memory used by the
container.

Fix this by making transport_container_release a void, but making it BUG
if attribute_container_release returns an error ... this catches the
root cause of a system panic much earlier.  If we don't do this, we get
an eventual BUG when the attribute container list notices the corruption
caused by the freed memory it's still referencing.

Also made attribute_container_release __must_check as a reminder.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

---

diff --git a/drivers/base/transport_class.c b/drivers/base/transport_class.c
index 40bca48..06861ae 100644
--- a/drivers/base/transport_class.c
+++ b/drivers/base/transport_class.c
@@ -108,7 +108,7 @@ EXPORT_SYMBOL_GPL(anon_transport_class_register);
  */
 void anon_transport_class_unregister(struct anon_transport_class *atc)
 {
-	attribute_container_unregister(&atc->container);
+	BUG_ON(attribute_container_unregister(&atc->container));
 }
 EXPORT_SYMBOL_GPL(anon_transport_class_unregister);
 
diff --git a/drivers/scsi/raid_class.c b/drivers/scsi/raid_class.c
index 86e1318..52182a7 100644
--- a/drivers/scsi/raid_class.c
+++ b/drivers/scsi/raid_class.c
@@ -289,7 +289,7 @@ raid_class_release(struct raid_template *r)
 {
 	struct raid_internal *i = to_raid_internal(r);
 
-	attribute_container_unregister(&i->r.raid_attrs.ac);
+	BUG_ON(attribute_container_unregister(&i->r.raid_attrs.ac));
 
 	kfree(i);
 }
diff --git a/include/linux/attribute_container.h b/include/linux/attribute_container.h
index f558233..574b201 100644
--- a/include/linux/attribute_container.h
+++ b/include/linux/attribute_container.h
@@ -37,7 +37,7 @@ attribute_container_set_no_classdevs(struct attribute_container *atc)
 }
 
 int attribute_container_register(struct attribute_container *cont);
-int attribute_container_unregister(struct attribute_container *cont);
+int __must_check attribute_container_unregister(struct attribute_container *cont);
 void attribute_container_create_device(struct device *dev,
 				       int (*fn)(struct attribute_container *,
 						 struct device *,
diff --git a/include/linux/transport_class.h b/include/linux/transport_class.h
index 1d6cc22..e74c6dc 100644
--- a/include/linux/transport_class.h
+++ b/include/linux/transport_class.h
@@ -86,9 +86,9 @@ static inline int transport_container_register(struct transport_container *tc)
 	return attribute_container_register(&tc->ac);
 }
 
-static inline int transport_container_unregister(struct transport_container *tc)
+static inline void transport_container_unregister(struct transport_container *tc)
 {
-	return attribute_container_unregister(&tc->ac);
+	BUG_ON(attribute_container_unregister(&tc->ac));
 }
 
 int transport_class_register(struct transport_class *);

Re: transport_class: BUG if we can't release the attribute container

[Greg KH]

On Sat, Mar 22, 2008 at 11:39:17AM -0500, James Bottomley wrote:
> Every current transport class calls transport_container_release but
> ignores the return value.  This is catastrophic if it returns an error
> because the containers are part of a global list and the next action of
> almost every transport class is to free the memory used by the
> container.
> 
> Fix this by making transport_container_release a void, but making it BUG
> if attribute_container_release returns an error ... this catches the
> root cause of a system panic much earlier.  If we don't do this, we get
> an eventual BUG when the attribute container list notices the corruption
> caused by the freed memory it's still referencing.
> 
> Also made attribute_container_release __must_check as a reminder.
> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> 
> ---
> 
> diff --git a/drivers/base/transport_class.c b/drivers/base/transport_class.c
> index 40bca48..06861ae 100644
> --- a/drivers/base/transport_class.c
> +++ b/drivers/base/transport_class.c
> @@ -108,7 +108,7 @@ EXPORT_SYMBOL_GPL(anon_transport_class_register);
>   */
>  void anon_transport_class_unregister(struct anon_transport_class *atc)
>  {
> -	attribute_container_unregister(&atc->container);
> +	BUG_ON(attribute_container_unregister(&atc->container));

BUG_ON() should not do anything in the macro except test for a value, no
function calling.  I think checkpatch.pl checks for this...

If you change that, I have no problem with this.

thanks,

greg k-h

Re: transport_class: BUG if we can't release the attribute container

[James Bottomley]

On Tue, 2008-04-01 at 23:32 -0700, Greg KH wrote:
> On Sat, Mar 22, 2008 at 11:39:17AM -0500, James Bottomley wrote:
> > Every current transport class calls transport_container_release but
> > ignores the return value.  This is catastrophic if it returns an error
> > because the containers are part of a global list and the next action of
> > almost every transport class is to free the memory used by the
> > container.
> > 
> > Fix this by making transport_container_release a void, but making it BUG
> > if attribute_container_release returns an error ... this catches the
> > root cause of a system panic much earlier.  If we don't do this, we get
> > an eventual BUG when the attribute container list notices the corruption
> > caused by the freed memory it's still referencing.
> > 
> > Also made attribute_container_release __must_check as a reminder.
> > 
> > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> > 
> > ---
> > 
> > diff --git a/drivers/base/transport_class.c b/drivers/base/transport_class.c
> > index 40bca48..06861ae 100644
> > --- a/drivers/base/transport_class.c
> > +++ b/drivers/base/transport_class.c
> > @@ -108,7 +108,7 @@ EXPORT_SYMBOL_GPL(anon_transport_class_register);
> >   */
> >  void anon_transport_class_unregister(struct anon_transport_class *atc)
> >  {
> > -	attribute_container_unregister(&atc->container);
> > +	BUG_ON(attribute_container_unregister(&atc->container));
> 
> BUG_ON() should not do anything in the macro except test for a value, no
> function calling.  I think checkpatch.pl checks for this...

Well, we can agree to differ on this.  The camp that wants no side
effects for BUG_ON() does so in case they want to define it to be a nop.
I've always argued that having special rules for this that differ from
functions is asking for trouble.  It's also easy to preserve the side
effects by making the compile out do this:

#define BUG_ON(x) (void)(x)

That lets the compiler process the side effects and discard the result.
This basically means the rules for BUG_ON() arguments are identical to
the rules for function arguments ... I think that's consonant with the
principle of least surprise.

Checkpatch also can't pick this up because it doesn't know which
statements have side effects and which don't without adding a whole lot
more logic to it.

> If you change that, I have no problem with this.

OK ... your subsystem tree your call, I suppose.  How about the
attached.

James

---

From: James Bottomley <James.Bottomley@HansenPartnership.com>
Date: Sat, 22 Mar 2008 11:39:17 -0500
Subject: [SCSI] transport_class: BUG if we can't release the attribute container

Every current transport class calls transport_container_release but
ignores the return value.  This is catastrophic if it returns an error
because the containers are part of a global list and the next action of
almost every transport class is to free the memory used by the
container.

Fix this by making transport_container_release a void, but making it BUG
if attribute_container_release returns an error ... this catches the
root cause of a system panic much earlier.  If we don't do this, we get
an eventual BUG when the attribute container list notices the corruption
caused by the freed memory it's still referencing.

Also made attribute_container_release __must_check as a reminder.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/base/transport_class.c      |    2 +-
 drivers/scsi/raid_class.c           |    2 +-
 include/linux/attribute_container.h |    2 +-
 include/linux/transport_class.h     |    4 ++--
 4 files changed, 5 insertions(+), 5 deletions(-)

Index: linux-2.6/drivers/base/transport_class.c
===================================================================
--- linux-2.6.orig/drivers/base/transport_class.c	2008-03-03 16:50:00.000000000 -0600
+++ linux-2.6/drivers/base/transport_class.c	2008-04-02 09:13:01.000000000 -0500
@@ -108,7 +108,7 @@ EXPORT_SYMBOL_GPL(anon_transport_class_r
  */
 void anon_transport_class_unregister(struct anon_transport_class *atc)
 {
-	attribute_container_unregister(&atc->container);
+	BUG_ON(attribute_container_unregister(&atc->container));
 }
 EXPORT_SYMBOL_GPL(anon_transport_class_unregister);
 
Index: linux-2.6/drivers/scsi/raid_class.c
===================================================================
--- linux-2.6.orig/drivers/scsi/raid_class.c	2007-07-05 13:01:17.000000000 -0500
+++ linux-2.6/drivers/scsi/raid_class.c	2008-04-02 09:13:01.000000000 -0500
@@ -289,7 +289,7 @@ raid_class_release(struct raid_template 
 {
 	struct raid_internal *i = to_raid_internal(r);
 
-	attribute_container_unregister(&i->r.raid_attrs.ac);
+	BUG_ON(attribute_container_unregister(&i->r.raid_attrs.ac));
 
 	kfree(i);
 }
Index: linux-2.6/include/linux/attribute_container.h
===================================================================
--- linux-2.6.orig/include/linux/attribute_container.h	2008-01-25 19:44:09.000000000 -0600
+++ linux-2.6/include/linux/attribute_container.h	2008-04-02 09:13:01.000000000 -0500
@@ -37,7 +37,7 @@ attribute_container_set_no_classdevs(str
 }
 
 int attribute_container_register(struct attribute_container *cont);
-int attribute_container_unregister(struct attribute_container *cont);
+int __must_check attribute_container_unregister(struct attribute_container *cont);
 void attribute_container_create_device(struct device *dev,
 				       int (*fn)(struct attribute_container *,
 						 struct device *,
Index: linux-2.6/include/linux/transport_class.h
===================================================================
--- linux-2.6.orig/include/linux/transport_class.h	2006-02-15 13:47:11.000000000 -0600
+++ linux-2.6/include/linux/transport_class.h	2008-04-02 09:13:58.000000000 -0500
@@ -86,9 +86,10 @@ static inline int transport_container_re
 	return attribute_container_register(&tc->ac);
 }
 
-static inline int transport_container_unregister(struct transport_container *tc)
+static inline void transport_container_unregister(struct transport_container *tc)
 {
-	return attribute_container_unregister(&tc->ac);
+	int err = attribute_container_unregister(&tc->ac);
+	BUG_ON(err);
 }
 
 int transport_class_register(struct transport_class *);

Re: transport_class: BUG if we can't release the attribute container

[Matthew Wilcox]

On Wed, Apr 02, 2008 at 09:15:53AM -0500, James Bottomley wrote:
> On Tue, 2008-04-01 at 23:32 -0700, Greg KH wrote:
> > BUG_ON() should not do anything in the macro except test for a value, no
> > function calling.  I think checkpatch.pl checks for this...
> 
> Well, we can agree to differ on this.  The camp that wants no side
> effects for BUG_ON() does so in case they want to define it to be a nop.

That's one argument, but to me, the most important thing is that reading
the content of BUG_ON is unnecessary for understanding the function.

> OK ... your subsystem tree your call, I suppose.  How about the
> attached.

> -static inline int transport_container_unregister(struct transport_container *tc)
> +static inline void transport_container_unregister(struct transport_container *tc)
>  {
> -	return attribute_container_unregister(&tc->ac);
> +	int err = attribute_container_unregister(&tc->ac);
> +	BUG_ON(err);
>  }

What's wrong with:

	if (attribute_container_unregister(&tc->ac))
		BUG();

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: transport_class: BUG if we can't release the attribute container

[James Bottomley]

On Wed, 2008-04-02 at 08:30 -0600, Matthew Wilcox wrote:
> On Wed, Apr 02, 2008 at 09:15:53AM -0500, James Bottomley wrote:
> > On Tue, 2008-04-01 at 23:32 -0700, Greg KH wrote:
> > > BUG_ON() should not do anything in the macro except test for a value, no
> > > function calling.  I think checkpatch.pl checks for this...
> > 
> > Well, we can agree to differ on this.  The camp that wants no side
> > effects for BUG_ON() does so in case they want to define it to be a nop.
> 
> That's one argument, but to me, the most important thing is that reading
> the content of BUG_ON is unnecessary for understanding the function.
> 
> > OK ... your subsystem tree your call, I suppose.  How about the
> > attached.
> 
> > -static inline int transport_container_unregister(struct transport_container *tc)
> > +static inline void transport_container_unregister(struct transport_container *tc)
> >  {
> > -	return attribute_container_unregister(&tc->ac);
> > +	int err = attribute_container_unregister(&tc->ac);
> > +	BUG_ON(err);
> >  }
> 
> What's wrong with:
> 
> 	if (attribute_container_unregister(&tc->ac))
> 		BUG();

You've lost the unlikely designation which is one of the main reasons
for using BUG_ON().  Most people who write in this form also forget it
leading to a heap of suboptimal jump prediction code in the kernel and
another reason not to encourage it.

James

Re: transport_class: BUG if we can't release the attribute container

[Greg KH]

On Wed, Apr 02, 2008 at 09:32:55AM -0500, James Bottomley wrote:
> On Wed, 2008-04-02 at 08:30 -0600, Matthew Wilcox wrote:
> > On Wed, Apr 02, 2008 at 09:15:53AM -0500, James Bottomley wrote:
> > > On Tue, 2008-04-01 at 23:32 -0700, Greg KH wrote:
> > > > BUG_ON() should not do anything in the macro except test for a value, no
> > > > function calling.  I think checkpatch.pl checks for this...
> > > 
> > > Well, we can agree to differ on this.  The camp that wants no side
> > > effects for BUG_ON() does so in case they want to define it to be a nop.
> > 
> > That's one argument, but to me, the most important thing is that reading
> > the content of BUG_ON is unnecessary for understanding the function.
> > 
> > > OK ... your subsystem tree your call, I suppose.  How about the
> > > attached.
> > 
> > > -static inline int transport_container_unregister(struct transport_container *tc)
> > > +static inline void transport_container_unregister(struct transport_container *tc)
> > >  {
> > > -	return attribute_container_unregister(&tc->ac);
> > > +	int err = attribute_container_unregister(&tc->ac);
> > > +	BUG_ON(err);
> > >  }
> > 
> > What's wrong with:
> > 
> > 	if (attribute_container_unregister(&tc->ac))
> > 		BUG();
> 
> You've lost the unlikely designation which is one of the main reasons
> for using BUG_ON().  Most people who write in this form also forget it
> leading to a heap of suboptimal jump prediction code in the kernel and
> another reason not to encourage it.

Most people who think that they need "unlikely" are also usually wrong
:)

This is on a register/unregister path, nothing "fast" about it at all,
so please don't be worrying about "unlikely" for stuff like this.

I prefer the form mentioned by Matthew above please.

thanks,

greg k-h

Re: transport_class: BUG if we can't release the attribute container

[James Bottomley]

On Wed, 2008-04-02 at 07:53 -0700, Greg KH wrote:
> On Wed, Apr 02, 2008 at 09:32:55AM -0500, James Bottomley wrote:
> > On Wed, 2008-04-02 at 08:30 -0600, Matthew Wilcox wrote:
> > > On Wed, Apr 02, 2008 at 09:15:53AM -0500, James Bottomley wrote:
> > > > On Tue, 2008-04-01 at 23:32 -0700, Greg KH wrote:
> > > > > BUG_ON() should not do anything in the macro except test for a value, no
> > > > > function calling.  I think checkpatch.pl checks for this...
> > > > 
> > > > Well, we can agree to differ on this.  The camp that wants no side
> > > > effects for BUG_ON() does so in case they want to define it to be a nop.
> > > 
> > > That's one argument, but to me, the most important thing is that reading
> > > the content of BUG_ON is unnecessary for understanding the function.
> > > 
> > > > OK ... your subsystem tree your call, I suppose.  How about the
> > > > attached.
> > > 
> > > > -static inline int transport_container_unregister(struct transport_container *tc)
> > > > +static inline void transport_container_unregister(struct transport_container *tc)
> > > >  {
> > > > -	return attribute_container_unregister(&tc->ac);
> > > > +	int err = attribute_container_unregister(&tc->ac);
> > > > +	BUG_ON(err);
> > > >  }
> > > 
> > > What's wrong with:
> > > 
> > > 	if (attribute_container_unregister(&tc->ac))
> > > 		BUG();
> > 
> > You've lost the unlikely designation which is one of the main reasons
> > for using BUG_ON().  Most people who write in this form also forget it
> > leading to a heap of suboptimal jump prediction code in the kernel and
> > another reason not to encourage it.
> 
> Most people who think that they need "unlikely" are also usually wrong
> :)

In this case, I hope not ... it better be incredibly unlikely to trigger
a BUG otherwise our users are going to be a bit unhappy ...

> This is on a register/unregister path, nothing "fast" about it at all,
> so please don't be worrying about "unlikely" for stuff like this.

It's irrelevant in this case because unregister isn't any form of
critical path.  However, people copy code so by not putting the unlikely
in here, you make it more possible to find this type of thing in the
critical path where it would be manifestly incorrect because they
noticed this form of it in a header file.

> I prefer the form mentioned by Matthew above please.

OK, try this.  I also fixed the BUG_ON in drivers/base

James

---

From: James Bottomley <James.Bottomley@HansenPartnership.com>
Date: Sat, 22 Mar 2008 11:39:17 -0500
Subject: [SCSI] transport_class: BUG if we can't release the attribute container

Every current transport class calls transport_container_release but
ignores the return value.  This is catastrophic if it returns an error
because the containers are part of a global list and the next action of
almost every transport class is to free the memory used by the
container.

Fix this by making transport_container_release a void, but making it BUG
if attribute_container_release returns an error ... this catches the
root cause of a system panic much earlier.  If we don't do this, we get
an eventual BUG when the attribute container list notices the corruption
caused by the freed memory it's still referencing.

Also made attribute_container_release __must_check as a reminder.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/base/transport_class.c      |    2 +-
 drivers/scsi/raid_class.c           |    2 +-
 include/linux/attribute_container.h |    2 +-
 include/linux/transport_class.h     |    4 ++--
 4 files changed, 5 insertions(+), 5 deletions(-)

Index: linux-2.6/drivers/base/transport_class.c
===================================================================
--- linux-2.6.orig/drivers/base/transport_class.c	2008-04-02 09:14:18.000000000 -0500
+++ linux-2.6/drivers/base/transport_class.c	2008-04-02 10:01:45.000000000 -0500
@@ -108,7 +108,8 @@ EXPORT_SYMBOL_GPL(anon_transport_class_r
  */
 void anon_transport_class_unregister(struct anon_transport_class *atc)
 {
-	attribute_container_unregister(&atc->container);
+	if (unlikely(attribute_container_unregister(&atc->container)))
+		BUG();
 }
 EXPORT_SYMBOL_GPL(anon_transport_class_unregister);
 
Index: linux-2.6/drivers/scsi/raid_class.c
===================================================================
--- linux-2.6.orig/drivers/scsi/raid_class.c	2008-04-02 09:14:18.000000000 -0500
+++ linux-2.6/drivers/scsi/raid_class.c	2008-04-02 10:01:14.000000000 -0500
@@ -289,7 +289,7 @@ raid_class_release(struct raid_template 
 {
 	struct raid_internal *i = to_raid_internal(r);
 
-	attribute_container_unregister(&i->r.raid_attrs.ac);
+	BUG_ON(attribute_container_unregister(&i->r.raid_attrs.ac));
 
 	kfree(i);
 }
Index: linux-2.6/include/linux/attribute_container.h
===================================================================
--- linux-2.6.orig/include/linux/attribute_container.h	2008-04-02 09:14:18.000000000 -0500
+++ linux-2.6/include/linux/attribute_container.h	2008-04-02 10:01:14.000000000 -0500
@@ -37,7 +37,7 @@ attribute_container_set_no_classdevs(str
 }
 
 int attribute_container_register(struct attribute_container *cont);
-int attribute_container_unregister(struct attribute_container *cont);
+int __must_check attribute_container_unregister(struct attribute_container *cont);
 void attribute_container_create_device(struct device *dev,
 				       int (*fn)(struct attribute_container *,
 						 struct device *,
Index: linux-2.6/include/linux/transport_class.h
===================================================================
--- linux-2.6.orig/include/linux/transport_class.h	2008-04-02 09:14:18.000000000 -0500
+++ linux-2.6/include/linux/transport_class.h	2008-04-02 10:02:45.000000000 -0500
@@ -86,9 +86,10 @@ static inline int transport_container_re
 	return attribute_container_register(&tc->ac);
 }
 
-static inline int transport_container_unregister(struct transport_container *tc)
+static inline void transport_container_unregister(struct transport_container *tc)
 {
-	return attribute_container_unregister(&tc->ac);
+	if (unlikely(attribute_container_unregister(&tc->ac)))
+		BUG();
 }
 
 int transport_class_register(struct transport_class *);

Re: transport_class: BUG if we can't release the attribute container

[Boaz Harrosh]

James Bottomley wrote:
<snip> 
> Well, we can agree to differ on this.  The camp that wants no side
> effects for BUG_ON() does so in case they want to define it to be a nop.
> I've always argued that having special rules for this that differ from
> functions is asking for trouble.  It's also easy to preserve the side
> effects by making the compile out do this:
> 
> #define BUG_ON(x) (void)(x)
> 
> That lets the compiler process the side effects and discard the result.
> This basically means the rules for BUG_ON() arguments are identical to
> the rules for function arguments ... I think that's consonant with the
> principle of least surprise.
> 

The Kernel seems to agree with you:
#define BUG_ON(condition) do { if (condition) ; } while(0)

what should we do with stuff like
	BUG_ON(check_foo_valid()); 
which is time consuming and should not run on production machines

should we directly use:
#ifdef CONFIG_BUG
or is there some other facility for that?

Boaz

Re: transport_class: BUG if we can't release the attribute container

[James Bottomley]

On Wed, 2008-04-02 at 17:32 +0300, Boaz Harrosh wrote:
> James Bottomley wrote:
> <snip> 
> > Well, we can agree to differ on this.  The camp that wants no side
> > effects for BUG_ON() does so in case they want to define it to be a nop.
> > I've always argued that having special rules for this that differ from
> > functions is asking for trouble.  It's also easy to preserve the side
> > effects by making the compile out do this:
> > 
> > #define BUG_ON(x) (void)(x)
> > 
> > That lets the compiler process the side effects and discard the result.
> > This basically means the rules for BUG_ON() arguments are identical to
> > the rules for function arguments ... I think that's consonant with the
> > principle of least surprise.
> > 
> 
> The Kernel seems to agree with you:
> #define BUG_ON(condition) do { if (condition) ; } while(0)
> 
> what should we do with stuff like
> 	BUG_ON(check_foo_valid()); 
> which is time consuming and should not run on production machines

create a separate CONFIG_FOO_DEBUG flag ... BUG_ON is designed to
operate as assertion checks on all systems ... including production
ones.  In fact, I've never really agreed with the need to compile it out
in the first place.  Extra and time consuming debugging should be
controlled by a config (or runtime, which is nice because you can turn
it on if there's a problem) variable.

> should we directly use:
> #ifdef CONFIG_BUG
> or is there some other facility for that?

James

Re: transport_class: BUG if we can't release the attribute container

[Greg KH]

On Wed, Apr 02, 2008 at 09:15:53AM -0500, James Bottomley wrote:
> > If you change that, I have no problem with this.
> 
> OK ... your subsystem tree your call, I suppose.  How about the
> attached.

No, you only changed one instance.  Can you please change them all?

thanks,

greg k-h