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

Xiao Yang
2020-09-02T02:41:25Z

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)
> 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)
+
/*
* 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
>>>>
>>>>
>>>>
>>> .
>>>
>>
>>
>
> .
>