Re: [PATCH 1/3] printk: Fix kdb_trap_printk placement

[Petr Mladek <pmladek@suse.com>]

Hi,

I thought about this a lot from several angles. And I would prefer
sligly different placement, see the patch below.

On Thu 2017-09-28 14:18:24, Peter Zijlstra wrote:
> Some people figured vprintk_emit() makes for a nice API and exported
> it, bypassing the kdb trap.

Sigh, printk() API is pretty complicated and this export
made it much worse. Well, there are two things:

First, kdb_trap_printk name is a bit misleading. It is not a
generic trap of any printk message. Instead it seems to be
used to redirect only particular messages from some existing
functions, e.g. show_regs() called from kdb_dumpregs().

Second, it seems that the only user of the exported vprintk_emit()
is dev_vprintk_emit(). I believe that code using this wrapper
is not called in the sections where kdb_trap_printk is incremented.

As a result, I think that we do not need to handle kdb_trap_printk
in vprintk_emit().


> This still leaves vprintk_nmi() outside of the kbd reach, should that
> be fixed too?

I think that it is safe after all, see the commit message in the patch
below.


>From 0da097266f617c2d62956f3abc8e5f39f119c674 Mon Sep 17 00:00:00 2001
From: Peter Zijlstra <peterz@infradead.org>
Date: Thu, 28 Sep 2017 14:18:24 +0200
Subject: [PATCH 1/3] printk: Fix kdb_trap_printk placement

The counter kdb_trap_printk marks parts of code where we want to redirect
printk() into vkdb_printf(). It is used to reuse existing non-trivial
functions, for example, show_regs() to print some information in
the kdb console.

This patch moves the check into printk_func() where the right
printk implementation is choosen also for other special contexts.

Also it would make sense to get rid of kdb_trap_printk counter
and use printk_context instead. The only problem is that
printk_context is per-CPU. It is most likely safe. It seems
that kdb_trap_printk is incremented only in code that is called
from the kdb console and constroling CPU. But I am not 100% sure.

This change allows to redirect the messages also from NMI or
printk_safe context. It looks safe from the printk() point
of view because kdb code prints many messages directly using
kdb_printf() directly. By other words, if kdb reaches the point
when kdb_trap_printk might be incremented, we should be on
the safe side.

Cc: Jason Wessel <jason.wessel@windriver.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
[pmladek@suse.com: Move the check to printk_func.]
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/printk/printk.c      | 14 +-------------
 kernel/printk/printk_safe.c | 10 ++++++++++
 2 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 512f7c2baedd..e4151b14509d 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -33,7 +33,6 @@
 #include <linux/memblock.h>
 #include <linux/syscalls.h>
 #include <linux/crash_core.h>
-#include <linux/kdb.h>
 #include <linux/ratelimit.h>
 #include <linux/kmsg_dump.h>
 #include <linux/syslog.h>
@@ -1784,18 +1783,7 @@ asmlinkage int printk_emit(int facility, int level,
 
 int vprintk_default(const char *fmt, va_list args)
 {
-	int r;
-
-#ifdef CONFIG_KGDB_KDB
-	/* Allow to pass printk() to kdb but avoid a recursion. */
-	if (unlikely(kdb_trap_printk && kdb_printf_cpu < 0)) {
-		r = vkdb_printf(KDB_MSGSRC_PRINTK, fmt, args);
-		return r;
-	}
-#endif
-	r = vprintk_emit(0, LOGLEVEL_DEFAULT, NULL, 0, fmt, args);
-
-	return r;
+	return vprintk_emit(0, LOGLEVEL_DEFAULT, NULL, 0, fmt, args);
 }
 EXPORT_SYMBOL_GPL(vprintk_default);
 
diff --git a/kernel/printk/printk_safe.c b/kernel/printk/printk_safe.c
index 3cdaeaef9ce1..45136f0c8189 100644
--- a/kernel/printk/printk_safe.c
+++ b/kernel/printk/printk_safe.c
@@ -21,6 +21,7 @@
 #include <linux/smp.h>
 #include <linux/cpumask.h>
 #include <linux/irq_work.h>
+#include <linux/kdb.h>
 #include <linux/printk.h>
 
 #include "internal.h"
@@ -363,6 +364,15 @@ void __printk_safe_exit(void)
 
 __printf(1, 0) int vprintk_func(const char *fmt, va_list args)
 {
+#ifdef CONFIG_KGDB_KDB
+	/*
+	 * Special context where printk() messages should appear on kdb
+	 * console. Allow logging by recursion detection.
+	 */
+	if (unlikely(kdb_trap_printk && kdb_printf_cpu < 0))
+		return vkdb_printf(KDB_MSGSRC_PRINTK, fmt, args);
+#endif
+
 	/* Use extra buffer in NMI when logbuf_lock is taken or in safe mode. */
 	if (this_cpu_read(printk_context) & PRINTK_NMI_CONTEXT_MASK)
 		return vprintk_nmi(fmt, args);
-- 
1.8.5.6