* [PATCH v4 0/2] group ownership changing test
@ 2022-09-20 11:50 Zorro Lang
2022-09-20 11:50 ` [PATCH v4 1/2] generic: basic " Zorro Lang
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Zorro Lang @ 2022-09-20 11:50 UTC (permalink / raw)
To: brauner, fstests; +Cc: sforshee, amir73il, hch, cyphar
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/
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH v4 1/2] generic: basic group ownership changing test 2022-09-20 11:50 [PATCH v4 0/2] group ownership changing test Zorro Lang @ 2022-09-20 11:50 ` Zorro Lang 2022-09-20 11:50 ` [PATCH v4 2/2] generic: overlay " Zorro Lang 2022-09-20 14:50 ` [PATCH v4 0/2] " Christian Brauner 2 siblings, 0 replies; 5+ messages in thread From: Zorro Lang @ 2022-09-20 11:50 UTC (permalink / raw) To: brauner, fstests; +Cc: sforshee, amir73il, hch, cyphar 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v4 2/2] generic: overlay group ownership changing test 2022-09-20 11:50 [PATCH v4 0/2] group ownership changing test Zorro Lang 2022-09-20 11:50 ` [PATCH v4 1/2] generic: basic " Zorro Lang @ 2022-09-20 11:50 ` Zorro Lang 2022-09-20 14:50 ` [PATCH v4 0/2] " Christian Brauner 2 siblings, 0 replies; 5+ messages in thread From: Zorro Lang @ 2022-09-20 11:50 UTC (permalink / raw) To: brauner, fstests; +Cc: sforshee, amir73il, hch, cyphar 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v4 0/2] group ownership changing test 2022-09-20 11:50 [PATCH v4 0/2] group ownership changing test Zorro Lang 2022-09-20 11:50 ` [PATCH v4 1/2] generic: basic " Zorro Lang 2022-09-20 11:50 ` [PATCH v4 2/2] generic: overlay " Zorro Lang @ 2022-09-20 14:50 ` Christian Brauner 2022-09-20 15:11 ` Zorro Lang 2 siblings, 1 reply; 5+ messages in thread From: Christian Brauner @ 2022-09-20 14:50 UTC (permalink / raw) To: Zorro Lang; +Cc: fstests, sforshee, amir73il, hch, cyphar 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4 0/2] group ownership changing test 2022-09-20 14:50 ` [PATCH v4 0/2] " Christian Brauner @ 2022-09-20 15:11 ` Zorro Lang 0 siblings, 0 replies; 5+ messages in thread From: Zorro Lang @ 2022-09-20 15:11 UTC (permalink / raw) To: Christian Brauner; +Cc: Zorro Lang, fstests, sforshee, amir73il, hch, cyphar 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-09-20 15:11 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-09-20 11:50 [PATCH v4 0/2] group ownership changing test Zorro Lang 2022-09-20 11:50 ` [PATCH v4 1/2] generic: basic " Zorro Lang 2022-09-20 11:50 ` [PATCH v4 2/2] generic: overlay " Zorro Lang 2022-09-20 14:50 ` [PATCH v4 0/2] " Christian Brauner 2022-09-20 15:11 ` Zorro Lang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox