linux-kernel - Re: [PATCH 2/2] cgroup: explain the race between updater and flusher

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: <CAGj-7pUdbtumOmfmW52F3aHJfkd5F+nGeH5LAf5muKqYR+xV-w@mail.gmail.com>
Date: Thu, 3 Jul 2025 18:54:02 -0700
From: Shakeel Butt <shakeel.butt@...ux.dev>
To: paulmck@...nel.org
Cc: Tejun Heo <tj@...nel.org>, Andrew Morton <akpm@...ux-foundation.org>, 
	JP Kobryn <inwardvessel@...il.com>, Johannes Weiner <hannes@...xchg.org>, 
	Ying Huang <huang.ying.caritas@...il.com>, Vlastimil Babka <vbabka@...e.cz>, 
	Alexei Starovoitov <ast@...nel.org>, Sebastian Andrzej Siewior <bigeasy@...utronix.de>, 
	Michal Koutný <mkoutny@...e.com>, bpf@...r.kernel.org, 
	linux-mm@...ck.org, cgroups@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Meta kernel team <kernel-team@...a.com>
Subject: Re: [PATCH 2/2] cgroup: explain the race between updater and flusher

On Thu, Jul 3, 2025 at 4:53 PM Paul E. McKenney <paulmck@...nel.org> wrote:
>
> On Thu, Jul 03, 2025 at 03:46:07PM -0700, Shakeel Butt wrote:
[...]
> > Let me answer this one first. The previous patch actually made
> > init_llist_node() do WRITE_ONCE().
> >
> > So the actual question is why do we need
> > data_race([READ|WRITE]_ONCE()) instead of just [READ|WRITE]_ONCE()?
>
> You should *almost* always use [READ|WRITE]_ONCE() instead of data_race().
>
> > Actually I had the similar question myself and found the following
> > comment in include/linux/compiler.h:
> >
> > /**
> >  * data_race - mark an expression as containing intentional data races
> >  *
> >  * This data_race() macro is useful for situations in which data races
> >  * should be forgiven.  One example is diagnostic code that accesses
> >  * shared variables but is not a part of the core synchronization design.
> >  * For example, if accesses to a given variable are protected by a lock,
> >  * except for diagnostic code, then the accesses under the lock should
> >  * be plain C-language accesses and those in the diagnostic code should
> >  * use data_race().  This way, KCSAN will complain if buggy lockless
> >  * accesses to that variable are introduced, even if the buggy accesses
> >  * are protected by READ_ONCE() or WRITE_ONCE().
> >  *
> >  * This macro *does not* affect normal code generation, but is a hint
> >  * to tooling that data races here are to be ignored.  If the access must
> >  * be atomic *and* KCSAN should ignore the access, use both data_race()
> >  * and READ_ONCE(), for example, data_race(READ_ONCE(x)).
> >  */
> >
> > IIUC correctly, I need to protect llist_node against tearing and as well
> > as tell KCSAN to ignore the access for race then I should use both.
> > Though I think KCSAN treat [READ|WRITE]_ONCE similar to data_race(), so
> > it kind of seem redundant but I think at least I want to convey that we
> > need protection against tearing and ignore KCSAN and using both conveys
> > that. Let me know if you think otherwise.
> >
> > thanks a lot for taking a look.
>
> The thing to remember is that data_race() does not affect the
> generated code (except of course when running KCSAN), and thus does
> absolutely nothing to prevent load/store tearing.  You need things like
> [READ|WRITE]_ONCE() to prevent tearing.
>
> So if it does not affect the generated code, what is the point of
> data_race()?
>
> One answer to this question is for diagnostics where you want KCSAN
> to check the main algorithm, but you don't want KCSAN to be confused
> by the diagnostic accesses.  For example, you might use something like
> ASSERT_EXCLUSIVE_ACCESS() as in __list_splice_init_rcu(), and not want
> your diagnostic accesses to result in false-positive KCSAN reports
> due to interactions with ASSERT_EXCLUSIVE_ACCESS() on some particular
> memory location.  And if you were to use READ_ONCE() to access that same
> memory location in your diagnostics, KCSAN would complain if they ran
> concurrently with that ASSERT_EXCLUSIVE_ACCESS().  So you would instead
> use data_race() to suppress such complaints.
>
> Does that make sense?
>

Thanks a lot Paul for the awesome explanation. Do you think keeping
data_race() here would be harmful in a sense that it might cause
confusion in future?