Re: [PATCH v2 2/2] vfs: Add new setgid_create_acl test

["xuyang2018.jy@fujitsu.com" <xuyang2018.jy@fujitsu.com>]

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@fujitsu.com>
>> ---
> 
> 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@kernel.org>
> Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>