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

Amir Goldstein
2022-06-23T04:47:46Z

On Thu, Jun 23, 2022 at 1:17 AM Leah Rumancik <leah.rumancik@gmail.com> 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@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.

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.