Commit e014f37d authored by Darrick J. Wong's avatar Darrick J. Wong
Browse files

xfs: use setattr_copy to set vfs inode attributes 

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: 7fa294c8 ("userns: Allow chown and setgid preservation")
Signed-off-by: default avatarDarrick J. Wong <djwong@kernel.org>
Reviewed-by: default avatarDave Chinner <dchinner@redhat.com>
Reviewed-by: default avatarChristoph Hellwig <hch@lst.de>
Reviewed-by: default avatarChristian Brauner <brauner@kernel.org>
parent eba0549b

fs/xfs/xfs_iops.c

+3 −53
Original line number Diff line number Diff line
@@ -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);

fs/xfs/xfs_pnfs.c

+2 −1
Original line number Diff line number Diff line
@@ -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;