Date: Thu, 9 Nov 2017 09:56:54 -0800
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
To: Ben Hutchings <ben.hutchings@codethink.co.uk>
Cc: Steven Rostedt <rostedt@goodmis.org>, stable@vger.kernel.org,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [4.4,06/28] rcu: Allow for page faults in NMI handlers
Reply-To: paulmck@linux.vnet.ibm.com
References: <20171016160923.860937466@linuxfoundation.org>
 <1510240648.2465.46.camel@codethink.co.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1510240648.2465.46.camel@codethink.co.uk>
Message-Id: <20171109175654.GK3624@linux.vnet.ibm.com>
Sender: stable-owner@vger.kernel.org

On Thu, Nov 09, 2017 at 03:17:28PM +0000, Ben Hutchings wrote:
> On Mon, 2017-10-16 at 18:11 +0200, gregkh@linuxfoundation.org wrote:
> > 4.4-stable review patch.��If anyone has any objections, please let me know.
> > 
> > ------------------
> > 
> > From: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > 
> > commit 28585a832602747cbfa88ad8934013177a3aae38 upstream.
> > 
> > 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.
> > 
> > Link: http://lkml.kernel.org/r/20170922211022.GA18084@linux.vnet.ibm.com
> > 
> > Cc: stable@vger.kernel.org
> > Fixes: 0be964be0 ("module: Sanitize RCU usage and locking")
> > > Reported-by: Steven Rostedt <rostedt@goodmis.org>
> > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> > �kernel/rcu/tree.c |���12 ++++++++++++
> > �1 file changed, 12 insertions(+)
> > 
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -759,6 +759,12 @@ void rcu_irq_exit(void)
> > �
> > �	local_irq_save(flags);
> > �	rdtp = this_cpu_ptr(&rcu_dynticks);
> > +
> > +	/* Page faults can happen in NMI handlers, so check... */
> > +	if (READ_ONCE(rdtp->dynticks_nmi_nesting))
> > +		return;
> 
> Shouldn't there be a local_irq_restore() on this return path?  Or does
> this condition imply that IRQs were already disabled?
> 
> > +	RCU_LOCKDEP_WARN(!irqs_disabled(), "rcu_irq_exit() invoked with irqs enabled!!!");
> 
> I don't see why you added RCU_LOCKDEP_WARN() here.  Prior to 4.5 it's
> not an error to call this function with IRQs disabled.  And after
> calling local_irq_save(), it's redundant to assert that IRQs are
> disabled.
> 
> > �	oldval = rdtp->dynticks_nesting;
> > �	rdtp->dynticks_nesting--;
> > �	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
> > @@ -887,6 +893,12 @@ void rcu_irq_enter(void)
> > �
> > �	local_irq_save(flags);
> > �	rdtp = this_cpu_ptr(&rcu_dynticks);
> > +
> > +	/* Page faults can happen in NMI handlers, so check... */
> > +	if (READ_ONCE(rdtp->dynticks_nmi_nesting))
> > +		return;
> > +
> > +	RCU_LOCKDEP_WARN(!irqs_disabled(), "rcu_irq_enter() invoked with irqs enabled!!!");
> 
> Same problems here.

Indeed, it looks like I need to rework this for 4.5 and earlier.
Sorry for the noise!

							Thanx, Paul