| Message ID | 20220615092826.2333847-1-brauner@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] generic/692: test group ownership change | expand |
On Wed, Jun 15, 2022 at 11:28:26AM +0200, Christian Brauner wrote: > 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. > > Cc: Seth Forshee <sforshee@digitalocean.com> > Cc: Amir Goldstein <amir73il@gmail.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Aleksa Sarai <cyphar@cyphar.com> > Cc: <fstests@vger.kernel.org> > Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org> > --- > /* v2 */ > - Zorro Lang <zlang@redhat.com>: > - various minor fixes > > - Christian Brauner (Microsoft) <brauner@kernel.org>: > - Expand test to also cover overlayfs on top of idmapped mounts. > > /* v3 */ > - Amir Goldstein <amir73il@gmail.com>: > - Switch from calls to the mount binary to _overlay_mount_dirs helper. > > - Christian Brauner (Microsoft) <brauner@kernel.org>: > - Expand test to also cover non-idmapped mounts. > --- We really make this test case become more complicated than its v1 version, for covering overlayfs on top of idmapped mounts. I'm wondering if it's worth splitting this case to two cases, one similar as v1 to cover the original regression test, the other expand for overlayfs, although that will cause some duplicate testing steps use? If most of you prefer this v3 version, I'd like to get more review points. Thanks, Zorro > tests/generic/692 | 195 ++++++++++++++++++++++++++++++++++++++++++ > tests/generic/692.out | 49 +++++++++++ > 2 files changed, 244 insertions(+) > create mode 100755 tests/generic/692 > create mode 100644 tests/generic/692.out > > diff --git a/tests/generic/692 b/tests/generic/692 > new file mode 100755 > index 00000000..20579334 > --- /dev/null > +++ b/tests/generic/692 > @@ -0,0 +1,195 @@ > +#! /bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (c) 2022 Christian Brauner (Microsoft). All Rights Reserved. > +# > +# FS QA Test 692 > +# > +# 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 > +. ./common/overlay > +_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/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 > +_supports_filetype $SCRATCH_MNT || _notrun "overlayfs test requires d_type" > +_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 > + > +_scratch_mkfs >> $seqres.full > +_scratch_mount > + > +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 > +} > + > +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 > +} > + > +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" > + reset_ownership > +} > + > +lower="$SCRATCH_MNT/target-mnt" > +upper="$SCRATCH_MNT/ovl-upper" > +work="$SCRATCH_MNT/ovl-work" > +merge="$SCRATCH_MNT/ovl-merge" > + > +# 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 > +} > + > +run_base_test > + > +setup_tree > +setup_idmapped_mnt > +run_idmapped_test > + > +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/692.out b/tests/generic/692.out > new file mode 100644 > index 00000000..1cb01f8e > --- /dev/null > +++ b/tests/generic/692.out > @@ -0,0 +1,49 @@ > +QA output created by 692 > + > +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 > + > +reset ownership > +65534:65534 > +65534:65535 > + > +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 > > base-commit: 568ac9fffeb6afec03e5d6c9936617232fd7fc6d > -- > 2.34.1 >
On Sat, Jun 25, 2022 at 11:17:15PM +0800, Zorro Lang wrote: > On Wed, Jun 15, 2022 at 11:28:26AM +0200, Christian Brauner wrote: > > 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. > > > > Cc: Seth Forshee <sforshee@digitalocean.com> > > Cc: Amir Goldstein <amir73il@gmail.com> > > Cc: Christoph Hellwig <hch@lst.de> > > Cc: Aleksa Sarai <cyphar@cyphar.com> > > Cc: <fstests@vger.kernel.org> > > Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org> > > --- > > /* v2 */ > > - Zorro Lang <zlang@redhat.com>: > > - various minor fixes > > > > - Christian Brauner (Microsoft) <brauner@kernel.org>: > > - Expand test to also cover overlayfs on top of idmapped mounts. > > > > /* v3 */ > > - Amir Goldstein <amir73il@gmail.com>: > > - Switch from calls to the mount binary to _overlay_mount_dirs helper. > > > > - Christian Brauner (Microsoft) <brauner@kernel.org>: > > - Expand test to also cover non-idmapped mounts. > > --- > > We really make this test case become more complicated than its v1 version, > for covering overlayfs on top of idmapped mounts. > > I'm wondering if it's worth splitting this case to two cases, one similar > as v1 to cover the original regression test, the other expand for overlayfs, > although that will cause some duplicate testing steps use? Both approaches would work. I chose to combine them because it's the same regression just under different circumstances and because it avoids duplicated code. > > If most of you prefer this v3 version, I'd like to get more review points. If you want to wait for additional acks then sure. But I'd like to not put merging this off for too long. I prefer to merge regression tests quickly so they are run as soon as possible especially since the fix is already upstream for about 2 weeks now. Christian
On Sat, Jun 25, 2022 at 06:25:40PM +0200, Christian Brauner wrote: > On Sat, Jun 25, 2022 at 11:17:15PM +0800, Zorro Lang wrote: > > On Wed, Jun 15, 2022 at 11:28:26AM +0200, Christian Brauner wrote: > > > 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. > > > > > > Cc: Seth Forshee <sforshee@digitalocean.com> > > > Cc: Amir Goldstein <amir73il@gmail.com> > > > Cc: Christoph Hellwig <hch@lst.de> > > > Cc: Aleksa Sarai <cyphar@cyphar.com> > > > Cc: <fstests@vger.kernel.org> > > > Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org> > > > --- > > > /* v2 */ > > > - Zorro Lang <zlang@redhat.com>: > > > - various minor fixes > > > > > > - Christian Brauner (Microsoft) <brauner@kernel.org>: > > > - Expand test to also cover overlayfs on top of idmapped mounts. > > > > > > /* v3 */ > > > - Amir Goldstein <amir73il@gmail.com>: > > > - Switch from calls to the mount binary to _overlay_mount_dirs helper. > > > > > > - Christian Brauner (Microsoft) <brauner@kernel.org>: > > > - Expand test to also cover non-idmapped mounts. > > > --- > > > > We really make this test case become more complicated than its v1 version, > > for covering overlayfs on top of idmapped mounts. > > > > I'm wondering if it's worth splitting this case to two cases, one similar > > as v1 to cover the original regression test, the other expand for overlayfs, > > although that will cause some duplicate testing steps use? > > Both approaches would work. I chose to combine them because it's the > same regression just under different circumstances and because it avoids > duplicated code. > > > > > If most of you prefer this v3 version, I'd like to get more review points. > > If you want to wait for additional acks then sure. But I'd like to not > put merging this off for too long. > I prefer to merge regression tests quickly so they are run as soon as > possible especially since the fix is already upstream for about 2 weeks > now. I'd like to split this test to two cases for two reasons: 1) Reduce the complexity of this case. 2) That overlayfs test part need a separated overlayfs idmapped feature which is not necessary to be a limitation of this regression test, cause this case can't be run on older kernel. And that: _supports_filetype $SCRATCH_MNT || _notrun "overlayfs test requires d_type" line should be called after _scratch_mount, due to $SCRATCH_MNT might not be mounted before _scratch_mount. Thanks, Zorro > > Christian >
On Wed, Jun 15, 2022 at 12:28 PM Christian Brauner <brauner@kernel.org> wrote: > > 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. > > Cc: Seth Forshee <sforshee@digitalocean.com> > Cc: Amir Goldstein <amir73il@gmail.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Aleksa Sarai <cyphar@cyphar.com> > Cc: <fstests@vger.kernel.org> > Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org> > --- > /* v2 */ > - Zorro Lang <zlang@redhat.com>: > - various minor fixes > > - Christian Brauner (Microsoft) <brauner@kernel.org>: > - Expand test to also cover overlayfs on top of idmapped mounts. > > /* v3 */ > - Amir Goldstein <amir73il@gmail.com>: > - Switch from calls to the mount binary to _overlay_mount_dirs helper. > > - Christian Brauner (Microsoft) <brauner@kernel.org>: > - Expand test to also cover non-idmapped mounts. > --- > tests/generic/692 | 195 ++++++++++++++++++++++++++++++++++++++++++ > tests/generic/692.out | 49 +++++++++++ > 2 files changed, 244 insertions(+) > create mode 100755 tests/generic/692 > create mode 100644 tests/generic/692.out > > diff --git a/tests/generic/692 b/tests/generic/692 > new file mode 100755 > index 00000000..20579334 > --- /dev/null > +++ b/tests/generic/692 > @@ -0,0 +1,195 @@ > +#! /bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (c) 2022 Christian Brauner (Microsoft). All Rights Reserved. > +# > +# FS QA Test 692 > +# > +# 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 > +. ./common/overlay > +_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/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 > +_supports_filetype $SCRATCH_MNT || _notrun "overlayfs test requires d_type" This requirement is a synonym for "very ancient filesystem" _require_idmapped_mounts is a much stronger requirement you can drop this one. > +_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 > + > +_scratch_mkfs >> $seqres.full > +_scratch_mount > + > +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 > +} > + > +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 > +} > + > +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" > + reset_ownership > +} > + > +lower="$SCRATCH_MNT/target-mnt" > +upper="$SCRATCH_MNT/ovl-upper" > +work="$SCRATCH_MNT/ovl-work" > +merge="$SCRATCH_MNT/ovl-merge" > + > +# 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" I agree with Zorro that this is going to limit running the base test on LTS 5.15.y (for example). One option is to not use _notrun. You could split this to another test as the common solution in fstests. You could make your own "skip test case" logic, i,e.: setup_overlayfs_idmapped_lower_metacopy off && \ run_overlayfs_idmapped_lower_metacopy off setup_overlayfs_idmapped_lower_metacopy on && \ run_overlayfs_idmapped_lower_metacopy on This is a very standard practice in LTP and its built in support for test case arrays. The problem is with fstests, there is no way to make that test case skip visible to the test runner. Splitting to two tests is the only way to communicate the result "this test passed and that test did not run" to the user. > +} > + > +# 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 > +} Those helpers are exactly the same. on/off could be passed as parameter. > + > +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 > +} Exactly the same. on/off can be passed as parameter. Thanks, Amir.
diff --git a/tests/generic/692 b/tests/generic/692 new file mode 100755 index 00000000..20579334 --- /dev/null +++ b/tests/generic/692 @@ -0,0 +1,195 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2022 Christian Brauner (Microsoft). All Rights Reserved. +# +# FS QA Test 692 +# +# 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 +. ./common/overlay +_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/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 +_supports_filetype $SCRATCH_MNT || _notrun "overlayfs test requires d_type" +_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 + +_scratch_mkfs >> $seqres.full +_scratch_mount + +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 +} + +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 +} + +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" + reset_ownership +} + +lower="$SCRATCH_MNT/target-mnt" +upper="$SCRATCH_MNT/ovl-upper" +work="$SCRATCH_MNT/ovl-work" +merge="$SCRATCH_MNT/ovl-merge" + +# 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 +} + +run_base_test + +setup_tree +setup_idmapped_mnt +run_idmapped_test + +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/692.out b/tests/generic/692.out new file mode 100644 index 00000000..1cb01f8e --- /dev/null +++ b/tests/generic/692.out @@ -0,0 +1,49 @@ +QA output created by 692 + +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 + +reset ownership +65534:65534 +65534:65535 + +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
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. Cc: Seth Forshee <sforshee@digitalocean.com> Cc: Amir Goldstein <amir73il@gmail.com> Cc: Christoph Hellwig <hch@lst.de> Cc: Aleksa Sarai <cyphar@cyphar.com> Cc: <fstests@vger.kernel.org> Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org> --- /* v2 */ - Zorro Lang <zlang@redhat.com>: - various minor fixes - Christian Brauner (Microsoft) <brauner@kernel.org>: - Expand test to also cover overlayfs on top of idmapped mounts. /* v3 */ - Amir Goldstein <amir73il@gmail.com>: - Switch from calls to the mount binary to _overlay_mount_dirs helper. - Christian Brauner (Microsoft) <brauner@kernel.org>: - Expand test to also cover non-idmapped mounts. --- tests/generic/692 | 195 ++++++++++++++++++++++++++++++++++++++++++ tests/generic/692.out | 49 +++++++++++ 2 files changed, 244 insertions(+) create mode 100755 tests/generic/692 create mode 100644 tests/generic/692.out base-commit: 568ac9fffeb6afec03e5d6c9936617232fd7fc6d