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 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).

In case of u64 we use _ONCE access because of 32bit architectures if
there's an unlocked access.