On 2020/9/7 7:01, Dave Chinner wrote:
On Fri, Sep 04, 2020 at 09:14:25AM +0800, Xiao Yang wrote:
On 2020/9/3 15:46, Dave Chinner wrote:
On Thu, Sep 03, 2020 at 11:57:13AM +0800, Xiao Yang wrote:
Current ioctl(FSSETXATTR) ignores unsupported xflags silently
so it is not clear for user to know unsupported xflags.
For example, use ioctl(FSSETXATTR) to set dax flag on kernel
v4.4 which doesn't support dax flag:
--------------------------------
# xfs_io -f -c "chattr +x" testfile;echo $?
0
# xfs_io -c "lsattr" testfile
----------------X testfile
--------------------------------
Add check to return -EOPNOTSUPP as ext4/f2fs/btrfs does.
Signed-off-by: Xiao Yang<yangx.jy@xxxxxxxxxxxxxx>
Reviewed-by: Darrick J. Wong<darrick.wong@xxxxxxxxxx>
---
fs/xfs/xfs_ioctl.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 6f22a66777cd..59f9a86f29f7 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1425,6 +1425,14 @@ xfs_ioctl_setattr_check_projid(
return 0;
}
+#define XFS_SUPPORTED_FS_XFLAGS \
+ (FS_XFLAG_REALTIME | FS_XFLAG_PREALLOC | FS_XFLAG_IMMUTABLE | \
+ FS_XFLAG_APPEND | FS_XFLAG_SYNC | FS_XFLAG_NOATIME | FS_XFLAG_NODUMP | \
+ FS_XFLAG_RTINHERIT | FS_XFLAG_PROJINHERIT | FS_XFLAG_NOSYMLINKS | \
+ FS_XFLAG_EXTSIZE | FS_XFLAG_EXTSZINHERIT | FS_XFLAG_NODEFRAG | \
+ FS_XFLAG_FILESTREAM | FS_XFLAG_DAX | FS_XFLAG_COWEXTSIZE | \
+ FS_XFLAG_HASATTR)
+
STATIC int
xfs_ioctl_setattr(
xfs_inode_t *ip,
@@ -1439,6 +1447,10 @@ xfs_ioctl_setattr(
trace_xfs_ioctl_setattr(ip);
+ /* Check if fsx_xflags has unsupported xflags */
+ if (fa->fsx_xflags& ~XFS_SUPPORTED_FS_XFLAGS)
+ return -EOPNOTSUPP;
I don't think we can do this as it may break existing applications
that have been working on XFS for many, many years that don't
correctly initialise fsx_xflags....
Hi Dave,
It seems that the only way is to keep the current behavior. :-(
Yes, unfortunately that is the case, but it does follow precedence
set by other syscalls with unchecked flags such as open() - they
mask off unknown flags so they don't do anything, but they do not
return an error if any unknown flag is set.
By the way, _require_xfs_io_command "chattr" in xfstests cannot check XFS's
unsupported xflags directly because of the behavior, so we may need to check
them by extra xfs_io -c "lsattr".
*nod*
Hi Darrick, Dave
I had another confusion when trying to add extra xfs_io -c "lsattr" in
xfstests:
--------------------------------------------------
# xfs_io -f -c "chattr +tPn" file
# xfs_io -f -c "lsattr" file
----------------X file
# xfs_io -f -c "chattr +E" file
xfs_io: cannot set flags on file: Invalid argument
--------------------------------------------------
These four flags are invalid for a regular file , but kernel maskes off
three flags and returns EINVAL for 'E' flag.
kernel can mask off all four flags for a regular file, so is it
necessary for 'E' flag to return EINVAL?
fs/xfs/xfs_ioctl.c:
-------------------------------------------------
1160 if (S_ISDIR(VFS_I(ip)->i_mode)) {
1161 if (xflags & FS_XFLAG_RTINHERIT)
1162 di_flags |= XFS_DIFLAG_RTINHERIT;
1163 if (xflags & FS_XFLAG_NOSYMLINKS)
1164 di_flags |= XFS_DIFLAG_NOSYMLINKS;
1165 if (xflags & FS_XFLAG_EXTSZINHERIT)
1166 di_flags |= XFS_DIFLAG_EXTSZINHERIT;
1167 if (xflags & FS_XFLAG_PROJINHERIT)
1168 di_flags |= XFS_DIFLAG_PROJINHERIT;
-------------------------------------------------
fs/inode.c:
-------------------------------------------------
2356 if ((fa->fsx_xflags & FS_XFLAG_EXTSZINHERIT) &&
2357 !S_ISDIR(inode->i_mode))
2358 return -EINVAL;
-------------------------------------------------
I think the behavior of chattr command seems inconsistent/messy.
Best Regards,
Xiao Yang
Cheers,
Dave.
