From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf0-f196.google.com ([209.85.192.196]:47058 "EHLO mail-pf0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934694AbeFRKHX (ORCPT ); Mon, 18 Jun 2018 06:07:23 -0400 Date: Mon, 18 Jun 2018 19:07:18 +0900 From: Sergey Senozhatsky To: Petr Mladek Cc: Sergey Senozhatsky , Sergey Senozhatsky , Steven Rostedt , Peter Zijlstra , Tetsuo Handa , linux-kernel@vger.kernel.org, "4 . 13+" Subject: Re: [PATCH] printk/nmi: Prevent deadlock when serializing NMI backtraces Message-ID: <20180618100718.GA555@jagdpanzerIV> References: <20180517143903.19339-1-pmladek@suse.com> <20180605124729.3vix5nlkjpjzdljx@pathway.suse.cz> <20180606051029.GA19211@tigerII.localdomain> <20180606103356.GB19211@tigerII.localdomain> <20180608104825.e7xoxteelaxnwx66@pathway.suse.cz> <20180618063738.GC27996@jagdpanzerIV> <20180618093917.qeaatnlqh5if6t3r@pathway.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180618093917.qeaatnlqh5if6t3r@pathway.suse.cz> Sender: stable-owner@vger.kernel.org List-ID: 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