[PATCH] Make snd_BUG_ON() always evaluate and return the conditional expression.

[PATCH] Make snd_BUG_ON() always evaluate and return the conditional expression.

[Christine Spang]

Having snd_BUG_ON() only evaluate its conditional when CONFIG_SND_DEBUG
is set leads to frequent bugs, since other similar macros in the kernel
have different behavior. Let's make snd_BUG_ON() act like those macros
so it will stop being accidentally misused.

Signed-off-by: Christine Spang <christine.spang@oracle.com>
---
 Documentation/DocBook/writing-an-alsa-driver.tmpl | 12 +++++-------
 include/sound/core.h                              | 24 ++++++++---------------
 2 files changed, 13 insertions(+), 23 deletions(-)

diff --git a/Documentation/DocBook/writing-an-alsa-driver.tmpl b/Documentation/DocBook/writing-an-alsa-driver.tmpl
index bd6fee2..06741e9 100644
--- a/Documentation/DocBook/writing-an-alsa-driver.tmpl
+++ b/Documentation/DocBook/writing-an-alsa-driver.tmpl
@@ -6164,14 +6164,12 @@ struct _snd_pcm_runtime {
 
       <para>
         The macro takes an conditional expression to evaluate.
-	When <constant>CONFIG_SND_DEBUG</constant>, is set, the
-	expression is actually evaluated. If it's non-zero, it shows
-	the warning message such as
+	When <constant>CONFIG_SND_DEBUG</constant>, is set, if the
+	expression is non-zero, it shows the warning message such as
 	<computeroutput>BUG? (xxx)</computeroutput>
-	normally followed by stack trace.  It returns the evaluated
-	value.
-	When no <constant>CONFIG_SND_DEBUG</constant> is set, this
-	macro always returns zero.
+	normally followed by stack trace.
+
+	In both cases it returns the evaluated value.
       </para>
 
     </section>
diff --git a/include/sound/core.h b/include/sound/core.h
index 7cede2d..a63680b 100644
--- a/include/sound/core.h
+++ b/include/sound/core.h
@@ -379,18 +379,10 @@ void __snd_printk(unsigned int level, const char *file, int line,
  * snd_BUG_ON - debugging check macro
  * @cond: condition to evaluate
  *
- * When CONFIG_SND_DEBUG is set, this macro evaluates the given condition,
- * and call WARN() and returns the value if it's non-zero.
- * 
- * When CONFIG_SND_DEBUG is not set, this just returns zero, and the given
- * condition is ignored.
- *
- * NOTE: the argument won't be evaluated at all when CONFIG_SND_DEBUG=n.
- * Thus, don't put any statement that influences on the code behavior,
- * such as pre/post increment, to the argument of this macro.
- * If you want to evaluate and give a warning, use standard WARN_ON().
+ * Has the same behavior as WARN_ON when CONFIG_SND_DEBUG is set,
+ * otherwise just evaluates the conditional and returns the value.
  */
-#define snd_BUG_ON(cond)	WARN((cond), "BUG? (%s)\n", __stringify(cond))
+#define snd_BUG_ON(cond)	WARN_ON((cond))
 
 #else /* !CONFIG_SND_DEBUG */
 
@@ -400,11 +392,11 @@ __printf(2, 3)
 static inline void _snd_printd(int level, const char *format, ...) {}
 
 #define snd_BUG()			do { } while (0)
-static inline int __snd_bug_on(int cond)
-{
-	return 0;
-}
-#define snd_BUG_ON(cond)	__snd_bug_on(0 && (cond))  /* always false */
+
+#define snd_BUG_ON(condition) ({ \
+	int __ret_warn_on = !!(condition); \
+	unlikely(__ret_warn_on); \
+})
 
 #endif /* CONFIG_SND_DEBUG */
 
-- 
1.8.2.rc0

Re: [PATCH] Make snd_BUG_ON() always evaluate and return the conditional expression.

[Takashi Iwai]

At Mon,  4 Mar 2013 17:02:59 -0500,
Christine Spang wrote:
> 
> Having snd_BUG_ON() only evaluate its conditional when CONFIG_SND_DEBUG
> is set leads to frequent bugs, since other similar macros in the kernel
> have different behavior. Let's make snd_BUG_ON() act like those macros
> so it will stop being accidentally misused.
> 
> Signed-off-by: Christine Spang <christine.spang@oracle.com>

Sounds reasonable.  The dependency on CONFIG_SND_DEBUG was for
allowing more optimization, but since we use this for more places than
expected, this change would be safer indeed.

If no one has objection, I'll apply it for 3.10 kernel.


thanks,

Takashi

> ---
>  Documentation/DocBook/writing-an-alsa-driver.tmpl | 12 +++++-------
>  include/sound/core.h                              | 24 ++++++++---------------
>  2 files changed, 13 insertions(+), 23 deletions(-)
> 
> diff --git a/Documentation/DocBook/writing-an-alsa-driver.tmpl b/Documentation/DocBook/writing-an-alsa-driver.tmpl
> index bd6fee2..06741e9 100644
> --- a/Documentation/DocBook/writing-an-alsa-driver.tmpl
> +++ b/Documentation/DocBook/writing-an-alsa-driver.tmpl
> @@ -6164,14 +6164,12 @@ struct _snd_pcm_runtime {
>  
>        <para>
>          The macro takes an conditional expression to evaluate.
> -	When <constant>CONFIG_SND_DEBUG</constant>, is set, the
> -	expression is actually evaluated. If it's non-zero, it shows
> -	the warning message such as
> +	When <constant>CONFIG_SND_DEBUG</constant>, is set, if the
> +	expression is non-zero, it shows the warning message such as
>  	<computeroutput>BUG? (xxx)</computeroutput>
> -	normally followed by stack trace.  It returns the evaluated
> -	value.
> -	When no <constant>CONFIG_SND_DEBUG</constant> is set, this
> -	macro always returns zero.
> +	normally followed by stack trace.
> +
> +	In both cases it returns the evaluated value.
>        </para>
>  
>      </section>
> diff --git a/include/sound/core.h b/include/sound/core.h
> index 7cede2d..a63680b 100644
> --- a/include/sound/core.h
> +++ b/include/sound/core.h
> @@ -379,18 +379,10 @@ void __snd_printk(unsigned int level, const char *file, int line,
>   * snd_BUG_ON - debugging check macro
>   * @cond: condition to evaluate
>   *
> - * When CONFIG_SND_DEBUG is set, this macro evaluates the given condition,
> - * and call WARN() and returns the value if it's non-zero.
> - * 
> - * When CONFIG_SND_DEBUG is not set, this just returns zero, and the given
> - * condition is ignored.
> - *
> - * NOTE: the argument won't be evaluated at all when CONFIG_SND_DEBUG=n.
> - * Thus, don't put any statement that influences on the code behavior,
> - * such as pre/post increment, to the argument of this macro.
> - * If you want to evaluate and give a warning, use standard WARN_ON().
> + * Has the same behavior as WARN_ON when CONFIG_SND_DEBUG is set,
> + * otherwise just evaluates the conditional and returns the value.
>   */
> -#define snd_BUG_ON(cond)	WARN((cond), "BUG? (%s)\n", __stringify(cond))
> +#define snd_BUG_ON(cond)	WARN_ON((cond))
>  
>  #else /* !CONFIG_SND_DEBUG */
>  
> @@ -400,11 +392,11 @@ __printf(2, 3)
>  static inline void _snd_printd(int level, const char *format, ...) {}
>  
>  #define snd_BUG()			do { } while (0)
> -static inline int __snd_bug_on(int cond)
> -{
> -	return 0;
> -}
> -#define snd_BUG_ON(cond)	__snd_bug_on(0 && (cond))  /* always false */
> +
> +#define snd_BUG_ON(condition) ({ \
> +	int __ret_warn_on = !!(condition); \
> +	unlikely(__ret_warn_on); \
> +})
>  
>  #endif /* CONFIG_SND_DEBUG */
>  
> -- 
> 1.8.2.rc0
> 

Re: [PATCH] Make snd_BUG_ON() always evaluate and return the conditional expression.

[Christine Spang]

On 03/05/2013 04:05 AM, Takashi Iwai wrote:
> At Mon,  4 Mar 2013 17:02:59 -0500,
> Christine Spang wrote:
>> Having snd_BUG_ON() only evaluate its conditional when CONFIG_SND_DEBUG
>> is set leads to frequent bugs, since other similar macros in the kernel
>> have different behavior. Let's make snd_BUG_ON() act like those macros
>> so it will stop being accidentally misused.
>>
>> Signed-off-by: Christine Spang <christine.spang@oracle.com>
> Sounds reasonable.  The dependency on CONFIG_SND_DEBUG was for
> allowing more optimization, but since we use this for more places than
> expected, this change would be safer indeed.
>
> If no one has objection, I'll apply it for 3.10 kernel.
>
>
> thanks,
>
> Takashi

This ought to be considered for 3.9 and stable@ as
well. It fixes NULL derefs all over the place, e.g.

sound/core/device.c:126

    if (snd_BUG_ON(!card || !device_data))
        return -ENXIO;
    list_for_each_entry(dev, &card->devices, list) {
       [...]

If card == NULL and CONFIG_SND_DEBUG is off, this code will NULL deref.

There are some 600 other instances of snd_BUG_ON() being used dubiously
in the current tree. Some of these instances even perform extra cleanup
before returning in error conditions. It's really broken with
CONFIG_SND_DEBUG off, and no major distro ships production kernels with
this setting enabled.

Christine

Re: [PATCH] Make snd_BUG_ON() always evaluate and return the conditional expression.

[Takashi Iwai]

At Tue, 05 Mar 2013 15:41:06 -0500,
Christine Spang wrote:
> 
> On 03/05/2013 04:05 AM, Takashi Iwai wrote:
> > At Mon,  4 Mar 2013 17:02:59 -0500,
> > Christine Spang wrote:
> >> Having snd_BUG_ON() only evaluate its conditional when CONFIG_SND_DEBUG
> >> is set leads to frequent bugs, since other similar macros in the kernel
> >> have different behavior. Let's make snd_BUG_ON() act like those macros
> >> so it will stop being accidentally misused.
> >>
> >> Signed-off-by: Christine Spang <christine.spang@oracle.com>
> > Sounds reasonable.  The dependency on CONFIG_SND_DEBUG was for
> > allowing more optimization, but since we use this for more places than
> > expected, this change would be safer indeed.
> >
> > If no one has objection, I'll apply it for 3.10 kernel.
> >
> >
> > thanks,
> >
> > Takashi
> 
> This ought to be considered for 3.9 and stable@ as
> well. It fixes NULL derefs all over the place, e.g.

No, it doesn't "fix".

> sound/core/device.c:126
> 
>     if (snd_BUG_ON(!card || !device_data))
>         return -ENXIO;
>     list_for_each_entry(dev, &card->devices, list) {
>        [...]
> 
> If card == NULL and CONFIG_SND_DEBUG is off, this code will NULL deref.

Yes, but such a condition must not happen.  If card = NULL is passed,
it's an error of the caller side, and the code there doesn't have to
take care in general.

In other words, it's a good stuff to catch a bug.  But it's never been
there for "fixing" anything per se.  That's the reason why it is
turned off when no debug option is set.

But I agree that enabling such a check would be safer, indeed, since
people tend to rely on the checks.  So I'm for your patch, but this
isn't the thing for the current tree or stable trees.

> There are some 600 other instances of snd_BUG_ON() being used dubiously
> in the current tree. Some of these instances even perform extra cleanup
> before returning in error conditions.

Give more concrete examples.  Such places must be fixed instead.

> It's really broken with
> CONFIG_SND_DEBUG off, and no major distro ships production kernels with
> this setting enabled.

If it's broken, it's the caller side, not the fact that snd_BUG_ON()
isn't compiled in.


Takashi

Re: [alsa-devel] [PATCH] Make snd_BUG_ON() always evaluate and return the conditional expression.

[David Henningsson]

2013-03-05 21:41, Christine Spang skrev:
> On 03/05/2013 04:05 AM, Takashi Iwai wrote:
>> At Mon,  4 Mar 2013 17:02:59 -0500,
>> Christine Spang wrote:
>>> Having snd_BUG_ON() only evaluate its conditional when CONFIG_SND_DEBUG
>>> is set leads to frequent bugs, since other similar macros in the kernel
>>> have different behavior. Let's make snd_BUG_ON() act like those macros
>>> so it will stop being accidentally misused.
>>>
>>> Signed-off-by: Christine Spang <christine.spang@oracle.com>
>> Sounds reasonable.  The dependency on CONFIG_SND_DEBUG was for
>> allowing more optimization, but since we use this for more places than
>> expected, this change would be safer indeed.
>>
>> If no one has objection, I'll apply it for 3.10 kernel.

If snd_BUG_ON now works like WARN_ON rather than BUG_ON (at least it 
does with this change, if I understand things right), maybe we should 
rename it to snd_WARN_ON for consistency?


-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

Re: [alsa-devel] [PATCH] Make snd_BUG_ON() always evaluate and return the conditional expression.

[Takashi Iwai]

At Wed, 06 Mar 2013 14:49:28 +0100,
David Henningsson wrote:
> 
> 2013-03-05 21:41, Christine Spang skrev:
> > On 03/05/2013 04:05 AM, Takashi Iwai wrote:
> >> At Mon,  4 Mar 2013 17:02:59 -0500,
> >> Christine Spang wrote:
> >>> Having snd_BUG_ON() only evaluate its conditional when CONFIG_SND_DEBUG
> >>> is set leads to frequent bugs, since other similar macros in the kernel
> >>> have different behavior. Let's make snd_BUG_ON() act like those macros
> >>> so it will stop being accidentally misused.
> >>>
> >>> Signed-off-by: Christine Spang <christine.spang@oracle.com>
> >> Sounds reasonable.  The dependency on CONFIG_SND_DEBUG was for
> >> allowing more optimization, but since we use this for more places than
> >> expected, this change would be safer indeed.
> >>
> >> If no one has objection, I'll apply it for 3.10 kernel.
> 
> If snd_BUG_ON now works like WARN_ON rather than BUG_ON (at least it 
> does with this change, if I understand things right),

No, snd_BUG_ON() has always been equivalent with WARN_ON() when
CONFIG_SND_DEBUG is set.  But it's empty when CONFIG_SND_DEBUG=n
(i.e. the conditional is ignored).

Christine's patch changes the behavior in only the latter case.  It
enables the conditional but doesn't involve WARN_ON(), so the check is
done silently.

> maybe we should 
> rename it to snd_WARN_ON for consistency?

Maybe.  As an additional note, BUG_ON() should be almost never used in
the normal driver codes. If you find BUG_ON() in a driver code, doubt
it twice whether it's right.


Takashi