Re: [PATCH] btrfs: fix data race when accessing the last_trans field of a root

[David Sterba <dsterba@suse.cz>]

On Mon, Jul 01, 2024 at 11:01:53AM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> KCSAN complains about a data race when accessing the last_trans field of a
> root:
> 
>   [  199.553628] BUG: KCSAN: data-race in btrfs_record_root_in_trans [btrfs] / record_root_in_trans [btrfs]
> 
>   [  199.555186] read to 0x000000008801e308 of 8 bytes by task 2812 on cpu 1:
>   [  199.555210]  btrfs_record_root_in_trans+0x9a/0x128 [btrfs]
>   [  199.555999]  start_transaction+0x154/0xcd8 [btrfs]
>   [  199.556780]  btrfs_join_transaction+0x44/0x60 [btrfs]
>   [  199.557559]  btrfs_dirty_inode+0x9c/0x140 [btrfs]
>   [  199.558339]  btrfs_update_time+0x8c/0xb0 [btrfs]
>   [  199.559123]  touch_atime+0x16c/0x1e0
>   [  199.559151]  pipe_read+0x6a8/0x7d0
>   [  199.559179]  vfs_read+0x466/0x498
>   [  199.559204]  ksys_read+0x108/0x150
>   [  199.559230]  __s390x_sys_read+0x68/0x88
>   [  199.559257]  do_syscall+0x1c6/0x210
>   [  199.559286]  __do_syscall+0xc8/0xf0
>   [  199.559318]  system_call+0x70/0x98
> 
>   [  199.559431] write to 0x000000008801e308 of 8 bytes by task 2808 on cpu 0:
>   [  199.559464]  record_root_in_trans+0x196/0x228 [btrfs]
>   [  199.560236]  btrfs_record_root_in_trans+0xfe/0x128 [btrfs]
>   [  199.561097]  start_transaction+0x154/0xcd8 [btrfs]
>   [  199.561927]  btrfs_join_transaction+0x44/0x60 [btrfs]
>   [  199.562700]  btrfs_dirty_inode+0x9c/0x140 [btrfs]
>   [  199.563493]  btrfs_update_time+0x8c/0xb0 [btrfs]
>   [  199.564277]  file_update_time+0xb8/0xf0
>   [  199.564301]  pipe_write+0x8ac/0xab8
>   [  199.564326]  vfs_write+0x33c/0x588
>   [  199.564349]  ksys_write+0x108/0x150
>   [  199.564372]  __s390x_sys_write+0x68/0x88
>   [  199.564397]  do_syscall+0x1c6/0x210
>   [  199.564424]  __do_syscall+0xc8/0xf0
>   [  199.564452]  system_call+0x70/0x98
> 
> This is because we update and read last_trans concurrently without any
> type of synchronization. This should be generally harmless and in the
> worst case it can make us do extra locking (btrfs_record_root_in_trans())
> trigger some warnings at ctree.c or do extra work during relocation - this
> would probably only happen in case of load or store tearing.
> 
> So fix this by always reading and updating the field using READ_ONCE()
> and WRITE_ONCE(), this silences KCSAN and prevents load and store tearing.

I'm curious why you mention the load/store tearing, as we discussed this
last time under some READ_ONCE/WRITE_ONCE change it's not happening on
aligned addresses for any integer type, I provided links to intel manuals.

I suggest using data_race as is more suitable in this case, it's more
specific than READ_ONCE/WRITE_ONCE that is for preventing certain
compiler optimizations.