linux-kernel - Re: [PATCH tip/core/rcu 06/30] rcu: Add WRITE_ONCE to rcu_node ->exp_seq

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: <20200215105803.GY2935@paulmck-ThinkPad-P72>
Date:   Sat, 15 Feb 2020 02:58:03 -0800
From:   "Paul E. McKenney" <paulmck@...nel.org>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     rcu@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel-team@...com, mingo@...nel.org, jiangshanlai@...il.com,
        dipankar@...ibm.com, akpm@...ux-foundation.org,
        mathieu.desnoyers@...icios.com, josh@...htriplett.org,
        tglx@...utronix.de, peterz@...radead.org, dhowells@...hat.com,
        edumazet@...gle.com, fweisbec@...il.com, oleg@...hat.com,
        joel@...lfernandes.org,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH tip/core/rcu 06/30] rcu: Add WRITE_ONCE to rcu_node
 ->exp_seq_rq store

On Fri, Feb 14, 2020 at 10:47:43PM -0500, Steven Rostedt wrote:
> On Fri, 14 Feb 2020 15:55:43 -0800
> paulmck@...nel.org wrote:
> 
> > From: "Paul E. McKenney" <paulmck@...nel.org>
> > 
> > The rcu_node structure's ->exp_seq_rq field is read locklessly, so
> > this commit adds the WRITE_ONCE() to a load in order to provide proper
> > documentation and READ_ONCE()/WRITE_ONCE() pairing.
> > 
> > This data race was reported by KCSAN.  Not appropriate for backporting
> > due to failure being unlikely.
> > 
> > Signed-off-by: Paul E. McKenney <paulmck@...nel.org>
> > ---
> >  kernel/rcu/tree_exp.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> > index d7e0484..85b009e 100644
> > --- a/kernel/rcu/tree_exp.h
> > +++ b/kernel/rcu/tree_exp.h
> > @@ -314,7 +314,7 @@ static bool exp_funnel_lock(unsigned long s)
> >  				   sync_exp_work_done(s));
> >  			return true;
> >  		}
> > -		rnp->exp_seq_rq = s; /* Followers can wait on us. */
> > +		WRITE_ONCE(rnp->exp_seq_rq, s); /* Followers can wait on us. */
> 
> Didn't Linus say this is basically bogus?
> 
> Perhaps just using it as documenting that it's read locklessly, but is
> it really needed?

Yes, Linus explicitly stated that WRITE_ONCE() is not required in
this case, but he also said that he was OK with it being there for
documentation purposes.

And within RCU, I -do- need it because I absolutely need to see if a
given patch introduced new KCSAN reports.  So I need it for the same
reason that I need the build to proceed without warnings.

Others who are working with less concurrency-intensive code might quite
reasonably make other choices, of course.  And my setting certain KCSAN
config options in my own builds doesn't inconvenience them, so we should
all be happy, right?  :-)

							Thanx, Paul

> -- Steve
> 
> 
> 
> >  		spin_unlock(&rnp->exp_lock);
> >  		trace_rcu_exp_funnel_lock(rcu_state.name, rnp->level,
> >  					  rnp->grplo, rnp->grphi, TPS("nxtlvl"));
>