Re: [PATCH v2 rev log added] fstests: btrfs verify hardening agaist duplicate fsid

[Anand Jain <anand.jain@oracle.com>]

On 10/26/2018 11:02 PM, Nikolay Borisov wrote:
> 
> 
> On 8.10.18 г. 21:28 ч., Anand Jain wrote:
>> 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. So this test case reproduces this issue.
>>
>> 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
>> the sda out of the system on to another system to change its fsid using
>> the 'btrfstune -u' command.
> 
> Is there a pending fix for this?

Yes.
https://patchwork.kernel.org/patch/10641041/

Test case header mentioned it.

> 
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> v1->v2:
>>    dont play around with dev patch use it as it is.
>>    do not use SCRATCH_MNT instead create it at the TEST_DIR and its related
>>     changes.
>>    golden out changes
>>     
>>   tests/btrfs/173     | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   tests/btrfs/173.out |  6 ++++
>>   tests/btrfs/group   |  1 +
>>   3 files changed, 95 insertions(+)
>>   create mode 100755 tests/btrfs/173
>>   create mode 100644 tests/btrfs/173.out
>>
>> diff --git a/tests/btrfs/173 b/tests/btrfs/173
>> new file mode 100755
>> index 000000000000..b466ae921e19
>> --- /dev/null
>> +++ b/tests/btrfs/173
>> @@ -0,0 +1,88 @@
>> +#! /bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (c) 2018 Oracle. All Rights Reserved.
>> +#
>> +# FS QA Test 173
>> +#
>> +# Fuzzy test for FS image duplication.
>> +#  Could be fixed by
>> +#    [patch] btrfs: harden agaist duplicate fsid
>> +#
>> +seq=`basename $0`
>> +seqres=$RESULT_DIR/$seq
>> +echo "QA output created by $seq"
>> +
>> +here=`pwd`
>> +tmp=/tmp/$$
>> +status=1	# failure is the default!
>> +trap "_cleanup; exit \$status" 0 1 2 3 15
>> +
>> +mnt=$TEST_DIR/$seq.mnt
>> +_cleanup()
>> +{
>> +	rm -rf $mnt > /dev/null 2>&1
>> +	cd /
>> +	rm -f $tmp.*
>> +}
>> +
>> +# get standard environment, filters and checks
>> +. ./common/rc
>> +. ./common/filter
>> +
>> +# remove previous $seqres.full before test
>> +rm -f $seqres.full
>> +
>> +# real QA test starts here
>> +
>> +# Modify as appropriate.
>> +_supported_fs btrfs
>> +_supported_os Linux
>> +_require_scratch_dev_pool 2
>> +_scratch_dev_pool_get 2
>> +
>> +dev_foo=$(echo $SCRATCH_DEV_POOL | awk '{print $1}')
>> +dev_bar=$(echo $SCRATCH_DEV_POOL | awk '{print $2}')
> 
> Naming devices _foo and _bar just shows you could not care less about
> the test.

   Hmm. Will make it device_1 and device_2.

> Either use sane names - primary_dev/secondary dev or device_1
> and device_2.

>> +
>> +echo dev_foo=$dev_foo >> $seqres.full
>> +echo dev_bar=$dev_bar >> $seqres.full
>> +echo | tee -a $seqres.full
>> +
>> +rm -rf $mnt > /dev/null 2>&1
> 
> So what is $mnt? I can see it's used by very few tests and it's not
> obvious? Generally you should either define it as a local variable or
> use one of the SCRATCH_MNT/TEST_MNT global variables. Also I checked
> with Eric re. the use of $mnt in tests and his conclusion is :
> 
> "<sandeen> looks like a bug"

  No its not. As few lines above, I have assigned it as..
     mnt=$TEST_DIR/$seq.mnt


>> +mkdir $mnt
>> +_mkfs_dev $dev_foo
>> +_mount $dev_foo $mnt
>> +
>> +check_btrfs_mount()
>> +{
>> +	local x=$(findmnt $mnt | grep -v TARGET | awk '{print $2}')
>> +	[[ $x == $dev_foo ]] && echo DEV_FOO
>> +	[[ $x == $dev_bar ]] && echo DEV_BAR
>> +}
> 
> Same thing here re. DEV_(FOO|BAR).
> 
>> +
>> +echo MNT $(check_btrfs_mount)
>> +
>> +for sb_bytenr in 65536 67108864
>> +do
>> +	echo -n "dd status=none if=$dev_foo of=$dev_bar bs=1 "\
>> +		"seek=$sb_bytenr skip=$sb_bytenr count=4096" >> $seqres.full
>> +	dd status=none if=$dev_foo of=$dev_bar bs=1 seek=$sb_bytenr \
>> +				skip=$sb_bytenr count=4096 >> $seqres.full 2>&1
>> +	echo ..:$? >> $seqres.full
> 
> This is an overkill, just use dd ... >/dev/null 2>&1, no one cares about
> the output of that. Also use DIO so that sb's are directly written to
> the devices.

  Yeah right will fix.

>> +done
>> +
>> +#Original device is mounted, scan of its clone should fail
>> +$BTRFS_UTIL_PROG device scan $dev_bar >> $seqres.full 2>&1
>> +echo btrfs device scan dev_bar ...:$?| tee -a $seqres.full
>> +
>> +echo MNT $(check_btrfs_mount)
>> +
>> +#Original device scan should be successful
>> +$BTRFS_UTIL_PROG device scan $dev_foo >> $seqres.full 2>&1
>> +echo btrfs device scan dev_foo ...:$?| tee -a $seqres.full
>> +
>> +umount $mnt > /dev/null 2>&1
>> +_scratch_dev_pool_put
>> +
>> +# success, all done
>> +status=0
>> +exit
>> diff --git a/tests/btrfs/173.out b/tests/btrfs/173.out
>> new file mode 100644
>> index 000000000000..3c7e3fb4e3f7
>> --- /dev/null
>> +++ b/tests/btrfs/173.out
>> @@ -0,0 +1,6 @@
>> +QA output created by 173
>> +
>> +MNT DEV_FOO
>> +btrfs device scan dev_bar ...:1
>> +MNT DEV_FOO
>> +btrfs device scan dev_foo ...:0
> 
> It seems the output is really pointless it should just be "Silence is
> golden". Then in the test you would do all your run-time checks of
> what's the source of the mount etc etc and if your expectations don't
> match you fail the test and don't output "Silence is golden" which would
> signal success. Check for example

  I could do that. Will fix.

Thanks, Anand

>> diff --git a/tests/btrfs/group b/tests/btrfs/group
>> index 45782565c3b7..b2f1393f3e97 100644
>> --- a/tests/btrfs/group
>> +++ b/tests/btrfs/group
>> @@ -175,3 +175,4 @@
>>   170 auto quick snapshot
>>   171 auto quick qgroup
>>   172 auto quick punch
>> +173 volume
>>