linux-btrfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH RFC RESEND] btrfs: harden agaist duplicate fsid
@ 2018-10-15  2:45 Anand Jain
  2018-11-01 17:04 ` David Sterba
  2018-11-13 15:21 ` David Sterba
  0 siblings, 2 replies; 4+ messages in thread
From: Anand Jain @ 2018-10-15  2:45 UTC (permalink / raw)
  To: linux-btrfs

(Thanks for the comments on requiring to warn_on if we fail the device change.)
(This fixes an ugly bug, I appreciate if you have any further comments).

Its not that impossible to imagine that a device OR a btrfs image is
been 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 its
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>
---

Hi,

There was previous attempt to fix this bug ref: 
   www.spinics.net/lists/linux-btrfs/msg37466.html

which broke the Ubuntu subvol mount at boot. The reason
for that is, Ubuntu changes the device path in the boot
process, and the earlier fix checked for the device-path
instead of block_device and and so we failed the
subvol mount request and thus the bootup process.
This patch instead checks for the block_device instead of
the device-path.

I have tested this with Oracle Linux with btrfs as boot device
with a subvol to be mounted at boot. And also have verified
with new test case btrfs/173.

It will be good if someone run this through Ubuntu boot test case.

 fs/btrfs/volumes.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index f4405e430da6..62173a3abcc4 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -850,6 +850,29 @@ static noinline struct btrfs_device *device_list_add(const char *path,
 			return ERR_PTR(-EEXIST);
 		}
 
+		/*
+		 * we are going to replace the device path, make sure its the
+		 * same device if the device 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);
+				return ERR_PTR(-EEXIST);
+			}
+			bdput(path_bdev);
+			pr_info("BTRFS: device fsid:devid %pU:%llu old path:%s new path:%s\n",
+				disk_super->fsid, devid, rcu_str_deref(device->name), path);
+		}
+
 		name = rcu_string_strdup(path, GFP_NOFS);
 		if (!name) {
 			mutex_unlock(&fs_devices->device_list_mutex);
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH RFC RESEND] btrfs: harden agaist duplicate fsid
  2018-10-15  2:45 [PATCH RFC RESEND] btrfs: harden agaist duplicate fsid Anand Jain
@ 2018-11-01 17:04 ` David Sterba
  2018-11-13 15:21 ` David Sterba
  1 sibling, 0 replies; 4+ messages in thread
From: David Sterba @ 2018-11-01 17:04 UTC (permalink / raw)
  To: Anand Jain; +Cc: linux-btrfs

On Mon, Oct 15, 2018 at 10:45:17AM +0800, Anand Jain wrote:
> (Thanks for the comments on requiring to warn_on if we fail the device change.)
> (This fixes an ugly bug, I appreciate if you have any further comments).
> 
> Its not that impossible to imagine that a device OR a btrfs image is
> been 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 its
> 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>

The fix for that case looks ok to me, block devices with duplicate UUIDs
are problematic. I'll add the patch to for-next, thanks.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH RFC RESEND] btrfs: harden agaist duplicate fsid
  2018-10-15  2:45 [PATCH RFC RESEND] btrfs: harden agaist duplicate fsid Anand Jain
  2018-11-01 17:04 ` David Sterba
@ 2018-11-13 15:21 ` David Sterba
  2018-11-13 15:42   ` Anand Jain
  1 sibling, 1 reply; 4+ messages in thread
From: David Sterba @ 2018-11-13 15:21 UTC (permalink / raw)
  To: Anand Jain; +Cc: linux-btrfs

On Mon, Oct 15, 2018 at 10:45:17AM +0800, Anand Jain wrote:
> (Thanks for the comments on requiring to warn_on if we fail the device change.)
> (This fixes an ugly bug, I appreciate if you have any further comments).
> 
> Its not that impossible to imagine that a device OR a btrfs image is
> been 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 its
> 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>

I'm adding the patch to misc-next now, with an update message that
matches the format when a device is scanned.

"BTRFS: device fsid %pU devid %llu moved old:%s new:%s\n",

That way it should be possible to grep for all messages that are related
to the scanning ioctl.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH RFC RESEND] btrfs: harden agaist duplicate fsid
  2018-11-13 15:21 ` David Sterba
@ 2018-11-13 15:42   ` Anand Jain
  0 siblings, 0 replies; 4+ messages in thread
From: Anand Jain @ 2018-11-13 15:42 UTC (permalink / raw)
  To: dsterba, linux-btrfs



On 11/13/2018 11:21 PM, David Sterba wrote:
> On Mon, Oct 15, 2018 at 10:45:17AM +0800, Anand Jain wrote:
>> (Thanks for the comments on requiring to warn_on if we fail the device change.)
>> (This fixes an ugly bug, I appreciate if you have any further comments).
>>
>> Its not that impossible to imagine that a device OR a btrfs image is
>> been 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 its
>> 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>
> 
> I'm adding the patch to misc-next now, with an update message that
> matches the format when a device is scanned.
> 
> "BTRFS: device fsid %pU devid %llu moved old:%s new:%s\n",
> 
> That way it should be possible to grep for all messages that are related
> to the scanning ioctl.

  Right. Looks fine to me. Thanks, Anand

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2018-11-13 15:42 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-15  2:45 [PATCH RFC RESEND] btrfs: harden agaist duplicate fsid Anand Jain
2018-11-01 17:04 ` David Sterba
2018-11-13 15:21 ` David Sterba
2018-11-13 15:42   ` Anand Jain

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).