linux-kernel - [GIT PULL v2] tracing/NMI/printk: Use seq_buf for safe printing from NMI context

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>] [day] [month] [year] [list]

Message-ID: <20141212233307.509234ad@gandalf.local.home>
Date:	Fri, 12 Dec 2014 23:33:07 -0500
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Ingo Molnar <mingo@...nel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Jiri Kosina <jkosina@...e.cz>, Petr Mladek <pmladek@...e.cz>
Subject: [GIT PULL v2] tracing/NMI/printk: Use seq_buf for safe printing
 from NMI context

Linus,

This code is a fork from the trace-3.19 pull as it needed the trace_seq
clean ups from that branch.

This code solves the issue of performing stack dumps from NMI context.
The issue is that printk() is not safe from NMI context as if the NMI
were to trigger when a printk() was being performed, the NMI could
deadlock from the printk() internal locks. This has been seen in practice.

With lots of review from Petr Mladek, this code went through several
iterations, and we feel that it is now at a point of quality to be
accepted into mainline.

Here's what is contained in this patch set:

 o Creates a "seq_buf" generic buffer utility that allows a descriptor
   to be passed around where functions can write their own "printk()"
   formatted strings into it. The generic version was pulled out of
   the trace_seq() code that was made specifically for tracing.

 o The seq_buf code was change to model the seq_file code. I have
   a patch (not included for 3.19) that converts the seq_file.c code
   over to use seq_buf.c like the trace_seq.c code does. This was done
   to make sure that seq_buf.c is compatible with seq_file.c. I may
   try to get that patch in for 3.20.

 o The seq_buf.c file was moved to lib/ to remove it from being dependent
   on CONFIG_TRACING.

 o The printk() was updated to allow for a per_cpu "override" of
   the internal calls. That is, instead of writing to the console, a call
   to printk() may do something else. This made it easier to allow the
   NMI to change what printk() does in order to call dump_stack() without
   needing to update that code as well.

 o Finally, the dump_stack from all CPUs via NMI code was converted to
   use the seq_buf code. The caller to trigger the NMI code would wait
   till all the NMIs finished, and then it would print the seq_buf
   data to the console safely from a non NMI context.

[ Updated to remove unnecessary preempt_disable in printk() ]

Note, I only showed the difference between V1, as that full diff was
already posted here:

  http://lkml.kernel.org/r/20141208100528.5f97106f@gandalf.local.home

Please pull the latest trace-seq-buf-3.19-v2 tree, which can be found at:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
trace-seq-buf-3.19-v2

Tag SHA1: b7a8e382200950f8f596376bfef82754569ccbaf
Head SHA1: 1fb8915b9876a80f43732980208b39d013f8da9d

Steven Rostedt (Red Hat) (1):
      printk: Do not disable preemption for accessing printk_func

----
 kernel/printk/printk.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
---------------------------
commit 1fb8915b9876a80f43732980208b39d013f8da9d
Author: Steven Rostedt (Red Hat) <rostedt@...dmis.org>
Date:   Thu Dec 11 09:12:01 2014 -0500

    printk: Do not disable preemption for accessing printk_func

    As printk_func will either be the default function, or a per_cpu function
    for the current CPU, there's no reason to disable preemption to access
    it from printk. That's because if the printk_func is not the default
    then the caller had better disabled preemption as they were the one to
    change it.

    Link: http://lkml.kernel.org/r/CA+55aFz5-_LKW4JHEBoWinN9_ouNcGRWAF2FUA35u46FRN-Kxw@mail.gmail.com

    Suggested-by: Linus Torvalds <torvalds@...ux-foundation.org>
    Signed-off-by: Steven Rostedt <rostedt@...dmis.org>

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 5af2b8bc88f0..9b896e7a50a9 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1859,10 +1859,16 @@ asmlinkage __visible int printk(const char *fmt, ...)
 	int r;

 	va_start(args, fmt);
-	preempt_disable();
+
+	/*
+	 * If a caller overrides the per_cpu printk_func, then it needs
+	 * to disable preemption when calling printk(). Otherwise
+	 * the printk_func should be set to the default. No need to
+	 * disable preemption here.
+	 */
 	vprintk_func = this_cpu_read(printk_func);
 	r = vprintk_func(fmt, args);
-	preempt_enable();
+
 	va_end(args);

 	return r;
--
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/