Re: kprobes broken since 0d00449c7a28 ("x86: Replace ist_enter() with nmi_enter()")

[Steven Rostedt <rostedt@goodmis.org>]

On Sat, 30 Jan 2021 09:28:32 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Fri, Jan 29, 2021 at 04:24:54PM -0500, Steven Rostedt wrote:
> > Specifically, kprobe and ftrace callbacks may have this:
> > 
> > 	if (in_nmi())
> > 		return;
> > 
> > 	raw_spin_lock_irqsave(&lock, flags);
> > 	[..]
> > 	raw_spin_unlock_irqrestore(&lock, flags);
> > 
> > Which is totally fine to have,  
> 
> Why? There's a distinct lack of explaining here.
> 
> Note that we ripped out all such dodgy locking from kretprobes.

Actually, I think you helped explain the distinction. You mention
"kretpobes" do you mean the infrastructure of kretprobes or all its
users?

The infrastructure of ftrace and kprobes can work in any context, it
does not mean that the callbacks must. Again, these are more like
exceptions. Why have "in_nmi()"? If anything that can be called by an
NMI should just work, right? That's basically your argument for having
ftrace and kprobes set "in_nmi".

You can have locking in NMIs if the locking is *only* in NMI handlers,
right? If that's the case, then so should ftrace and kprobe callbacks.

The stack tracer checks the size of the stack, compares it to the
largest recorded size, and if it's bigger, it will save the stack. But
if this happens on two CPUs at the same time, only one can do the
recording at the same time. To synchronize this, a spin lock must be
taken. Similar to spin locks in an NMI.

But the problem here is, the callbacks can also be done from an NMI
context, so if we are in NMI, we don't want to take any locks, and
simply don't record the stack traces from NMIs.

The more I think about it, the more I hate the idea that ftrace
callbacks and kprobes are considered NMIs. Simply because they are not!

-- Steve