Re: [PATCH v3] xfs: Add check for unsupported xflags

[Xiao Yang <yangx.jy@cn.fujitsu.com>]

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@cn.fujitsu.com>
>>>> Reviewed-by: Darrick J. Wong<darrick.wong@oracle.com>
>>>> ---
>>>>    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.