Re: [PATCH 5.10 CANDIDATE 11/11] xfs: use setattr_copy to set vfs inode attributes

[Leah Rumancik <leah.rumancik@gmail.com>]

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@kernel.org> wrote:
> >
> > On Fri, Jun 17, 2022 at 01:06:41PM +0300, Amir Goldstein wrote:
> > > From: "Darrick J. Wong" <djwong@kernel.org>
> > >
> > > 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@mail.gmail.com/
> > > [2] https://lore.kernel.org/linux-xfs/20220221182218.748084-1-andrey.zhadchenko@virtuozzo.com/
> > >
> > > Fixes: 7fa294c8991c ("userns: Allow chown and setgid preservation")
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > Reviewed-by: Dave Chinner <dchinner@redhat.com>
> > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > > Reviewed-by: Christian Brauner <brauner@kernel.org>
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >
> > 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.

Best,
Leah