On Thu, Jun 23, 2022 at 1:17 AM Leah Rumancik <leah.rumancik@xxxxxxxxx> wrote: > > On Wed, Jun 22, 2022 at 09:36:53PM +0300, Amir Goldstein wrote: > > On Wed, Jun 22, 2022 at 7:41 PM Darrick J. Wong <djwong@xxxxxxxxxx> wrote: > > > > > > On Fri, Jun 17, 2022 at 01:06:41PM +0300, Amir Goldstein wrote: > > > > From: "Darrick J. Wong" <djwong@xxxxxxxxxx> > > > > > > > > commit e014f37db1a2d109afa750042ac4d69cf3e3d88e upstream. > > > > > > > > [remove userns argument of setattr_copy() for backport] > > > > > > > > Filipe Manana pointed out that XFS' behavior w.r.t. setuid/setgid > > > > revocation isn't consistent with btrfs[1] or ext4. Those two > > > > filesystems use the VFS function setattr_copy to convey certain > > > > attributes from struct iattr into the VFS inode structure. > > > > > > > > Andrey Zhadchenko reported[2] that XFS uses the wrong user namespace to > > > > decide if it should clear setgid and setuid on a file attribute update. > > > > This is a second symptom of the problem that Filipe noticed. > > > > > > > > XFS, on the other hand, open-codes setattr_copy in xfs_setattr_mode, > > > > xfs_setattr_nonsize, and xfs_setattr_time. Regrettably, setattr_copy is > > > > /not/ a simple copy function; it contains additional logic to clear the > > > > setgid bit when setting the mode, and XFS' version no longer matches. > > > > > > > > The VFS implements its own setuid/setgid stripping logic, which > > > > establishes consistent behavior. It's a tad unfortunate that it's > > > > scattered across notify_change, should_remove_suid, and setattr_copy but > > > > XFS should really follow the Linux VFS. Adapt XFS to use the VFS > > > > functions and get rid of the old functions. > > > > > > > > [1] https://lore.kernel.org/fstests/CAL3q7H47iNQ=Wmk83WcGB-KBJVOEtR9+qGczzCeXJ9Y2KCV25Q@xxxxxxxxxxxxxx/ > > > > [2] https://lore.kernel.org/linux-xfs/20220221182218.748084-1-andrey.zhadchenko@xxxxxxxxxxxxx/ > > > > > > > > Fixes: 7fa294c8991c ("userns: Allow chown and setgid preservation") > > > > Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx> > > > > Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx> > > > > Reviewed-by: Christoph Hellwig <hch@xxxxxx> > > > > Reviewed-by: Christian Brauner <brauner@xxxxxxxxxx> > > > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx> > > > > > > Same question as I posted to Leah's series -- have all the necessary VFS > > > fixes and whatnot been backported to 5.10? Such that all the new sgid > > > inheritance tests actually pass with this patch applied? :) > > > > The only patch I backorted to 5.10 is: > > xfs: fix up non-directory creation in SGID directories > > > > I will check which SGID tests ran on my series. > > > > Personally, I would rather defer THIS patch to a later post to stable > > (Leah's patch as well) until we have a better understanding of the state > > of SGID issues. > > > > Thanks, > > Amir. > > On the latest version of the SGID tests, I see failures of > generic/68[3-7] and xfs/019 on both the baseline and backports branch. > generic/673 fails on most configs for the baseline but seems to be fixed > in the backports branch. Regardless, I am fine dropping this patch for > this round. Let's do that then. I actually didn't plan to post this patch for this round to begin with. I only posted it because you did. Thanks, Amir.
