Re: [PATCH v7 4/4] ceph: Remove S_ISGID stripping code in ceph_finish_async_create

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

on 2022/4/26 15:14, Christian Brauner wrote:
> On Tue, Apr 26, 2022 at 12:19:52PM +0800, Yang Xu wrote:
>> Previous patches moved sgid stripping exclusively into the vfs. So
>> manual sgid stripping by the filesystem isn't needed anymore.
>>
>> Reviewed-by: Xiubo Li<xiubli@redhat.com>
>> Reviewed-by: Christian Brauner (Microsoft)<brauner@kernel.org>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>
> Since this is a very sensitive patch series I think we need to be
> annoyingly pedantic about the commit messages. This is really only
> necessary because of the nature of these changes so you'll forgive me
> for being really annoying about this. Here's what I'd change the commit
> messages to:
>
> ceph: rely on vfs for setgid stripping
>
> Now that we finished moving setgid stripping for regular files in setgid
> directories into the vfs, individual filesystem don't need to manually
> strip the setgid bit anymore. Drop the now unneeded code from ceph.

This seems better, thanks.
>
>
>>   fs/ceph/file.c | 4 ----
>>   1 file changed, 4 deletions(-)
>>
>> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
>> index 6c9e837aa1d3..8e3b99853333 100644
>> --- a/fs/ceph/file.c
>> +++ b/fs/ceph/file.c
>> @@ -651,10 +651,6 @@ static int ceph_finish_async_create(struct inode *dir, struct dentry *dentry,
>>   		/* Directories always inherit the setgid bit. */
>>   		if (S_ISDIR(mode))
>>   			mode |= S_ISGID;
>
> (Frankly, this ideally shouldn't be necessary as well, i.e. it'd be
> great if that part would've been done by the vfs already too but it's
> not as security sensitive as setgid stripping for regular files.)

Maybe we can just add mode_add_sgid api into vfs_prepare_mode in the 
future or only just add mode_add_sgid into do_mkdirat?

static inline umode_t vfs_prepare_mode(struct user_namespace *mnt_userns,
				   const struct inode *dir, umode_t mode)
{
	mode = mode_strip_sgid(mnt_userns, dir, mode);

	if (!IS_POSIXACL(dir))
		mode &= ~current_umask();

        mode = mode_add_sgid(dir, mode)

        return mode;
}

fs/inode.c
umodet mode_add_sgid(const struct inode *dir, umode_t mode)
{
	if (dir && dir->i_mode & S_ISGID && S_ISDIR(mode))
		 mode |= S_ISGID;

	return mode;		
}


Then we can remove  "mode |= S_ISGID" code in inode_init_owner after we 
check all places called inode_init_owner.

But now, let's finished S_ISGID stripping patchset firstly.