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
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?
