linux-kernel - Re: [PATCH] seqlock: Use WRITE

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: <f34fb68f-4749-440a-912d-2cb182284cf4@paulmck-laptop>
Date: Wed, 18 Dec 2024 08:57:12 -0800
From: "Paul E. McKenney" <paulmck@...nel.org>
To: Waiman Long <llong@...hat.com>
Cc: Daniel Xu <dxu@...uu.xyz>, mingo@...hat.com, will@...nel.org,
	peterz@...radead.org, boqun.feng@...il.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] seqlock: Use WRITE_ONCE() when updating sequence

On Wed, Dec 18, 2024 at 11:10:01AM -0500, Waiman Long wrote:
> On 12/18/24 10:45 AM, Paul E. McKenney wrote:
> > On Tue, Dec 17, 2024 at 10:30:39PM -0500, Waiman Long wrote:
> > > On 12/17/24 6:17 PM, Daniel Xu wrote:
> > > > `sequence` is a concurrently accessed shared variable on the reader
> > > > side. Therefore, it needs to be wrapped in WRITE_ONCE() in order to
> > > > prevent unwanted compiler optimizations like store tearing.
> > > > 
> > > > Signed-off-by: Daniel Xu <dxu@...uu.xyz>
> > > > ---
> > > >    include/linux/seqlock.h | 14 +++++++-------
> > > >    1 file changed, 7 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h
> > > > index 5298765d6ca4..f4c6f2507742 100644
> > > > --- a/include/linux/seqlock.h
> > > > +++ b/include/linux/seqlock.h
> > > > @@ -45,7 +45,7 @@ static inline void __seqcount_init(seqcount_t *s, const char *name,
> > > >    	 * Make sure we are not reinitializing a held lock:
> > > >    	 */
> > > >    	lockdep_init_map(&s->dep_map, name, key, 0);
> > > > -	s->sequence = 0;
> > > > +	WRITE_ONCE(s->sequence, 0);
> > > >    }
> > > The init function certainly doesn't need to use WRITE_ONCE().
> > > >    #ifdef CONFIG_DEBUG_LOCK_ALLOC
> > > > @@ -405,7 +405,7 @@ do {									\
> > > >    static inline void do_raw_write_seqcount_begin(seqcount_t *s)
> > > >    {
> > > >    	kcsan_nestable_atomic_begin();
> > > > -	s->sequence++;
> > > > +	WRITE_ONCE(s->sequence, READ_ONCE(s->sequence) + 1);
> > > >    	smp_wmb();
> > > >    }
> > > > @@ -426,7 +426,7 @@ do {									\
> > > >    static inline void do_raw_write_seqcount_end(seqcount_t *s)
> > > >    {
> > > >    	smp_wmb();
> > > > -	s->sequence++;
> > > > +	WRITE_ONCE(s->sequence, READ_ONCE(s->sequence) + 1);
> > > >    	kcsan_nestable_atomic_end();
> > > >    }
> > > > @@ -548,9 +548,9 @@ static inline void do_write_seqcount_end(seqcount_t *s)
> > > >    static inline void do_raw_write_seqcount_barrier(seqcount_t *s)
> > > >    {
> > > >    	kcsan_nestable_atomic_begin();
> > > > -	s->sequence++;
> > > > +	WRITE_ONCE(s->sequence, READ_ONCE(s->sequence) + 1);
> > > >    	smp_wmb();
> > > > -	s->sequence++;
> > > > +	WRITE_ONCE(s->sequence, READ_ONCE(s->sequence) + 1);
> > > >    	kcsan_nestable_atomic_end();
> > > >    }
> > > > @@ -569,7 +569,7 @@ static inline void do_write_seqcount_invalidate(seqcount_t *s)
> > > >    {
> > > >    	smp_wmb();
> > > >    	kcsan_nestable_atomic_begin();
> > > > -	s->sequence+=2;
> > > > +	WRITE_ONCE(s->sequence, READ_ONCE(s->sequence) + 2);
> > > >    	kcsan_nestable_atomic_end();
> > > >    }
> > > > @@ -673,7 +673,7 @@ read_seqcount_latch_retry(const seqcount_latch_t *s, unsigned start)
> > > >    static __always_inline void raw_write_seqcount_latch(seqcount_latch_t *s)
> > > >    {
> > > >    	smp_wmb();	/* prior stores before incrementing "sequence" */
> > > > -	s->seqcount.sequence++;
> > > > +	WRITE_ONCE(s->seqcount.sequence, READ_ONCE(s->seqcount.sequence) + 1);
> > > >    	smp_wmb();      /* increment "sequence" before following stores */
> > > >    }
> > > For seqcount, its actual value isn't important. What is important is whether
> > > the value changes and whether it is even or odd. So even if store tearing is
> > > happening, it shouldn't affect its operation. I doubt we need to use
> > > WRITE_ONCE() here. Could you come up with a scenario where store tearing
> > > will make it behave incorrectly?
> > But why expand the state space?
> I don't quite get what you mean by "state space".

The state space includes the set of possible values that might be stored
in .sequence at any given point in time.  So if you allow the compiler to
tear the store, you are increasing the number of values that a concurrent
reader can return, and thus also increasing the number of states that must
be considered.  On the other hand, if you use WRITE_ONCE() to constrain
the compiler to do a single store, then you don't have to even worry
about the added states due to these partial updates.

> > Also, there are potentially "optimizations" other than store tearing.
> > No, I haven't seen them yet, but then again, there were a great many
> > optimizations that were not being used back when I started coding C.
> 
> All the inc's are bounded by smp_wmb()'s which should limit the kind of
> optimizations that can be done by the compiler. That is why I don't see a
> need to use WRITE_ONCE() here. Of course, there may be some corner cases
> that I may miss. So I ask for whether such a case exists.

One other potential optimization is using the variable for scratch storage
immediately prior to the update, which could store any value there.
I haven't seen this myself, but the standard permits it, and in some
cases register pressure might encourage it.

And who knows what else our creative compiler writers will come up with?

							Thanx, Paul