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

[Nikolay Borisov <nborisov@suse.com>]

On 26.10.18 г. 18:34 ч., Anand Jain wrote:
> 
> 
> 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

I missed that, I will recommend moving the assignment near where you set
the devices

> 
> 
>>> +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
>>>
>