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

From: Christian Brauner <brauner@kernel.org>
To: Yang Xu <xuyang2018.jy@fujitsu.com>
Cc: linux-fsdevel@vger.kernel.org, ceph-devel@vger.kernel.org,
	viro@zeniv.linux.org.uk, david@fromorbit.com, djwong@kernel.org,
	willy@infradead.org, jlayton@kernel.org, pvorel@suse.cz,
	Amir Goldstein <amir73il@gmail.com>
Subject: Re: [PATCH v10 1/4] fs: add mode_strip_sgid() helper
Date: Thu, 21 Jul 2022 12:20:02 +0200	[thread overview]
Message-ID: <20220721102002.cgufvxxtv7zk4yhk@wittgenstein> (raw)
In-Reply-To: <20220714125620.iwww4eivtbytzlgg@wittgenstein>

On Thu, Jul 14, 2022 at 02:56:26PM +0200, Christian Brauner wrote:
> On Thu, Jul 14, 2022 at 02:11:25PM +0800, Yang Xu wrote:
> > Add a dedicated helper to handle the setgid bit when creating a new file
> > in a setgid directory. This is a preparatory patch for moving setgid
> > stripping into the vfs. The patch contains no functional changes.
> > 
> > Currently the setgid stripping logic is open-coded directly in
> > inode_init_owner() and the individual filesystems are responsible for
> > handling setgid inheritance. Since this has proven to be brittle as
> > evidenced by old issues we uncovered over the last months (see [1] to
> > [3] below) we will try to move this logic into the vfs.
> > 
> > Link: e014f37db1a2 ("xfs: use setattr_copy to set vfs inode attributes") [1]
> > Link: 01ea173e103e ("xfs: fix up non-directory creation in SGID directories") [2]
> > Link: fd84bfdddd16 ("ceph: fix up non-directory creation in SGID directories") [3]
> > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> > Reviewed-and-Tested-by: Jeff Layton <jlayton@kernel.org>
> > Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> > ---
> 
> Fyi, I'm on vacation this week and will review this when I get back and
> if things look sane plan to pick it up for the next mw.

Getting back I immediately got a massive summer cold so I'm fairly slow
at picking things back up. Sorry for the delays.

So I rewrote parts of the commit message and specifically added more
details to explicitly point out what regression risks exists. But
overall I don't see any big issues with this anymore. A full xfstests
run for both xfs and ext4 didn't show any regressions. The full LTP
testsuite also didn't find anything to complain about. Still doesn't
mean we won't have people yell but hey it's a start.

I think the benefits of moving S_ISGID handling into the VFS are fairly
obvious and described in detail in [3/4]. Weighing benefits vs
regression risks it seems that we are inclined to try this approach. We
might just fall flat on our face with this but then we'll just have to
revert.

As always, if someone else wants to get their fingers burned by
proposing this during the next mw I'll happily drop it.

[1/4] fs: add mode_strip_sgid() helper
      commit: 2b3416ceff5e6bd4922f6d1c61fb68113dd82302
[2/4] fs: Add missing umask strip in vfs_tmpfile
      commit: ac6800e279a22b28f4fc21439843025a0d5bf03e
[3/4] fs: move S_ISGID stripping into the vfs_*() helpers
      commit: 1639a49ccdce58ea248841ed9b23babcce6dbb0b
[4/4] ceph: rely on vfs for setgid stripping
      commit: 5fadbd992996e9dda7ebcb62f5352866057bd619

Thanks!
Christian