On Thu, Apr 28, 2022 at 02:49:11AM +0000, Al Viro wrote:
> On Thu, Apr 28, 2022 at 02:23:25AM +0000, xuyang2018.jy@xxxxxxxxxxx wrote:
>
> > > BTW, xfs has grpid option as well:
> > > if (dir&& !(dir->i_mode& S_ISGID)&& xfs_has_grpid(mp)) {
> > > inode_fsuid_set(inode, mnt_userns);
> > > inode->i_gid = dir->i_gid;
> > > inode->i_mode = mode;
> > > } else {
> > > inode_init_owner(mnt_userns, inode, dir, mode);
> > > }
> > >
> > > We could lift that stuff into VFS, but it would require lifting that flag
> > > (BSD vs. SysV behaviour wrt GID - BSD *always* inherits GID from parent
> > > and ignores SGID on directories) into generic superblock. Otherwise we'd
> > > be breaking existing behaviour for ext* and xfs...
> >
> > I also mentioned it in my previous version(in the 3/4 patch)
> > "This patch also changed grpid behaviour for ext4/xfs because the mode
> > passed to them may been changed by vfs_prepare_mode.
> > "
> >
> > I guess we can add a grpid option check in vfs_prepare_mode or in
> > mode_strip_sgid, then it should not break the existing behaviour for
> > ext* and xfs.
>
> I don't like it, TBH. That way we have
> 1) caller mangles mode (after having looked at grpid, etc.)
> 2) filesystem checks grpid and either calls inode_init_owner(),
> which might (or might not) modify the gid to be used or skips the
> call and assigns gid directly.
>
> It's asking for trouble. We have two places where the predicate is
> checked; the first mangles mode (and I'm still not convinced that we
> need to bother with that at all), the second affects gid (and for
> mkdir in sgid directory on non-grpid ones inherits sgid).
>
> That kind of structure is asking for trouble. *IF* we make inode_init_owner()
> aware of grpid (and make that predicate usable from generic helper), we might
> as well just make inode_init_owner() mandatory (for the first time ever) and
> get rid of grpid checks in filesystems themselves. But then there's no point
> doing it in method callers.
>
> Note, BTW, that while XFS has inode_fsuid_set() on the non-inode_init_owner()
> path, it doesn't have inode_fsgid_set() there. Same goes for ext4, while
> ext2 doesn't bother with either in such case...
>
> Let's try to separate the issues here. Jann, could you explain what makes
> empty sgid files dangerous?
Found the original thread in old mailbox, and the method of avoiding the
SGID removal on modification is usable. Which answers the question above...
