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:   Tue, 19 Jun 2018 09:52:19 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Cc:     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 Mon 2018-06-18 19:07:18, Sergey Senozhatsky wrote:
> 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 :)

Please, let me know what name you would like ;-)


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

This won't be needed in v2 where the decision will be made in vprintk_func().


> > > 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.
> 
> Does this mean that we agreed to handle the printk_nmi per-CPU buffer
> fallback in printk_safe.c?

Yes, I feel persuaded.

Best Regards,
Petr