On Thu, Aug 18, 2022 at 4:08 AM Anand Jain <anand.jain@xxxxxxxxxx> 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@xxxxxxxx>
> >
> > 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.
>
>
