Re: [PATCH] btrfs: prevent device path updating during mount

[Boris Burkov <boris@bur.io>]

On Mon, Aug 11, 2025 at 05:03:15PM +0930, Qu Wenruo wrote:
> [REGRESSION]
> After commit bddf57a70781 ("btrfs: delay btrfs_open_devices() until
> super block is created"), test case btrfs/315 can fail like this
> randomly:
> 
> btrfs/315 1s ... - output mismatch (see /home/adam/xfstests/results//btrfs/315.out.bad)
>     --- tests/btrfs/315.out	2025-08-11 16:40:36.496000000 +0930
>     +++ /home/adam/xfstests/results//btrfs/315.out.bad	2025-08-11 16:41:04.304000000 +0930
>     @@ -1,7 +1,7 @@
>      QA output created by 315
>      ---- seed_device_must_fail ----
>      mount: SCRATCH_MNT: WARNING: source write-protected, mounted read-only.
>     -mount: File exists
>     +mount: /mnt/test/315/tempfsid_mnt: WARNING: source write-protected, mounted read-only
>      ---- device_add_must_fail ----
>      wrote 9000/9000 bytes at offset 0
>     ...
>     (Run 'diff -u /home/adam/xfstests/tests/btrfs/315.out /home/adam/xfstests/results//btrfs/315.out.bad'  to see the entire diff)
> Ran: btrfs/315
> Failures: btrfs/315
> Failed 1 of 1 tests
> 
> [CAUSE]
> The failure is that the second seed device (with a duplicated fsid and
> dev uuid) is mounted successfully, which is unexpected.
> 
> In my environment, the following 2 devices involved in the
> "seed_device_must_fail" run are:
> 
>  lrwxrwxrwx 1 root root 7 Aug 11 09:03 /dev/test/scratch1 -> ../dm-2
>  lrwxrwxrwx 1 root root 7 Aug 11 09:03 /dev/test/scratch2 -> ../dm-4
> 
> Note the kernel dmesg of that run, when mounting the first seed device
> (scratch1), the real device got mounted is the second seed device
> (scratch2):
> 
>  BTRFS: device fsid 7c8a5017-0c44-456f-8acf-57663f954e53 devid 1 transid 9 /dev/mapper/test-scratch1 (253:2) scanned by mount (3343974)
>  BTRFS info (device dm-4): first mount of filesystem 7c8a5017-0c44-456f-8acf-57663f954e53
>  BTRFS info (device dm-4): using crc32c (crc32c-generic) checksum algorithm
>  BTRFS info (device dm-4): using free-space-tre
> 
> Note that "(device dm-4)" line, this means /dev/test/scratch2 is
> mounted when running "mount /dev/test/scratch1 /mnt/scratch".
> 
> Then when trying to mount /dev/test/scratch2, since the same device is
> already mounted, it returns the same super block and do not fail.
> 
> The root cause is, when setting seed device flags for both devices,
> a btrfs device scan is triggered, that scan is delayed and can happen at
> any time.
> 
> So there is a race window between scanning the second device and
> mounting the first device, where the device path can be replaced
> halfway:
> 
>      Mount scratch1                |          Scanning scratch2
> -----------------------------------+-------------------------------------
> btrfs_get_tree_super()             |
> |- btrfs_scan_one_device()         |
> |  We're mounting "scratch1"       |
> |                                  |
> |- btrfs_fs_devices_inc_holding()  |
> |- mutex_unlock(&uuid_mutex);      |
> |                                  | btrfs_scan_one_device()
> |                                  | |- device_list_add()
> |                                  |    |- rcu_assign_pointer()
> |                                  |       This changes the device->name
> |                                  |       to "scratch2"
> |- sget_fc()                       |
> |  This creates a new super block  |
> |- mutex_lock(&uuid_mutex)         |
> |- btrfs_fs_devices_dec_holding()  |
> |- btrfs_open_devices()            |
>    |- btrfs_open_one_device()      |
>       "scratch2" is opened as that |
>       is recorded in device->name   |
> 
> Commit bddf57a70781 ("btrfs: delay btrfs_open_devices() until super
> block is created") introduced fs_devices holding mechanism to allow
> devices to be opened after super block is created.
> 
> But that holding period doesn't keep device->name untouched, allowing
> a duplicated device to replace the path, thus mounting the incorrect
> device.
> 
> [FIX]
> Also check fs_devices->holding value before replacing the device->name.
> If fs_devices->holding is not zero, meaning someone is trying to mount
> the fs, then do not allow device->name to be updated.
> 
> Although this situation is rare, require certain race window and
> two devices with duplicated fsid/dev uuid (which is already very cursed),
> still output a warning message so that end users can be informed about
> such cursed situation.

Thanks for working on this, the last time I worked in this code it made
my head hurt, so I appreciate this is annoying to fix.

With that said, I have one concern: do you know if there are existing
tests exercising the legit name change behaviors outlined in the big
comment starting with "When FS is already mounted"? I have a feeling
that this check will prevent those as well, but I am not sufficiently
familiar with the new holding delayed open logic to be sure.

Thanks,
Boris

> 
> Fixes: bddf57a70781 ("btrfs: delay btrfs_open_devices() until super block is created")
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/volumes.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index fa7a929a0461..4fdd84e0bff9 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -934,6 +934,20 @@ static noinline struct btrfs_device *device_list_add(const char *path,
>  				  path, found_transid, device->generation);
>  			return ERR_PTR(-EEXIST);
>  		}
> +		if (fs_devices->holding) {
> +			/*
> +			 * The fs_devices are already hold by an ongoing mount.
> +			 *
> +			 * We can not update the device path, or a duplicated
> +			 * fsid/dev-uuid can replace the original path, causing
> +			 * another device to be mounted.
> +			 */
> +			btrfs_warn(NULL,
> +				   "device %s is trying to update path for a device being mounted",
> +				   path);
> +			mutex_unlock(&fs_devices->device_list_mutex);
> +			return ERR_PTR(-EEXIST);
> +		}
>  
>  		/*
>  		 * We are going to replace the device path for a given devid,
> -- 
> 2.50.1
>