linux-kernel - Re: [RFC 0/3] Revert SRCU from tracepoint infrastructure

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]

Message-ID: <20200210150348.7d0979e6@gandalf.local.home>
Date:   Mon, 10 Feb 2020 15:03:48 -0500
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Joel Fernandes <joel@...lfernandes.org>
Cc:     Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Peter Zijlstra <peterz@...radead.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Gustavo A. R. Silva" <gustavo@...eddedor.com>,
        Ingo Molnar <mingo@...hat.com>,
        Richard Fontana <rfontana@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        paulmck <paulmck@...nel.org>,
        Josh Triplett <josh@...htriplett.org>,
        Lai Jiangshan <jiangshanlai@...il.com>,
        Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>
Subject: Re: [RFC 0/3] Revert SRCU from tracepoint infrastructure

On Mon, 10 Feb 2020 14:53:02 -0500
Joel Fernandes <joel@...lfernandes.org> wrote:

> > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> > index 1fb11daa5c53..a83fd076a312 100644
> > --- a/include/linux/tracepoint.h
> > +++ b/include/linux/tracepoint.h
> > @@ -179,10 +179,8 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> >  		 * For rcuidle callers, use srcu since sched-rcu	\
> >  		 * doesn't work from the idle path.			\
> >  		 */							\
> > -		if (rcuidle) {						\
> > +		if (rcuidle)						\
> >  			__idx = srcu_read_lock_notrace(&tracepoint_srcu);\
> > -			rcu_irq_enter_irqson();				\
> > -		}							\  
> 
> This would still break out-of-tree modules or future code that does
> rcu_read_lock() right in a tracepoint callback right?

Yes, and that's fine.

> 
> Or are we saying that rcu_read_lock() in a tracepoint callback is not
> allowed? I believe this should then at least be documented somewhere.  Also,

No, it's only not allowed if you you attached to a tracepoint that can
be called without rcu watching. That's up to the caller to figure it
out. Tracepoints were never meant to be a generic thing people should
use without knowing what they are really doing.

> what about code in tracepoint callback that calls rcu_read_lock() indirectly
> through a path in the kernel, and also code that may expect RCU readers when
> doing preempt_disable()?

Then they need to know what they are doing.

> 
> So basically we are saying with this patch:
> 1. Don't call in a callback: rcu_read_lock() or preempt_disable() and expect RCU to do
> anything for you.

We can just say, "If you plan on using RCU, be aware that it man not be
watching and you get do deal with the fallout. Use rcu_is_watching() to
figure it out."

> 2. Don't call code that does anything that 1. needs.
> 
> Is that intended? thanks,
> 

No, look what the patch did for perf. Why make *all* callbacks suffer
if only some use RCU? If you use RCU from a callback, then you need to
figure it out. The same goes for attaching to the function tracer.

-- Steve