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

Xiao Yang
2020-09-01T06:06:10Z

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
>>
>>
>>
>
> .
>