Re: [PATCH 0/3] btrfs: avoid data races when accessing an inode's delayed_node

[David Sterba <dsterba@suse.cz>]

On Mon, May 20, 2024 at 10:20:26PM +0200, David Sterba wrote:
> On Mon, May 20, 2024 at 05:58:37PM +0100, Filipe Manana wrote:
> > On Mon, May 20, 2024 at 4:48 PM David Sterba <dsterba@suse.cz> wrote:
> > >
> > > On Fri, May 17, 2024 at 02:13:23PM +0100, fdmanana@kernel.org wrote:
> > > > From: Filipe Manana <fdmanana@suse.com>
> > > >
> > > > We do have some data races when accessing an inode's delayed_node, namely
> > > > we use READ_ONCE() in a couple places while there's no pairing WRITE_ONCE()
> > > > anywhere, and in one place (btrfs_dirty_inode()) we neither user READ_ONCE()
> > > > nor take the lock that protects the delayed_node. So fix these and add
> > > > helpers to access and update an inode's delayed_node.
> > > >
> > > > Filipe Manana (3):
> > > >   btrfs: always set an inode's delayed_inode with WRITE_ONCE()
> > > >   btrfs: use READ_ONCE() when accessing delayed_node at btrfs_dirty_node()
> > > >   btrfs: add and use helpers to get and set an inode's delayed_node
> > >
> > > The READ_ONCE for delayed nodes has been there historically but I don't
> > > think it's needed everywhere. The legitimate case is in
> > > btrfs_get_delayed_node() where first use is without lock and then again
> > > recheck under the lock so we do want to read fresh value. This is to
> > > prevent compiler optimization to coalesce the reads.
> > >
> > > Writing to delayed node under lock also does not need WRITE_ONCE.
> > >
> > > IOW, I would rather remove use of the _ONCE helpers and not add more as
> > > it is not the pattern where it's supposed to be used. You say it's to
> > > prevent load tearing but for a pointer type this does not happen and is
> > > an assumption of the hardware.
> > 
> > If you are sure that pointers aren't subject to load/store tearing
> > issues, then I'm fine with dropping the patchset.
> 
> This will be in some CPU manual, the thread on LWN
> https://lwn.net/Articles/793895/
> mentions that. I base my claim on reading about that in various other
> discussions on LKML over time. Pointers match the unsigned long type
> that is the machine word and register size, assignment from register to
> register/memory happens in one go. What could be problematic is constant
> (immediate) assigned to register on architectures like SPARC that have
> fixed size instructions and the constatnt has to be written in two
> steps.
> 
> The need for READ_ONCE/WRITE_ONCE is to prevent compiler optimizations
> and also the load tearing, but for native types up to unsigned long this
> seems to be true.
> 
> https://elixir.bootlin.com/linux/latest/source/include/asm-generic/rwonce.h#L29
> 
> The only requirement that can possibly cause the tearing even for
> pointer is if it's not aligned and could be split over two cachelines.
> This should be the case for structures defined in a normal way (ie. no
> forced mis-alignment or __packed).

For future reference: documented for example in Intel 64 and IA-32
Architectures, Software Developer's Manual, volume 3A, 9.2 Memory
ordering.

From section 9.2.3.1: "[...] Instructions that read or write a quadword
(8 bytes) whose address is aligned on an 8 byte boundary."