[PATCH v4 0/2] group ownership changing test

[PATCH v4 0/2] group ownership changing test

[Zorro Lang]

Hi Christian,

It's been 3 month past, since your patch [1] was reviewed last time. I'm still
waititng for this test coverage for fs. According to the review points last
time, I split your v3 patch [1] to 2 separated cases, one basic testing for
covering that fix, one extend testing for overlay part.

I only did below changes (compare with v3):
1) Split one case to two cases.
2) Move "_supports_filetype" line under the _scratch_mount.

I still keep the patch Author as you, and signed-off-by you, due to I just
hope to bring this discussion back, and push the proceeding of this patch
be merged.

(CC the old cc list to get more review)

Thanks,
Zorro

[1]
https://lore.kernel.org/fstests/20220615092826.2333847-1-brauner@kernel.org/

[PATCH v4 1/2] generic: basic group ownership changing test

[Zorro Lang]

From: Christian Brauner <brauner@kernel.org>

When group ownership is changed a caller whose fsuid owns the inode
can change the group of the inode to any group they are a member of.
When searching through the caller's groups we failed to use the gid
mapped according to the idmapped mount otherwise we fail to change
ownership. Add a test for this.

Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---
 tests/generic/698     | 117 ++++++++++++++++++++++++++++++++++++++++++
 tests/generic/698.out |  19 +++++++
 2 files changed, 136 insertions(+)
 create mode 100755 tests/generic/698
 create mode 100644 tests/generic/698.out

diff --git a/tests/generic/698 b/tests/generic/698
new file mode 100755
index 00000000..143490b2
--- /dev/null
+++ b/tests/generic/698
@@ -0,0 +1,117 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 Christian Brauner (Microsoft).  All Rights Reserved.
+#
+# FS QA Test 698
+#
+# Test that users can changed group ownership of a file they own to a group
+# they are a member of.
+#
+# Regression test for commit:
+# 168f91289340 ("fs: account for group membership")
+#
+. ./common/preamble
+_begin_fstest auto quick perms attr idmapped mount
+
+# Override the default cleanup function.
+_cleanup()
+{
+	cd /
+	$UMOUNT_PROG $SCRATCH_MNT/target-mnt 2>/dev/null
+	$UMOUNT_PROG $SCRATCH_MNT 2>/dev/null
+	rm -r -f $tmp.*
+}
+
+# real QA test starts here
+_supported_fs generic
+_fixed_by_kernel_commit 168f91289340 \
+	"fs: account for group membership"
+_require_scratch
+_require_chown
+_require_idmapped_mounts
+_require_test_program "vfs/mount-idmapped"
+_require_user fsgqa2
+_require_group fsgqa2
+# Do this SECOND so that qa_user is fsgqa, and _user_do uses that account
+_require_user fsgqa
+_require_group fsgqa
+
+user_foo=`id -u fsgqa`
+group_foo=`id -g fsgqa`
+user_bar=`id -u fsgqa2`
+group_bar=`id -g fsgqa2`
+
+setup_tree()
+{
+	mkdir -p $SCRATCH_MNT/source-mnt
+	chmod 0777 $SCRATCH_MNT/source-mnt
+	touch $SCRATCH_MNT/source-mnt/file1
+	chown 65534:65534 $SCRATCH_MNT
+	chown 65534:65534 $SCRATCH_MNT/source-mnt
+	chown 65534:65535 $SCRATCH_MNT/source-mnt/file1
+
+	mkdir -p $SCRATCH_MNT/target-mnt
+	chmod 0777 $SCRATCH_MNT/target-mnt
+}
+
+# Setup an idmapped mount where uid and gid 65534 are mapped to fsgqa and uid
+# and gid 65535 are mapped to fsgqa2.
+setup_idmapped_mnt()
+{
+	$here/src/vfs/mount-idmapped \
+		--map-mount=u:65534:$user_foo:1 \
+		--map-mount=g:65534:$group_foo:1 \
+		--map-mount=u:65535:$user_bar:1 \
+		--map-mount=g:65535:$group_bar:1 \
+		$SCRATCH_MNT/source-mnt $SCRATCH_MNT/target-mnt
+}
+
+# We've created a layout where fsgqa owns the target file but the group of the
+# target file is owned by another group. We now test that user fsgqa can change
+# the group ownership of the file to a group they control. In this case to the
+# fsgqa group.
+change_group_ownership()
+{
+	local path="$1"
+
+	stat -c '%U:%G' $path
+	_user_do "id -u --name; id -g --name; chgrp $group_foo $path"
+	stat -c '%U:%G' $path
+	_user_do "id -u --name; id -g --name; chgrp $group_bar $path > /dev/null 2>&1"
+	stat -c '%U:%G' $path
+}
+
+run_base_test()
+{
+	mkdir -p $SCRATCH_MNT/source-mnt
+	chmod 0777 $SCRATCH_MNT/source-mnt
+	touch $SCRATCH_MNT/source-mnt/file1
+	chown $user_foo:$group_foo $SCRATCH_MNT
+	chown $user_foo:$group_foo $SCRATCH_MNT/source-mnt
+	chown $user_foo:$group_bar $SCRATCH_MNT/source-mnt/file1
+
+	echo ""
+	echo "base test"
+	change_group_ownership "$SCRATCH_MNT/source-mnt/file1"
+	rm -rf "$SCRATCH_MNT/source-mnt"
+}
+
+# Basic test as explained in the comment for change_group_ownership().
+run_idmapped_test()
+{
+	echo ""
+	echo "base idmapped test"
+	change_group_ownership "$SCRATCH_MNT/target-mnt/file1"
+}
+
+_scratch_mkfs >> $seqres.full
+_scratch_mount
+
+run_base_test
+setup_tree
+setup_idmapped_mnt
+run_idmapped_test
+
+# success, all done
+status=0
+exit
diff --git a/tests/generic/698.out b/tests/generic/698.out
new file mode 100644
index 00000000..519234b5
--- /dev/null
+++ b/tests/generic/698.out
@@ -0,0 +1,19 @@
+QA output created by 698
+
+base test
+fsgqa:fsgqa2
+fsgqa
+fsgqa
+fsgqa:fsgqa
+fsgqa
+fsgqa
+fsgqa:fsgqa
+
+base idmapped test
+fsgqa:fsgqa2
+fsgqa
+fsgqa
+fsgqa:fsgqa
+fsgqa
+fsgqa
+fsgqa:fsgqa
-- 
2.31.1

[PATCH v4 2/2] generic: overlay group ownership changing test

[Zorro Lang]

From: Christian Brauner <brauner@kernel.org>

This's copied from generic/698, extend it to test overlayfs on top of
idmapped mounts specifically.

When group ownership is changed a caller whose fsuid owns the inode can
change the group of the inode to any group they are a member of. When
searching through the caller's groups we failed to use the gid mapped
according to the idmapped mount otherwise we fail to change ownership.
Add a test for this.

Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
---
 tests/generic/699     | 163 ++++++++++++++++++++++++++++++++++++++++++
 tests/generic/699.out |  27 +++++++
 2 files changed, 190 insertions(+)
 create mode 100755 tests/generic/699
 create mode 100644 tests/generic/699.out

diff --git a/tests/generic/699 b/tests/generic/699
new file mode 100755
index 00000000..82e83644
--- /dev/null
+++ b/tests/generic/699
@@ -0,0 +1,163 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 Christian Brauner (Microsoft).  All Rights Reserved.
+#
+# FS QA Test 698
+#
+# This's copied from generic/698, extend it to test overlayfs on top of idmapped
+# mounts specifically.
+#
+. ./common/preamble
+. ./common/overlay
+_begin_fstest auto quick perms attr idmapped mount
+
+# Override the default cleanup function.
+_cleanup()
+{
+	cd /
+	$UMOUNT_PROG $SCRATCH_MNT/target-mnt
+	$UMOUNT_PROG $SCRATCH_MNT/ovl-merge 2>/dev/null
+	$UMOUNT_PROG $SCRATCH_MNT 2>/dev/null
+	rm -r -f $tmp.*
+}
+
+# real QA test starts here
+_supported_fs ^overlay
+_require_extra_fs overlay
+_require_scratch
+_require_chown
+_require_idmapped_mounts
+_require_test_program "vfs/mount-idmapped"
+_require_user fsgqa2
+_require_group fsgqa2
+# Do this SECOND so that qa_user is fsgqa, and _user_do uses that account
+_require_user fsgqa
+_require_group fsgqa
+
+user_foo=`id -u fsgqa`
+group_foo=`id -g fsgqa`
+user_bar=`id -u fsgqa2`
+group_bar=`id -g fsgqa2`
+
+setup_tree()
+{
+	mkdir -p $SCRATCH_MNT/source-mnt
+	chmod 0777 $SCRATCH_MNT/source-mnt
+	touch $SCRATCH_MNT/source-mnt/file1
+	chown 65534:65534 $SCRATCH_MNT
+	chown 65534:65534 $SCRATCH_MNT/source-mnt
+	chown 65534:65535 $SCRATCH_MNT/source-mnt/file1
+
+	mkdir -p $SCRATCH_MNT/target-mnt
+	chmod 0777 $SCRATCH_MNT/target-mnt
+}
+
+# Setup an idmapped mount where uid and gid 65534 are mapped to fsgqa and uid
+# and gid 65535 are mapped to fsgqa2.
+setup_idmapped_mnt()
+{
+	$here/src/vfs/mount-idmapped \
+		--map-mount=u:65534:$user_foo:1 \
+		--map-mount=g:65534:$group_foo:1 \
+		--map-mount=u:65535:$user_bar:1 \
+		--map-mount=g:65535:$group_bar:1 \
+		$SCRATCH_MNT/source-mnt $SCRATCH_MNT/target-mnt
+}
+
+# We've created a layout where fsgqa owns the target file but the group of the
+# target file is owned by another group. We now test that user fsgqa can change
+# the group ownership of the file to a group they control. In this case to the
+# fsgqa group.
+change_group_ownership()
+{
+	local path="$1"
+
+	stat -c '%U:%G' $path
+	_user_do "id -u --name; id -g --name; chgrp $group_foo $path"
+	stat -c '%U:%G' $path
+	_user_do "id -u --name; id -g --name; chgrp $group_bar $path > /dev/null 2>&1"
+	stat -c '%U:%G' $path
+}
+
+lower="$SCRATCH_MNT/target-mnt"
+upper="$SCRATCH_MNT/ovl-upper"
+work="$SCRATCH_MNT/ovl-work"
+merge="$SCRATCH_MNT/ovl-merge"
+
+reset_ownership()
+{
+	local path="$SCRATCH_MNT/source-mnt/file1"
+
+	echo ""
+	echo "reset ownership"
+	chown 65534:65534 $path
+	stat -c '%u:%g' $path
+	chown 65534:65535 $path
+	stat -c '%u:%g' $path
+}
+
+# Prepare overlayfs with metacopy turned off.
+setup_overlayfs_idmapped_lower_metacopy_off()
+{
+	mkdir -p $upper $work $merge
+	_overlay_mount_dirs $lower $upper $work \
+			    overlay $merge -ometacopy=off || \
+			    _notrun "overlayfs doesn't support idmappped layers"
+}
+
+# Prepare overlayfs with metacopy turned on.
+setup_overlayfs_idmapped_lower_metacopy_on()
+{
+	mkdir -p $upper $work $merge
+	_overlay_mount_dirs $lower $upper $work overlay $merge -ometacopy=on
+}
+
+reset_overlayfs()
+{
+	$UMOUNT_PROG $SCRATCH_MNT/ovl-merge 2>/dev/null
+	rm -rf $upper $work $merge
+}
+
+# Overlayfs can be mounted on top of idmapped layers. Make sure that the basic
+# test explained in the comment for change_group_ownership() passes with
+# overlayfs mounted on top of it.
+# This tests overlayfs with metacopy turned off, i.e., changing a file copies
+# up data and metadata.
+run_overlayfs_idmapped_lower_metacopy_off()
+{
+	echo ""
+	echo "overlayfs idmapped lower metacopy off"
+	change_group_ownership "$SCRATCH_MNT/ovl-merge/file1"
+	reset_overlayfs
+	reset_ownership
+}
+
+# Overlayfs can be mounted on top of idmapped layers. Make sure that the basic
+# test explained in the comment for change_group_ownership() passes with
+# overlayfs mounted on top of it.
+# This tests overlayfs with metacopy turned on, i.e., changing a file tries to
+# only copy up metadata.
+run_overlayfs_idmapped_lower_metacopy_on()
+{
+	echo ""
+	echo "overlayfs idmapped lower metacopy on"
+	change_group_ownership "$SCRATCH_MNT/ovl-merge/file1"
+	reset_overlayfs
+	reset_ownership
+}
+
+_scratch_mkfs >> $seqres.full
+_scratch_mount
+_supports_filetype $SCRATCH_MNT || _notrun "overlayfs test requires d_type"
+
+setup_tree
+setup_idmapped_mnt
+setup_overlayfs_idmapped_lower_metacopy_off
+run_overlayfs_idmapped_lower_metacopy_off
+
+setup_overlayfs_idmapped_lower_metacopy_on
+run_overlayfs_idmapped_lower_metacopy_on
+
+# success, all done
+status=0
+exit
diff --git a/tests/generic/699.out b/tests/generic/699.out
new file mode 100644
index 00000000..e5786d8e
--- /dev/null
+++ b/tests/generic/699.out
@@ -0,0 +1,27 @@
+QA output created by 699
+
+overlayfs idmapped lower metacopy off
+fsgqa:fsgqa2
+fsgqa
+fsgqa
+fsgqa:fsgqa
+fsgqa
+fsgqa
+fsgqa:fsgqa
+
+reset ownership
+65534:65534
+65534:65535
+
+overlayfs idmapped lower metacopy on
+fsgqa:fsgqa2
+fsgqa
+fsgqa
+fsgqa:fsgqa
+fsgqa
+fsgqa
+fsgqa:fsgqa
+
+reset ownership
+65534:65534
+65534:65535
-- 
2.31.1

Re: [PATCH v4 0/2] group ownership changing test

[Christian Brauner]

On Tue, Sep 20, 2022 at 07:50:33PM +0800, Zorro Lang wrote:
> Hi Christian,
> 
> It's been 3 month past, since your patch [1] was reviewed last time. I'm still
> waititng for this test coverage for fs. According to the review points last
> time, I split your v3 patch [1] to 2 separated cases, one basic testing for
> covering that fix, one extend testing for overlay part.
> 
> I only did below changes (compare with v3):
> 1) Split one case to two cases.
> 2) Move "_supports_filetype" line under the _scratch_mount.
> 
> I still keep the patch Author as you, and signed-off-by you, due to I just
> hope to bring this discussion back, and push the proceeding of this patch
> be merged.
> 
> (CC the old cc list to get more review)
> 
> Thanks,
> Zorro
> 
> [1]
> https://lore.kernel.org/fstests/20220615092826.2333847-1-brauner@kernel.org/

Zorro,

Thank you for picking this patchset up! I just didn't have the time.
I've applied it, reviewed, and tested it:

Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>
Tested-by: Christian Brauner (Microsoft) <brauner@kernel.org>

We really need this regression test!

Christian

Re: [PATCH v4 0/2] group ownership changing test

[Zorro Lang]

On Tue, Sep 20, 2022 at 04:50:06PM +0200, Christian Brauner wrote:
> On Tue, Sep 20, 2022 at 07:50:33PM +0800, Zorro Lang wrote:
> > Hi Christian,
> > 
> > It's been 3 month past, since your patch [1] was reviewed last time. I'm still
> > waititng for this test coverage for fs. According to the review points last
> > time, I split your v3 patch [1] to 2 separated cases, one basic testing for
> > covering that fix, one extend testing for overlay part.
> > 
> > I only did below changes (compare with v3):
> > 1) Split one case to two cases.
> > 2) Move "_supports_filetype" line under the _scratch_mount.
> > 
> > I still keep the patch Author as you, and signed-off-by you, due to I just
> > hope to bring this discussion back, and push the proceeding of this patch
> > be merged.
> > 
> > (CC the old cc list to get more review)
> > 
> > Thanks,
> > Zorro
> > 
> > [1]
> > https://lore.kernel.org/fstests/20220615092826.2333847-1-brauner@kernel.org/
> 
> Zorro,
> 
> Thank you for picking this patchset up! I just didn't have the time.
> I've applied it, reviewed, and tested it:
> 
> Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> Tested-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> 
> We really need this regression test!

Np, I'd like to have this testing coverage too, so I keep it in my queueing list
long time. I'd like to add your RVB, but it's weird to be reviewed by yourself,
due to I keep the author and signed-off-by to you :) And this patchset is sent
by me now, so it might be weird too if I ack it :-D

Anyway, if there's no objection or more review points from others, I'd like to
merge this patchset (signed-off-by you, reviewed by me) this week.

Thanks,
Zorro

> 
> Christian
>