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 <[email protected]> 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 <[email protected]> Signed-off-by: Peter Zijlstra (Intel) <[email protected]> [[email protected]: Move the check to printk_func.] Signed-off-by: Petr Mladek <[email protected]> --- 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
