linux-kernel - Re: [PATCH] Fix print out of function which called WARN

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:	Fri, 15 May 2009 10:52:44 -0700 (PDT)
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Ian Campbell <ian.campbell@...rix.com>,
	Jakub Jelinek <jakub@...hat.com>
cc:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Jesper Nilsson <jesper.nilsson@...s.com>,
	Johannes Weiner <hannes@...xchg.org>,
	Arjan van de Ven <arjan@...ux.intel.com>,
	Andi Kleen <ak@...ux.intel.com>,
	Hugh Dickins <hugh@...itas.com>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] Fix print out of function which called WARN_ON()


[ Jakub - I added you to the participants list because it really looks 
  like this patch (without 'noinline') triggers a gcc bug.

  Or maybe gcc is truly smart enough to turn the "NULL arg pointer" case 
  into a "empty format" call, in which case we actually just solved the 
  original warning case in a _really_ subtle way. Somebody who understands 
  gcc varags should take a look.

  Regardless, even if the optimization happens to _work_, it seems bogus. 
  Even if "vprintk()" has been marked to have a printf-like format, that 
  doesn't mean that you can call it unconditionally by making the format 
  empty - it might have side effects.

  This is with 'gcc -v' reporting:

	gcc version 4.4.0 20090506 (Red Hat 4.4.0-4) (GCC) 

  and is the current fedora-11 compiler (as of a yum update yesterday or 
  the day before) on x86-64 ]

On Fri, 15 May 2009, Ian Campbell wrote:
>
> All WARN_ON()'s currently appear to come from warn_slowpath_null e.g.:
>  WARNING: at kernel/softirq.c:143 warn_slowpath_null+0x1c/0x20()
> 
> This is because since:
> 
>   commit 57adc4d2dbf968fdbe516359688094eef4d46581
>   Author: Andi Kleen <andi@...stfloor.org>
>   Date:   Wed May 6 16:02:53 2009 -0700
> 
>       Eliminate thousands of warnings with gcc 3.2 build
> 
> the caller of warn_slowpath_fmt really is warn_flowpath_null not the
> interesting caller next up the chain. Since __builtin_return_address(X) for X >
> 0 is not reliable, pass the real caller as an argument to warn_slowpath_fmt.

I'd actually much rather re-organize it so that there are the two exported 
functions (with the current interface) that use a common shared static 
function. Rather than passing in '0' in a macro.

IOW, something like this..

Btw, when doing this, it really looked to me like the "noinline" matters. 
I did it first to try to make the assembly clearer to read, but without it 
gcc-4.4 seems to actually generate the wrong code, and does an 
_unconditional_ call to vprintk().

Very odd. I've seen other bugs triggered by gcc-4.4 (alpine miscompiles), 
it all makes me very nervous about the compiler in current fedora-11.

Anybody want to double-check this?

		Linus

---
 kernel/panic.c |   35 ++++++++++++++++++++---------------
 1 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index 874ecf1..3c8048d 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -340,39 +340,44 @@ void oops_exit(void)
 }
 
 #ifdef WANT_WARN_ON_SLOWPATH
-void warn_slowpath_fmt(const char *file, int line, const char *fmt, ...)
-{
+struct slowpath_args {
+	const char *fmt;
 	va_list args;
-	char function[KSYM_SYMBOL_LEN];
-	unsigned long caller = (unsigned long)__builtin_return_address(0);
-	const char *board;
+};
 
-	sprint_symbol(function, caller);
+static void noinline warn_slowpath_common(const char *file, int line, void *caller, struct slowpath_args *args)
+{
+	const char *board;
 
 	printk(KERN_WARNING "------------[ cut here ]------------\n");
-	printk(KERN_WARNING "WARNING: at %s:%d %s()\n", file,
-		line, function);
+	printk(KERN_WARNING "WARNING: at %s:%d %pS()\n", file, line, caller);
 	board = dmi_get_system_info(DMI_PRODUCT_NAME);
 	if (board)
 		printk(KERN_WARNING "Hardware name: %s\n", board);
 
-	if (*fmt) {
-		va_start(args, fmt);
-		vprintk(fmt, args);
-		va_end(args);
-	}
+	if (args)
+		vprintk(args->fmt, args->args);
 
 	print_modules();
 	dump_stack();
 	print_oops_end_marker();
 	add_taint(TAINT_WARN);
 }
+
+void warn_slowpath_fmt(const char *file, int line, const char *fmt, ...)
+{
+	struct slowpath_args args;
+
+	args.fmt = fmt;
+	va_start(args.args, fmt);
+	warn_slowpath_common(file, line, __builtin_return_address(0), &args);
+	va_end(args.args);
+}
 EXPORT_SYMBOL(warn_slowpath_fmt);
 
 void warn_slowpath_null(const char *file, int line)
 {
-	static const char *empty = "";
-	warn_slowpath_fmt(file, line, empty);
+	warn_slowpath_fmt(file, line, __builtin_return_address(0), NULL);
 }
 EXPORT_SYMBOL(warn_slowpath_null);
 #endif
--
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/