linux-kernel - Re: [PATCHv7 4/8] printk: always use deferred printk when flush printk

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: <20170206014846.GA1750@jagdpanzerIV.localdomain>
Date:   Mon, 6 Feb 2017 10:48:46 +0900
From:   Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
To:     Steven Rostedt <rostedt@...dmis.org>,
        Petr Mladek <pmladek@...e.com>
Cc:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Jan Kara <jack@...e.cz>, Tejun Heo <tj@...nel.org>,
        Calvin Owens <calvinowens@...com>,
        Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Andy Lutomirski <luto@...nel.org>,
        Peter Hurley <peter@...leysoftware.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCHv7 4/8] printk: always use deferred printk when flush
 printk_safe lines

On (02/03/17 12:18), Petr Mladek wrote:
[..]
> > by "the new approved variant" do you mean resend of the entire
> > patch set (v8) or just 0004 patch?
> 
> I depends on how many and how complicated are possible conflicts
> with the followup patches.
> 
> It is perfectly fine to send just 0004 if there are no conflicts
> or if they are trivial to resolve.

so there are no conflicts at all. 0004 can be replaced with the patch below.
but the thing is - I want to keep printk_safe_flush_line() around.

we call

static void printk_safe_flush_line(const char *text, int len)
{
	/*
	 * Avoid any console drivers calls from here, because we may be
	 * in NMI or printk_safe context (when in panic). The messages
	 * must go only into the ring buffer at this stage.  Consoles will
	 * get explicitly called later when a crashdump is not generated.
	 */
	printk_deferred("%.*s", len, text);
}

from many places:

  4x   printk_safe_flush_buffer()
  1x   __printk_safe_flush()


replacing printk_safe_flush_line() with printk_deferred() produces things
like these

               printk_deferred("%.*s", end - start, start);
               printk_deferred("%.*s", strlen(newline), newline);

which, I think, makes the code worse. any objections?

just in case, the patch (which I prefer to be ignored)

===8<======8<===

>From daa9bd8c2659a91036c00e22bb218be6871a371d Mon Sep 17 00:00:00 2001
From: Sergey Senozhatsky <sergey.senozhatsky@...il.com>
Date: Tue, 27 Dec 2016 23:16:07 +0900
Subject: [PATCH] printk: always use deferred printk when flush printk_safe
 lines

Always use printk_deferred() in printk_safe_flush_line().
Flushing can be done from NMI or printk_safe contexts (when
we are in panic), so we can't call console drivers, yet still
want to store the messages in the logbuf buffer. Therefore we
use a deferred printk version.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@...il.com>
Suggested-by: Petr Mladek <pmladek@...e.com>
---
 kernel/printk/printk_safe.c | 30 ++++++++++++------------------
 1 file changed, 12 insertions(+), 18 deletions(-)

diff --git a/kernel/printk/printk_safe.c b/kernel/printk/printk_safe.c
index efc89a4e9df5..801f0f1c7547 100644
--- a/kernel/printk/printk_safe.c
+++ b/kernel/printk/printk_safe.c
@@ -110,19 +110,6 @@ static int printk_safe_log_store(struct printk_safe_seq_buf *s,
 	return add;
 }
 
-static void printk_safe_flush_line(const char *text, int len)
-{
-	/*
-	 * The buffers are flushed in NMI only on panic.  The messages must
-	 * go only into the ring buffer at this stage.  Consoles will get
-	 * explicitly called later when a crashdump is not generated.
-	 */
-	if (in_nmi())
-		printk_deferred("%.*s", len, text);
-	else
-		printk("%.*s", len, text);
-}
-
 /* printk part of the temporary buffer line by line */
 static int printk_safe_flush_buffer(const char *start, size_t len)
 {
@@ -136,7 +123,14 @@ static int printk_safe_flush_buffer(const char *start, size_t len)
 	/* Print line by line. */
 	while (c < end) {
 		if (*c == '\n') {
-			printk_safe_flush_line(start, c - start + 1);
+			/*
+			 * Avoid any console drivers calls from here, because
+			 * we may be in NMI or printk_safe context (when in
+			 * panic). The messages must go only into the ring
+			 * buffer at this stage.  Consoles will get explicitly
+			 * called later when a crashdump is not generated.
+			 */
+			printk_deferred("%.*s", c - start + 1, start);
 			start = ++c;
 			header = true;
 			continue;
@@ -149,7 +143,7 @@ static int printk_safe_flush_buffer(const char *start, size_t len)
 				continue;
 			}
 
-			printk_safe_flush_line(start, c - start);
+			printk_deferred("%.*s", c - start, start);
 			start = c++;
 			header = true;
 			continue;
@@ -163,8 +157,8 @@ static int printk_safe_flush_buffer(const char *start, size_t len)
 	if (start < end && !header) {
 		static const char newline[] = KERN_CONT "\n";
 
-		printk_safe_flush_line(start, end - start);
-		printk_safe_flush_line(newline, strlen(newline));
+		printk_deferred("%.*s", end - start, start);
+		printk_deferred("%.*s", strlen(newline), newline);
 	}
 
 	return len;
@@ -206,7 +200,7 @@ static void __printk_safe_flush(struct irq_work *work)
 	if ((i && i >= len) || len > sizeof(s->buffer)) {
 		const char *msg = "printk_safe_flush: internal error\n";
 
-		printk_safe_flush_line(msg, strlen(msg));
+		printk_deferred("%.*s", strlen(msg), msg);
 		len = 0;
 	}
 
-- 
2.11.1