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

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

on 2022/4/18 11:08, Matthew Wilcox wrote:
> On Fri, Apr 15, 2022 at 04:09:24PM +0200, Christian Brauner wrote:
>>> +			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.
>>
>> 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. :)
>
> No, I think your instincts are correct.  This should be
I can't understand why returning umode_t is better. So Does kernel have 
some rules for adding new function I don't notice before? Just need a 
reason.

ps: I will decide whether use pointer or use return umode_t value before 
I send v4.

Best Regards
Yang Xu
>
> umode_t inode_sgid_strip(struct user_namespace *mnt_userns,
> 		const struct inode *dir, umode_t mode)
> {
> 	if (S_ISDIR(mode) || !dir || !(dir->i_mode&  S_ISGID))
> 		return mode;
> 	if (mode&  (S_ISGID | S_IXGRP) != (S_ISGID | S_IXGRP))
> 		return mode;
> ...
>
> and the same for prepare_mode().
>
> And really, I think this should be called inode_strip_sgid().  Right?