Re: [PATCH] printk/nmi: Prevent deadlock when serializing NMI backtraces

[Sergey Senozhatsky]

On (06/18/18 11:39), Petr Mladek wrote:
[..]
> > >  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?

Hmm. Can't answer right now :)

> > > +#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.

Just wanted to suggest to keep printk_safe/printk_nmi stuff in printk_safe.c.
We already have everything we need there, so let's just add the vprintk_nmi()
fallback, avoiding spreading printk_safe/printk_nmi logic and details across
printk.c and printk_safe.c

> > 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.

Does this mean that we agreed to handle the printk_nmi per-CPU buffer
fallback in printk_safe.c?

	-ss