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

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

[Goldwyn Rodrigues]

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.

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
+
+# Import common functions.
+#. ./common/filter
+. ./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
+
+# 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
+# file: scratch/foo
+user.one="1"
+
+getfattr: Removing leading '/' from absolute path names
+# file: scratch/foo
+security.one="1"
+
+getfattr: Removing leading '/' from absolute path names
+# file: scratch/foo
+trusted.one="1"
+
+setfattr: /scratch/foo: Read-only file system
+setfattr: /scratch/foo: Read-only file system
+setfattr: /scratch/foo: Read-only file system
+ro=false
+getfattr: Removing leading '/' from absolute path names
+# file: scratch/foo
+user.one="2"
+
+getfattr: Removing leading '/' from absolute path names
+# file: scratch/foo
+security.one="2"
+
+getfattr: Removing leading '/' from absolute path names
+# file: scratch/foo
+trusted.one="2"
+

-- 
Goldwyn

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

[Filipe Manana]

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.

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

> +user.one="1"
> +
> +getfattr: Removing leading '/' from absolute path names
> +# file: scratch/foo
> +security.one="1"
> +
> +getfattr: Removing leading '/' from absolute path names
> +# file: scratch/foo
> +trusted.one="1"
> +
> +setfattr: /scratch/foo: Read-only file system

Same here. The test fails if SCRATCH_MNT is not "/scratch".
We need to append "2>&1 | _filter_scratch" to every setfattr call in the test.

And then have the following in the golden output instead:

setfattr: SCRATCH_MNT/foo: Read-only file system

Here's a diff that fixes the golden output and adds the other things
mentioned before:

diff --git a/tests/btrfs/273 b/tests/btrfs/273
index ec7d264d..37e9e52c 100755
--- a/tests/btrfs/273
+++ b/tests/btrfs/273
@@ -7,14 +7,15 @@
 # Test that no xattr can be changed once btrfs property is set to RO
 #
 . ./common/preamble
-_begin_fstest auto quick attr
+_begin_fstest auto quick attr subvol
 
 # Import common functions.
-#. ./common/filter
+. ./common/filter
 . ./common/attr
 
 # real QA test starts here
 _supported_fs btrfs
+_fixed_by_kernel_commit XXXXXXXXXXXX "btrfs: check if root is readonly while setting security xattr"
 _require_attrs
 _require_btrfs_command "property"
 _require_scratch
@@ -23,24 +24,23 @@ _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
+	$SETFATTR_PROG -n "user.one" -v $value $FILENAME 2>&1 | _filter_scratch
+	$SETFATTR_PROG -n "security.one" -v $value $FILENAME 2>&1 | _filter_scratch
+	$SETFATTR_PROG -n "trusted.one" -v $value $FILENAME 2>&1 | _filter_scratch
 }
 
 get_xattr() {
-	$GETFATTR_PROG -n "user.one" $FILENAME
-	$GETFATTR_PROG -n "security.one" $FILENAME
-	$GETFATTR_PROG -n "trusted.one" $FILENAME
+	$GETFATTR_PROG -n "user.one" --absolute-names $FILENAME | _filter_scratch
+	$GETFATTR_PROG -n "security.one" --absolute-names $FILENAME | _filter_scratch
+	$GETFATTR_PROG -n "trusted.one" --absolute-names $FILENAME | _filter_scratch
 }
 
 del_xattr() {
-	$SETFATTR_PROG -x "user.one" $FILENAME
-	$SETFATTR_PROG -x "security.one" $FILENAME
-	$SETFATTR_PROG -x "trusted.one" $FILENAME
+	$SETFATTR_PROG -x "user.one" $FILENAME 2>&1 | _filter_scratch
+	$SETFATTR_PROG -x "security.one" $FILENAME 2>&1 | _filter_scratch
+	$SETFATTR_PROG -x "trusted.one" $FILENAME 2>&1 | _filter_scratch
 }
 
 # Create a test file.
@@ -54,12 +54,15 @@ $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
+# Check the values of RO (property) filesystem are not changed
 get_xattr
 
 # Attempt to remove xattr from RO (property) filesystem
 del_xattr
 
+# Check the values of RO (property) filesystem are not changed
+get_xattr
+
 # Change filesystem property RO to false
 
 $BTRFS_UTIL_PROG property set $SCRATCH_MNT ro false
diff --git a/tests/btrfs/273.out b/tests/btrfs/273.out
index f6fca029..ae85b0a5 100644
--- a/tests/btrfs/273.out
+++ b/tests/btrfs/273.out
@@ -1,33 +1,36 @@
 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
-# file: scratch/foo
+setfattr: SCRATCH_MNT/foo: Read-only file system
+setfattr: SCRATCH_MNT/foo: Read-only file system
+setfattr: SCRATCH_MNT/foo: Read-only file system
+# file: SCRATCH_MNT/foo
 user.one="1"
 
-getfattr: Removing leading '/' from absolute path names
-# file: scratch/foo
+# file: SCRATCH_MNT/foo
 security.one="1"
 
-getfattr: Removing leading '/' from absolute path names
-# file: scratch/foo
+# file: SCRATCH_MNT/foo
+trusted.one="1"
+
+setfattr: SCRATCH_MNT/foo: Read-only file system
+setfattr: SCRATCH_MNT/foo: Read-only file system
+setfattr: SCRATCH_MNT/foo: Read-only file system
+# file: SCRATCH_MNT/foo
+user.one="1"
+
+# file: SCRATCH_MNT/foo
+security.one="1"
+
+# file: SCRATCH_MNT/foo
 trusted.one="1"
 
-setfattr: /scratch/foo: Read-only file system
-setfattr: /scratch/foo: Read-only file system
-setfattr: /scratch/foo: Read-only file system
 ro=false
-getfattr: Removing leading '/' from absolute path names
-# file: scratch/foo
+# file: SCRATCH_MNT/foo
 user.one="2"
 
-getfattr: Removing leading '/' from absolute path names
-# file: scratch/foo
+# file: SCRATCH_MNT/foo
 security.one="2"
 
-getfattr: Removing leading '/' from absolute path names
-# file: scratch/foo
+# file: SCRATCH_MNT/foo
 trusted.one="2"
 

Thanks.


> +setfattr: /scratch/foo: Read-only file system
> +setfattr: /scratch/foo: Read-only file system
> +ro=false
> +getfattr: Removing leading '/' from absolute path names
> +# file: scratch/foo
> +user.one="2"
> +
> +getfattr: Removing leading '/' from absolute path names
> +# file: scratch/foo
> +security.one="2"
> +
> +getfattr: Removing leading '/' from absolute path names
> +# file: scratch/foo
> +trusted.one="2"
> +
> 
> -- 
> Goldwyn

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

[Goldwyn Rodrigues]

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

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

[Anand Jain]

On 8/17/22 05:40, 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.
> 
> 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
> +
> +# Import common functions.
> +#. ./common/filter
> +. ./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
> +}
> +

This output contains mnt references that need to be filtered.
Filter _filter_scratch should help.



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

  Nit:  We are reusing the value "2" and changing it to "3"  makes it
  unique and so the debugging easier.


> +# 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
> +# file: scratch/foo
> +user.one="1"
> +
> +getfattr: Removing leading '/' from absolute path names
> +# file: scratch/foo
> +security.one="1"
> +
> +getfattr: Removing leading '/' from absolute path names
> +# file: scratch/foo
> +trusted.one="1"
> +
> +setfattr: /scratch/foo: Read-only file system
> +setfattr: /scratch/foo: Read-only file system
> +setfattr: /scratch/foo: Read-only file system
> +ro=false
> +getfattr: Removing leading '/' from absolute path names
> +# file: scratch/foo
> +user.one="2"
> +
> +getfattr: Removing leading '/' from absolute path names
> +# file: scratch/foo
> +security.one="2"
> +
> +getfattr: Removing leading '/' from absolute path names
> +# file: scratch/foo
> +trusted.one="2"


> +

Nit: A whitespace.

Thanks.

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

[Filipe Manana]

On Thu, Aug 18, 2022 at 4:08 AM Anand Jain <anand.jain@oracle.com> wrote:
>
>
>
>
> On 8/17/22 05:40, 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.
> >
> > 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
> > +
> > +# Import common functions.
> > +#. ./common/filter
> > +. ./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
> > +}
> > +
>
> This output contains mnt references that need to be filtered.
> Filter _filter_scratch should help.

That was already pointed out in my previous review, wasn't it?

>
>
>
> > +# 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
> > +
> > +# 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
> > +
>
>   Nit:  We are reusing the value "2" and changing it to "3"  makes it
>   unique and so the debugging easier.

That's a good idea.

>
>
> > +# 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
> > +# file: scratch/foo
> > +user.one="1"
> > +
> > +getfattr: Removing leading '/' from absolute path names
> > +# file: scratch/foo
> > +security.one="1"
> > +
> > +getfattr: Removing leading '/' from absolute path names
> > +# file: scratch/foo
> > +trusted.one="1"
> > +
> > +setfattr: /scratch/foo: Read-only file system
> > +setfattr: /scratch/foo: Read-only file system
> > +setfattr: /scratch/foo: Read-only file system
> > +ro=false
> > +getfattr: Removing leading '/' from absolute path names
> > +# file: scratch/foo
> > +user.one="2"
> > +
> > +getfattr: Removing leading '/' from absolute path names
> > +# file: scratch/foo
> > +security.one="2"
> > +
> > +getfattr: Removing leading '/' from absolute path names
> > +# file: scratch/foo
> > +trusted.one="2"
>
>
> > +
>
> Nit: A whitespace.

It's needed, getfattr prints a blank line at the end.

>
> Thanks.
>
>

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

[Anand Jain]

On 8/18/22 16:09, Filipe Manana wrote:
> On Thu, Aug 18, 2022 at 4:08 AM Anand Jain <anand.jain@oracle.com> wrote:
>>
>>
>>
>>
>> On 8/17/22 05:40, 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.
>>>
>>> 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
>>> +
>>> +# Import common functions.
>>> +#. ./common/filter
>>> +. ./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
>>> +}
>>> +
>>
>> This output contains mnt references that need to be filtered.
>> Filter _filter_scratch should help.
> 
> That was already pointed out in my previous review, wasn't it?

  Oh right. I missed it.

> 
>>
>>
>>
>>> +# 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
>>> +
>>> +# 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
>>> +
>>
>>    Nit:  We are reusing the value "2" and changing it to "3"  makes it
>>    unique and so the debugging easier.
> 
> That's a good idea. >
>>
>>
>>> +# 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
>>> +# file: scratch/foo
>>> +user.one="1"
>>> +
>>> +getfattr: Removing leading '/' from absolute path names
>>> +# file: scratch/foo
>>> +security.one="1"
>>> +
>>> +getfattr: Removing leading '/' from absolute path names
>>> +# file: scratch/foo
>>> +trusted.one="1"
>>> +
>>> +setfattr: /scratch/foo: Read-only file system
>>> +setfattr: /scratch/foo: Read-only file system
>>> +setfattr: /scratch/foo: Read-only file system
>>> +ro=false
>>> +getfattr: Removing leading '/' from absolute path names
>>> +# file: scratch/foo
>>> +user.one="2"
>>> +
>>> +getfattr: Removing leading '/' from absolute path names
>>> +# file: scratch/foo
>>> +security.one="2"
>>> +
>>> +getfattr: Removing leading '/' from absolute path names
>>> +# file: scratch/foo
>>> +trusted.one="2"
>>
>>
>>> +
>>
>> Nit: A whitespace.
> 
> It's needed, getfattr prints a blank line at the end.
> 

Ok.

Thanks.

>>
>> Thanks.
>>
>>