Message-ID: <3316b26c-346a-43a7-bfe3-b9c2d3b81c6f@suse.com>
Date: Thu, 26 Jun 2025 18:40:34 +0930
From: Qu Wenruo <wqu@...e.com>
To: haoran zheng <zhenghaoran154@...il.com>, clm@...com,
 josef@...icpanda.com, dsterba@...e.com
Cc: linux-btrfs@...r.kernel.org, linux-kernel@...r.kernel.org,
 baijiaju1990@...il.com, zzzccc427@...il.com
Subject: Re: [BUG] Five data races in btrfs



在 2025/6/26 17:53, haoran zheng 写道:
> [BUG] Five data races in btrfs
> 
> Hello maintainers,
> 
> I would like to report five data race bugs we discovered in the BTRFS
> filesystem on Linux kernel v6.16-rc3. These issues were identified
> using our data race detector.
> 
> We are currently analyzing the Btrfs codebase and have identified
> several instances that may involve potential data races during
> concurrent execution. While we have reviewed the relevant code
> paths carefully, we are uncertain whether these issues could lead
> to any practical impact or system instability.
> 
> Below is a summary of the findings:
> 
> ---
> 
> 1. Race between `__btrfs_set_fs_incompat()` and `btrfs_fs_incompat()`
> 
> `__btrfs_set_fs_incompat()` performs a write to the
> `super_copy->incompat_flags` field under `fs_info->super_lock` while
> `btrfs_need_stripe_tree_update()` calls `btrfs_fs_incompat()` without
> acquiring `super_lock`, which may read a stale or partially updated
> value of `incompat_flags`.

I think we should cache the super block incompat/compat_ro flags into 
btrfs_fs_info, so that we can use bit operations.
> ---
> 
> 2. Race between `btrfs_defrag_file()` and `inode_need_compress()`
> 
> In `btrfs_defrag_file()`, the field `inode->defrag_compress` is
> assigned while holding the inode lock via `btrfs_inode_lock()`.
> But in `inode_need_compress()`, the same field is read without
> any apparent locking or memory barrier.
I do not think we should hold any inode lock just to access defrag_compress.

Mind to explain why accessing that member without any lock can be 
problematic?

The only thing I can think of is some unaligned access,
but accessing a u8 alone shouldn't need special lock.

> ---
> 
> 3. Race between `join_transaction()` and `btrfs_get_fs_generation()`
> 
> In `join_transaction()`, `fs_info->generation` is assigned while
> holding the lock `fs_info->trans_lock`. But reads of
> `fs_info->generation` are done using READ_ONCE() in
> `btrfs_get_fs_generation()`.

Again, I didn't see much problem accessing a single u64 value with or 
without holding a lock.


> ---
> 
> 4. Race between `btrfs_super_bytes_used()` and `btrfs_update_block_group()`
> 
> In `btrfs_set_backup_bytes_used()`, super_copy is read and stored  without
> holding lock `info->delalloc_root_lock`. But in `btrfs_update_block_group()`
> the `info->super_copy` is set concurrently.

This is definitely false alert.

In btrfs_update_block_group() it holds a transaction handle, thus no one 
can commit the current transaction until the transaction handle is released.

Furthermore, btrfs_super_bytes_used() is accessing the superblock copy, 
but super block copy is only updated during the end of commit transaction.

So it's very safe to access the super block members with a trans handle 
hold.


> ---
> 
> 5. Race between `btrfs_defrag_file()` and `btrfs_defrag_file()`
> 
> In the function btrfs_defrag_file(), we also noticed a possible
> race condition involving the writeback_index field of the
> address_space structure associated with the inode. Specifically,
> the code performs a read and conditional write without any
> evident locking:

This may need some extra digging, but so far I'm seeing other fses doing 
a similar work.

And considering all the above 4 cases, I'm not that confident if your 
reports are that helpful.

Thanks,
Qu

Powered by blists - more mailing lists