[PATCH] Make WARN_ON/WARN_ON_ONCE no-ops when CONFIG_BUG is off

[PATCH] Make WARN_ON/WARN_ON_ONCE no-ops when CONFIG_BUG is off

[Herbert Xu]

Hi:

[PATCH] Make WARN_ON/WARN_ON_ONCE no-ops when CONFIG_BUG is off

The description of CONFIG_BUG clearly states that both BUG and
WARN_ON may be skipped.  However, our actual implementation still
checks the condition on WARN_ON if it's used as part of an if
statement or such.

This patch makes it return 0 after evaluating the expression
if CONFIG_BUG is disabled.  This is consistent with the spirit
of the CONFIG_BUG option.

The same change is made to WARN_ON_ONCE.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index d56fedb..3e74278 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -43,6 +43,16 @@ struct bug_entry {
 })
 #endif
 
+#define WARN_ON_ONCE(condition)	({				\
+	static int __warned;					\
+	int __ret_warn_once = !!(condition);			\
+								\
+	if (unlikely(__ret_warn_once))				\
+		if (WARN_ON(!__warned)) 			\
+			__warned = 1;				\
+	unlikely(__ret_warn_once);				\
+})
+
 #else /* !CONFIG_BUG */
 #ifndef HAVE_ARCH_BUG
 #define BUG()
@@ -53,22 +63,11 @@ struct bug_entry {
 #endif
 
 #ifndef HAVE_ARCH_WARN_ON
-#define WARN_ON(condition) ({						\
-	int __ret_warn_on = !!(condition);				\
-	unlikely(__ret_warn_on);					\
-})
-#endif
+#define WARN_ON(condition) ((condition), 0)
 #endif
 
-#define WARN_ON_ONCE(condition)	({				\
-	static int __warned;					\
-	int __ret_warn_once = !!(condition);			\
-								\
-	if (unlikely(__ret_warn_once))				\
-		if (WARN_ON(!__warned)) 			\
-			__warned = 1;				\
-	unlikely(__ret_warn_once);				\
-})
+#define WARN_ON_ONCE(condition) ((condition), 0)
+#endif
 
 #ifdef CONFIG_SMP
 # define WARN_ON_SMP(x)			WARN_ON(x)

Re: [PATCH] Make WARN_ON/WARN_ON_ONCE no-ops when CONFIG_BUG is off

[Matt Mackall]

On Fri, Dec 14, 2007 at 09:27:55PM +0800, Herbert Xu wrote:
> Hi:
> 
> [PATCH] Make WARN_ON/WARN_ON_ONCE no-ops when CONFIG_BUG is off
> 
> The description of CONFIG_BUG clearly states that both BUG and
> WARN_ON may be skipped.  However, our actual implementation still
> checks the condition on WARN_ON if it's used as part of an if
> statement or such.
> 
> This patch makes it return 0 after evaluating the expression
> if CONFIG_BUG is disabled.  This is consistent with the spirit
> of the CONFIG_BUG option.

I added CONFIG_BUG, and I think the current behavior is correct. As
you've noticed, we have to evaluate condition, it may have
side-effects. And if code does:

	/* this indicates a driver bug, report and fail gracefully */
	if (WARN_ON(val == NULL))
		return -EFAULT;

..we surely want it to continue returning -EFAULT, regardless of
whether we log it, no? What use case did you have in mind?

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH] Make WARN_ON/WARN_ON_ONCE no-ops when CONFIG_BUG is off

[Herbert Xu]

On Fri, Dec 14, 2007 at 12:02:46PM -0600, Matt Mackall wrote:
>
> I added CONFIG_BUG, and I think the current behavior is correct. As
> you've noticed, we have to evaluate condition, it may have
> side-effects. And if code does:
> 
> 	/* this indicates a driver bug, report and fail gracefully */
> 	if (WARN_ON(val == NULL))
> 		return -EFAULT;

That's exactly the sort of use I had in mind :) I'm actually the
one who added the ability to use WARN_ON inside an if clause.

Just as the case of a BUG_ON, a WARN_ON should never occur in
practice, unless there is a bug which the code is not aware of.
As such we want it to go away completely if CONFIG_BUG is off.

> ..we surely want it to continue returning -EFAULT, regardless of
> whether we log it, no? What use case did you have in mind?

If you're using it for a scenario which is known to actually
occur, then some other mechanism should be chosen in place of
WARN_ON.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] Make WARN_ON/WARN_ON_ONCE no-ops when CONFIG_BUG is off

[Matt Mackall]

On Sat, Dec 15, 2007 at 12:16:59PM +0800, Herbert Xu wrote:
> On Fri, Dec 14, 2007 at 12:02:46PM -0600, Matt Mackall wrote:
> >
> > I added CONFIG_BUG, and I think the current behavior is correct. As
> > you've noticed, we have to evaluate condition, it may have
> > side-effects. And if code does:
> > 
> > 	/* this indicates a driver bug, report and fail gracefully */
> > 	if (WARN_ON(val == NULL))
> > 		return -EFAULT;
> 
> That's exactly the sort of use I had in mind :) I'm actually the
> one who added the ability to use WARN_ON inside an if clause.
> 
> Just as the case of a BUG_ON, a WARN_ON should never occur in
> practice, unless there is a bug which the code is not aware of.

Agreed.

> As such we want it to go away completely if CONFIG_BUG is off.

No. The code as written above should reduce to:

	if (val == NULL)
		return -EFAULT;

If I hadn't wanted to return -EFAULT in this case, I would have just written:

	WARN_ON(val == NULL);

I don't want code that was running safely (ie returning -EFAULT) to
start crashing the system just because I've, say, disabled printk.
That's creating an obnoxious heisenbug.

> > ..we surely want it to continue returning -EFAULT, regardless of
> > whether we log it, no? What use case did you have in mind?
> 
> If you're using it for a scenario which is known to actually
> occur, then some other mechanism should be chosen in place of
> WARN_ON.

Then I kindly submit that you should instead withdraw the code that
allows you to use WARN_ON in a condition in the first place.

Note that Dave Jones is currently poking at making WARN_ON
out-of-line, so you're liable to collide with him.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH] Make WARN_ON/WARN_ON_ONCE no-ops when CONFIG_BUG is off

[Herbert Xu]

On Fri, Dec 14, 2007 at 11:52:18PM -0600, Matt Mackall wrote:
>
> No. The code as written above should reduce to:
> 
> 	if (val == NULL)
> 		return -EFAULT;
> 
> If I hadn't wanted to return -EFAULT in this case, I would have just written:
> 
> 	WARN_ON(val == NULL);

Well the only reason I introduced

	if (WARN_ON)

is so that what would otherwise be a BUG_ON condition would have
a chance to get written to disk when invoked from an IRQ handler.

> I don't want code that was running safely (ie returning -EFAULT) to
> start crashing the system just because I've, say, disabled printk.
> That's creating an obnoxious heisenbug.

I'm disappointed that it has been used in ways that it shouldn't
have been.

I suppose we'll have to either introduce a new primitive or just
go back to using BUG_ON.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] Make WARN_ON/WARN_ON_ONCE no-ops when CONFIG_BUG is off

[Matt Mackall]

On Sat, Dec 15, 2007 at 02:04:49PM +0800, Herbert Xu wrote:
> On Fri, Dec 14, 2007 at 11:52:18PM -0600, Matt Mackall wrote:
> >
> > No. The code as written above should reduce to:
> > 
> > 	if (val == NULL)
> > 		return -EFAULT;
> > 
> > If I hadn't wanted to return -EFAULT in this case, I would have just written:
> > 
> > 	WARN_ON(val == NULL);
> 
> Well the only reason I introduced
> 
> 	if (WARN_ON)
> 
> is so that what would otherwise be a BUG_ON condition would have
> a chance to get written to disk when invoked from an IRQ handler.
> 
> > I don't want code that was running safely (ie returning -EFAULT) to
> > start crashing the system just because I've, say, disabled printk.
> > That's creating an obnoxious heisenbug.
> 
> I'm disappointed that it has been used in ways that it shouldn't
> have been.
> 
> I suppose we'll have to either introduce a new primitive or just
> go back to using BUG_ON.

Seems we haven't yet reached concensus on what an appropriate use for
BUG_ON is. There's a fairly large camp who think that there are
basically no good reasons to outright crash a machine and that WARN_ON
should replace BUG_ON everywhere.

I tend to agree with this position, except when it comes to handling
filesystems, where panic is often (but not always) the right thing to
do.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH] Make WARN_ON/WARN_ON_ONCE no-ops when CONFIG_BUG is off

[Herbert Xu]

On Sat, Dec 15, 2007 at 12:12:00AM -0600, Matt Mackall wrote:
>
> I tend to agree with this position, except when it comes to handling
> filesystems, where panic is often (but not always) the right thing to
> do.

Given the choice between crashing the machine or potentially giving
an attacker remote root access, I definitely prefer the former in
many instances in the network stack.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] Make WARN_ON/WARN_ON_ONCE no-ops when CONFIG_BUG is off

[Herbert Xu]

On Sat, Dec 15, 2007 at 02:04:49PM +0800, Herbert Xu wrote:
>
> I suppose we'll have to either introduce a new primitive or just
> go back to using BUG_ON.

Let's do the conservative thing and add a new primitive.

[PATCH] Added BARF_ON/BARF_ON_ONCE

The description of CONFIG_BUG clearly states that both BUG and
WARN_ON may be skipped.  However, our actual implementation still
checks the condition on WARN_ON if it's used as part of an if
statement or such.

I tried to compile it away but Matt Mackall pointed out that
people may already be using it in ways that were not intended.
In particular, it may have been used for conditions that are
thought to be possible.

So this patch adds a new pair of macros BARF_ON and BARF_ON_ONCE
that are clearly marked as such to prevent abuse.  The intended
usage model is identical to WARN_ON except that they should only
be used for conditions that are thought to be impossible.  In
other words, only use them to replace BUG_ON's that may be called
by an IRQ handler or within locks.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index d56fedb..36e565c 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -43,6 +43,19 @@ struct bug_entry {
 })
 #endif
 
+/*
+ * BARF_ON should only be used in situations which are thought to be
+ * impossible in practice.  In other words, do NOT use it in a place
+ * where you would not use BUG_ON.  It only exists so that the message
+ * gets a chance to be written to disk.
+ *
+ * It should only be used if we can afford to not kill the current
+ * process or the kernel.  So don't use it if we may end up scribbling
+ * over the disk or give people remote root access.
+ */
+#define BARF_ON(condition) WARN_ON(condition)
+#define BARF_ON_ONCE(condition) WARN_ON_ONCE(condition)
+
 #else /* !CONFIG_BUG */
 #ifndef HAVE_ARCH_BUG
 #define BUG()
@@ -58,6 +71,9 @@ struct bug_entry {
 	unlikely(__ret_warn_on);					\
 })
 #endif
+
+#define BARF_ON(condition) ((condition), 0)
+#define BARF_ON_ONCE(condition) BARF_ON(condition)
 #endif
 
 #define WARN_ON_ONCE(condition)	({				\

Re: [PATCH] Make WARN_ON/WARN_ON_ONCE no-ops when CONFIG_BUG is off

[Matt Mackall]

On Sat, Dec 15, 2007 at 02:52:11PM +0800, Herbert Xu wrote:
> On Sat, Dec 15, 2007 at 02:04:49PM +0800, Herbert Xu wrote:
> >
> > I suppose we'll have to either introduce a new primitive or just
> > go back to using BUG_ON.
> 
> Let's do the conservative thing and add a new primitive.
> 
> [PATCH] Added BARF_ON/BARF_ON_ONCE
> 
> The description of CONFIG_BUG clearly states that both BUG and
> WARN_ON may be skipped.  However, our actual implementation still
> checks the condition on WARN_ON if it's used as part of an if
> statement or such.
> 
> I tried to compile it away but Matt Mackall pointed out that
> people may already be using it in ways that were not intended.
> In particular, it may have been used for conditions that are
> thought to be possible.
> 
> So this patch adds a new pair of macros BARF_ON and BARF_ON_ONCE
> that are clearly marked as such to prevent abuse.  The intended
> usage model is identical to WARN_ON except that they should only
> be used for conditions that are thought to be impossible.  In
> other words, only use them to replace BUG_ON's that may be called
> by an IRQ handler or within locks.

Egads, this just makes the whole bad BUG_ON vs WARN_ON situation
worse. This adds yet another non-distinction, because the difference
between BUG_ON and WARN_ON has absolutely nothing to do with the
(perceived or actual) probability of the condition.

We've got plenty of evidence that BUG_ON and WARN_ON conditions -do-
happen. So choosing between BUG_ON and WARN_ON is choosing whether a
box should be forcibly killed or not and nothing more.

You've clearly got a use-case in mind where WARN_ON + !CONFIG_BUG
isn't doing what you want, please share it with us.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH] Make WARN_ON/WARN_ON_ONCE no-ops when CONFIG_BUG is off

[Dave Jones]

On Fri, Dec 14, 2007 at 11:52:18PM -0600, Matt Mackall wrote:
 
 > Then I kindly submit that you should instead withdraw the code that
 > allows you to use WARN_ON in a condition in the first place.
 > 
 > Note that Dave Jones is currently poking at making WARN_ON
 > out-of-line, so you're liable to collide with him.
 
Actually I'm not likely to get back to this until post xmas.
I'm on vacation until then from today, so I'll be paying less
attention to stuff than usual.

	Dave

-- 
http://www.codemonkey.org.uk

Re: [PATCH] Make WARN_ON/WARN_ON_ONCE no-ops when CONFIG_BUG is off

[Benjamin Herrenschmidt]

On Fri, 2007-12-14 at 12:02 -0600, Matt Mackall wrote:
> 
> I added CONFIG_BUG, and I think the current behavior is correct. As
> you've noticed, we have to evaluate condition, it may have
> side-effects. And if code does:
> 
>         /* this indicates a driver bug, report and fail gracefully */
>         if (WARN_ON(val == NULL))
>                 return -EFAULT;
> 
> ..we surely want it to continue returning -EFAULT, regardless of
> whether we log it, no? What use case did you have in mind?

I find such code totally distateful.

Ben.

Re: [PATCH] Make WARN_ON/WARN_ON_ONCE no-ops when CONFIG_BUG is off

[Benjamin Herrenschmidt]

On Fri, 2007-12-14 at 21:27 +0800, Herbert Xu wrote:
> Hi:
> 
> [PATCH] Make WARN_ON/WARN_ON_ONCE no-ops when CONFIG_BUG is off
> 
> The description of CONFIG_BUG clearly states that both BUG and
> WARN_ON may be skipped.  However, our actual implementation still
> checks the condition on WARN_ON if it's used as part of an if
> statement or such.
> 
> This patch makes it return 0 after evaluating the expression
> if CONFIG_BUG is disabled.  This is consistent with the spirit
> of the CONFIG_BUG option.
> 
> The same change is made to WARN_ON_ONCE.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

That's something I've actually never quite liked... the fact that we
evaluate the expression anyway. I'm pretty happy with -not- evaluating
the expression when CONFIG_BUG is on most of the time since whatever is
in there is purely here for the sake of the BUG/WARN test.

I understand why some people may want it the other way around, but I
personally find it a very bad idea in the first place to write a normal
statement part of the program as

	BUG_ON(do_something());

It's way clearer to me I believe to write:

	rc = do_something();
	BUG_ON(rc);

And thus, when I write:

	BUG_ON(do_sanity_check());

I'm actually execting the function call to disappear when CONFIG_BUG
is turned off...

Cheers,
Ben.

Re: [PATCH] Make WARN_ON/WARN_ON_ONCE no-ops when CONFIG_BUG is off

[Herbert Xu]

On Sat, Dec 15, 2007 at 05:31:30PM +1100, Benjamin Herrenschmidt wrote:
>
> That's something I've actually never quite liked... the fact that we
> evaluate the expression anyway. I'm pretty happy with -not- evaluating
> the expression when CONFIG_BUG is on most of the time since whatever is
> in there is purely here for the sake of the BUG/WARN test.

Whether we evaluate the expression is a completely different debate.
I personally agree with you that expressions that have side-effects
are a stupid idea for either WARN_ON or BUG_ON.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] Make WARN_ON/WARN_ON_ONCE no-ops when CONFIG_BUG is off

[Matt Mackall]

On Sat, Dec 15, 2007 at 02:34:42PM +0800, Herbert Xu wrote:
> On Sat, Dec 15, 2007 at 05:31:30PM +1100, Benjamin Herrenschmidt wrote:
> >
> > That's something I've actually never quite liked... the fact that we
> > evaluate the expression anyway. I'm pretty happy with -not- evaluating
> > the expression when CONFIG_BUG is on most of the time since whatever is
> > in there is purely here for the sake of the BUG/WARN test.
> 
> Whether we evaluate the expression is a completely different debate.
> I personally agree with you that expressions that have side-effects
> are a stupid idea for either WARN_ON or BUG_ON.

As do I. But we can't really do anything about it short of auditing
them all.

-- 
Mathematics is the supreme nostalgia of our time.