[PATCHSET 0/2] xfs: use setattr_copy to set VFS file attributes

[PATCHSET 0/2] xfs: use setattr_copy to set VFS file attributes

[Darrick J. Wong]

Hi all,

A few weeks ago, Filipe Manana reported[1] that the new generic/673 test
fails on btrfs because btrfs is more aggressive about dropping the
setgid bit when reflinking into a file.  After some more digging, we
discovered that btrfs calls the VFS helpers to handle updating VFS
inode attributes, whereas XFS has open-coded logic dating from ~1997
that have not been kept up to date.

A few days later, Andrey Zhadchenko reported[2] that XFS can mistakenly
clear S_ISUID and S_ISGID on idmapped mounts.  After further discussion,
it was pointed out that the VFS already handles all these fiddly file
mode changes, and that it was the XFS implementation that is out of
date.

Both of these reports resolve to the same cause, which is that XFS needs
to call setattr_copy to update i_mode instead of doing it directly.
This series replaces all of our bespoke code with VFS calls to fix the
problem and reduce the size of the codebase by ~70 lines.

[1] https://lore.kernel.org/linux-xfs/CAL3q7H47iNQ=Wmk83WcGB-KBJVOEtR9+qGczzCeXJ9Y2KCV25Q@mail.gmail.com/
[2] https://lore.kernel.org/linux-xfs/20220221182218.748084-1-andrey.zhadchenko@virtuozzo.com/

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=setattr-copy-5.18
---
 fs/xfs/xfs_iops.c |  116 +++++++++++------------------------------------------
 fs/xfs/xfs_pnfs.c |    3 +
 2 files changed, 25 insertions(+), 94 deletions(-)

[PATCH 1/2] xfs: use setattr_copy to set vfs inode attributes

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

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: f736d93d76d3 ("xfs: support idmapped mounts")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 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 b79b3846e71b..4132026f5fb0 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -613,37 +613,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 user_namespace	*mnt_userns,
@@ -742,16 +711,6 @@ xfs_setattr_nonsize(
 		gid = (mask & ATTR_GID) ? iattr->ia_gid : igid;
 		uid = (mask & ATTR_UID) ? iattr->ia_uid : iuid;
 
-		/*
-		 * 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.
@@ -763,7 +722,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_GQUOTA_ON(mp)) {
@@ -774,15 +732,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(mnt_userns, inode, iattr);
 	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
 
 	XFS_STATS_INC(mp, xs_ig_attrchg);
@@ -1006,11 +959,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(mnt_userns, 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 4abe17312c2b..37a24f0f7cd4 100644
--- a/fs/xfs/xfs_pnfs.c
+++ b/fs/xfs/xfs_pnfs.c
@@ -319,7 +319,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(&init_user_ns, inode, iattr);
 	if (update_isize) {
 		i_size_write(inode, iattr->ia_size);
 		ip->i_disk_size = iattr->ia_size;

[PATCH 2/2] xfs: refactor user/group quota chown in xfs_setattr_nonsize

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Combine if tests to reduce the indentation levels of the quota chown
calls in xfs_setattr_nonsize.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_iops.c |   60 ++++++++++++++++++-----------------------------------
 1 file changed, 20 insertions(+), 40 deletions(-)


diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 4132026f5fb0..f6680dade1d9 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -647,10 +647,10 @@ xfs_setattr_nonsize(
 	int			mask = iattr->ia_valid;
 	xfs_trans_t		*tp;
 	int			error;
-	kuid_t			uid = GLOBAL_ROOT_UID, iuid = GLOBAL_ROOT_UID;
-	kgid_t			gid = GLOBAL_ROOT_GID, igid = GLOBAL_ROOT_GID;
+	kuid_t			uid = GLOBAL_ROOT_UID;
+	kgid_t			gid = GLOBAL_ROOT_GID;
 	struct xfs_dquot	*udqp = NULL, *gdqp = NULL;
-	struct xfs_dquot	*olddquot1 = NULL, *olddquot2 = NULL;
+	struct xfs_dquot	*old_udqp = NULL, *old_gdqp = NULL;
 
 	ASSERT((mask & ATTR_SIZE) == 0);
 
@@ -697,42 +697,22 @@ xfs_setattr_nonsize(
 		goto out_dqrele;
 
 	/*
-	 * Change file ownership.  Must be the owner or privileged.
+	 * Register quota modifications in the transaction.  Must be the owner
+	 * or privileged.  These IDs could have changed since we last looked at
+	 * them.  But, we're assured that if the ownership did change while we
+	 * didn't have the inode locked, inode's dquot(s) would have changed
+	 * also.
 	 */
-	if (mask & (ATTR_UID|ATTR_GID)) {
-		/*
-		 * These IDs could have changed since we last looked at them.
-		 * But, we're assured that if the ownership did change
-		 * while we didn't have the inode locked, inode's dquot(s)
-		 * would have changed also.
-		 */
-		iuid = inode->i_uid;
-		igid = inode->i_gid;
-		gid = (mask & ATTR_GID) ? iattr->ia_gid : igid;
-		uid = (mask & ATTR_UID) ? iattr->ia_uid : iuid;
-
-		/*
-		 * Change the ownerships and register quota modifications
-		 * in the transaction.
-		 */
-		if (!uid_eq(iuid, uid)) {
-			if (XFS_IS_UQUOTA_ON(mp)) {
-				ASSERT(mask & ATTR_UID);
-				ASSERT(udqp);
-				olddquot1 = xfs_qm_vop_chown(tp, ip,
-							&ip->i_udquot, udqp);
-			}
-		}
-		if (!gid_eq(igid, gid)) {
-			if (XFS_IS_GQUOTA_ON(mp)) {
-				ASSERT(xfs_has_pquotino(mp) ||
-				       !XFS_IS_PQUOTA_ON(mp));
-				ASSERT(mask & ATTR_GID);
-				ASSERT(gdqp);
-				olddquot2 = xfs_qm_vop_chown(tp, ip,
-							&ip->i_gdquot, gdqp);
-			}
-		}
+	if ((mask & ATTR_UID) && XFS_IS_UQUOTA_ON(mp) &&
+					!uid_eq(inode->i_uid, iattr->ia_uid)) {
+		ASSERT(udqp);
+		old_udqp = xfs_qm_vop_chown(tp, ip, &ip->i_udquot, udqp);
+	}
+	if ((mask & ATTR_GID) && XFS_IS_GQUOTA_ON(mp) &&
+					!gid_eq(inode->i_gid, iattr->ia_gid)) {
+		ASSERT(xfs_has_pquotino(mp) || !XFS_IS_PQUOTA_ON(mp));
+		ASSERT(gdqp);
+		old_gdqp = xfs_qm_vop_chown(tp, ip, &ip->i_gdquot, gdqp);
 	}
 
 	setattr_copy(mnt_userns, inode, iattr);
@@ -747,8 +727,8 @@ xfs_setattr_nonsize(
 	/*
 	 * Release any dquot(s) the inode had kept before chown.
 	 */
-	xfs_qm_dqrele(olddquot1);
-	xfs_qm_dqrele(olddquot2);
+	xfs_qm_dqrele(old_udqp);
+	xfs_qm_dqrele(old_gdqp);
 	xfs_qm_dqrele(udqp);
 	xfs_qm_dqrele(gdqp);
 

Re: [PATCH 1/2] xfs: use setattr_copy to set vfs inode attributes

[Dave Chinner]

On Wed, Mar 09, 2022 at 11:22:11AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> 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: f736d93d76d3 ("xfs: support idmapped mounts")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_iops.c |   56 +++--------------------------------------------------
>  fs/xfs/xfs_pnfs.c |    3 ++-
>  2 files changed, 5 insertions(+), 54 deletions(-)

Looks good, nice cleanup as well as being more correct.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] xfs: refactor user/group quota chown in xfs_setattr_nonsize

[Dave Chinner]

On Wed, Mar 09, 2022 at 11:22:17AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Combine if tests to reduce the indentation levels of the quota chown
> calls in xfs_setattr_nonsize.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_iops.c |   60 ++++++++++++++++++-----------------------------------
>  1 file changed, 20 insertions(+), 40 deletions(-)

Looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCHSET 0/2] xfs: use setattr_copy to set VFS file attributes

[Andrey Zhadchenko]

On 3/9/22 22:22, Darrick J. Wong wrote:
> Hi all,
> 
> A few weeks ago, Filipe Manana reported[1] that the new generic/673 test
> fails on btrfs because btrfs is more aggressive about dropping the
> setgid bit when reflinking into a file.  After some more digging, we
> discovered that btrfs calls the VFS helpers to handle updating VFS
> inode attributes, whereas XFS has open-coded logic dating from ~1997
> that have not been kept up to date.
> 
> A few days later, Andrey Zhadchenko reported[2] that XFS can mistakenly
> clear S_ISUID and S_ISGID on idmapped mounts.  After further discussion,
> it was pointed out that the VFS already handles all these fiddly file
> mode changes, and that it was the XFS implementation that is out of
> date.
> 
> Both of these reports resolve to the same cause, which is that XFS needs
> to call setattr_copy to update i_mode instead of doing it directly.
> This series replaces all of our bespoke code with VFS calls to fix the
> problem and reduce the size of the codebase by ~70 lines.
> 
> [1] https://lore.kernel.org/linux-xfs/CAL3q7H47iNQ=Wmk83WcGB-KBJVOEtR9+qGczzCeXJ9Y2KCV25Q@mail.gmail.com/
> [2] https://lore.kernel.org/linux-xfs/20220221182218.748084-1-andrey.zhadchenko@virtuozzo.com/
> 
> If you're going to start using this mess, you probably ought to just
> pull from my git trees, which are linked below.
> 
> This is an extraordinary way to destroy everything.  Enjoy!
> Comments and questions are, as always, welcome.
> 
> --D
> 
> kernel git tree:
> https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=setattr-copy-5.18
> ---
>   fs/xfs/xfs_iops.c |  116 +++++++++++------------------------------------------
>   fs/xfs/xfs_pnfs.c |    3 +
>   2 files changed, 25 insertions(+), 94 deletions(-)
> 

Thanks for fixing this. I will soon send a corresponding tests to 
xfstests - it's almost done.

Also while you are at it, what do you think of the following?

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index fdab467c034f..871576723145 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1436,17 +1436,6 @@ xfs_fileattr_set(

         if (!fa->fsx_valid)
                 goto skip_xattr;
-       /*
-        * Change file ownership.  Must be the owner or privileged. 
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 ((VFS_I(ip)->i_mode & (S_ISUID|S_ISGID)) &&
-           !capable_wrt_inode_uidgid(mnt_userns, VFS_I(ip), CAP_FSETID))
-               VFS_I(ip)->i_mode &= ~(S_ISUID|S_ISGID);

         /* Change the ownerships and register project quota 
modifications */
         if (ip->i_projid != fa->fsx_projid) {


xfs_fileattr_set() also may clear S_ISUID|S_ISGID for FS_IOC_FSSETXATTR 
and FS_IOC_SETFLAGS ioctls.
For example, setting projid definitely do clear it for not CAP_FSETID 
users. I wonder if it is documented and intentional? ext4 do not touch 
this bits, but I have no idea if it should

Re: [PATCH 1/2] xfs: use setattr_copy to set vfs inode attributes

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/2] xfs: refactor user/group quota chown in xfs_setattr_nonsize

[Christoph Hellwig]

On Wed, Mar 09, 2022 at 11:22:17AM -0800, Darrick J. Wong wrote:
> +	if ((mask & ATTR_UID) && XFS_IS_UQUOTA_ON(mp) &&
> +					!uid_eq(inode->i_uid, iattr->ia_uid)) {

Nit: I think in this case an indentation like:

	if ((mask & ATTR_UID) && XFS_IS_UQUOTA_ON(mp) &&
	    !uid_eq(inode->i_uid, iattr->ia_uid)) {

is way more readable.  Same for the gid case.

Otherwise this looks like a nice cleanup:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/2] xfs: refactor user/group quota chown in xfs_setattr_nonsize

[Christian Brauner]

On Wed, Mar 09, 2022 at 11:22:17AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Combine if tests to reduce the indentation levels of the quota chown
> calls in xfs_setattr_nonsize.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---

Looks good to me,
Reviewed-by: Christian Brauner <brauner@kernel.org>

Re: [PATCH 1/2] xfs: use setattr_copy to set vfs inode attributes

[Christian Brauner]

On Wed, Mar 09, 2022 at 11:22:11AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> 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: f736d93d76d3 ("xfs: support idmapped mounts")

Fwiw, as I've pointed out in
https://lore.kernel.org/linux-xfs/20220222122331.ijeapomur76h7xf6@wittgenstein/
the original analysis that this commit message links to in [2] is not
correct. But the thread clarifies it so I think it's fine.

But I think the fixes tag is wrong here afaict. As I've pointed out in
https://lore.kernel.org/linux-xfs/20220222123656.433l67bxhv3s2vbo@wittgenstein/
the faulty behavior should predate idmapped mounts by a lot. The bug is
with capable(CAP_FSETID). A simple reproducer that should work on any
pre 5.12 kernel is:

brauner@wittgenstein|~/src/git/linux/ltp/testcases/kernel/syscalls/chown|master %=
> unshare -U --map-root
root@wittgenstein|~/src/git/linux/ltp/testcases/kernel/syscalls/chown|master %=
> PATH=$PATH:$PWD ./chown02
tst_memutils.c:157: TWARN: Can't adjust score, even with capabilities!?
tst_test.c:1455: TINFO: Timeout per run is 0h 05m 00s
chown02.c:45: TPASS: chown(testfile1, 0, 0) passed
chown02.c:45: TPASS: chown(testfile2, 0, 0) passed
chown02.c:57: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700

Summary:
passed   2
failed   1
broken   0
skipped  0
warnings 1

There's no idmapped mounts here in play. The caller simply has been
placed in a new user namespace and thus they fail the current
capable(CAP_FSETID) check which will cause xfs to strip the sgid bit.

Now trying the same with ext4:

ubuntu@f2-vm:~/src/git/linux/ltp/testcases/kernel/syscalls/chown$ unshare -U --map-root
root@f2-vm:~/src/git/linux/ltp/testcases/kernel/syscalls/chown# PATH=$PATH:$PWD ./chown02
tst_memutils.c:157: TWARN: Can't adjust score, even with capabilities!?
tst_test.c:1455: TINFO: Timeout per run is 0h 05m 00s
chown02.c:45: TPASS: chown(testfile1, 0, 0) passed
chown02.c:45: TPASS: chown(testfile2, 0, 0) passed

Summary:
passed   2
failed   0
broken   0
skipped  0
warnings 1

it passes since ext4 uses setattr_copy() and thus the capability is
checked for in the caller's user namespace.

> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---

Other than the wrong Fixes:,
Reviewed-by: Christian Brauner <brauner@kernel.org>

Re: [PATCH 1/2] xfs: use setattr_copy to set vfs inode attributes

[Darrick J. Wong]

On Thu, Mar 10, 2022 at 11:11:58AM +0100, Christian Brauner wrote:
> On Wed, Mar 09, 2022 at 11:22:11AM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > 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: f736d93d76d3 ("xfs: support idmapped mounts")
> 
> Fwiw, as I've pointed out in
> https://lore.kernel.org/linux-xfs/20220222122331.ijeapomur76h7xf6@wittgenstein/
> the original analysis that this commit message links to in [2] is not
> correct. But the thread clarifies it so I think it's fine.
> 
> But I think the fixes tag is wrong here afaict. As I've pointed out in
> https://lore.kernel.org/linux-xfs/20220222123656.433l67bxhv3s2vbo@wittgenstein/
> the faulty behavior should predate idmapped mounts by a lot. The bug is
> with capable(CAP_FSETID). A simple reproducer that should work on any
> pre 5.12 kernel is:
> 
> brauner@wittgenstein|~/src/git/linux/ltp/testcases/kernel/syscalls/chown|master %=
> > unshare -U --map-root
> root@wittgenstein|~/src/git/linux/ltp/testcases/kernel/syscalls/chown|master %=
> > PATH=$PATH:$PWD ./chown02
> tst_memutils.c:157: TWARN: Can't adjust score, even with capabilities!?
> tst_test.c:1455: TINFO: Timeout per run is 0h 05m 00s
> chown02.c:45: TPASS: chown(testfile1, 0, 0) passed
> chown02.c:45: TPASS: chown(testfile2, 0, 0) passed
> chown02.c:57: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700
> 
> Summary:
> passed   2
> failed   1
> broken   0
> skipped  0
> warnings 1
> 
> There's no idmapped mounts here in play. The caller simply has been
> placed in a new user namespace and thus they fail the current
> capable(CAP_FSETID) check which will cause xfs to strip the sgid bit.
> 
> Now trying the same with ext4:
> 
> ubuntu@f2-vm:~/src/git/linux/ltp/testcases/kernel/syscalls/chown$ unshare -U --map-root
> root@f2-vm:~/src/git/linux/ltp/testcases/kernel/syscalls/chown# PATH=$PATH:$PWD ./chown02
> tst_memutils.c:157: TWARN: Can't adjust score, even with capabilities!?
> tst_test.c:1455: TINFO: Timeout per run is 0h 05m 00s
> chown02.c:45: TPASS: chown(testfile1, 0, 0) passed
> chown02.c:45: TPASS: chown(testfile2, 0, 0) passed
> 
> Summary:
> passed   2
> failed   0
> broken   0
> skipped  0
> warnings 1
> 
> it passes since ext4 uses setattr_copy() and thus the capability is
> checked for in the caller's user namespace.

Hmm.  The last person to touch the ATTR_MODE part of setattr_copy was
you, back in January 2021:

2f221d6f7b88 ("attr: handle idmapped mounts")

though I think that was merely switching the user_ns parameter to
inode_capable_wrt_uidgid.  The previous major change was made by Andy
Lutomirski in 2014:

23adbe12ef7d ("fs,userns: Change inode_capable to capable_wrt_inode_uidgid")

This seems to be the start(?) of the "_wrt_inode_uidgid" variants,
though I think the only real change in behavior was checking that the
inode's gid is mapped in the current userns?  Going back even further,
it looks like Eric Biederman started thsi whole process in 2012 with:

7fa294c8991c ("userns: Allow chown and setgid preservation")

This patch switched the VFS from purely checking process capabilities to
checking the privileges of the userns, I think.  From a purely code
inspection perspective, this is where the behavior divergence between
XFS and VFS began.

I'm ok with switching the fixes tag to 7fa294, though I won't be shocked
if this patch doesn't apply cleanly to fs/xfs/ from that era.

Thanks for the review!

<rant>
That said, I now have this bad habit of picking more recent commits for
Fixes because I get so much blowback from the stable maintainers it's
not worth any of the frustration.

I *do* still agree with the principle that a fixpatch should reference
the exact moment things went wrong, even if some autobackport bot can't
trivially apply the patch!

I fully expect to get complaints from the LTS maintainers like I always
do when I attach a Fixes tag without throughly compile-testing every LTS
branch between the tag and HEAD.  They clearly haven't taken any of my
hints, so I'll just say it: I DON'T HAVE TIME TO QA ANYTHING OTHER THAN
UPSTREAM!  Six LTS kernels (plus 3 distro kernels) will eat a week and a
half of time on the testing cloud, **per fix**.

Hell, you all have probably noticed: I haven't had sufficient mental
spoons to do much of anything with upstream, so there is no 5.18 merge
branch and reviews are getting sloppier.
</rant>

--D

> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> 
> Other than the wrong Fixes:,
> Reviewed-by: Christian Brauner <brauner@kernel.org>

Re: [PATCH 1/2] xfs: use setattr_copy to set vfs inode attributes

[Christian Brauner]

On Thu, Mar 10, 2022 at 09:34:50AM -0800, Darrick J. Wong wrote:
> On Thu, Mar 10, 2022 at 11:11:58AM +0100, Christian Brauner wrote:
> > On Wed, Mar 09, 2022 at 11:22:11AM -0800, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > 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: f736d93d76d3 ("xfs: support idmapped mounts")
> > 
> > Fwiw, as I've pointed out in
> > https://lore.kernel.org/linux-xfs/20220222122331.ijeapomur76h7xf6@wittgenstein/
> > the original analysis that this commit message links to in [2] is not
> > correct. But the thread clarifies it so I think it's fine.
> > 
> > But I think the fixes tag is wrong here afaict. As I've pointed out in
> > https://lore.kernel.org/linux-xfs/20220222123656.433l67bxhv3s2vbo@wittgenstein/
> > the faulty behavior should predate idmapped mounts by a lot. The bug is
> > with capable(CAP_FSETID). A simple reproducer that should work on any
> > pre 5.12 kernel is:
> > 
> > brauner@wittgenstein|~/src/git/linux/ltp/testcases/kernel/syscalls/chown|master %=
> > > unshare -U --map-root
> > root@wittgenstein|~/src/git/linux/ltp/testcases/kernel/syscalls/chown|master %=
> > > PATH=$PATH:$PWD ./chown02
> > tst_memutils.c:157: TWARN: Can't adjust score, even with capabilities!?
> > tst_test.c:1455: TINFO: Timeout per run is 0h 05m 00s
> > chown02.c:45: TPASS: chown(testfile1, 0, 0) passed
> > chown02.c:45: TPASS: chown(testfile2, 0, 0) passed
> > chown02.c:57: TFAIL: testfile2: wrong mode permissions 0100700, expected 0102700
> > 
> > Summary:
> > passed   2
> > failed   1
> > broken   0
> > skipped  0
> > warnings 1
> > 
> > There's no idmapped mounts here in play. The caller simply has been
> > placed in a new user namespace and thus they fail the current
> > capable(CAP_FSETID) check which will cause xfs to strip the sgid bit.
> > 
> > Now trying the same with ext4:
> > 
> > ubuntu@f2-vm:~/src/git/linux/ltp/testcases/kernel/syscalls/chown$ unshare -U --map-root
> > root@f2-vm:~/src/git/linux/ltp/testcases/kernel/syscalls/chown# PATH=$PATH:$PWD ./chown02
> > tst_memutils.c:157: TWARN: Can't adjust score, even with capabilities!?
> > tst_test.c:1455: TINFO: Timeout per run is 0h 05m 00s
> > chown02.c:45: TPASS: chown(testfile1, 0, 0) passed
> > chown02.c:45: TPASS: chown(testfile2, 0, 0) passed
> > 
> > Summary:
> > passed   2
> > failed   0
> > broken   0
> > skipped  0
> > warnings 1
> > 
> > it passes since ext4 uses setattr_copy() and thus the capability is
> > checked for in the caller's user namespace.
> 
> Hmm.  The last person to touch the ATTR_MODE part of setattr_copy was
> you, back in January 2021:
> 
> 2f221d6f7b88 ("attr: handle idmapped mounts")
> 
> though I think that was merely switching the user_ns parameter to
> inode_capable_wrt_uidgid.  The previous major change was made by Andy
> Lutomirski in 2014:
> 
> 23adbe12ef7d ("fs,userns: Change inode_capable to capable_wrt_inode_uidgid")
> 
> This seems to be the start(?) of the "_wrt_inode_uidgid" variants,
> though I think the only real change in behavior was checking that the
> inode's gid is mapped in the current userns?  Going back even further,
> it looks like Eric Biederman started thsi whole process in 2012 with:
> 
> 7fa294c8991c ("userns: Allow chown and setgid preservation")
> 
> This patch switched the VFS from purely checking process capabilities to
> checking the privileges of the userns, I think.  From a purely code
> inspection perspective, this is where the behavior divergence between
> XFS and VFS began.
> 
> I'm ok with switching the fixes tag to 7fa294, though I won't be shocked
> if this patch doesn't apply cleanly to fs/xfs/ from that era.

It very likely won't but from what I understand it would hopefully only
need to be backported to 5.<something>. But even then there's a good
chance it won't apply.

> 
> Thanks for the review!
> 
> <rant>
> That said, I now have this bad habit of picking more recent commits for
> Fixes because I get so much blowback from the stable maintainers it's
> not worth any of the frustration.
> 
> I *do* still agree with the principle that a fixpatch should reference
> the exact moment things went wrong, even if some autobackport bot can't
> trivially apply the patch!
> 
> I fully expect to get complaints from the LTS maintainers like I always
> do when I attach a Fixes tag without throughly compile-testing every LTS
> branch between the tag and HEAD.  They clearly haven't taken any of my

Oh, I didn't even know that that was expected. That's a rather time
intensive ask. I understand why that is frustrating! :(

> hints, so I'll just say it: I DON'T HAVE TIME TO QA ANYTHING OTHER THAN
> UPSTREAM!  Six LTS kernels (plus 3 distro kernels) will eat a week and a
> half of time on the testing cloud, **per fix**.
> 
> Hell, you all have probably noticed: I haven't had sufficient mental
> spoons to do much of anything with upstream, so there is no 5.18 merge
> branch and reviews are getting sloppier.
> </rant>