Re: [PATCH 2/3] livepatch/rcu: Warn when system consistency is broken in RCU code

[Paul E. McKenney]

On Wed, May 10, 2017 at 06:04:23PM +0200, Petr Mladek wrote:
> On Tue 2017-05-09 11:18:35, Josh Poimboeuf wrote:
> > On Mon, May 08, 2017 at 03:36:00PM -0700, Paul E. McKenney wrote:
> > > On Mon, May 08, 2017 at 05:16:09PM -0500, Josh Poimboeuf wrote:
> > > > On Mon, May 08, 2017 at 02:07:54PM -0700, Paul E. McKenney wrote:
> > > But do we really need this, given the in_nmi() check that Steven
> > > pointed out?
> > 
> > The in_nmi() check doesn't work for non-NMI exceptions.  An exception
> > can come from anywhere, which is presumably why ist_enter() calls
> > rcu_nmi_enter(), even though it might not have been in NMI context.  The
> > exception could, for example, happen while you're twiddling important
> > bits in rcu_irq_enter().  Or it could happen early in do_nmi(), before
> > it had a chance to set NMI_MASK or call rcu_nmi_enter().  In either
> > case, in_nmi() would be false, yet calling rcu_irq_enter() would be bad.
> > 
> > I think I have convinced myself that, as long as the user doesn't patch
> > ist_enter() or rcu_dynticks_eqs_enter(), it'll be fine.  So the
> > following should be sufficient:
> > 
> > 	if (in_nmi())
> > 		rcu_nmi_enter(); /* in case we're called before nmi_enter() */
> 
> This does not work as expected. in_nmi() is implemented as
> 
> 	(preempt_count() & NMI_MASK)
> 
> These bits are set in nmi_enter(), see
> 
> 	preempt_count_add(NMI_OFFSET + HARDIRQ_OFFSET);
> 
> Note that nmi_enter() calls rcu_nmi_enter() right after
> setting the preempt_count bit.
> 
> It means that if in_nmi() returns true, we should already
> on the safe side regarding using rcu_read_lock()/unlock().
> 
> 
> The patch was designed to use basically the same solution
> as is used in the stack tracer. It is using
> rcu_read_lock()/unlock() as we do.
> 
> The stack tracer is different in the following ways:
> 
>     + It takes a spin lock. This is why it has to give
>       up in NMI completely.
> 
>     + It disables interrupts. I guess that it is because
>       of the spin lock as well. Otherwise, it would not
>       be safe in IRQ context.
> 
>     + It checks whether local_irq_save() has a chance to
>       work and gives up if it does not.
> 
> 
> On the other hand, the live patch handler:
> 
>     + does not need any lock => could be used in NMI
> 
>     + does not need to disable interrupts because
>       it does not use any lock
> 
>     + checks if local_irq_save() actually succeeded.
>       It seems more reliable to me.
> 
> 
> I am not sure if we all understand the problem. IMHO, the point
> is that RCU must be aware when we call rcu_read_lock()/unlock().

I for one am sure that I do -not- fully understand the problem.  ;-)
But yes, the key point is that RCU be able to see and respond to
the read-side critical sections.

> My understanding is that rcu_irq_enter() tries to make RCU watching
> when it was not. Then rcu_is_watching() reports if we are on
> the safe side.
> 
> But it is possible that I miss something. One question is if
> rcu_irq_enter()/exit() calls can be nested.

Yes, they can.  You get about 50 bits worth of nesting counter.

You can also nest rcu_nmi_enter()/exit() calls, but you "only"
get 31 bits of nesting counter.

							Thanx, Paul

> I still need to think about it.
> 
> Best Regards,
> Petr
>