linux-kernel - Re: [PATCH RESEND] bug: When !CONFIG_BUG, simplify WARN_ON

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]

Date:	Mon, 24 Feb 2014 09:02:35 +0100
From:	Arnd Bergmann <arnd@...db.de>
To:	Josh Triplett <josh@...htriplett.org>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	linux-arch@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RESEND] bug: When !CONFIG_BUG, simplify WARN_ON_ONCE and family

On Saturday 22 February 2014, Josh Triplett wrote:
> When !CONFIG_BUG, WARN_ON and family become simple passthroughs of their
> condition argument; however, WARN_ON_ONCE and family still have
> conditions and a boolean to detect one-time invocation, even though the
> warning they'd emit doesn't exist.  Make the existing definitions
> conditional on CONFIG_BUG, and map them all to the passthrough WARN_ON
> when !CONFIG_BUG.
> 
> This saves 4.4k on a minimized configuration (smaller than
> allnoconfig), and 20.6k with defconfig plus CONFIG_BUG=n.

This looks good, but it reminds me of a patch that I did a while ago
and that got lost while I was on leave:

> +#else /* !CONFIG_BUG */
> +#ifndef HAVE_ARCH_BUG
> +#define BUG() do {} while(0)
> +#endif
> +
> +#ifndef HAVE_ARCH_BUG_ON
> +#define BUG_ON(condition) do { if (condition) ; } while(0)
> +#endif

I've done some analysis of this before[1] and came to the conclusion that
this definition (which I realize you are not changing) is bad.

For one thing, it will cause lots of gcc warnings about code that
should have been unreachable being compiled. It also causes
misoptimizations for code that should be detected as unused or
(worse) lets us run into undefined behavior if we ever get into
the BUG() case.

This means we actually want BUG() to end with __builtin_unreachable()
as in the CONFIG_BUG=y case, and also ensure it actually is
unreachable. As I have shown in [1], the there is a small overhead
of doing this in terms of code size.

> +#ifndef HAVE_ARCH_WARN_ON
> +#define WARN_ON(condition) ({						\
> +	int __ret_warn_on = !!(condition);				\
> +	unlikely(__ret_warn_on);					\
> +})
> +#endif
> +
> +#ifndef WARN
> +#define WARN(condition, format...) ({					\
> +	int __ret_warn_on = !!(condition);				\
> +	unlikely(__ret_warn_on);					\
> +})
> +#endif


FWIW, there is an easy extension to this to get rid of some "unused variable"
warnings, but using the format string in an unreachable part of the macro,
as I did in my patch (but didn't explain there):

@@ -125,6 +126,8 @@ extern void warn_slowpath_null(const char *file, const int line);
 #ifndef WARN
 #define WARN(condition, format...) ({					\
 	int __ret_warn_on = !!(condition);				\
+	if (0 && (__ret_warn_on))					\
+		printk(format);						\
 	unlikely(__ret_warn_on);					\
 })
 #endif


	Arnd

[1] http://lkml.org/lkml/2013/7/12/121
--
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/