Re: [PATCH v2] btrfs: test POSIX ACL changes for RO btrfs property

This is my first time to contribute to fstests project so thanks a lot 
for pointing those things out and it really helps a lot. I'll fix them 
and send a v2 version later.

Thanks,
Sun YangKai

On 2026/7/21 00:16, Filipe Manana wrote:
> On Mon, Jul 13, 2026 at 10:50 AM Sun YangKai <[email protected]> wrote:
>>
>> From: Sun YangKai <[email protected]>
>>
>> Test creation, modification and deletion of POSIX ACLs on a btrfs
>> filesystem that has the read-only property set to true, and re-test the
>> same after the property is cleared.
>>
>> Commit 353c2ea735e4 ("btrfs: remove redundant readonly root check in
>> btrfs_setxattr_trans") removed the read-only root check from
>> btrfs_setxattr_trans() under the assumption that none of its callers can
>> run on a read-only root.  That assumption does not hold for the ->set_acl
>> path: btrfs_set_acl() -> __btrfs_set_acl() reaches
>> btrfs_setxattr_trans() directly, bypassing the handler-level read-only
>> checks that were later (re)added by commit b51111271b03 ("btrfs: check
>> if root is readonly while setting security xattr").  As a result, ACLs
>> could still be set or removed on a read-only subvolume, bypassing the RO
>> protection that applies to every other xattr.
> 
> You don't need to explain this in a test case, those are details for
> the kernel patch...
> 
> Just say what the test does and that it exercises a bug fixed by a
> kernel patch that has the subject:
> 
>       "btrfs: check if root is readonly when setting posix acl"

>>
>> The restoring kernel fix adds the read-only check directly to
>> btrfs_set_acl().  This test complements btrfs/275, which covers the
>> generic ->setxattr path.
>>
>> The test exercises the set, get and remove cycle of an access ACL on a
>> regular file, and expects all modifications to fail with EROFS while the
>> subvolume is read-only, then succeed once it is writable again.
>>
>> Signed-off-by: Sun YangKai <[email protected]>
>> ---
>> Changes from v1:
>>
>> - fix copywrite header
>> - better wording for commit message
>>
>> Sorry for bothering.
>>
>> ---
>>   tests/btrfs/353     | 97 +++++++++++++++++++++++++++++++++++++++++++++
>>   tests/btrfs/353.out | 39 ++++++++++++++++++
>>   2 files changed, 136 insertions(+)
>>   create mode 100755 tests/btrfs/353
>>   create mode 100644 tests/btrfs/353.out
>>
>> diff --git a/tests/btrfs/353 b/tests/btrfs/353
>> new file mode 100755
>> index 00000000..586f722a
>> --- /dev/null
>> +++ b/tests/btrfs/353
>> @@ -0,0 +1,97 @@
>> +#! /bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (C) 2026 Fygo OS. All Rights Reserved.
>> +#
>> +# FS QA Test No. 353
>> +#
>> +# Test that POSIX ACLs cannot be changed once a btrfs subvolume has the
>> +# read-only property set.
>> +#
>> +# Setting or removing a POSIX ACL goes through the ->set_acl inode
>> +# operation, which is a different code path from the generic ->setxattr
>> +# one covered by btrfs/275.  It used to be allowed on a read-only
>> +# subvolume and thus bypassed the RO protection.  Such modifications must
>> +# fail with EROFS, just like any other xattr.
>> +#
>> +. ./common/preamble
>> +_begin_fstest auto quick acl attr
>> +
>> +. ./common/filter
>> +. ./common/attr
>> +
>> +# TODO: fill in the kernel commit hash that fixes the ->set_acl path once
>> +# it is merged.
>> +# _fixed_by_kernel_commit <HASH> \
>> +#      "btrfs: check if root is readonly when setting posix acl"
> 
> That's not how we handle patches not yet in Linus' tree.
> First don't comment it, second use xxxxxxxxxxxx for the hash.
> Like this:
> 
> _fixed_by_kernel_commit xxxxxxxxxxxx \
>       "btrfs: check if root is readonly when setting posix acl"
> 
> We normally find and update tests that need the hash by greping with
> something like:  egrep -ri 'commit xxxxx' tests/
> 
> 
> 
>> +
>> +_require_acls
>> +_require_btrfs_command "property"
>> +_require_scratch
>> +
>> +_scratch_mkfs >> $seqres.full 2>&1
> 
> Nowadays we have the practice of also adding:   || _fail "mkfs failed"
> 
>> +_scratch_mount
>> +
>> +FILENAME=$SCRATCH_MNT/foo
> 
> Don't use upper case variable names.
> 
> With that fixed:
> 
> Reviewed-by: Filipe Manana <[email protected]>
> 
> Thanks.
> 
> 
>> +
>> +set_acl()
>> +{
>> +       local perm=$1
>> +
>> +       # Use -n so setfacl does not recalculate the mask, keeping the
>> +       # golden output deterministic regardless of the named user's
>> +       # permissions.
>> +       setfacl -n -m u:$acl2:$perm,m::rwx $FILENAME 2>&1 | _filter_scratch
>> +}
>> +
>> +get_acl()
>> +{
>> +       getfacl --absolute-names -n $FILENAME | _filter_scratch | _getfacl_filter_id
>> +}
>> +
>> +del_acl()
>> +{
>> +       setfacl -b $FILENAME 2>&1 | _filter_scratch
>> +}
>> +
>> +_acl_setup_ids
>> +
>> +# Create a test file.
>> +echo "hello world" > $FILENAME
>> +
>> +# Set an initial ACL while the subvolume is writable.
>> +set_acl rwx
>> +
>> +# Attempt to change the ACL once the subvolume is read-only.  This must
>> +# fail with EROFS.
>> +$BTRFS_UTIL_PROG property set $SCRATCH_MNT ro true
>> +$BTRFS_UTIL_PROG property get $SCRATCH_MNT ro
>> +
>> +set_acl r--
>> +
>> +# The ACL must not have changed.
>> +get_acl
>> +
>> +# Attempt to remove the ACL from the read-only subvolume.  This must
>> +# fail with EROFS as well.
>> +del_acl
>> +
>> +# The ACL must still be present.
>> +get_acl
>> +
>> +# Make the subvolume writable again.
>> +$BTRFS_UTIL_PROG property set $SCRATCH_MNT ro false
>> +$BTRFS_UTIL_PROG property get $SCRATCH_MNT ro
>> +
>> +# Now changing the ACL must succeed.
>> +set_acl r--
>> +
>> +get_acl
>> +
>> +# And removing it must succeed too.
>> +del_acl
>> +
>> +# Check the ACL is really gone.
>> +get_acl
>> +
>> +status=0
>> +exit
>> diff --git a/tests/btrfs/353.out b/tests/btrfs/353.out
>> new file mode 100644
>> index 00000000..66f4a0e5
>> --- /dev/null
>> +++ b/tests/btrfs/353.out
>> @@ -0,0 +1,39 @@
>> +QA output created by 353
>> +ro=true
>> +setfacl: SCRATCH_MNT/foo: Read-only file system
>> +# file: SCRATCH_MNT/foo
>> +# owner: 0
>> +# group: 0
>> +user::rw-
>> +user:id2:rwx
>> +group::r--
>> +mask::rwx
>> +other::r--
>> +
>> +setfacl: SCRATCH_MNT/foo: Read-only file system
>> +# file: SCRATCH_MNT/foo
>> +# owner: 0
>> +# group: 0
>> +user::rw-
>> +user:id2:rwx
>> +group::r--
>> +mask::rwx
>> +other::r--
>> +
>> +ro=false
>> +# file: SCRATCH_MNT/foo
>> +# owner: 0
>> +# group: 0
>> +user::rw-
>> +user:id2:r--
>> +group::r--
>> +mask::rwx
>> +other::r--
>> +
>> +# file: SCRATCH_MNT/foo
>> +# owner: 0
>> +# group: 0
>> +user::rw-
>> +group::r--
>> +other::r--
>> +
>> --
>> 2.54.0
>>
>>