linux-kernel - Re: [PATCH] xfs: mark the i_delayed_blks access in xfs_file

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-next>] [day] [month] [year] [list]

Message-ID: <20250514042946.GA23355@lst.de>
Date: Wed, 14 May 2025 06:29:46 +0200
From: Christoph Hellwig <hch@....de>
To: Dave Chinner <david@...morbit.com>
Cc: Christoph Hellwig <hch@....de>, cem@...nel.org,
	linux-xfs@...r.kernel.org, cen zhang <zzzccc427@...il.com>,
	lkmm@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] xfs: mark the i_delayed_blks access in
 xfs_file_release as racy

On Wed, May 14, 2025 at 07:37:14AM +1000, Dave Chinner wrote:
> On Tue, May 13, 2025 at 07:26:14AM +0200, Christoph Hellwig wrote:
> > We don't bother with the ILOCK as this is best-effort and thus a racy
> > access is ok.  Add a data_race() annotation to make that clear to
> > memory model verifiers.
> 
> IMO, that's the thin edge of a wedge. There are dozens of places in
> XFS where we check variable values without holding the lock needed
> to serialise the read against modification.

Yes. And the linux kernel memory consistency model ask us to mark them,
see tools/memory-model/Documentation/access-marking.txt.

This fails painful at first, but I'd actually wish we'd have tools
enforcing this as strongly as possible as developers (well me at least)
seem to think a racy access is just fine more often than they should, and
needing an annotation and a comment is a pretty good way to sure that.

> Hence my question - are we now going to make it policy that every
> possible racy access must be marked with data_race() because there
> is some new bot that someone is running that will complain if we
> don't? Are you committing to playing whack-a-mole with the memory
> model verifiers to silence all the false positives from these
> known-to-be-safe access patterns?

It's not really a "new bot".  It has been official memory consistency
policy for a while, but it just hasn't been well enforced.  For new code
asking if the review is racy and needs a marking or use READ_ONCE() and
WRITE_ONCE() has been part of the usual review protocol.  Reviewing old
code and fixing things we got wrong will take a while, but I'm actually
glad about more bots for that.