linux-kernel - Re: [RFC] WRITE_ONCE

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: <20200420150545.GB17661@paulmck-ThinkPad-P72>
Date:   Mon, 20 Apr 2020 08:05:45 -0700
From:   "Paul E. McKenney" <paulmck@...nel.org>
To:     David Laight <David.Laight@...LAB.COM>
Cc:     'Petko Manolov' <petko.manolov@...sulko.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RFC] WRITE_ONCE_INC() and friends

On Sun, Apr 19, 2020 at 09:37:10PM +0000, David Laight wrote:
> From: Petko Manolov
> > Sent: 19 April 2020 19:30
> > 
> > On 20-04-19 18:02:50, David Laight wrote:
> > > From: Petko Manolov
> > > > Sent: 19 April 2020 10:45
> > > > Recently I started reading up on KCSAN and at some point I ran into stuff like:
> > > >
> > > > WRITE_ONCE(ssp->srcu_lock_nesting[idx], ssp->srcu_lock_nesting[idx] + 1);
> > > > WRITE_ONCE(p->mm->numa_scan_seq, READ_ONCE(p->mm->numa_scan_seq) + 1);
> > >
> > > If all the accesses use READ/WRITE_ONCE() why not just mark the structure
> > > field 'volatile'?
> > 
> > This is a bit heavy.  I guess you've read this one:
> > 
> > 	https://lwn.net/Articles/233479/
> 
> I remember reading something similar before.
> I also remember a very old gcc (2.95?) that did a readback
> after every volatile write on sparc (to flush the store buffer).
> That broke everything.
> 
> I suspect there is a lot more code that is attempting to be lockless
> these days.
> Ring buffers (one writer and one reader) are a typical example where
> you don't need locks but do need to use a consistent value.
> 
> Now you may also need ordering between accesses - which I think needs
> more than volatile.

In Petko's patch, all needed ordering is supplied by the fact that it
is the same variable being read and written.  But yes, in many other
cases, more ordering is required.

> > And no, i am not sure all accesses are through READ/WRITE_ONCE().  If, for
> > example, all others are from withing spin_lock/unlock pairs then we _may_ not
> > need READ/WRITE_ONCE().
> 
> The cost of volatile accesses is probably minimal unless the
> code is written assuming the compiler will only access things once.

And there are variables marked as volatile, for example, jiffies.

But one downside of declaring variables volatile is that it can prevent
KCSAN from spotting violations of the concurrency design for those
variables.

> > I merely proposed the _INC() variant for better readability.
> 
> More like shorter code lines :-)

That too!  ;-)

							Thanx, Paul