linux-kernel - Re: [PATCH tip/core/rcu 1/3] rcu-tasks: *_ONCE() for rcu_tasks_cbs

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: <20200219011359.GA29762@google.com>
Date:   Tue, 18 Feb 2020 20:13:59 -0500
From:   Joel Fernandes <joel@...lfernandes.org>
To:     "Paul E. McKenney" <paulmck@...nel.org>
Cc:     Steven Rostedt <rostedt@...dmis.org>,
        Peter Zijlstra <peterz@...radead.org>, 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, dhowells@...hat.com,
        edumazet@...gle.com, fweisbec@...il.com, oleg@...hat.com
Subject: Re: [PATCH tip/core/rcu 1/3] rcu-tasks: *_ONCE() for
 rcu_tasks_cbs_head

On Tue, Feb 18, 2020 at 04:16:40PM -0800, Paul E. McKenney wrote:
> On Tue, Feb 18, 2020 at 07:01:44PM -0500, Joel Fernandes wrote:
> > On Tue, Feb 18, 2020 at 02:54:55PM -0800, Paul E. McKenney wrote:
> > > On Tue, Feb 18, 2020 at 05:45:03PM -0500, Steven Rostedt wrote:
> > > > On Tue, 18 Feb 2020 12:22:26 -0800
> > > > "Paul E. McKenney" <paulmck@...nel.org> wrote:
> > > > 
> > > > > On Tue, Feb 18, 2020 at 09:11:42PM +0100, Peter Zijlstra wrote:
> > > > > > On Tue, Feb 18, 2020 at 08:27:19AM -0800, Paul E. McKenney wrote:  
> > > > > > > On Tue, Feb 18, 2020 at 08:56:48AM +0100, Peter Zijlstra wrote:  
> > > > > >   
> > > > > > > > I just took offence at the Changelog wording. It seems to suggest there
> > > > > > > > actually is a problem, there is not.  
> > > > > > > 
> > > > > > > Quoting the changelog: "Not appropriate for backporting due to failure
> > > > > > > being unlikely."  
> > > > > > 
> > > > > > That implies there is failure, however unlikely.
> > > > > > 
> > > > > > In this particular case there is absolutely no failure, except perhaps
> > > > > > in KCSAN. This patch is a pure annotation such that KCSAN can understand
> > > > > > the code.
> > > > > > 
> > > > > > Like said, I don't object to the actual patch, but I do think it is
> > > > > > important to call out false negatives or to describe the actual problem
> > > > > > found.  
> > > > > 
> > > > > I don't feel at all comfortable declaring that there is absolutely
> > > > > no possibility of failure.
> > > > 
> > > > Perhaps wording it like so:
> > > > 
> > > > "There's know known issue with the current code, but the *_ONCE()
> > > > annotations here makes KCSAN happy, allowing us to focus on KCSAN
> > > > warnings that can help bring about known issues in other code that we
> > > > can fix, without being distracted by KCSAN warnings that we do not see
> > > > a problem with."
> > > > 
> > > > ?
> > > 
> > > That sounds more like something I might put in rcutodo.html as a statement
> > > of the RCU approach to KCSAN reports.
> > > 
> > > But switching to a different situation (for variety, if nothing else),
> > > what about the commit shown below?
> > > 
> > > 							Thanx, Paul
> > > 
> > > ------------------------------------------------------------------------
> > > 
> > > commit 35bc02b04a041f32470ae6d959c549bcce8483db
> > > Author: Paul E. McKenney <paulmck@...nel.org>
> > > Date:   Tue Feb 18 13:41:02 2020 -0800
> > > 
> > >     rcutorture: Mark data-race potential for rcu_barrier() test statistics
> > >     
> > >     The n_barrier_successes, n_barrier_attempts, and
> > >     n_rcu_torture_barrier_error variables are updated (without access
> > >     markings) by the main rcu_barrier() test kthread, and accessed (also
> > >     without access markings) by the rcu_torture_stats() kthread.  This of
> > >     course can result in KCSAN complaints.
> > >     
> > >     Because the accesses are in diagnostic prints, this commit uses
> > >     data_race() to excuse the diagnostic prints from the data race.  If this
> > >     were to ever cause bogus statistics prints (for example, due to store
> > >     tearing), any misleading information would be disambiguated by the
> > >     presence or absence of an rcutorture splat.
> > >     
> > >     This data race was reported by KCSAN.  Not appropriate for backporting
> > >     due to failure being unlikely and due to the mild consequences of the
> > >     failure, namely a confusing rcutorture console message.
> > >     
> > >     Signed-off-by: Paul E. McKenney <paulmck@...nel.org>
> > > 
> > > diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
> > > index 5453bd5..b3301f3 100644
> > > --- a/kernel/rcu/rcutorture.c
> > > +++ b/kernel/rcu/rcutorture.c
> > > @@ -1444,9 +1444,9 @@ rcu_torture_stats_print(void)
> > >  		atomic_long_read(&n_rcu_torture_timers));
> > >  	torture_onoff_stats();
> > >  	pr_cont("barrier: %ld/%ld:%ld\n",
> > > -		n_barrier_successes,
> > > -		n_barrier_attempts,
> > > -		n_rcu_torture_barrier_error);
> > > +		data_race(n_barrier_successes),
> > > +		data_race(n_barrier_attempts),
> > > +		data_race(n_rcu_torture_barrier_error));
> > 
> > Would it be not worth just fixing the data-race within rcutorture itself?
> 
> I could use WRITE_ONCE() for updates and READ_ONCE() for statistics.
> However, my current rule is that diagnostic code that is not participating
> in the core synchronization uses data_race().  That way, if I do a typo
> and write to (say) n_barrier_attempts in some other thread, KCSAN will
> know to yell at me.

Oh, ok. That makes sense.

Reviewed-by: Joel Fernandes (Google) <joel@...lfernandes.org>

thanks,

 - Joel