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

Commit Message

Amir Goldstein June 17, 2022, 10:06 a.m. UTC

From: "Darrick J. Wong" <[e-mail]>

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=[e-mail]/
[2] https://lore.kernel.org/linux-xfs/[e-mail]/

Fixes: 7fa294c8991c ("userns: Allow chown and setgid preservation")
Signed-off-by: Darrick J. Wong <[e-mail]>
Reviewed-by: Dave Chinner <[e-mail]>
Reviewed-by: Christoph Hellwig <[e-mail]>
Reviewed-by: Christian Brauner <[e-mail]>
Signed-off-by: Amir Goldstein <[e-mail]>
---
 fs/xfs/xfs_iops.c | 56 +++--------------------------------------------
 fs/xfs/xfs_pnfs.c |  3 ++-
 2 files changed, 5 insertions(+), 54 deletions(-)

Comments

Darrick J. Wong June 22, 2022, 4:41 p.m. UTC | #1

On Fri, Jun 17, 2022 at 01:06:41PM +0300, Amir Goldstein wrote:
> From: "Darrick J. Wong" <[e-mail]>
> 
> 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=[e-mail]/
> [2] https://lore.kernel.org/linux-xfs/[e-mail]/
> 
> Fixes: 7fa294c8991c ("userns: Allow chown and setgid preservation")
> Signed-off-by: Darrick J. Wong <[e-mail]>
> Reviewed-by: Dave Chinner <[e-mail]>
> Reviewed-by: Christoph Hellwig <[e-mail]>
> Reviewed-by: Christian Brauner <[e-mail]>
> Signed-off-by: Amir Goldstein <[e-mail]>

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? :)

--D

> ---
>  fs/xfs/xfs_iops.c | 56 +++--------------------------------------------
>  fs/xfs/xfs_pnfs.c |  3 ++-
>  2 files changed, 5 insertions(+), 54 deletions(-)
> 
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index b7f7b31a77d5..5711c8c12625 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -595,37 +595,6 @@ xfs_vn_getattr(
>  	return 0;
>  }
>  
> -static void
> -xfs_setattr_mode(
> -	struct xfs_inode	*ip,
> -	struct iattr		*iattr)
> -{
> -	struct inode		*inode = VFS_I(ip);
> -	umode_t			mode = iattr->ia_mode;
> -
> -	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> -
> -	inode->i_mode &= S_IFMT;
> -	inode->i_mode |= mode & ~S_IFMT;
> -}
> -
> -void
> -xfs_setattr_time(
> -	struct xfs_inode	*ip,
> -	struct iattr		*iattr)
> -{
> -	struct inode		*inode = VFS_I(ip);
> -
> -	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> -
> -	if (iattr->ia_valid & ATTR_ATIME)
> -		inode->i_atime = iattr->ia_atime;
> -	if (iattr->ia_valid & ATTR_CTIME)
> -		inode->i_ctime = iattr->ia_ctime;
> -	if (iattr->ia_valid & ATTR_MTIME)
> -		inode->i_mtime = iattr->ia_mtime;
> -}
> -
>  static int
>  xfs_vn_change_ok(
>  	struct dentry	*dentry,
> @@ -740,16 +709,6 @@ xfs_setattr_nonsize(
>  				goto out_cancel;
>  		}
>  
> -		/*
> -		 * CAP_FSETID overrides the following restrictions:
> -		 *
> -		 * The set-user-ID and set-group-ID bits of a file will be
> -		 * cleared upon successful return from chown()
> -		 */
> -		if ((inode->i_mode & (S_ISUID|S_ISGID)) &&
> -		    !capable(CAP_FSETID))
> -			inode->i_mode &= ~(S_ISUID|S_ISGID);
> -
>  		/*
>  		 * Change the ownerships and register quota modifications
>  		 * in the transaction.
> @@ -761,7 +720,6 @@ xfs_setattr_nonsize(
>  				olddquot1 = xfs_qm_vop_chown(tp, ip,
>  							&ip->i_udquot, udqp);
>  			}
> -			inode->i_uid = uid;
>  		}
>  		if (!gid_eq(igid, gid)) {
>  			if (XFS_IS_QUOTA_RUNNING(mp) && XFS_IS_GQUOTA_ON(mp)) {
> @@ -772,15 +730,10 @@ xfs_setattr_nonsize(
>  				olddquot2 = xfs_qm_vop_chown(tp, ip,
>  							&ip->i_gdquot, gdqp);
>  			}
> -			inode->i_gid = gid;
>  		}
>  	}
>  
> -	if (mask & ATTR_MODE)
> -		xfs_setattr_mode(ip, iattr);
> -	if (mask & (ATTR_ATIME|ATTR_CTIME|ATTR_MTIME))
> -		xfs_setattr_time(ip, iattr);
> -
> +	setattr_copy(inode, iattr);
>  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
>  
>  	XFS_STATS_INC(mp, xs_ig_attrchg);
> @@ -1025,11 +978,8 @@ xfs_setattr_size(
>  		xfs_inode_clear_eofblocks_tag(ip);
>  	}
>  
> -	if (iattr->ia_valid & ATTR_MODE)
> -		xfs_setattr_mode(ip, iattr);
> -	if (iattr->ia_valid & (ATTR_ATIME|ATTR_CTIME|ATTR_MTIME))
> -		xfs_setattr_time(ip, iattr);
> -
> +	ASSERT(!(iattr->ia_valid & (ATTR_UID | ATTR_GID)));
> +	setattr_copy(inode, iattr);
>  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
>  
>  	XFS_STATS_INC(mp, xs_ig_attrchg);
> diff --git a/fs/xfs/xfs_pnfs.c b/fs/xfs/xfs_pnfs.c
> index f3082a957d5e..ae61094bc9d1 100644
> --- a/fs/xfs/xfs_pnfs.c
> +++ b/fs/xfs/xfs_pnfs.c
> @@ -283,7 +283,8 @@ xfs_fs_commit_blocks(
>  	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
>  	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
>  
> -	xfs_setattr_time(ip, iattr);
> +	ASSERT(!(iattr->ia_valid & (ATTR_UID | ATTR_GID)));
> +	setattr_copy(inode, iattr);
>  	if (update_isize) {
>  		i_size_write(inode, iattr->ia_size);
>  		ip->i_d.di_size = iattr->ia_size;
> -- 
> 2.25.1
>

Amir Goldstein June 22, 2022, 6:36 p.m. UTC | #2

On Wed, Jun 22, 2022 at 7:41 PM Darrick J. Wong <[e-mail]> wrote:
>
> On Fri, Jun 17, 2022 at 01:06:41PM +0300, Amir Goldstein wrote:
> > From: "Darrick J. Wong" <[e-mail]>
> >
> > 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=[e-mail]/
> > [2] https://lore.kernel.org/linux-xfs/[e-mail]/
> >
> > Fixes: 7fa294c8991c ("userns: Allow chown and setgid preservation")
> > Signed-off-by: Darrick J. Wong <[e-mail]>
> > Reviewed-by: Dave Chinner <[e-mail]>
> > Reviewed-by: Christoph Hellwig <[e-mail]>
> > Reviewed-by: Christian Brauner <[e-mail]>
> > Signed-off-by: Amir Goldstein <[e-mail]>
>
> 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.

Leah Rumancik June 22, 2022, 10:17 p.m. UTC | #3

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 <[e-mail]> wrote:
> >
> > On Fri, Jun 17, 2022 at 01:06:41PM +0300, Amir Goldstein wrote:
> > > From: "Darrick J. Wong" <[e-mail]>
> > >
> > > 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=[e-mail]/
> > > [2] https://lore.kernel.org/linux-xfs/[e-mail]/
> > >
> > > Fixes: 7fa294c8991c ("userns: Allow chown and setgid preservation")
> > > Signed-off-by: Darrick J. Wong <[e-mail]>
> > > Reviewed-by: Dave Chinner <[e-mail]>
> > > Reviewed-by: Christoph Hellwig <[e-mail]>
> > > Reviewed-by: Christian Brauner <[e-mail]>
> > > Signed-off-by: Amir Goldstein <[e-mail]>
> >
> > 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

Amir Goldstein June 23, 2022, 4:22 a.m. UTC | #4

On Thu, Jun 23, 2022 at 1:17 AM Leah Rumancik <[e-mail]> 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 <[e-mail]> wrote:
> > >
> > > On Fri, Jun 17, 2022 at 01:06:41PM +0300, Amir Goldstein wrote:
> > > > From: "Darrick J. Wong" <[e-mail]>
> > > >
> > > > 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=[e-mail]/
> > > > [2] https://lore.kernel.org/linux-xfs/[e-mail]/
> > > >
> > > > Fixes: 7fa294c8991c ("userns: Allow chown and setgid preservation")
> > > > Signed-off-by: Darrick J. Wong <[e-mail]>
> > > > Reviewed-by: Dave Chinner <[e-mail]>
> > > > Reviewed-by: Christoph Hellwig <[e-mail]>
> > > > Reviewed-by: Christian Brauner <[e-mail]>
> > > > Signed-off-by: Amir Goldstein <[e-mail]>
> > >
> > > 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.

Patch

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index b7f7b31a77d5..5711c8c12625 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -595,37 +595,6 @@  xfs_vn_getattr(
 	return 0;
 }
 
-static void
-xfs_setattr_mode(
-	struct xfs_inode	*ip,
-	struct iattr		*iattr)
-{
-	struct inode		*inode = VFS_I(ip);
-	umode_t			mode = iattr->ia_mode;
-
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
-
-	inode->i_mode &= S_IFMT;
-	inode->i_mode |= mode & ~S_IFMT;
-}
-
-void
-xfs_setattr_time(
-	struct xfs_inode	*ip,
-	struct iattr		*iattr)
-{
-	struct inode		*inode = VFS_I(ip);
-
-	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
-
-	if (iattr->ia_valid & ATTR_ATIME)
-		inode->i_atime = iattr->ia_atime;
-	if (iattr->ia_valid & ATTR_CTIME)
-		inode->i_ctime = iattr->ia_ctime;
-	if (iattr->ia_valid & ATTR_MTIME)
-		inode->i_mtime = iattr->ia_mtime;
-}
-
 static int
 xfs_vn_change_ok(
 	struct dentry	*dentry,
@@ -740,16 +709,6 @@  xfs_setattr_nonsize(
 				goto out_cancel;
 		}
 
-		/*
-		 * CAP_FSETID overrides the following restrictions:
-		 *
-		 * The set-user-ID and set-group-ID bits of a file will be
-		 * cleared upon successful return from chown()
-		 */
-		if ((inode->i_mode & (S_ISUID|S_ISGID)) &&
-		    !capable(CAP_FSETID))
-			inode->i_mode &= ~(S_ISUID|S_ISGID);
-
 		/*
 		 * Change the ownerships and register quota modifications
 		 * in the transaction.
@@ -761,7 +720,6 @@  xfs_setattr_nonsize(
 				olddquot1 = xfs_qm_vop_chown(tp, ip,
 							&ip->i_udquot, udqp);
 			}
-			inode->i_uid = uid;
 		}
 		if (!gid_eq(igid, gid)) {
 			if (XFS_IS_QUOTA_RUNNING(mp) && XFS_IS_GQUOTA_ON(mp)) {
@@ -772,15 +730,10 @@  xfs_setattr_nonsize(
 				olddquot2 = xfs_qm_vop_chown(tp, ip,
 							&ip->i_gdquot, gdqp);
 			}
-			inode->i_gid = gid;
 		}
 	}
 
-	if (mask & ATTR_MODE)
-		xfs_setattr_mode(ip, iattr);
-	if (mask & (ATTR_ATIME|ATTR_CTIME|ATTR_MTIME))
-		xfs_setattr_time(ip, iattr);
-
+	setattr_copy(inode, iattr);
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
 
 	XFS_STATS_INC(mp, xs_ig_attrchg);
@@ -1025,11 +978,8 @@  xfs_setattr_size(
 		xfs_inode_clear_eofblocks_tag(ip);
 	}
 
-	if (iattr->ia_valid & ATTR_MODE)
-		xfs_setattr_mode(ip, iattr);
-	if (iattr->ia_valid & (ATTR_ATIME|ATTR_CTIME|ATTR_MTIME))
-		xfs_setattr_time(ip, iattr);
-
+	ASSERT(!(iattr->ia_valid & (ATTR_UID | ATTR_GID)));
+	setattr_copy(inode, iattr);
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
 
 	XFS_STATS_INC(mp, xs_ig_attrchg);
diff --git a/fs/xfs/xfs_pnfs.c b/fs/xfs/xfs_pnfs.c
index f3082a957d5e..ae61094bc9d1 100644
--- a/fs/xfs/xfs_pnfs.c
+++ b/fs/xfs/xfs_pnfs.c
@@ -283,7 +283,8 @@  xfs_fs_commit_blocks(
 	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
 
-	xfs_setattr_time(ip, iattr);
+	ASSERT(!(iattr->ia_valid & (ATTR_UID | ATTR_GID)));
+	setattr_copy(inode, iattr);
 	if (update_isize) {
 		i_size_write(inode, iattr->ia_size);
 		ip->i_d.di_size = iattr->ia_size;