On Wed, Jun 15, 2022 at 12:28 PM Christian Brauner <brauner@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
> Cc: Amir Goldstein <amir73il@xxxxxxxxx>
> Cc: Christoph Hellwig <hch@xxxxxx>
> Cc: Aleksa Sarai <cyphar@xxxxxxxxxx>
> Cc: <fstests@xxxxxxxxxxxxxxx>
> Signed-off-by: Christian Brauner (Microsoft) <brauner@xxxxxxxxxx>
> ---
> /* v2 */
> - Zorro Lang <zlang@xxxxxxxxxx>:
> - various minor fixes
>
> - Christian Brauner (Microsoft) <brauner@xxxxxxxxxx>:
> - Expand test to also cover overlayfs on top of idmapped mounts.
>
> /* v3 */
> - Amir Goldstein <amir73il@xxxxxxxxx>:
> - Switch from calls to the mount binary to _overlay_mount_dirs helper.
>
> - Christian Brauner (Microsoft) <brauner@xxxxxxxxxx>:
> - 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.
