linux-kernel - Re: [PATCH 1/4] rcu: Allow for page faults in NMI handlers

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]

Message-Id: <20170925003456.GA13412@linux.vnet.ibm.com>
Date:   Sun, 24 Sep 2017 17:34:56 -0700
From:   "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Steven Rostedt <rostedt@...dmis.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        stable <stable@...r.kernel.org>
Subject: Re: [PATCH 1/4] rcu: Allow for page faults in NMI handlers

On Sun, Sep 24, 2017 at 05:26:53PM -0700, Paul E. McKenney wrote:
> On Sun, Sep 24, 2017 at 05:12:13PM -0700, Linus Torvalds wrote:
> > On Sun, Sep 24, 2017 at 5:03 PM, Paul E. McKenney
> > <paulmck@...ux.vnet.ibm.com> wrote:
> > >
> > > Mostly just paranoia on my part.  I would be happy to remove it if
> > > you prefer.  Or you or Steve can do so if that is more convenient.
> > 
> > I really don't think it's warranted. The values are *stable*. There's
> > no subtle lack of locking, or some optimistic access to a value that
> > can change.
> > 
> > The compiler can generate code to read the value fifteen billion
> > times, and it will always get the same value.
> > 
> > Yes, maybe in between the different accesses, an NMI will happen, and
> > the value will be incremented, but then as the NMI exits, it will
> > decrement again, so the code that got interrupted will not actually
> > see the change.
> > 
> > So the READ_ONCE() isn't "paranoia". It's just confusing.
> > 
> > > And yes, consistency would dictate that the uses in rcu_nmi_enter()
> > > and rcu_nmi_exit() should be _ONCE(), particularly the stores to
> > > ->dynticks_nmi_nesting.
> > 
> > NO.
> > 
> > That would be just more of that confusion.
> > 
> > That value is STABLE. It's stable even within an NMI handler. The NMI
> > code can read it, modify it, write it back, do a little dance, all
> > without having to care. There's no "_ONCE()" about it - not for the
> > readers, not for the writers, not for _anybody_.
> > 
> > So adding even more READ/WRITE_ONCE() accesses wouldn't be
> > "consistent", it would just be insanity.
> > 
> > Now, if an NMI happens and the value would be different on entry than
> > it is on exit, that would be something else. Then it really wouldn't
> > be stable wrt random users. But that would also be a major bug in the
> > NMI handler, as far as I can tell.
> > 
> > So the reason I'm objecting to that READ_ONCE() is that it isn't
> > "paranoia", it's "voodoo programming". And we don't do voodoo
> > programming.
> 
> I already agreed that the READ_ONCE() can be removed.

And for whatever it is worth, here is the updated patch.

							Thanx, Paul

------------------------------------------------------------------------

commit 3e2baa988b9c13095995c46c51e0e32c0b6a7d43
Author: Paul E. McKenney <paulmck@...ux.vnet.ibm.com>
Date:   Fri Sep 22 13:14:42 2017 -0700

    rcu: Allow for page faults in NMI handlers
    
    A number of architecture invoke rcu_irq_enter() on exception entry in
    order to allow RCU read-side critical sections in the exception handler
    when the exception is from an idle or nohz_full CPU.  This works, at
    least unless the exception happens in an NMI handler.  In that case,
    rcu_nmi_enter() would already have exited the extended quiescent state,
    which would mean that rcu_irq_enter() would (incorrectly) cause RCU
    to think that it is again in an extended quiescent state.  This will
    in turn result in lockdep splats in response to later RCU read-side
    critical sections.
    
    This commit therefore causes rcu_irq_enter() and rcu_irq_exit() to
    take no action if there is an rcu_nmi_enter() in effect, thus avoiding
    the unscheduled return to RCU quiescent state.  This in turn should
    make the kernel safe for on-demand RCU voyeurism.
    
    Reported-by: Steven Rostedt <rostedt@...dmis.org>
    Signed-off-by: Paul E. McKenney <paulmck@...ux.vnet.ibm.com>
    [ paulmck: Remove READ_ONCE() per Linux Torvalds feedback. ]

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index db5eb8c3f7af..e4fe06d42385 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -891,6 +891,11 @@ void rcu_irq_exit(void)
 
 	RCU_LOCKDEP_WARN(!irqs_disabled(), "rcu_irq_exit() invoked with irqs enabled!!!");
 	rdtp = this_cpu_ptr(&rcu_dynticks);
+
+	/* Page faults can happen in NMI handlers, so check... */
+	if (rdtp->dynticks_nmi_nesting)
+		return;
+
 	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
 		     rdtp->dynticks_nesting < 1);
 	if (rdtp->dynticks_nesting <= 1) {
@@ -1036,6 +1041,11 @@ void rcu_irq_enter(void)
 
 	RCU_LOCKDEP_WARN(!irqs_disabled(), "rcu_irq_enter() invoked with irqs enabled!!!");
 	rdtp = this_cpu_ptr(&rcu_dynticks);
+
+	/* Page faults can happen in NMI handlers, so check... */
+	if (rdtp->dynticks_nmi_nesting)
+		return;
+
 	oldval = rdtp->dynticks_nesting;
 	rdtp->dynticks_nesting++;
 	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&