Re: [PATCH v8 1/4] fs: add mode_strip_sgid() helper

From: Al Viro <viro@zeniv.linux.org.uk>
To: "xuyang2018.jy@fujitsu.com" <xuyang2018.jy@fujitsu.com>
Cc: "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
	"ceph-devel@vger.kernel.org" <ceph-devel@vger.kernel.org>,
	"david@fromorbit.com" <david@fromorbit.com>,
	"djwong@kernel.org" <djwong@kernel.org>,
	"brauner@kernel.org" <brauner@kernel.org>,
	"willy@infradead.org" <willy@infradead.org>,
	"jlayton@kernel.org" <jlayton@kernel.org>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Jann Horn <jannh@google.com>
Subject: Re: [PATCH v8 1/4] fs: add mode_strip_sgid() helper
Date: Thu, 28 Apr 2022 02:49:11 +0000	[thread overview]
Message-ID: <YmoAp+yWBpH5T8rt@zeniv-ca.linux.org.uk> (raw)
In-Reply-To: <626A08DA.3060802@fujitsu.com>

On Thu, Apr 28, 2022 at 02:23:25AM +0000, xuyang2018.jy@fujitsu.com 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?