On Fri, Feb 14, 2020 at 03:19:06PM +0900, Masami Hiramatsu wrote:

On Thu, 13 Feb 2020 14:39:18 -0800
"Paul E. McKenney" [off-list ref] wrote:

quoted

On Thu, Feb 13, 2020 at 05:04:51PM -0500, Steven Rostedt wrote:

quoted

On Thu, 13 Feb 2020 13:50:04 -0800
"Paul E. McKenney" [off-list ref] wrote:

quoted

On Thu, Feb 13, 2020 at 04:38:25PM -0500, Steven Rostedt wrote:

quoted

[ Added Masami ]

On Thu, 13 Feb 2020 16:19:30 -0500
Joel Fernandes [off-list ref] wrote:
  

quoted

On Thu, Feb 13, 2020 at 12:54:42PM -0800, Paul E. McKenney wrote:  

quoted

On Thu, Feb 13, 2020 at 03:44:44PM -0500, Joel Fernandes wrote:    

quoted

On Thu, Feb 13, 2020 at 10:56:12AM -0800, Paul E. McKenney wrote:
[...]     

quoted

quoted

quoted

It might well be that I could make these functions be NMI-safe, but
rcu_prepare_for_idle() in particular would be a bit ugly at best.
So, before looking into that, I have a question.  Given these proposed
changes, will rcu_nmi_exit_common() and rcu_nmi_enter_common() be able
to just use in_nmi()?    

That _should_ already be the case today. That is, if we end up in a
tracer and in_nmi() is unreliable we're already screwed anyway.    

So something like this, then?  This is untested, probably doesn't even
build, and could use some careful review from both Peter and Steve,
at least.  As in the below is the second version of the patch, the first
having been missing a couple of important "!" characters.    

I removed the static from rcu_nmi_enter()/exit() as it is called from
outside, that makes it build now. Updated below is Paul's diff. I also added
NOKPROBE_SYMBOL() to rcu_nmi_exit() to match rcu_nmi_enter() since it seemed
asymmetric.    

My compiler complained about the static and the __always_inline, so I
fixed those.  But please help me out on adding the NOKPROBE_SYMBOL()
to rcu_nmi_exit().  What bad thing happens if we leave this on only
rcu_nmi_enter()?    

It seemed odd to me we were not allowing kprobe on the rcu_nmi_enter() but
allowing it on exit (from a code reading standpoint) so my reaction was to
add it to both, but we could probably keep that as a separate
patch/discussion since it is slightly unrelated to the patch.. Sorry to
confuse the topic.
 

rcu_nmi_enter() was marked NOKPROBE or other reasons. See commit
c13324a505c77 ("x86/kprobes: Prohibit probing on functions before
kprobe_int3_handler()")

The issue was that we must not allow anything in do_int3() call kprobe
code before kprobe_int3_handler() is called. Because ist_enter() (in
do_int3()) calls rcu_nmi_enter() it had to be marked NOKPROBE. It had
nothing to do with it being RCU nor NMI, but because it was simply
called in do_int3().

Thus, there's no reason to make rcu_nmi_exit() NOKPROBE. But a commont
to why rcu_nmi_enter() would probably be useful, like below:  

Thank you, Steve!  Could I please have your Signed-off-by for this?

Sure, but it was untested ;-)

No problem!  I will fire up rcutorture on it.  ;-)

But experience indicates that you cannot even make a joke around here.
There is probably already someone out there somewhere building a
comment-checker based on deep semantic analysis and machine learning.  :-/

quoted

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

I'd like a Reviewed-by from Masami though.

Sounds good!  Masami, would you be willing to review?

Yes, the functions before calling kprobe_int3_handler() must not
be kprobed. It can cause an infinite recursive int3 trapping.

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>

Thank you both!

Like this?

							Thanx, Paul

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

commit 1817fdc8f4e4bd18c76305c9b937fb0dccbb1583
Author: Steven Rostedt [off-list ref]
Date:   Sat Feb 15 06:54:50 2020 -0800

    rcu: Provide comment for NOKPROBE() on rcu_nmi_enter()
    
    The rcu_nmi_enter() function was marked NOKPROBE() by commit
    c13324a505c77 ("x86/kprobes: Prohibit probing on functions before
    kprobe_int3_handler()") because the do_int3() call kprobe code must
    not be invoked before kprobe_int3_handler() is called.  It turns out
    that ist_enter() (in do_int3()) calls rcu_nmi_enter(), hence the
    marking NOKPROBE() being added to rcu_nmi_enter().
    
    This commit therefore adds a comment documenting this line of reasoning.
    
    Signed-off-by: Steven Rostedt [off-list ref]
    Reviewed-by: Masami Hiramatsu [off-list ref]
    Signed-off-by: Paul E. McKenney [off-list ref]

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 132b53e..4a885af 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c

@@ -835,6 +835,12 @@ void rcu_nmi_enter(void)
 		   rdp->dynticks_nmi_nesting + incby);
 	barrier();
 }
+/*
+ * On x86, All functions in do_int3() must be marked NOKPROBE before
+ * kprobe_int3_handler() is called. ist_enter() which is called in do_int3()
+ * before kprobe_int3_handle() happens to call rcu_nmi_enter() which means
+ * that rcu_nmi_enter() must be marked NOKRPOBE.
+ */
 NOKPROBE_SYMBOL(rcu_nmi_enter);
 
 /**