From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx2.suse.de ([195.135.220.15]:33718 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726113AbeJ0A3z (ORCPT ); Fri, 26 Oct 2018 20:29:55 -0400 Subject: Re: [PATCH v2 rev log added] fstests: btrfs verify hardening agaist duplicate fsid References: <1538383475-2532-1-git-send-email-anand.jain@oracle.com> <1539023301-4583-1-git-send-email-anand.jain@oracle.com> <6b58c095-d0a1-6330-0385-b06aa9c99483@oracle.com> From: Nikolay Borisov Message-ID: <8003c16e-1eef-1e7e-d697-11d8d32c2e91@suse.com> Date: Fri, 26 Oct 2018 18:52:18 +0300 MIME-Version: 1.0 In-Reply-To: <6b58c095-d0a1-6330-0385-b06aa9c99483@oracle.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Sender: fstests-owner@vger.kernel.org Content-Transfer-Encoding: quoted-printable To: Anand Jain , fstests@vger.kernel.org Cc: linux-btrfs@vger.kernel.org List-ID: On 26.10.18 =D0=B3. 18:34 =D1=87., Anand Jain wrote: >=20 >=20 > On 10/26/2018 11:02 PM, Nikolay Borisov wrote: >> >> >> On 8.10.18 =D0=B3. 21:28 =D1=87., 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=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 MAJ:MIN RM=C2=A0 SIZE = RO TYPE MOUNTPOINT >>> mmcblk0=C2=A0=C2=A0=C2=A0=C2=A0 179:0=C2=A0=C2=A0=C2=A0 0 29.2G=C2=A0= 0 disk >>> |-mmcblk0p4 179:4=C2=A0=C2=A0=C2=A0 0=C2=A0=C2=A0=C2=A0 4G=C2=A0 0 pa= rt / >>> |-mmcblk0p2 179:2=C2=A0=C2=A0=C2=A0 0=C2=A0 500M=C2=A0 0 part /boot >>> |-mmcblk0p3 179:3=C2=A0=C2=A0=C2=A0 0=C2=A0 256M=C2=A0 0 part [SWAP] >>> `-mmcblk0p1 179:1=C2=A0=C2=A0=C2=A0 0=C2=A0 256M=C2=A0 0 part /boot/e= fi >>> >>> btrfs fi show >>> Label: none=C2=A0 uuid: 07892354-ddaa-4443-90ea-f76a06accaba >>> =C2=A0=C2=A0=C2=A0=C2=A0 Total devices 1 FS bytes used 1.40GiB >>> =C2=A0=C2=A0=C2=A0=C2=A0 devid=C2=A0=C2=A0=C2=A0 1 size 4.00GiB used = 3.00GiB path /dev/mmcblk0p4 >>> >>> Copy mmcblk0 to sda >>> dd if=3D/dev/mmcblk0 of=3D/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=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 MAJ:MIN RM=C2=A0 SIZE = RO TYPE MOUNTPOINT >>> sda=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 8:0=C2= =A0=C2=A0=C2=A0 1 14.9G=C2=A0 0 disk >>> |-sda4=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 8:4=C2=A0=C2=A0=C2=A0= 1=C2=A0=C2=A0=C2=A0 4G=C2=A0 0 part / >>> |-sda2=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 8:2=C2=A0=C2=A0=C2=A0= 1=C2=A0 500M=C2=A0 0 part >>> |-sda3=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 8:3=C2=A0=C2=A0=C2=A0= 1=C2=A0 256M=C2=A0 0 part >>> `-sda1=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 8:1=C2=A0=C2=A0=C2=A0= 1=C2=A0 256M=C2=A0 0 part >>> mmcblk0=C2=A0=C2=A0=C2=A0=C2=A0 179:0=C2=A0=C2=A0=C2=A0 0 29.2G=C2=A0= 0 disk >>> |-mmcblk0p4 179:4=C2=A0=C2=A0=C2=A0 0=C2=A0=C2=A0=C2=A0 4G=C2=A0 0 pa= rt >>> |-mmcblk0p2 179:2=C2=A0=C2=A0=C2=A0 0=C2=A0 500M=C2=A0 0 part /boot >>> |-mmcblk0p3 179:3=C2=A0=C2=A0=C2=A0 0=C2=A0 256M=C2=A0 0 part [SWAP] >>> `-mmcblk0p1 179:1=C2=A0=C2=A0=C2=A0 0=C2=A0 256M=C2=A0 0 part /boot/e= fi >>> btrfs fi show / >>> Label: none=C2=A0 uuid: 07892354-ddaa-4443-90ea-f76a06accaba >>> =C2=A0=C2=A0=C2=A0=C2=A0 Total devices 1 FS bytes used 1.40GiB >>> =C2=A0=C2=A0=C2=A0=C2=A0 devid=C2=A0=C2=A0=C2=A0 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 usi= ng >>> the 'btrfstune -u' command. >> >> Is there a pending fix for this? >=20 > Yes. > https://patchwork.kernel.org/patch/10641041/ >=20 > Test case header mentioned it. >=20 >> >>> >>> Signed-off-by: Anand Jain >>> --- >>> v1->v2: >>> =C2=A0=C2=A0 dont play around with dev patch use it as it is. >>> =C2=A0=C2=A0 do not use SCRATCH_MNT instead create it at the TEST_DIR= and its >>> related >>> =C2=A0=C2=A0=C2=A0 changes. >>> =C2=A0=C2=A0 golden out changes >>> =C2=A0=C2=A0=C2=A0 =C2=A0 tests/btrfs/173=C2=A0=C2=A0=C2=A0=C2=A0 | 8= 8 >>> +++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> =C2=A0 tests/btrfs/173.out |=C2=A0 6 ++++ >>> =C2=A0 tests/btrfs/group=C2=A0=C2=A0 |=C2=A0 1 + >>> =C2=A0 3 files changed, 95 insertions(+) >>> =C2=A0 create mode 100755 tests/btrfs/173 >>> =C2=A0 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. >>> +#=C2=A0 Could be fixed by >>> +#=C2=A0=C2=A0=C2=A0 [patch] btrfs: harden agaist duplicate fsid >>> +# >>> +seq=3D`basename $0` >>> +seqres=3D$RESULT_DIR/$seq >>> +echo "QA output created by $seq" >>> + >>> +here=3D`pwd` >>> +tmp=3D/tmp/$$ >>> +status=3D1=C2=A0=C2=A0=C2=A0 # failure is the default! >>> +trap "_cleanup; exit \$status" 0 1 2 3 15 >>> + >>> +mnt=3D$TEST_DIR/$seq.mnt >>> +_cleanup() >>> +{ >>> +=C2=A0=C2=A0=C2=A0 rm -rf $mnt > /dev/null 2>&1 >>> +=C2=A0=C2=A0=C2=A0 cd / >>> +=C2=A0=C2=A0=C2=A0 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=3D$(echo $SCRATCH_DEV_POOL | awk '{print $1}') >>> +dev_bar=3D$(echo $SCRATCH_DEV_POOL | awk '{print $2}') >> >> Naming devices _foo and _bar just shows you could not care less about >> the test. >=20 > =C2=A0 Hmm. Will make it device_1 and device_2. >=20 >> Either use sane names - primary_dev/secondary dev or device_1 >> and device_2. >=20 >>> + >>> +echo dev_foo=3D$dev_foo >> $seqres.full >>> +echo dev_bar=3D$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 : >> >> " looks like a bug" >=20 > =C2=A0No its not. As few lines above, I have assigned it as.. > =C2=A0=C2=A0=C2=A0 mnt=3D$TEST_DIR/$seq.mnt I missed that, I will recommend moving the assignment near where you set the devices >=20 >=20 >>> +mkdir $mnt >>> +_mkfs_dev $dev_foo >>> +_mount $dev_foo $mnt >>> + >>> +check_btrfs_mount() >>> +{ >>> +=C2=A0=C2=A0=C2=A0 local x=3D$(findmnt $mnt | grep -v TARGET | awk '= {print $2}') >>> +=C2=A0=C2=A0=C2=A0 [[ $x =3D=3D $dev_foo ]] && echo DEV_FOO >>> +=C2=A0=C2=A0=C2=A0 [[ $x =3D=3D $dev_bar ]] && echo DEV_BAR >>> +} >> >> Same thing here re. DEV_(FOO|BAR). >> >>> + >>> +echo MNT $(check_btrfs_mount) >>> + >>> +for sb_bytenr in 65536 67108864 >>> +do >>> +=C2=A0=C2=A0=C2=A0 echo -n "dd status=3Dnone if=3D$dev_foo of=3D$dev= _bar bs=3D1 "\ >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 "seek=3D$sb_bytenr skip=3D= $sb_bytenr count=3D4096" >> $seqres.full >>> +=C2=A0=C2=A0=C2=A0 dd status=3Dnone if=3D$dev_foo of=3D$dev_bar bs=3D= 1 seek=3D$sb_bytenr \ >>> +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0 skip=3D$sb_bytenr count=3D4096 >> $seqres.full 2>&1 >>> +=C2=A0=C2=A0=C2=A0 echo ..:$? >> $seqres.full >> >> This is an overkill, just use dd ... >/dev/null 2>&1, no one cares abo= ut >> the output of that. Also use DIO so that sb's are directly written to >> the devices. >=20 > =C2=A0Yeah right will fix. >=20 >>> +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=3D0 >>> +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 wou= ld >> signal success. Check for example >=20 > =C2=A0I could do that. Will fix. >=20 > Thanks, Anand >=20 >>> 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 @@ >>> =C2=A0 170 auto quick snapshot >>> =C2=A0 171 auto quick qgroup >>> =C2=A0 172 auto quick punch >>> +173 volume >>> >=20