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?
