linux-kernel - Re: [PATCH] rcutorture: Fix rcu_torture_pipe_update_one()/rcu_torture

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: <20240306103719.1d241b93@gandalf.local.home>
Date: Wed, 6 Mar 2024 10:37:19 -0500
From: Steven Rostedt <rostedt@...dmis.org>
To: linke li <lilinke99@...com>
Cc: joel@...lfernandes.org, boqun.feng@...il.com, dave@...olabs.net,
 frederic@...nel.org, jiangshanlai@...il.com, josh@...htriplett.org,
 linux-kernel@...r.kernel.org, mathieu.desnoyers@...icios.com,
 paulmck@...nel.org, qiang.zhang1211@...il.com, quic_neeraju@...cinc.com,
 rcu@...r.kernel.org, Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH] rcutorture: Fix
 rcu_torture_pipe_update_one()/rcu_torture_writer() data race and
 concurrency bug

On Tue,  5 Mar 2024 14:24:20 +0800
linke li <lilinke99@...com> wrote:

> > Anyway, a slightly related/different question:
> > 
> > Should that:
> > WRITE_ONCE(rp->rtort_pipe_count, rp->rtort_pipe_count + 1);
> > 
> > Be:
> > WRITE_ONCE(rp->rtort_pipe_count, READ_ONCE(rp->rtort_pipe_count) + 1);
> > 
> > ?  
> 
> Hi, Joel. Sorry, I am not very sure about this. I do a little research on
> it.
> 
> I looked through all code in kernel that looks like this, both kinds are
> used. I also try to compile this file with and without the READ_ONCE in
> WRITE_ONCE using gcc-9. Their binary is just the same. 
> 
> Thus I think in the current compiler version, they do not have a big
> difference, but if the compiler wants to optimize more, maybe the latter
> one is better.

I'm sorry but all these READ_ONCE/WRITE_ONCE() additions that are being
added because there's a fear of the compiler tearing long words, is going to
make the code really ugly and hard to read.

If we take the policy of handling a compiler that can tear reads and writes
of any size word, then we should have proper macros to handle it.

Perhaps READ_SHARED(), WRITE_SHARED(), ADD_SHARED(), SUB_SHARED(). The ONCE
has nothing to do with the reasons for these changes. But at least "SHARED"
can be considered "this variable is shared between different contexts".
Note, this is different than "atomic". It's just to document that this
variable must be loaded or stored in one transaction.

I don't know if Linus even cares about fixing "read/write tearing" which is
why I Cc'd him.

But I'm not going to take any patches that add these macros to fix
compilers that tear words on load and store until we have a set policy on
what to do with them.

-- Steve