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@xxxxxxxxxxxxxx/
[2] https://lore.kernel.org/linux-xfs/20220221182218.748084-1-andrey.zhadchenko@xxxxxxxxxxxxx/
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
