Re: [PATCH v2 1/3] vfs: Add inode_sgid_strip() api

From: "xuyang2018.jy@fujitsu.com" <xuyang2018.jy@fujitsu.com>
To: Christian Brauner <brauner@kernel.org>,
	"david@fromorbit.com" <david@fromorbit.com>
Cc: "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
	"ceph-devel@vger.kernel.org" <ceph-devel@vger.kernel.org>,
	"ocfs2-devel@oss.oracle.com" <ocfs2-devel@oss.oracle.com>,
	"viro@zeniv.linux.org.uk" <viro@zeniv.linux.org.uk>,
	"djwong@kernel.org" <djwong@kernel.org>,
	"jlayton@kernel.org" <jlayton@kernel.org>
Subject: Re: [PATCH v2 1/3] vfs: Add inode_sgid_strip() api
Date: Fri, 15 Apr 2022 01:39:46 +0000	[thread overview]
Message-ID: <6258DB31.4020403@fujitsu.com> (raw)
In-Reply-To: <20220414120217.fbsljr7alpvy5nmy@wittgenstein>

on 2022/4/14 20:02, Christian Brauner wrote:
> On Thu, Apr 14, 2022 at 03:57:17PM +0800, Yang Xu wrote:
>> inode_sgid_strip() function is used to strip S_ISGID mode
>> when creat/open/mknod file.
>>
>> Reviewed-by: Christian Brauner (Microsoft)<brauner@kernel.org>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>>   fs/inode.c         | 18 ++++++++++++++++++
>>   include/linux/fs.h |  3 ++-
>>   2 files changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/inode.c b/fs/inode.c
>> index 9d9b422504d1..d63264998855 100644
>> --- a/fs/inode.c
>> +++ b/fs/inode.c
>> @@ -2405,3 +2405,21 @@ struct timespec64 current_time(struct inode *inode)
>>   	return timestamp_truncate(now, inode);
>>   }
>>   EXPORT_SYMBOL(current_time);
>> +
>> +void inode_sgid_strip(struct user_namespace *mnt_userns, struct inode *dir,
>> +		      umode_t *mode)
>> +{
>> +	if (!dir || !(dir->i_mode&  S_ISGID))
>> +		return;
>> +	if ((*mode&  (S_ISGID | S_IXGRP)) != (S_ISGID | S_IXGRP))
>> +		return;
>> +	if (S_ISDIR(*mode))
>> +		return;
>> +	if (in_group_p(i_gid_into_mnt(mnt_userns, dir)))
>> +		return;
>> +	if (capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
>> +		return;
>> +
>> +	*mode&= ~S_ISGID;
>> +}
>> +EXPORT_SYMBOL(inode_sgid_strip);
>
>
> I still think this should return umode_t with the setgid bit stripped
> instead of modifying the mode directly. I may have misunderstood Dave,
> but I thought he preferred to return umode_t too?
Dave's comment as below:
"
Agreed, that's a much nicer API for this function - it makes it
clear that it can modifying the mode that is passed in.
"

So I think Dave should like modify mode directly instead of returning a 
umode_t value.

@Dave  So which way do you mean?

Best Regards
Yang Xu
>
> umode_t inode_sgid_strip(struct user_namespace *mnt_userns, struct inode *dir, umode_t mode)
> {
> 	if (S_ISDIR(mode))
> 		return mode;
>
> 	if (!dir || !(dir->i_mode&  S_ISGID))
> 		return;
>
> 	if ((mode&  (S_ISGID | S_IXGRP)) != (S_ISGID | S_IXGRP))
> 		return;
>
> 	if (in_group_p(i_gid_into_mnt(mnt_userns, dir)))
> 		return;
>
> 	if (capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
> 		return;
>
> 	return mode&  ~S_ISGID;
> }