* [PATCH] xfs: Add check for unsupported xflags @ 2020-08-31 13:37 Xiao Yang 2020-08-31 17:22 ` Darrick J. Wong 0 siblings, 1 reply; 13+ messages in thread From: Xiao Yang @ 2020-08-31 13:37 UTC (permalink / raw) To: linux-xfs; +Cc: darrick.wong, ira.weiny, Xiao Yang Current ioctl(FSSETXATTR) ignores unsupported xflags silently so it it not clear for user to know unsupported xflags. 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; + 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] xfs: Add check for unsupported xflags 2020-08-31 13:37 [PATCH] xfs: Add check for unsupported xflags Xiao Yang @ 2020-08-31 17:22 ` Darrick J. Wong 2020-09-01 6:05 ` Xiao Yang 0 siblings, 1 reply; 13+ messages in thread From: Darrick J. Wong @ 2020-08-31 17:22 UTC (permalink / raw) To: Xiao Yang; +Cc: linux-xfs, ira.weiny 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. > 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? --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 > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] xfs: Add check for unsupported xflags 2020-08-31 17:22 ` Darrick J. Wong @ 2020-09-01 6:05 ` Xiao Yang 2020-09-01 16:35 ` Darrick J. Wong 0 siblings, 1 reply; 13+ messages in thread From: Xiao Yang @ 2020-09-01 6:05 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs, ira.weiny 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 >> >> >> > > . > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] xfs: Add check for unsupported xflags 2020-09-01 6:05 ` Xiao Yang @ 2020-09-01 16:35 ` Darrick J. Wong 2020-09-02 2:41 ` Xiao Yang 0 siblings, 1 reply; 13+ messages in thread From: Darrick J. Wong @ 2020-09-01 16:35 UTC (permalink / raw) To: Xiao Yang; +Cc: linux-xfs, ira.weiny 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. 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. 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. --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 > > > > > > > > > > > > > . > > > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] xfs: Add check for unsupported xflags 2020-09-01 16:35 ` Darrick J. Wong @ 2020-09-02 2:41 ` Xiao Yang 2020-09-02 3:09 ` Darrick J. Wong 0 siblings, 1 reply; 13+ messages in thread From: Xiao Yang @ 2020-09-02 2:41 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs, ira.weiny 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 >>>> >>>> >>>> >>> . >>> >> >> > > . > ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] xfs: Add check for unsupported xflags 2020-09-02 2:41 ` Xiao Yang @ 2020-09-02 3:09 ` Darrick J. Wong 2020-09-02 3:34 ` Xiao Yang 0 siblings, 1 reply; 13+ messages in thread From: Darrick J. Wong @ 2020-09-02 3:09 UTC (permalink / raw) To: Xiao Yang; +Cc: linux-xfs, ira.weiny 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) > > 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. --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 > > > > > > > > > > > > > > > > > > > . > > > > > > > > > > > > > > . > > > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] xfs: Add check for unsupported xflags 2020-09-02 3:09 ` Darrick J. Wong @ 2020-09-02 3:34 ` Xiao Yang 2020-09-02 4:10 ` Darrick J. Wong 0 siblings, 1 reply; 13+ messages in thread From: Xiao Yang @ 2020-09-02 3:34 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs, ira.weiny 于 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 >>>>>> >>>>>> >>>>>> >>>>> . >>>>> >>>> >>> . >>> >> >> > > . > ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] xfs: Add check for unsupported xflags 2020-09-02 3:34 ` Xiao Yang @ 2020-09-02 4:10 ` Darrick J. Wong 2020-09-02 5:11 ` Xiao Yang 0 siblings, 1 reply; 13+ messages in thread From: Darrick J. Wong @ 2020-09-02 4:10 UTC (permalink / raw) To: Xiao Yang; +Cc: linux-xfs, ira.weiny 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. (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 > > > > > > > > > > > > > > > > > > > > > > > > > > > . > > > > > > > > > > > > > > > . > > > > > > > > > > > > > > . > > > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] xfs: Add check for unsupported xflags 2020-09-02 4:10 ` Darrick J. Wong @ 2020-09-02 5:11 ` Xiao Yang 2020-09-02 17:03 ` Darrick J. Wong 0 siblings, 1 reply; 13+ messages in thread From: Xiao Yang @ 2020-09-02 5:11 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs, ira.weiny 于 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 >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> . >>>>>>> >>>>> . >>>>> >>>> >>> . >>> >> >> > > . > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] xfs: Add check for unsupported xflags 2020-09-02 5:11 ` Xiao Yang @ 2020-09-02 17:03 ` Darrick J. Wong 2020-09-02 17:38 ` Ira Weiny 0 siblings, 1 reply; 13+ messages in thread From: Darrick J. Wong @ 2020-09-02 17:03 UTC (permalink / raw) To: Xiao Yang; +Cc: linux-xfs, ira.weiny On Wed, Sep 02, 2020 at 01:11:00PM +0800, Xiao Yang wrote: > 于 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. :-) I was advocating for two levels of flags checks: one in the VFS for undefined flags, and a second check in each filesystem for whichever flag it wants to recognize. I was not implying that the VFS checks would be sufficient on their own. --D > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > . > > > > > > > > > > > > > > . > > > > > > > > > > > > > > > . > > > > > > > > > > > > > > . > > > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] xfs: Add check for unsupported xflags 2020-09-02 17:03 ` Darrick J. Wong @ 2020-09-02 17:38 ` Ira Weiny 2020-09-02 17:45 ` Darrick J. Wong 0 siblings, 1 reply; 13+ messages in thread From: Ira Weiny @ 2020-09-02 17:38 UTC (permalink / raw) To: Darrick J. Wong; +Cc: Xiao Yang, linux-xfs On Wed, Sep 02, 2020 at 10:03:26AM -0700, Darrick J. Wong wrote: > On Wed, Sep 02, 2020 at 01:11:00PM +0800, Xiao Yang wrote: > > 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. :-) > > I was advocating for two levels of flags checks: one in the VFS for > undefined flags, and a second check in each filesystem for whichever > flag it wants to recognize. I was not implying that the VFS checks > would be sufficient on their own. > I've not really followed this thread completely but wouldn't this proposed check in the VFS layer be redundant because the set of flags the filesystem accepts should always be a strict subset of the VFS flags? Ira ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] xfs: Add check for unsupported xflags 2020-09-02 17:38 ` Ira Weiny @ 2020-09-02 17:45 ` Darrick J. Wong 2020-09-03 2:58 ` Xiao Yang 0 siblings, 1 reply; 13+ messages in thread From: Darrick J. Wong @ 2020-09-02 17:45 UTC (permalink / raw) To: Ira Weiny; +Cc: Xiao Yang, linux-xfs On Wed, Sep 02, 2020 at 10:38:28AM -0700, Ira Weiny wrote: > On Wed, Sep 02, 2020 at 10:03:26AM -0700, Darrick J. Wong wrote: > > On Wed, Sep 02, 2020 at 01:11:00PM +0800, Xiao Yang wrote: > > > 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. :-) > > > > I was advocating for two levels of flags checks: one in the VFS for > > undefined flags, and a second check in each filesystem for whichever > > flag it wants to recognize. I was not implying that the VFS checks > > would be sufficient on their own. > > > > I've not really followed this thread completely but wouldn't this proposed > check in the VFS layer be redundant because the set of flags the filesystem > accepts should always be a strict subset of the VFS flags? Yes. It's 100% CYA. I wouldn't be that bent out of shape if the vfs part never happens, but as we already have a vfs argument checker function in addition to the per-fs validation I don't see why we would leave a gap... ;) --D > Ira ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] xfs: Add check for unsupported xflags 2020-09-02 17:45 ` Darrick J. Wong @ 2020-09-03 2:58 ` Xiao Yang 0 siblings, 0 replies; 13+ messages in thread From: Xiao Yang @ 2020-09-03 2:58 UTC (permalink / raw) To: Darrick J. Wong, Ira Weiny; +Cc: linux-xfs On 2020/9/3 1:45, Darrick J. Wong wrote: > On Wed, Sep 02, 2020 at 10:38:28AM -0700, Ira Weiny wrote: >> On Wed, Sep 02, 2020 at 10:03:26AM -0700, Darrick J. Wong wrote: >>> On Wed, Sep 02, 2020 at 01:11:00PM +0800, Xiao Yang wrote: >>>> 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. :-) >>> I was advocating for two levels of flags checks: one in the VFS for >>> undefined flags, and a second check in each filesystem for whichever >>> flag it wants to recognize. I was not implying that the VFS checks >>> would be sufficient on their own. >>> >> I've not really followed this thread completely but wouldn't this proposed >> check in the VFS layer be redundant because the set of flags the filesystem >> accepts should always be a strict subset of the VFS flags? Hi, I also think this check in the VFS is redundant. :-) > Yes. It's 100% CYA. I wouldn't be that bent out of shape if the vfs > part never happens, but as we already have a vfs argument checker > function in addition to the per-fs validation I don't see why we would > leave a gap... ;) After looking at vfs_ioc_fssetxattr_check(), why do we need to move the check of extent size hint to vfs? It seems a xfs-specific flag, right? btw: It is fine to move DAX and project id to vfs because they are supported by more than one filesystem(e.g. ext4 and xfs). Best Regards, Xiao Yang > --D > >> Ira > > . > ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-09-03 2:58 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-08-31 13:37 [PATCH] xfs: Add check for unsupported xflags Xiao Yang 2020-08-31 17:22 ` Darrick J. Wong 2020-09-01 6:05 ` Xiao Yang 2020-09-01 16:35 ` Darrick J. Wong 2020-09-02 2:41 ` Xiao Yang 2020-09-02 3:09 ` Darrick J. Wong 2020-09-02 3:34 ` Xiao Yang 2020-09-02 4:10 ` Darrick J. Wong 2020-09-02 5:11 ` Xiao Yang 2020-09-02 17:03 ` Darrick J. Wong 2020-09-02 17:38 ` Ira Weiny 2020-09-02 17:45 ` Darrick J. Wong 2020-09-03 2:58 ` Xiao Yang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).