Re: [PATCH] btrfs: Test xattr changes for read-only btrfs property

[Goldwyn Rodrigues <rgoldwyn@suse.de>]

On 11:51 17/08, Filipe Manana wrote:
> On Tue, Aug 16, 2022 at 04:40:51PM -0500, Goldwyn Rodrigues wrote:
> > Test creation, modification and deletion of xattr for a BTRFS filesystem
> > which has the read-only property set to true.
> > 
> > Re-test the same after BTRFS read-only property is set to false.
> > 
> > This tests the bug for "security.*" modifications which escape
> > xattr_permission(), because security parameters are let through
> > in xattr_permission(), without checks from
> > inode_permission()->btrfs_permission(). There is no restriction on
> > security.* from VFS and decision is left to the underlying filesystem.
> 
> When a test currently fails and we have a patch that fixes it, we mention
> it in the changelog and/or the test itself.
> 
> Nowadays (and I learned this recently as it's new), we have an annotation
> to add to the test itself, like this:
> 
> _fixed_by_kernel_commit XXXXXXXXXXXX "btrfs: check if root is readonly while setting security xattr"
> 
> (Notice that I changed "Check" to "check", because that's the convention
> for btrfs patches, and David would fix that when picking the patch anyway)
> 
> In this case since the patch is not yet in Linus' tree, we can leave
> the XXXXXXXXXXXX marker and later update it once it lands in his tree.
> 
> > 
> > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > 
> > diff --git a/tests/btrfs/273 b/tests/btrfs/273
> > new file mode 100755
> > index 00000000..ec7d264d
> > --- /dev/null
> > +++ b/tests/btrfs/273
> > @@ -0,0 +1,78 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (C) 2022 SUSE Linux Products GmbH. All Rights Reserved.
> > +#
> > +# FS QA Test No. 273
> > +#
> > +# Test that no xattr can be changed once btrfs property is set to RO
> > +#
> > +. ./common/preamble
> > +_begin_fstest auto quick attr
> 
> subvol group too.
> 
> > +
> > +# Import common functions.
> > +#. ./common/filter
> 
> Ratther than comment, remove it, but we'll need a filter, see below.
> 
> > +. ./common/attr
> > +
> > +# real QA test starts here
> > +_supported_fs btrfs
> > +_require_attrs
> > +_require_btrfs_command "property"
> > +_require_scratch
> > +
> > +_scratch_mkfs > /dev/null 2>&1 || _fail "mkfs failed"
> > +_scratch_mount
> > +
> > +FILENAME=$SCRATCH_MNT/foo
> > +
> > +set_xattr() {
> > +	local value=$1
> > +	$SETFATTR_PROG -n "user.one" -v $value $FILENAME
> > +	$SETFATTR_PROG -n "security.one" -v $value $FILENAME
> > +	$SETFATTR_PROG -n "trusted.one" -v $value $FILENAME
> > +}
> > +
> > +get_xattr() {
> > +	$GETFATTR_PROG -n "user.one" $FILENAME
> > +	$GETFATTR_PROG -n "security.one" $FILENAME
> > +	$GETFATTR_PROG -n "trusted.one" $FILENAME
> > +}
> > +
> > +del_xattr() {
> > +	$SETFATTR_PROG -x "user.one" $FILENAME
> > +	$SETFATTR_PROG -x "security.one" $FILENAME
> > +	$SETFATTR_PROG -x "trusted.one" $FILENAME
> > +}
> > +
> > +# Create a test file.
> > +echo "hello world" > $FILENAME
> > +
> > +set_xattr 1
> > +
> > +$BTRFS_UTIL_PROG property set $SCRATCH_MNT ro true
> > +$BTRFS_UTIL_PROG property get $SCRATCH_MNT ro
> > +
> > +# Attempt to change values of RO (property) filesystem
> > +set_xattr 2
> > +
> > +# Check the values of RO (property) filesystem is not changed
> > +get_xattr
> > +
> > +# Attempt to remove xattr from RO (property) filesystem
> > +del_xattr
> 
> Here we should call get_xattr() again to verify the xattrs were
> not deleted, just like we did before.
> 

This should be called after they are truly deleted once the filesystem
is RW. I will add that.

> > +
> > +# Change filesystem property RO to false
> > +
> > +$BTRFS_UTIL_PROG property set $SCRATCH_MNT ro false
> > +$BTRFS_UTIL_PROG property get $SCRATCH_MNT ro
> > +
> > +# Change the xattrs after RO is false
> > +set_xattr 2
> > +
> > +# Get the changed values
> > +get_xattr
> > +
> > +# Remove xattr
> > +del_xattr
> > +
> > +status=0
> > +exit
> > diff --git a/tests/btrfs/273.out b/tests/btrfs/273.out
> > new file mode 100644
> > index 00000000..f6fca029
> > --- /dev/null
> > +++ b/tests/btrfs/273.out
> > @@ -0,0 +1,33 @@
> > +QA output created by 273
> > +ro=true
> > +setfattr: /scratch/foo: Read-only file system
> > +setfattr: /scratch/foo: Read-only file system
> > +setfattr: /scratch/foo: Read-only file system
> > +getfattr: Removing leading '/' from absolute path names
> 
> We can get rid of this message by passing --absolute-names to getfattr.
> We do that in every generic test case I can find.
> 
> > +# file: scratch/foo
> 
> And --absolute-names is also used in order to be able to filter the
> golden output.
> 
> The test fails on any config where SCRATCH_MNT is not "/scratch",
> like in my environment:
> 
>    -# file: scratch/foo
>    +# file: home/fdmanana/btrfs-tests/scratch_1/foo
> 
> By passing --absolute-names we can then use _filter_scratch and have
> this instead in the golden output:
> 
> # file: SCRATCH_MNT/foo

Yes, missed that.


-- 
Goldwyn