on 2022/08/29 21:20, Christian Brauner wrote:
> On Tue, Jul 26, 2022 at 05:31:21PM +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"
>>
>> Signed-off-by: Yang Xu <xuyang2018.jy@xxxxxxxxxxx>
>> ---
>
> Hey Yang,
>
> Thanks for your patches! Just a minor comment below.
>
>> 1.add kernel commit id
>> 2.move umask into parent process intead of child process
>> 3.remove duplicated comment for ACL_GROUP_OBJ and ACL_MASK because
>> we have mentioned it before the function
>> src/vfs/idmapped-mounts.c | 704 ++++++++++++++++++++++++++++++++++++++
>> src/vfs/idmapped-mounts.h | 1 +
>> src/vfs/vfstest.c | 345 ++++++++++++++++++-
>> tests/generic/999 | 33 ++
>> tests/generic/999.out | 2 +
>> 5 files changed, 1084 insertions(+), 1 deletion(-)
>> 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 f63ac44b..f934cf4a 100644
>> --- a/src/vfs/idmapped-mounts.c
>> +++ b/src/vfs/idmapped-mounts.c
>> @@ -8083,6 +8083,700 @@ out:
>> return fret;
>> }
>>
>> +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;
>> +
>> + if (!caps_supported())
>> + return 0;
>> +
>> + if (fchmod(info->t_dir1_fd, S_IRUSR |
>> + S_IWUSR |
>> + S_IRGRP |
>> + S_IWGRP |
>> + S_IROTH |
>> + S_IWOTH |
>> + S_IXUSR |
>> + S_IXGRP |
>> + S_IXOTH |
>> + S_ISGID), 0) {
>> + log_stderr("failure: fchmod");
>> + goto out;
>> + }
>> +
>> + /* Verify that the sid bits got raised. */
>> + if (!is_setgid(info->t_dir1_fd, "", AT_EMPTY_PATH)) {
>> + log_stderr("failure: is_setgid");
>> + goto out;
>> + }
>> +
>> + /* Changing mount properties on a detached mount. */
>> + attr.userns_fd = get_userns_fd(0, 10000, 10000);
>> + if (attr.userns_fd < 0) {
>> + log_stderr("failure: get_userns_fd");
>> + goto out;
>> + }
>> +
>> + open_tree_fd = sys_open_tree(info->t_dir1_fd, "",
>> + AT_EMPTY_PATH |
>> + AT_NO_AUTOMOUNT |
>> + AT_SYMLINK_NOFOLLOW |
>> + OPEN_TREE_CLOEXEC |
>> + OPEN_TREE_CLONE);
>> + if (open_tree_fd < 0) {
>> + log_stderr("failure: sys_open_tree");
>> + goto out;
>> + }
>> +
>> + if (sys_mount_setattr(open_tree_fd, "", AT_EMPTY_PATH, &attr, sizeof(attr))) {
>> + log_stderr("failure: sys_mount_setattr");
>> + goto out;
>> + }
>> +
>> + supported = openat_tmpfile_supported(open_tree_fd);
>> +
>> + /* Only umask with S_IXGRP because inode strip S_ISGID will check mode
>> + * whether has group execute or search permission.
>> + */
>> + umask(S_IXGRP);
>> + mode = umask(S_IXGRP);
>> + if (!(mode & S_IXGRP))
>> + die("failure: umask");
>
> All the calls to umask() in this patch should be after the fork() in the
> child process. The die() helper is only supposed to be used in child
> processes anyway.
Oh, yes, I did't notice it before. Will move it into child process on v3.
Best Regards
Yang Xu
> The rest looks fine to me. So with the umask() moved
> you can add,
> Tested-by: Christian Brauner (Microsoft) <brauner@xxxxxxxxxx>
> Reviewed-by: Christian Brauner (Microsoft) <brauner@xxxxxxxxxx>
