diff mbox series

[3/4] xfs/148: fix failure from bad shortform size assumptions

Message ID 20220516085922.1306879-4-david@fromorbit.com (mailing list archive)
State New, archived
Headers show
Series fstests: more fixes... | expand

Commit Message

Dave Chinner May 16, 2022, 8:59 a.m. UTC 
From: Dave Chinner <dchinner@redhat.com>

We replaced an attr named:

slashstr="are_bad_for_you"

with this substitution:

cp $imgfile $imgfile.old
sed -b \
        -e "s/$nullstr/too_many\x00beans/g" \
        -e "s/$slashstr/are_bad\/for_you/g" \
        -i $imgfile

We then try to retreive the attr named 'a_are_bad/for_you'. The
failure is:

    -Attribute "a_are_bad/for_you" had a 3 byte value for TEST_DIR/mount-148/testfile:
    -heh
    +attr_get: No data available
    +Could not get "a_are_bad/for_you" for TEST_DIR/mount-148/testfile

The error returned is ENODATA - the xattr does not exist. While the
name might exist in the attr leaf block:

....
nvlist[0].valuelen = 3
nvlist[0].namelen = 17
nvlist[0].name = "a_are_bad/for_you"
nvlist[0].value = "heh"
nvlist[1].valuelen = 3
....

xattrs are not looked up by name matches when in leaf or node form
like they are in short form.  They are looked up by *name hash*
matches, and if the hash is not found, then the name does not exist.
Only if the has match is found, then it goes and retrieves the xattr
pointed to by the hash and checks the name.

At this point, it should be obvious that the hash of
"a_are_bad_for_you" is different to "a_a_are_bad/for_you". Hence the
leaf lookup is always rejected at the hash match stage and never
gets to the name compare stage.

IOWs, this test can *never* pass when the xattr is in leaf/node
form, only when it is in short form.

The reason the attr fork is in leaf form is that we are using
"-m crc=0" and so the inodes are only 256 bytes in size and can only
hold ~150 bytes in the literal area. That leaves ~100 bytes maximum
for shortform attr data. The test consumes ~80 bytes of shortform
space, so it should fit and the test pass.

However:

nvlist[4].valuelen = 37
nvlist[4].namelen = 7
nvlist[4].name = "selinux"
nvlist[4].value = "unconfined_u:object_r:unlabeled_t:s0\000"

Yes, I run the fstests with selinux enabled on some of test
machines. The selinux attr pushes the attr fork way over the size
that can fit in the shortform literal area, and so it moves to leaf
form as the attrs are initially added and the test fails.

Fix this by forcing the test to use 512 byte inodes, so as to
provide around 350 bytes of usable attr fork literal area so it's
not affected by security attributes.

While there, clean up the silly conditional loop device cleanup
code.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 tests/xfs/148 | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Darrick J. Wong May 16, 2022, 3:37 p.m. UTC | #1 
On Mon, May 16, 2022 at 06:59:21PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> We replaced an attr named:
> 
> slashstr="are_bad_for_you"
> 
> with this substitution:
> 
> cp $imgfile $imgfile.old
> sed -b \
>         -e "s/$nullstr/too_many\x00beans/g" \
>         -e "s/$slashstr/are_bad\/for_you/g" \
>         -i $imgfile
> 
> We then try to retreive the attr named 'a_are_bad/for_you'. The
> failure is:
> 
>     -Attribute "a_are_bad/for_you" had a 3 byte value for TEST_DIR/mount-148/testfile:
>     -heh
>     +attr_get: No data available
>     +Could not get "a_are_bad/for_you" for TEST_DIR/mount-148/testfile
> 
> The error returned is ENODATA - the xattr does not exist. While the
> name might exist in the attr leaf block:
> 
> ....
> nvlist[0].valuelen = 3
> nvlist[0].namelen = 17
> nvlist[0].name = "a_are_bad/for_you"
> nvlist[0].value = "heh"
> nvlist[1].valuelen = 3
> ....
> 
> xattrs are not looked up by name matches when in leaf or node form
> like they are in short form.  They are looked up by *name hash*
> matches, and if the hash is not found, then the name does not exist.
> Only if the has match is found, then it goes and retrieves the xattr
> pointed to by the hash and checks the name.
> 
> At this point, it should be obvious that the hash of
> "a_are_bad_for_you" is different to "a_a_are_bad/for_you". Hence the
> leaf lookup is always rejected at the hash match stage and never
> gets to the name compare stage.
> 
> IOWs, this test can *never* pass when the xattr is in leaf/node
> form, only when it is in short form.
> 
> The reason the attr fork is in leaf form is that we are using
> "-m crc=0" and so the inodes are only 256 bytes in size and can only
> hold ~150 bytes in the literal area. That leaves ~100 bytes maximum
> for shortform attr data. The test consumes ~80 bytes of shortform
> space, so it should fit and the test pass.
> 
> However:
> 
> nvlist[4].valuelen = 37
> nvlist[4].namelen = 7
> nvlist[4].name = "selinux"
> nvlist[4].value = "unconfined_u:object_r:unlabeled_t:s0\000"
> 
> Yes, I run the fstests with selinux enabled on some of test
> machines. The selinux attr pushes the attr fork way over the size
> that can fit in the shortform literal area, and so it moves to leaf
> form as the attrs are initially added and the test fails.
> 
> Fix this by forcing the test to use 512 byte inodes, so as to
> provide around 350 bytes of usable attr fork literal area so it's
> not affected by security attributes.

I've long wondered if I should patch in a "security" module that
automatically pastes in a fake "s3linux" attribute so that I can
experience the different fs behavior that y'all see.

> While there, clean up the silly conditional loop device cleanup
> code.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Looks good,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  tests/xfs/148 | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/tests/xfs/148 b/tests/xfs/148
> index 8d50a642..5d0a0bf4 100755
> --- a/tests/xfs/148
> +++ b/tests/xfs/148
> @@ -15,7 +15,7 @@ _cleanup()
>  {
>  	cd /
>  	$UMOUNT_PROG $mntpt > /dev/null 2>&1
> -	test -n "$loopdev" && _destroy_loop_device $loopdev > /dev/null 2>&1
> +	_destroy_loop_device $loopdev > /dev/null 2>&1
>  	rm -r -f $tmp.*
>  }
>  
> @@ -41,9 +41,12 @@ test_names=("something" "$nullstr" "$slashstr" "another")
>  rm -f $imgfile $imgfile.old
>  
>  # Format image file w/o crcs so we can sed the image file
> +# We need to use 512 byte inodes to ensure the attr forks remain in short form
> +# even when security xattrs are present so we are always doing name matches on
> +# lookup and not name hash compares as leaf/node forms will do.
>  $XFS_IO_PROG -f -c 'truncate 40m' $imgfile
>  loopdev=$(_create_loop_device $imgfile)
> -MKFS_OPTIONS="-m crc=0" _mkfs_dev $loopdev >> $seqres.full
> +MKFS_OPTIONS="-m crc=0 -i size=512" _mkfs_dev $loopdev >> $seqres.full
>  
>  # Mount image file
>  mkdir -p $mntpt
> @@ -121,9 +124,6 @@ res=$?
>  test $res -eq 1 || \
>  	echo "repair failed to report corruption ($res)"
>  
> -_destroy_loop_device $loopdev
> -loopdev=
> -
>  # success, all done
>  status=0
>  exit
> -- 
> 2.35.1
>
Dave Chinner May 16, 2022, 9:40 p.m. UTC | #2 
On Mon, May 16, 2022 at 08:37:04AM -0700, Darrick J. Wong wrote:
> On Mon, May 16, 2022 at 06:59:21PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > We replaced an attr named:
> > 
> > slashstr="are_bad_for_you"
> > 
> > with this substitution:
> > 
> > cp $imgfile $imgfile.old
> > sed -b \
> >         -e "s/$nullstr/too_many\x00beans/g" \
> >         -e "s/$slashstr/are_bad\/for_you/g" \
> >         -i $imgfile
> > 
> > We then try to retreive the attr named 'a_are_bad/for_you'. The
> > failure is:
> > 
> >     -Attribute "a_are_bad/for_you" had a 3 byte value for TEST_DIR/mount-148/testfile:
> >     -heh
> >     +attr_get: No data available
> >     +Could not get "a_are_bad/for_you" for TEST_DIR/mount-148/testfile
> > 
> > The error returned is ENODATA - the xattr does not exist. While the
> > name might exist in the attr leaf block:
> > 
> > ....
> > nvlist[0].valuelen = 3
> > nvlist[0].namelen = 17
> > nvlist[0].name = "a_are_bad/for_you"
> > nvlist[0].value = "heh"
> > nvlist[1].valuelen = 3
> > ....
> > 
> > xattrs are not looked up by name matches when in leaf or node form
> > like they are in short form.  They are looked up by *name hash*
> > matches, and if the hash is not found, then the name does not exist.
> > Only if the has match is found, then it goes and retrieves the xattr
> > pointed to by the hash and checks the name.
> > 
> > At this point, it should be obvious that the hash of
> > "a_are_bad_for_you" is different to "a_a_are_bad/for_you". Hence the
> > leaf lookup is always rejected at the hash match stage and never
> > gets to the name compare stage.
> > 
> > IOWs, this test can *never* pass when the xattr is in leaf/node
> > form, only when it is in short form.
> > 
> > The reason the attr fork is in leaf form is that we are using
> > "-m crc=0" and so the inodes are only 256 bytes in size and can only
> > hold ~150 bytes in the literal area. That leaves ~100 bytes maximum
> > for shortform attr data. The test consumes ~80 bytes of shortform
> > space, so it should fit and the test pass.
> > 
> > However:
> > 
> > nvlist[4].valuelen = 37
> > nvlist[4].namelen = 7
> > nvlist[4].name = "selinux"
> > nvlist[4].value = "unconfined_u:object_r:unlabeled_t:s0\000"
> > 
> > Yes, I run the fstests with selinux enabled on some of test
> > machines. The selinux attr pushes the attr fork way over the size
> > that can fit in the shortform literal area, and so it moves to leaf
> > form as the attrs are initially added and the test fails.
> > 
> > Fix this by forcing the test to use 512 byte inodes, so as to
> > provide around 350 bytes of usable attr fork literal area so it's
> > not affected by security attributes.
> 
> I've long wondered if I should patch in a "security" module that
> automatically pastes in a fake "s3linux" attribute so that I can
> experience the different fs behavior that y'all see.

fstests basically already does that when selinux is turned on by
setting a context mount option automatically:

MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 /dev/vdb /mnt/scratch

The "-o context" options basically defines the security xattr that
is written to every file that is created on the scratch device. All
I've done on this VM is add this to the kernel CLI options:

selinux=1 security=selinux

and nothing else. Everyone should have at least on test VM set up
this way - we need to ensure that LSM paths are actually exercised
at some point during testing.

Cheers,

Dave.
diff mbox series

Patch

diff --git a/tests/xfs/148 b/tests/xfs/148
index 8d50a642..5d0a0bf4 100755
--- a/tests/xfs/148
+++ b/tests/xfs/148
@@ -15,7 +15,7 @@  _cleanup()
 {
 	cd /
 	$UMOUNT_PROG $mntpt > /dev/null 2>&1
-	test -n "$loopdev" && _destroy_loop_device $loopdev > /dev/null 2>&1
+	_destroy_loop_device $loopdev > /dev/null 2>&1
 	rm -r -f $tmp.*
 }
 
@@ -41,9 +41,12 @@  test_names=("something" "$nullstr" "$slashstr" "another")
 rm -f $imgfile $imgfile.old
 
 # Format image file w/o crcs so we can sed the image file
+# We need to use 512 byte inodes to ensure the attr forks remain in short form
+# even when security xattrs are present so we are always doing name matches on
+# lookup and not name hash compares as leaf/node forms will do.
 $XFS_IO_PROG -f -c 'truncate 40m' $imgfile
 loopdev=$(_create_loop_device $imgfile)
-MKFS_OPTIONS="-m crc=0" _mkfs_dev $loopdev >> $seqres.full
+MKFS_OPTIONS="-m crc=0 -i size=512" _mkfs_dev $loopdev >> $seqres.full
 
 # Mount image file
 mkdir -p $mntpt
@@ -121,9 +124,6 @@  res=$?
 test $res -eq 1 || \
 	echo "repair failed to report corruption ($res)"
 
-_destroy_loop_device $loopdev
-loopdev=
-
 # success, all done
 status=0
 exit