lkml.org 
[lkml]   [2017]   [Oct]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH 1/3] printk: Fix kdb_trap_printk placement
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
\
 
 \ /
  Last update: 2017-10-12 11:46    [W:0.116 / U:4.808 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site