Re: [PATCH v2] btrfs: reject refs=0 subvolume without DEAD in SUBVOL_SYNC_WAIT

[Qu Wenruo <wqu@suse.com>]

在 2026/11/1 16:55, Cheng Yongkang 写道:
> btrfs_ioctl_subvol_sync() assumes that a cached root with zero
> references has BTRFS_ROOT_SUBVOL_DEAD set, and asserts that before
> waiting for the cleaner. Two states break the assumption:
> 
> - A corrupt ROOT_ITEM with zero references and a matching orphan item,
>    on a writable filesystem.
> - A failed subvolume deletion. btrfs_delete_subvolume() zeroes the
>    root references, aborts the transaction, which forces the filesystem
>    read-only, and then clears DEAD again. No on-disk metadata is
>    corrupted in this case.
> 
> With CONFIG_BTRFS_ASSERT=y, both reach a BUG().
> 
> Before waiting for a deleted subvolume, return -EROFS if the
> filesystem is read-only, since the cleaner does not drop dead roots
> then. The check runs on every iteration, so it also stops a waiter
> when the filesystem becomes read-only during the wait. Subvolume
> deletion holds subvol_sem for writing across the abort and the DEAD
> rollback, and each iteration takes it for reading before inspecting
> the root, so the read-only state and the cleared flag are seen
> together. On a writable filesystem, report a zero-reference root
> without DEAD and return -EUCLEAN instead of asserting.
> 
> Subvolumes that were not deleted or do not exist still return -EEXIST
> and -ENOENT, also on a read-only filesystem.
> 
> Tested on btrfs for-next with CONFIG_BTRFS_ASSERT=y. A deletion made
> to fail by fault injection after the references are cleared hits the
> assertion in SUBVOL_SYNC_WAIT without this change and gets -EROFS with
> it. A waiter on a deleted subvolume now returns -EROFS when the
> filesystem is remounted read-only, instead of waiting until it is
> signalled.
> 
> Fixes: 6c83d153ed86 ("btrfs: add new ioctl to wait for cleaned subvolumes")
> Cc: stable@vger.kernel.org
> Suggested-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
> Link: https://lore.kernel.org/linux-btrfs/dd97efe9-91ec-495b-b698-acff1447421c@gmx.com/
> Signed-off-by: Cheng Yongkang <teel4res@gmail.com>
> ---
> Changes in v2:
> - Return -EROFS on a read-only filesystem before waiting for a deleted
>    subvolume (Qu).
> - Use unlikely() for the inconsistent-root check (Qu).
> - Describe the failed-delete case separately from corrupt metadata.
> - Add the Fixes tag and Cc stable.
> 
> v1: https://lore.kernel.org/linux-btrfs/20261025180258.349175-1-teel4res@gmail.com/
> 
> Testing notes (btrfs for-next d1dca33127ef, KASAN):
> - The fault-injection case reached the cleared-references state in 2
>    of 4 attempts on each kernel. The other attempts aborted earlier, and
>    both kernels returned -EEXIST.
> - FOR_ONE, FOR_QUEUED, COUNT and PEEK return the same results with and
>    without this change for live, missing and deleted subvolumes, on
>    writable and read-only mounts.
> - The -EUCLEAN branch was not exercised. A crafted image with a
>    zero-reference root and no DEAD flag hits the assertion without this
>    change, but remounting that image read-write stalls in a transaction
>    commit with or without this change, before the filesystem becomes
>    writable.
> 
>   fs/btrfs/ioctl.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 9857d6a9d456..5a1f2edfd831 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -5166,8 +5166,14 @@ static int btrfs_ioctl_subvol_sync(struct btrfs_fs_info *fs_info, void __user *a
>   			return -ENOENT;
>   
>   		wait_for_deletion = true;
> -		ASSERT(root_flags & BTRFS_ROOT_SUBVOL_DEAD, "root_flags=0x%llx",
> -		       root_flags);
> +		if (unlikely(test_bit(BTRFS_FS_STATE_RO, &fs_info->fs_state)))
> +			return -EROFS;

I'm not suggesting such an ad-hoc check, but some more comprehensive 
checks, like mnt_want_write_file().
As this one won't handle aborted transaction.

But it's not a good idea using that mnt_want_write_file() either, as 
that will block pm suspension/hibernation, especially considering the 
wait will only be handled by cleaner thread.

The default timeout for waking up cleaner is 30s, which is definitely 
not suitable for such calls.


If you are not sure what to do with the RW check and cleaner behavior, 
just don't put an adhoc solution into the update.
At least it won't be any worse.


> +		if (unlikely(!(root_flags & BTRFS_ROOT_SUBVOL_DEAD))) {
> +			btrfs_err(fs_info,
> +				  "subvol %llu has refs 0 without DEAD flag",
> +				  args.subvolid);
> +			return -EUCLEAN;
> +		}
>   		sched_ret = schedule_timeout_interruptible(HZ);
>   		/* Early wake up or error. */
>   		if (sched_ret != 0)
> 
> base-commit: d1dca33127efacd86ac550aa86352f8ea9f29a5a