On Mon 2018-06-18 15:37:38, Sergey Senozhatsky wrote:
On (06/08/18 12:48), Petr Mladek wrote: [..]
diff --git a/include/linux/printk.h b/include/linux/printk.h index 6d7e800affd8..872fbdf8df26 100644 --- a/include/linux/printk.h +++ b/include/linux/printk.h @@ -148,9 +148,13 @@ void early_printk(const char *s, ...) { } #ifdef CONFIG_PRINTK_NMI extern void printk_nmi_enter(void); extern void printk_nmi_exit(void); +extern void printk_nmi_direct_enter(void); +extern void printk_nmi_direct_exit(void); #else static inline void printk_nmi_enter(void) { } static inline void printk_nmi_exit(void) { } +static void printk_nmi_direct_enter(void) { } +static void printk_nmi_direct_exit(void) { }
Can we have better names may be? Since direct printk_nmi is not in fact always `direct'.
What about printk_chatty_nmi_enter(), printk_large_nmi_enter() or something similar?
+#ifdef CONFIG_PRINTK_NMI +__printf(1, 0) int vprintk_nmi(const char *fmt, va_list args); +#else +__printf(1, 0) int vprintk_nmi(const char *fmt, va_list args) { return 0; } +#endif
Hmm, printk_safe.c knows about printk.c, printk.c knows about printk_safe.c.
I am sorry but I do not understand the problem. The function is defined in printk_safe.c and we need to call it also from printk.c. It seems reasonable to declare it in kernel/printk/internal.h.
__printf(1, 0) int vprintk_default(const char *fmt, va_list args); __printf(1, 0) int vprintk_deferred(const char *fmt, va_list args); __printf(1, 0) int vprintk_func(const char *fmt, va_list args); diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index 247808333ba4..cf55bece43d9 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -1845,7 +1845,13 @@ asmlinkage int vprintk_emit(int facility, int level, printk_delay(); /* This stops the holder of console_sem just where we want him */
- logbuf_lock_irqsave(flags);
- printk_safe_enter_irqsave(flags);
- if (in_nmi() && !raw_spin_trylock(&logbuf_lock)) {
printed_len = vprintk_nmi(fmt, args);printk_safe_exit_irqrestore(flags);return printed_len;- } else
raw_spin_lock(&logbuf_lock);OK... Can we do this in vprintk_func()? The race window should be super tiny [if matters at all], but in exchange we don't have to mix nmi, printk, printk_mni, etc.
You are right that it would still solve the main risk (NMI comes inside logbuf_lock critical section).
In fact, the only real risk would be another lock serializing NMIs and printk() called with that lock. This patch removes one in nmi_backtrace() and I am not aware of any other.
The less hairy code really might be worth the rather theoretical risk.
So over all I understand why you did it this way. May be I'd prefer to have less universal but shorter solution (e.g. modify only nmi_backtrace function and put there "printk_nmi_restricted_buffer"), but I won't really object your patch [unless I see some real issues with it].
Thanks in advance. I'll send v2 once we have a conclusion on the function names and includes.
Best Regards, Petr