lkml.org 
[lkml]   [2018]   [Jun]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] printk/nmi: Prevent deadlock when serializing NMI backtraces
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

\
 
 \ /
  Last update: 2018-06-18 11:40    [W:0.192 / U:0.436 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site