On Tue, Aug 30, 2022 at 01:07:43PM +0200, 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.
>
> I'll apply locally for review.
Ok, still seems good!
Reviewed-by: Christian Brauner (Microsoft) <brauner@xxxxxxxxxx>
