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

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

于 2020/9/2 11:09, Darrick J. Wong 写道:
> On Wed, Sep 02, 2020 at 10:41:11AM +0800, Xiao Yang wrote:
>> On 2020/9/2 0:35, Darrick J. Wong wrote:
>>> On Tue, Sep 01, 2020 at 02:05:53PM +0800, Xiao Yang wrote:
>>>> 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():
>>> I guess I wasn't clear enough about the xflags checks.
>>>
>>> Historically, XFS never checked the flags value for set bits that don't
>>> correspond to a known (X)FS_XFLAG_ value.  If your program passes in a
>>> set bit that the kernel doesn't know about, the kernel does nothing
>>> about it, and a subsequent FSGETXATTR will not have that bit set.
>> Hi Darrick,
>>
>> Yes, we have to confirm if XFS supports the specified xflag by both
>> FSSETXATTR and
>> FSGETXATT(instead of the single FSSETXATTR) so it is not clear and simple
>> for user.
>> This patch just makes ioctl(FSSETXATTR) return -EOPNOTSUPP when XFS doesn't
>> support
>> the specified xflag.
>> Note: ext4/f2fs/btrfs have implemented the behavior.
>>
>> BTW:
>> With this patch, current '_require_xfs_io_command "chattr"' in xfstests can
>> check XFS's
>> supported xflags directly and don't need to check them by extra
>> ioctl(FSGETXATTR).
>>> The old ioctl (back when it was xfs only) wasn't officially documented,
>>> so it wasn't clear whether the kernel should do that or return EINVAL.
>>>
>>> Then the ioctl pair was hoisted to the VFS, a manpage was written
>>> specifying an EINVAL return for invalid arguments, and ext4, f2fs, and
>>> btrfs followed this.
>>>
>>> FS_XFLAG_ALL is the set of all defined FS_XFLAG_* values.  Therefore,
>>> the VFS needs to check that userspace does not try to pass in a flags
>>> value with totally unknown bits set in it.  That's what I thought you
>>> were trying to do with this patch.
>>>
>>> Since you bring it up, however -- ext4/f2fs/btrfs support only a subset
>>> of the (X)FS_XFLAG values, so they implement a second check to constrain
>>> the flags values to the ones that those filesystems support.  I doubt
>>> that the set of flags that XFS supports will stay the same as the set of
>>> flags that the VFS header establishes, so it would be wise to implement
>>> a second check in XFS, even if right now it provides no added benefit
>>> over the VFS check.
>> This patch just tries to implement the second check in XFS.
>>
>> Different filesystems(ext4/f2fs/btrfs/xfs) support different xflags so the
>> check depends
>> on these filesystems instead of vfs.  I am not sure why we need to implement
>> the first
>> check?(I think the first check seems surplus)

Hi Darrick,

Do you agree this point that only implements the second check in XFS? :-)

>>> IOWs, I'm suggesting that you write one patch to define a FS_XFLAG_ALL
>>> consisting of all known FS_XFLAG_* values, and a check in
>>> vfs_ioc_fssetxattr_check that uses that to establish basic sanity of the
>>> arguments; and a second patch to define a XFS_XFLAG_ALL consisting of
>>> all the flags that XFS supports, and a check in xfs_ioctl_setattr that
>>> uses XFS_XFLAG_ALL to establish that we're not passing in an XFLAG that
>>> XFS doesn't support.
>> How about the following patch(i.e. add a check in xfs_ioctl_setattr):
>> -----------------------------------------------------------
>> diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
>> index 84bcffa87753..8ac19f55c701 100644
>> --- a/fs/xfs/libxfs/xfs_fs.h
>> +++ b/fs/xfs/libxfs/xfs_fs.h
>> @@ -92,6 +92,14 @@ struct getbmapx {
>>   #define XFS_FMR_OWN_COW                FMR_OWNER('X', 7) /* cow staging */
>>   #define XFS_FMR_OWN_DEFECTIVE  FMR_OWNER('X', 8) /* bad blocks */
>>
>> +#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)
> This is an implementation detail, so you might as well put it right
> above xfs_ioctl_setattr.
>
> That and xfs_fs.h gets packaged in /usr/include so we don't want to have
> to support that symbol for userspace programs forever.

Will put the macro above xfs_ioctl_setattr, as below:
-------------------------------------------------------------
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 6f22a66777cd..e188e81961bd 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;
+
code = xfs_ioctl_setattr_check_projid(ip, fa);
if (code)
return code;
-------------------------------------------------------------

Best Regards,
Xiao Yang
> --D
>
>> +
>>   /*
>>    * Structure for XFS_IOC_FSSETDM.
>>    * For use by backup and restore programs to set the XFS on-disk inode
>> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
>> index 6f22a66777cd..ec5feaa8dec8 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&  ~XFS_SUPPORTED_FS_XFLAGS)
>> +                return -EOPNOTSUPP;
>> +
>>          code = xfs_ioctl_setattr_check_projid(ip, fa);
>>          if (code)
>>                  return code;
>> -----------------------------------------------------------
>>
>> Best Regards,
>> Xiao Yang
>>> --D
>>>
>>>> -------------------------------------------------------------------------------
>>>> 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
>>>>>>
>>>>>>
>>>>>>
>>>>> .
>>>>>
>>>>
>>> .
>>>
>>
>>
>
> .
>