On Fri, Mar 08, 2024 at 08:15:07AM +0530, Anand Jain wrote:
> Boris managed to create a device capable of changing its maj:min without
> altering its device path.
>
> Only multi-devices can be scanned. A device that gets scanned and remains
> in the Btrfs kernel cache might end up with an incorrect maj:min.
>
> Despite the tempfsid feature patch did not introduce this bug, it could
> lead to issues if the above multi-device is converted to a single device
> with a stale maj:min. Subsequently, attempting to mount the same device
> with the correct maj:min might mistake it for another device with the same
> fsid, potentially resulting in wrongly auto-enabling the tempfsid feature.
>
> To address this, this patch validates the device's maj:min at the time of
> device open and updates it if it has changed since the last scan.
You and Dave have convinced me that it is important to fix this in the
kernel. I still have a hope of simplifying this further, while we are
here and have the code kicking around in our heads.
>
> CC: stable@xxxxxxxxxxxxxxx # 6.7+
> Fixes: a5b8a5f9f835 ("btrfs: support cloned-device mount capability")
> Reported-by: Boris Burkov <boris@xxxxxx>
> Co-developed-by: Boris Burkov <boris@xxxxxx>
> Signed-off-by: Anand Jain <anand.jain@xxxxxxxxxx>
> ---
> v2:
> Drop using lookup_bdev() instead, get it from device->bdev->bd_dev.
>
> v1:
> https://lore.kernel.org/linux-btrfs/752b8526be21d984e0ee58c7f66d312664ff5ac5.1709256891.git.anand.jain@xxxxxxxxxx/
>
> fs/btrfs/volumes.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index e49935a54da0..c318640b4472 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -692,6 +692,16 @@ static int btrfs_open_one_device(struct btrfs_fs_devices *fs_devices,
> device->bdev = bdev_handle->bdev;
> clear_bit(BTRFS_DEV_STATE_IN_FS_METADATA, &device->dev_state);
>
> + if (device->devt != device->bdev->bd_dev) {
> + btrfs_warn(NULL,
> + "device %s maj:min changed from %d:%d to %d:%d",
> + device->name->str, MAJOR(device->devt),
> + MINOR(device->devt), MAJOR(device->bdev->bd_dev),
> + MINOR(device->bdev->bd_dev));
> +
> + device->devt = device->bdev->bd_dev;
> + }
> +
If we are permanently maintaining an invariant that device->devt ==
device->bdev->bd_dev, do we even need device->devt? As far as I can
tell, all the logic that uses device->devt assumes that the device is
not stale, both in the temp_fsid found_by_devt lookup and in the "device
changed name" check. If so, we could just always use
device->bdev->bd_dev and eliminate this confusion/source of bugs
entirely.
> fs_devices->open_devices++;
> if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state) &&
> device->devid != BTRFS_DEV_REPLACE_DEVID) {
> --
> 2.38.1
>
