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

[Anand Jain <anand.jain@oracle.com>]

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.
>>
>>