on 2022/08/30 19:07, Christian Brauner wrote:
> On Tue, Aug 30, 2022 at 07:13:23PM +0800, Yang Xu wrote:
>> The current_umask() is stripped from the mode directly in the vfs if the
>> filesystem either doesn't support acls or the filesystem has been
>> mounted without posic acl support.
>>
>> If the filesystem does support acls then current_umask() stripping is
>> deferred to posix_acl_create(). So when the filesystem calls
>> posix_acl_create() and there are no acls set or not supported then
>> current_umask() will be stripped.
>>
>> If the parent directory has a default acl then permissions are based off
>> of that and current_umask() is ignored. Specifically, if the ACL has an
>> ACL_MASK entry, the group permissions correspond to the permissions of
>> the ACL_MASK entry. Otherwise, if the ACL has no ACL_MASK entry, the
>> group permissions correspond to the permissions of the ACL_GROUP_OBJ
>> entry.
>>
>> Here we only use setfacl to set default acl or add ACL_MASK to check
>> whether inode strip S_ISGID works correctly.
>>
>> Like umask test, this patch is also designed to test kernel patchset behaviour
>> "move S_ISGID stripping into the vfs* helper"
>>
>> Tested-by: Christian Brauner (Microsoft) <brauner@xxxxxxxxxx>
>> Reviewed-by: Christian Brauner (Microsoft) <brauner@xxxxxxxxxx>
>> Signed-off-by: Yang Xu <xuyang2018.jy@xxxxxxxxxxx>
>> ---
>> src/vfs/idmapped-mounts.c | 907 ++++++++++++++++++++++++++++++++++----
>> src/vfs/idmapped-mounts.h | 1 +
>> src/vfs/vfstest.c | 356 ++++++++++++++-
>> tests/generic/999 | 33 ++
>> tests/generic/999.out | 2 +
>> 5 files changed, 1218 insertions(+), 81 deletions(-)
>> create mode 100755 tests/generic/999
>> create mode 100644 tests/generic/999.out
>>
>> diff --git a/src/vfs/idmapped-mounts.c b/src/vfs/idmapped-mounts.c
>> index 01a5fbd4..c010dfa1 100644
>> --- a/src/vfs/idmapped-mounts.c
>> +++ b/src/vfs/idmapped-mounts.c
>> @@ -7664,7 +7664,6 @@ out:
>> return fret;
>> }
>>
>> -
>> /* The current_umask() is stripped from the mode directly in the vfs if the
>> * filesystem either doesn't support acls or the filesystem has been
>> * mounted without posic acl support.
>> @@ -8114,94 +8113,842 @@ out:
>> return fret;
>> }
>>
>> -static const struct test_struct t_idmapped_mounts[] = {
>> - { acls, true, "posix acls on regular mounts", },
>> - { create_in_userns, true, "create operations in user namespace", },
>> - { device_node_in_userns, true, "device node in user namespace", },
>> - { expected_uid_gid_idmapped_mounts, true, "expected ownership on idmapped mounts", },
>> - { fscaps_idmapped_mounts, true, "fscaps on idmapped mounts", },
>> - { fscaps_idmapped_mounts_in_userns, true, "fscaps on idmapped mounts in user namespace", },
>> - { fscaps_idmapped_mounts_in_userns_separate_userns, true, "fscaps on idmapped mounts in user namespace with different id mappings", },
>> - { fsids_mapped, true, "mapped fsids", },
>> - { fsids_unmapped, true, "unmapped fsids", },
>> - { hardlink_crossing_idmapped_mounts, true, "cross idmapped mount hardlink", },
>> - { hardlink_from_idmapped_mount, true, "hardlinks from idmapped mounts", },
>> - { hardlink_from_idmapped_mount_in_userns, true, "hardlinks from idmapped mounts in user namespace", },
>> -#ifdef HAVE_LIBURING_H
>> - { io_uring_idmapped, true, "io_uring from idmapped mounts", },
>> - { io_uring_idmapped_userns, true, "io_uring from idmapped mounts in user namespace", },
>> - { io_uring_idmapped_unmapped, true, "io_uring from idmapped mounts with unmapped ids", },
>> - { io_uring_idmapped_unmapped_userns, true, "io_uring from idmapped mounts with unmapped ids in user namespace", },
>> -#endif
>> - { protected_symlinks_idmapped_mounts, true, "following protected symlinks on idmapped mounts", },
>> - { protected_symlinks_idmapped_mounts_in_userns, true, "following protected symlinks on idmapped mounts in user namespace", },
>> - { rename_crossing_idmapped_mounts, true, "cross idmapped mount rename", },
>> - { rename_from_idmapped_mount, true, "rename from idmapped mounts", },
>> - { rename_from_idmapped_mount_in_userns, true, "rename from idmapped mounts in user namespace", },
>> - { setattr_truncate_idmapped, true, "setattr truncate on idmapped mounts", },
>> - { setattr_truncate_idmapped_in_userns, true, "setattr truncate on idmapped mounts in user namespace", },
>> - { setgid_create_idmapped, true, "create operations in directories with setgid bit set on idmapped mounts", },
>> - { setgid_create_idmapped_in_userns, true, "create operations in directories with setgid bit set on idmapped mounts in user namespace", },
>> - { setid_binaries_idmapped_mounts, true, "setid binaries on idmapped mounts", },
>> - { setid_binaries_idmapped_mounts_in_userns, true, "setid binaries on idmapped mounts in user namespace", },
>> - { setid_binaries_idmapped_mounts_in_userns_separate_userns, true, "setid binaries on idmapped mounts in user namespace with different id mappings", },
>> - { sticky_bit_unlink_idmapped_mounts, true, "sticky bit unlink operations on idmapped mounts", },
>> - { sticky_bit_unlink_idmapped_mounts_in_userns, true, "sticky bit unlink operations on idmapped mounts in user namespace", },
>> - { sticky_bit_rename_idmapped_mounts, true, "sticky bit rename operations on idmapped mounts", },
>> - { sticky_bit_rename_idmapped_mounts_in_userns, true, "sticky bit rename operations on idmapped mounts in user namespace", },
>> - { symlink_idmapped_mounts, true, "symlink from idmapped mounts", },
>> - { symlink_idmapped_mounts_in_userns, true, "symlink from idmapped mounts in user namespace", },
>> -};
>> +/*
>> + * If the parent directory has a default acl then permissions are based off
>> + * of that and current_umask() is ignored. Specifically, if the ACL has an
>> + * ACL_MASK entry, the group permissions correspond to the permissions of
>> + * the ACL_MASK entry. Otherwise, if the ACL has no ACL_MASK entry, the
>> + * group permissions correspond to the permissions of the ACL_GROUP_OBJ
>> + * entry.
>> + *
>> + * Use setfacl to check whether inode strip S_ISGID works correctly under
>> + * the above two situations when enabling idmapped.
>> + *
>> + * Test for commit
>> + * 1639a49ccdce ("fs: move S_ISGID stripping into the vfs_*() helpers").
>> + */
>> +static int setgid_create_acl_idmapped(const struct vfstest_info *info)
>> +{
>> + int fret = -1;
>> + int file1_fd = -EBADF, open_tree_fd = -EBADF;
>> + struct mount_attr attr = {
>> + .attr_set = MOUNT_ATTR_IDMAP,
>> + };
>> + pid_t pid;
>> + int tmpfile_fd = -EBADF;
>> + bool supported = false;
>> + char path[PATH_MAX];
>> + mode_t mode;
>>
>> -const struct test_suite s_idmapped_mounts = {
>> - .tests = t_idmapped_mounts,
>> - .nr_tests = ARRAY_SIZE(t_idmapped_mounts),
>> -};
>> + if (!caps_supported())
>> + return 0;
>>
>> -static const struct test_struct t_fscaps_in_ancestor_userns[] = {
>> - { fscaps_idmapped_mounts_in_userns_valid_in_ancestor_userns, true, "fscaps on idmapped mounts in user namespace writing fscap valid in ancestor userns", },
>> -};
>> + if (fchmod(info->t_dir1_fd, S_IRUSR |
>> + S_IWUSR |
>> + S_IRGRP |
>
> This is difficult to review on list and it's not your fault but for the
> future when you see patch that seems really unclean and messed-up it can
> help to pass --minimal to git format-patch.
Oh, thanks. I did't know the minimal usage before.
Best Regards
Yang Xu
>
> I'll apply locally for review.
