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

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

于 2020/9/2 12:10, Darrick J. Wong 写道:
> On Wed, Sep 02, 2020 at 11:34:58AM +0800, Xiao Yang wrote:
>> 于 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)
> Oops, sorry, I didn't notice this.
>
> The reason for checking both in the VFS and in the fs driver itself is
> to ensure that there's at least some checking of the syscall inputs even
> if a new fs implementation neglects to check the flags.
Hi Darrick,

It is reasonable for your concern to add a check in VFS, but checking all
defined xflags is too rough in VFS if one filesystem only supports few 
xflags. :-)

Best Regards,
Xiao Yang
> (Or the original implementation<cough>.)
>
>> Hi Darrick,
>>
>> Do you agree this point that only implements the second check in XFS? :-)
> Yes.
>
>>>>> 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
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>> .
>>>>>>>
>>>>> .
>>>>>
>>>>
>>> .
>>>
>>
>>
>
> .
>