linux-kernel - Re: [RFC][PATCH 11/18 v2] ftrace: Adde infrastructure to stop RCU unsafe checker from checking

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]

Date:	Tue, 3 Sep 2013 09:43:52 -0400
From:	Steven Rostedt <rostedt@...dmis.org>
To:	paulmck@...ux.vnet.ibm.com
Cc:	linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Jiri Olsa <jolsa@...hat.com>
Subject: Re: [RFC][PATCH 11/18 v2] ftrace: Adde infrastructure to stop RCU
 unsafe checker from checking

On Sat, 31 Aug 2013 12:52:58 -0700
"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com> wrote:

> On Sat, Aug 31, 2013 at 01:11:28AM -0400, Steven Rostedt wrote:
> > From: "Steven Rostedt (Red Hat)" <rostedt@...dmis.org>
> > 
> > This is a light weight way to keep the rcu checker from checking
> > RCU safety. It adds a ftrace_unsafe_rcu_checker_disable/enable()
> > that increments or decrements a counter respectively. When the
> > counter is set, the RCU unsafe checker callback does not run the
> > tests to see if RCU is safe or not.
> 
> Please add something saying what we do instead of testing RCU safety.
> Looks to me like it skips not only the tests, but also invoking the
> callback, but I could easily be wrong.

Now that I'm awake and also not on holiday, I can reply with a clear
mind.

When disabled, it forces the callback to return immediately. I may in
the future, have it disable the callback altogether.

Yes, we can miss checks, but it's better than the test live locking
the system :-)

> 
> > This is required by the graph tracer because the checks can cause
> > the graph tracer to live lock the system by its own calls.

I added this (I quoted the added text):

    This is a light weight way to keep the rcu checker from checking
    RCU safety. It adds a ftrace_unsafe_rcu_checker_disable/enable()
    that increments or decrements a counter respectively. When the
    counter is set, the RCU unsafe checker callback does not run the
    tests to see if RCU is safe or not. "But the callback still gets
    called. It just does not call rcu_read_(un)lock()."
    
    This is required by the graph tracer because the checks can cause
    the graph tracer to live lock the system by its own calls. "That is,
    the graph tracer will still trace rcu and the rcu debugging, and
    this will slow down the checker, which is still calling all other
    functions (in interrupts and faults), which can cause the timer
    interrupt to take oven a millisecond to complete, and it will then
    trigger once it finishes, live locking the system."


> > 
> > It's also needed by the irqsoff tracer, because it may be called
> > in RCU unsafe regions and if its internal functions get traced
> > then the RCU unsafe checker may have some false positives.
> > 
> > Signed-off-by: Steven Rostedt <rostedt@...dmis.org>
> 
> With the augmented commit log as noted above:

Is the above OK?

-- Steve

> 
> Acked-by: Paul E. McKenney <paulmck@...ux.vnet.ibm.com>
> 
> > ---
> >  kernel/trace/trace.h           |   12 +++++++++---
> >  kernel/trace/trace_functions.c |   15 +++++++++++++++
> >  2 files changed, 24 insertions(+), 3 deletions(-)
> > 
> > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> > index e551316..58e4c37 100644
> > --- a/kernel/trace/trace.h
> > +++ b/kernel/trace/trace.h
> > @@ -760,9 +760,6 @@ static inline int ftrace_graph_addr(unsigned long addr)
> > 
> >  	return 0;
> >  }
> > -#ifdef CONFIG_FTRACE_UNSAFE_RCU_CHECKER
> > -extern bool ftrace_rcu_unsafe(unsigned long addr);
> > -#endif
> >  #else
> >  static inline int ftrace_graph_addr(unsigned long addr)
> >  {
> > @@ -1061,4 +1058,13 @@ int perf_ftrace_event_register(struct ftrace_event_call *call,
> >  #define perf_ftrace_event_register NULL
> >  #endif
> > 
> > +#ifdef CONFIG_FTRACE_UNSAFE_RCU_CHECKER
> > +extern bool ftrace_rcu_unsafe(unsigned long addr);
> > +extern void ftrace_unsafe_rcu_checker_disable(void);
> > +extern void ftrace_unsafe_rcu_checker_enable(void);
> > +#else
> > +static inline void ftrace_unsafe_rcu_checker_disable(void) { }
> > +static inline void ftrace_unsafe_rcu_checker_enable(void) { }
> > +#endif
> > +
> >  #endif /* _LINUX_KERNEL_TRACE_H */
> > diff --git a/kernel/trace/trace_functions.c b/kernel/trace/trace_functions.c
> > index 9dd4627..1d5f951 100644
> > --- a/kernel/trace/trace_functions.c
> > +++ b/kernel/trace/trace_functions.c
> > @@ -560,12 +560,27 @@ static inline int init_func_cmd_traceon(void)
> >  #endif /* CONFIG_DYNAMIC_FTRACE */
> > 
> >  #ifdef CONFIG_FTRACE_UNSAFE_RCU_CHECKER
> > +static atomic_t ftrace_unsafe_rcu_disabled;
> > +
> > +void ftrace_unsafe_rcu_checker_disable(void)
> > +{
> > +	atomic_inc(&ftrace_unsafe_rcu_disabled);
> > +}
> > +
> > +void ftrace_unsafe_rcu_checker_enable(void)
> > +{
> > +	atomic_dec(&ftrace_unsafe_rcu_disabled);
> > +}
> > +
> >  static void
> >  ftrace_unsafe_callback(unsigned long ip, unsigned long parent_ip,
> >  		       struct ftrace_ops *op, struct pt_regs *pt_regs)
> >  {
> >  	int bit;
> > 
> > +	if (atomic_read(&ftrace_unsafe_rcu_disabled))
> > +		return;
> > +
> >  	preempt_disable_notrace();
> > 
> >  	bit = trace_test_and_set_recursion(TRACE_FTRACE_START, TRACE_FTRACE_MAX);
> > -- 
> > 1.7.10.4
> > 
> > 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/