Re: Patch "btrfs: harden agaist duplicate fsid on scanned devices" has been added to the 4.14-stable tree

Re: Patch "btrfs: harden agaist duplicate fsid on scanned devices" has been added to the 4.14-stable tree

[Greg KH]

On Sat, Feb 09, 2019 at 12:28:20PM -0500, Sasha Levin wrote:
> This is a note to let you know that I've just added the patch titled
> 
>     btrfs: harden agaist duplicate fsid on scanned devices
> 
> to the 4.14-stable tree which can be found at:
>     http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
> 
> The filename of the patch is:
>      btrfs-harden-agaist-duplicate-fsid-on-scanned-device.patch
> and it can be found in the queue-4.14 subdirectory.
> 
> If you, or anyone else, feels it should not be added to the stable tree,
> please let <stable@vger.kernel.org> know about it.
> 
> 
> 
> commit 2590f717cfd952f726597e4ebf0800154be5e5f5
> Author: Anand Jain <anand.jain@oracle.com>
> Date:   Mon Oct 15 10:45:17 2018 +0800
> 
>     btrfs: harden agaist duplicate fsid on scanned devices
>     
>     [ Upstream commit a9261d4125c97ce8624e9941b75dee1b43ad5df9 ]
>     
>     It's not that impossible to imagine that a device OR a btrfs image is
>     copied just by using the dd or the cp command. Which in case both the
>     copies of the btrfs will have the same fsid. If on the system with
>     automount enabled, the copied FS gets scanned.
>     
>     We have a known bug in btrfs, that we let the device path be changed
>     after the device has been mounted. So using this loop hole the new
>     copied device would appears as if its mounted immediately after it's
>     been copied.
>     
>     For example:
>     
>     Initially.. /dev/mmcblk0p4 is mounted as /
>     
>       $ lsblk
>       NAME        MAJ:MIN RM  SIZE RO TYPE MOUNTPOINT
>       mmcblk0     179:0    0 29.2G  0 disk
>       |-mmcblk0p4 179:4    0    4G  0 part /
>       |-mmcblk0p2 179:2    0  500M  0 part /boot
>       |-mmcblk0p3 179:3    0  256M  0 part [SWAP]
>       `-mmcblk0p1 179:1    0  256M  0 part /boot/efi
>     
>       $ btrfs fi show
>          Label: none  uuid: 07892354-ddaa-4443-90ea-f76a06accaba
>          Total devices 1 FS bytes used 1.40GiB
>          devid    1 size 4.00GiB used 3.00GiB path /dev/mmcblk0p4
>     
>     Copy mmcblk0 to sda
>     
>       $ dd if=/dev/mmcblk0 of=/dev/sda
>     
>     And immediately after the copy completes the change in the device
>     superblock is notified which the automount scans using btrfs device scan
>     and the new device sda becomes the mounted root device.
>     
>       $ lsblk
>       NAME        MAJ:MIN RM  SIZE RO TYPE MOUNTPOINT
>       sda           8:0    1 14.9G  0 disk
>       |-sda4        8:4    1    4G  0 part /
>       |-sda2        8:2    1  500M  0 part
>       |-sda3        8:3    1  256M  0 part
>       `-sda1        8:1    1  256M  0 part
>       mmcblk0     179:0    0 29.2G  0 disk
>       |-mmcblk0p4 179:4    0    4G  0 part
>       |-mmcblk0p2 179:2    0  500M  0 part /boot
>       |-mmcblk0p3 179:3    0  256M  0 part [SWAP]
>       `-mmcblk0p1 179:1    0  256M  0 part /boot/efi
>     
>       $ btrfs fi show /
>         Label: none  uuid: 07892354-ddaa-4443-90ea-f76a06accaba
>         Total devices 1 FS bytes used 1.40GiB
>         devid    1 size 4.00GiB used 3.00GiB path /dev/sda4
>     
>     The bug is quite nasty that you can't either unmount /dev/sda4 or
>     /dev/mmcblk0p4. And the problem does not get solved until you take sda
>     out of the system on to another system to change its fsid using the
>     'btrfstune -u' command.
>     
>     Signed-off-by: Anand Jain <anand.jain@oracle.com>
>     Reviewed-by: David Sterba <dsterba@suse.com>
>     Signed-off-by: David Sterba <dsterba@suse.com>
>     Signed-off-by: Sasha Levin <sashal@kernel.org>
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 9663b6aa2a56..1e8d216720a2 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -696,6 +696,35 @@ static noinline int device_list_add(const char *path,
>  			return -EEXIST;
>  		}
>  
> +		/*
> +		 * We are going to replace the device path for a given devid,
> +		 * make sure it's the same device if the device is mounted
> +		 */
> +		if (device->bdev) {
> +			struct block_device *path_bdev;
> +
> +			path_bdev = lookup_bdev(path);
> +			if (IS_ERR(path_bdev)) {
> +				mutex_unlock(&fs_devices->device_list_mutex);
> +				return ERR_CAST(path_bdev);
> +			}
> +
> +			if (device->bdev != path_bdev) {
> +				bdput(path_bdev);
> +				mutex_unlock(&fs_devices->device_list_mutex);
> +				btrfs_warn_in_rcu(device->fs_info,
> +			"duplicate device fsid:devid for %pU:%llu old:%s new:%s",
> +					disk_super->fsid, devid,
> +					rcu_str_deref(device->name), path);
> +				return ERR_PTR(-EEXIST);
> +			}
> +			bdput(path_bdev);
> +			btrfs_info_in_rcu(device->fs_info,
> +				"device fsid %pU devid %llu moved old:%s new:%s",
> +				disk_super->fsid, devid,
> +				rcu_str_deref(device->name), path);
> +		}
> +
>  		name = rcu_string_strdup(path, GFP_NOFS);
>  		if (!name)
>  			return -ENOMEM;

This patch causes a build warning:

../fs/btrfs/volumes.c:709:12: warning: returning ‘void *’ from a function with return type ‘int’ makes integer from pointer without a cast [-Wint-conversion]
     return ERR_CAST(path_bdev);
            ^~~~~~~~~~~~~~~~~~~
../fs/btrfs/volumes.c:719:12: warning: returning ‘void *’ from a function with return type ‘int’ makes integer from pointer without a cast [-Wint-conversion]
     return ERR_PTR(-EEXIST);
            ^~~~~~~~~~~~~~~~

and it's correct, the code is should not do this.  I'll drop this
patch now.

thanks,

greg k-h