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

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

[Sun YangKai]

From: Sun YangKai <sunyangkai@fnnas.com>

Test creation, modification and deletion of POSIX ACLs for a btrfs
filesystem which has the read-only property set to true.

Re-test the same after the read-only property is set to false.

This complements btrfs/275, which covers the generic ->setxattr path.
POSIX ACLs are set and removed through the ->set_acl inode operation
(btrfs_set_acl), which is a separate code path that is not covered by
the RO check added for security xattrs in commit b51111271b03 ("btrfs:
check if root is readonly while setting security xattr").  As a result,
ACLs could still be modified on a read-only subvolume, bypassing the RO
protection that applies to every other xattr.

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.

Signed-off-by: Sun YangKai <sunyangkai@fnnas.com>
---
 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..60059d4a
--- /dev/null
+++ b/tests/btrfs/353
@@ -0,0 +1,97 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2026 YOUR NAME HERE. 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"
+
+_require_acls
+_require_btrfs_command "property"
+_require_scratch
+
+_scratch_mkfs >> $seqres.full 2>&1
+_scratch_mount
+
+FILENAME=$SCRATCH_MNT/foo
+
+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

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

[Sun YangKai]

From: Sun YangKai <sunyangkai@fnnas.com>

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.

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 <sunyangkai@fnnas.com>
---
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"
+
+_require_acls
+_require_btrfs_command "property"
+_require_scratch
+
+_scratch_mkfs >> $seqres.full 2>&1
+_scratch_mount
+
+FILENAME=$SCRATCH_MNT/foo
+
+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