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

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

On 2020/9/1 1:22, Darrick J. Wong wrote:
> On Mon, Aug 31, 2020 at 09:37:45PM +0800, Xiao Yang wrote:
>> Current ioctl(FSSETXATTR) ignores unsupported xflags silently
>> so it it not clear for user to know unsupported xflags.
Hi Darrick,

Sorry for a typo(s/it it/it is/).
>> 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 report unsupported info as ioctl(SETXFLAGS) does.
>>
>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com>
>> ---
>>   fs/xfs/xfs_ioctl.c      | 4 ++++
>>   include/uapi/linux/fs.h | 8 ++++++++
>>   2 files changed, 12 insertions(+)
>>
>> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
>> index 6f22a66777cd..cfe7f20c94fe 100644
>> --- a/fs/xfs/xfs_ioctl.c
>> +++ b/fs/xfs/xfs_ioctl.c
>> @@ -1439,6 +1439,10 @@ xfs_ioctl_setattr(
>>
>>   	trace_xfs_ioctl_setattr(ip);
>>
>> +	/* Check if fsx_xflags have unsupported xflags */
>> +	if (fa->fsx_xflags&  ~FS_XFLAG_ALL)
>> +                return -EOPNOTSUPP;
> Shouldn't this be in vfs_ioc_fssetxattr_check, since we're checking
> against all the vfs defined XFLAGS?
Right, different filesystems support different XFLAGS so I think it is 
hard to put this
check into vfs_ioc_fssetxattr_check().  For example,
1) ext4 defines EXT4_SUPPORTED_FS_XFLAGS and do the check before
vfs_ioc_fssetxattr_check():
-------------------------------------------------------------------------------
ext4/ioctl.c:
#define EXT4_SUPPORTED_FS_XFLAGS (FS_XFLAG_SYNC | FS_XFLAG_IMMUTABLE | \
                                   FS_XFLAG_APPEND | FS_XFLAG_NODUMP | \
                                   FS_XFLAG_NOATIME | 
FS_XFLAG_PROJINHERIT | \
                                   FS_XFLAG_DAX)
...
                 if (fa.fsx_xflags & ~EXT4_SUPPORTED_FS_XFLAGS)
                         return -EOPNOTSUPP;
...
-------------------------------------------------------------------------------
2) btrfs adds check_xflags() and calls it before vfs_ioc_fssetxattr_check():
-------------------------------------------------------------------------------
btrfs/ioctl.c:
static int check_xflags(unsigned int flags)
{
         if (flags & ~(FS_XFLAG_APPEND | FS_XFLAG_IMMUTABLE | 
FS_XFLAG_NOATIME |
                       FS_XFLAG_NODUMP | FS_XFLAG_SYNC))
                 return -EOPNOTSUPP;
         return 0;
}
...
         ret = check_xflags(fa.fsx_xflags);
         if (ret)
                 return ret;
...
-------------------------------------------------------------------------------

Perhaps, I should rename FS_XFLAG_ALL to XFS_SUPPORTED_FS_XFLAGS and move
it into libxfs/xfs_fs.h.

Best Regards,
Xiao Yang
> --D
>
>> +
>>   	code = xfs_ioctl_setattr_check_projid(ip, fa);
>>   	if (code)
>>   		return code;
>> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
>> index f44eb0a04afd..31b6856f6877 100644
>> --- a/include/uapi/linux/fs.h
>> +++ b/include/uapi/linux/fs.h
>> @@ -142,6 +142,14 @@ struct fsxattr {
>>   #define FS_XFLAG_COWEXTSIZE	0x00010000	/* CoW extent size allocator hint */
>>   #define FS_XFLAG_HASATTR	0x80000000	/* no DIFLAG for this	*/
>>
>> +#define FS_XFLAG_ALL \
>> +	(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)
>> +
>>   /* the read-only stuff doesn't really belong here, but any other place is
>>      probably as bad and I don't want to create yet another include file. */
>>
>> -- 
>> 2.25.1
>>
>>
>>
>
> .
>