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
