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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 18 Jun 2018 19:07:18 +0900
From:   Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
To:     Petr Mladek <pmladek@...e.com>
Cc:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>,
        linux-kernel@...r.kernel.org, "4 . 13+" <stable@...r.kernel.org>
Subject: Re: [PATCH] printk/nmi: Prevent deadlock when serializing NMI
 backtraces

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