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

From: Christian Brauner <brauner@kernel.org>
To: Al Viro <viro@zeniv.linux.org.uk>
Cc: Yang Xu <xuyang2018.jy@fujitsu.com>,
	linux-fsdevel@vger.kernel.org, ceph-devel@vger.kernel.org,
	david@fromorbit.com, djwong@kernel.org, willy@infradead.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 10:25:44 +0200	[thread overview]
Message-ID: <20220428082544.7v5s3dfgtsda6hll@wittgenstein> (raw)
In-Reply-To: <Ymn05eNgOnaYy36R@zeniv-ca.linux.org.uk>

On Thu, Apr 28, 2022 at 01:59:01AM +0000, Al Viro wrote:
> On Tue, Apr 26, 2022 at 07:11:27PM +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.
> 
> First of all, inode_init_owner() is (and always had been) an optional helper.

The whole patch series was triggered because ever since I added setgid
inheritance tests (see [1]) as part of the idmapped mounts test suite
into xfstests we found 3 setgid inheritance bugs (The bugs are linked in
the commit messages.).
The bugs showed up whenever a filesystem didn't call inode_init_owner()
or had custom code in place that deviated from expectations.

That's what triggered this whole patch series. Yang took it on and seems
here to see it through.

I should point out that it was rather unclear what expectations are btw
because of the ordering dependency between umask and POSIX ACLs and
setgid stripping. I've describe this at length in the commit message I
gave Yang.

It took a lot of digging and over the course of me reviewing this patch
series more and more corner-cases pop up that we haven't handled.

> Filesystems are *NOT* required to call it, so putting any common functionality
> in there had always been a mistake.

See above. I pointed this out in earlier version.
I very much agree which is why we should move it into the vfs proper if
we can with reasonably minimal regression risk.

[1]: https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/tree/src/idmapped-mounts/idmapped-mounts.c#n7812