Re: [PATCH v3 1/7] fs/inode: move sgid strip operation from inode_init_owner into inode_sgid_strip

From: "xuyang2018.jy@fujitsu.com" <xuyang2018.jy@fujitsu.com>
To: Christian Brauner <brauner@kernel.org>
Cc: "david@fromorbit.com" <david@fromorbit.com>,
	"djwong@kernel.org" <djwong@kernel.org>,
	"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
	"ceph-devel@vger.kernel.org" <ceph-devel@vger.kernel.org>,
	"linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
	"linux-xfs@vger.kernel.org" <linux-xfs@vger.kernel.org>,
	"viro@zeniv.linux.org.uk" <viro@zeniv.linux.org.uk>,
	"jlayton@kernel.org" <jlayton@kernel.org>
Subject: Re: [PATCH v3 1/7] fs/inode: move sgid strip operation from inode_init_owner into inode_sgid_strip
Date: Mon, 18 Apr 2022 02:08:12 +0000	[thread overview]
Message-ID: <625CD661.9070103@fujitsu.com> (raw)
In-Reply-To: <20220415140924.oirar6dklelujnxs@wittgenstein>

on 2022/4/15 22:09, Christian Brauner wrote:
> On Fri, Apr 15, 2022 at 07:02:17PM +0800, Yang Xu wrote:
>> This has no functional change. Just create and export inode_sgid_strip api for
>> the subsequent patch. This function is used to strip S_ISGID mode when init
>> a new inode.
>>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>> v2->v3:
>> 1.Use const struct inode * instead of struct inode *
>> 2.replace sgid strip with inode_sgid_strip in a single patch
>>   fs/inode.c         | 24 ++++++++++++++++++++----
>>   include/linux/fs.h |  3 ++-
>>   2 files changed, 22 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/inode.c b/fs/inode.c
>> index 9d9b422504d1..1b569ad882ce 100644
>> --- a/fs/inode.c
>> +++ b/fs/inode.c
>> @@ -2246,10 +2246,8 @@ void inode_init_owner(struct user_namespace *mnt_userns, struct inode *inode,
>>   		/* Directories are special, and always inherit S_ISGID */
>>   		if (S_ISDIR(mode))
>>   			mode |= S_ISGID;
>> -		else if ((mode&  (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)&&
>> -			 !in_group_p(i_gid_into_mnt(mnt_userns, dir))&&
>> -			 !capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
>> -			mode&= ~S_ISGID;
>> +		else
>> +			inode_sgid_strip(mnt_userns, dir,&mode);
>>   	} else
>>   		inode_fsgid_set(inode, mnt_userns);
>>   	inode->i_mode = mode;
>> @@ -2405,3 +2403,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,
>> +		      const 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;
>
> I'd place that check first as this whole function is really only
> relevant for non-directories.
Sound reasonable.

Best Regards
Yang Xu
>
> Otherwise I can live with *mode being a pointer although I still find
> this unpleasant API wise but the bikeshed does it's job without having
> my color. :)
>
> I'd like to do some good testing on this.
>
> Acked-by: Christian Brauner (Microsoft)<brauner@kernel.org>