linux-kernel - Re: Issue with BUG() in asm-gemeric/bug.h if CONFIG

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <52499E8B.6000702@gmail.com>
Date:	Mon, 30 Sep 2013 08:53:47 -0700
From:	David Daney <ddaney.cavm@...il.com>
To:	Ralf Baechle <ralf@...ux-mips.org>
CC:	linux-kernel@...r.kernel.org, Arnd Bergmann <arnd@...db.de>,
	David Howells <dhowells@...hat.com>,
	Markos Chandras <Markos.Chandras@...tec.com>,
	linux-mips@...ux-mips.org,
	Andrew Pinski <Andrew.Pinski@...iumnetworks.com>,
	John Crispin <blogic@...nwrt.org>
Subject: Re: Issue with BUG() in asm-gemeric/bug.h if CONFIG_BUG=n

On 09/30/2013 07:56 AM, Ralf Baechle wrote:
> Lately I received several patches for build issues that only strike if
> CONFIG_BUG is disabled.  Here's a test case extracted from one of them:
>
> /*
>   * Definition of BUG taken from asm-generic/bug.h for the CONFIG_BUG=n case
>   */
> #define BUG() 	do {} while(0)
>
> int foo(int arg)
> {
> 	int res;
>
> 	if (arg == 1)
> 		res = 23;
> 	else if (arg == 2)
> 		res = 42;
> 	else
> 		BUG();
>
> 	return res;
> }
>
> [ralf@h7 ~]$ gcc -O2 -Wall -c bug.c
> bug.c: In function ‘foo’:
> bug.c:17:2: warning: ‘res’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>    return res;
>    ^
>
> It's fairly obvious to see what's happening here - GCC doesn't know that
> the else case can not be reached, thus razorsharply concludes that res
> may be used uninitialized.
>
> There several locations where MIPS - possibly other architectures as well -
> is affected by this.
>
> I think the definition of BUG should be changed to something like
>
> #define BUG()	unreachable()
> 16304
> unreachable() will depending on the compiler being used, expand either
> into a call to __builtin_unreachable() or where that function is
> unavailable, into do {} while (1).

The *only* reason we have CONFIG_BUG=n is to reduce code size.

Sticking in that empty loop, negates the entire point.

IMHO: We should do one of:
  o Make CONFIG_BUG=y mandatory
  o Ignore the warnings.
  o Fix the warning sites so they quit Warning.

So I don't think the patch is really an improvement over the status quo.

David Daney

>
> __builtin_unreachable() was introduce for GCC 4.5.0.
>
> This means there'd be minor bloat for antique compilers - but probably
> even better code generation for compilers supporting __builtin_unreachable().
>
>    Ralf
>
> Signed-off-by: Ralf Baechle <ralf@...ux-mips.org>
>
>   include/asm-generic/bug.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
> index 7d10f96..6f78771 100644
> --- a/include/asm-generic/bug.h
> +++ b/include/asm-generic/bug.h
> @@ -108,7 +108,7 @@ extern void warn_slowpath_null(const char *file, const int line);
>
>   #else /* !CONFIG_BUG */
>   #ifndef HAVE_ARCH_BUG
> -#define BUG() do {} while(0)
> +#define BUG() unreachable()
>   #endif
>
>   #ifndef HAVE_ARCH_BUG_ON
>
> ----- End forwarded message -----
>
>    Ralf
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/