| Message ID | 20200903035713.60962-1-yangx.jy@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] xfs: Add check for unsupported xflags | expand |
On Thu, Sep 03, 2020 at 11:57:13AM +0800, Xiao Yang wrote: > Current ioctl(FSSETXATTR) ignores unsupported xflags silently > so it is 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 return -EOPNOTSUPP as ext4/f2fs/btrfs does. > > Signed-off-by: Xiao Yang <yangx.jy@cn.fujitsu.com> > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > fs/xfs/xfs_ioctl.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > index 6f22a66777cd..59f9a86f29f7 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; I don't think we can do this as it may break existing applications that have been working on XFS for many, many years that don't correctly initialise fsx_xflags.... Cheers, Dave.
On 2020/9/3 15:46, Dave Chinner wrote: > On Thu, Sep 03, 2020 at 11:57:13AM +0800, Xiao Yang wrote: >> Current ioctl(FSSETXATTR) ignores unsupported xflags silently >> so it is 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 return -EOPNOTSUPP as ext4/f2fs/btrfs does. >> >> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com> >> Reviewed-by: Darrick J. Wong<darrick.wong@oracle.com> >> --- >> fs/xfs/xfs_ioctl.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c >> index 6f22a66777cd..59f9a86f29f7 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; > I don't think we can do this as it may break existing applications > that have been working on XFS for many, many years that don't > correctly initialise fsx_xflags.... Hi Dave, It seems that the only way is to keep the current behavior. :-( By the way, _require_xfs_io_command "chattr" in xfstests cannot check XFS's unsupported xflags directly because of the behavior, so we may need to check them by extra xfs_io -c "lsattr". Best Regards, Xiao Yang > Cheers, > > Dave.
On Fri, Sep 04, 2020 at 09:14:25AM +0800, Xiao Yang wrote: > On 2020/9/3 15:46, Dave Chinner wrote: > > On Thu, Sep 03, 2020 at 11:57:13AM +0800, Xiao Yang wrote: > > > Current ioctl(FSSETXATTR) ignores unsupported xflags silently > > > so it is 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 return -EOPNOTSUPP as ext4/f2fs/btrfs does. > > > > > > Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com> > > > Reviewed-by: Darrick J. Wong<darrick.wong@oracle.com> > > > --- > > > fs/xfs/xfs_ioctl.c | 12 ++++++++++++ > > > 1 file changed, 12 insertions(+) > > > > > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c > > > index 6f22a66777cd..59f9a86f29f7 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; > > I don't think we can do this as it may break existing applications > > that have been working on XFS for many, many years that don't > > correctly initialise fsx_xflags.... > Hi Dave, > > It seems that the only way is to keep the current behavior. :-( Yes, unfortunately that is the case, but it does follow precedence set by other syscalls with unchecked flags such as open() - they mask off unknown flags so they don't do anything, but they do not return an error if any unknown flag is set. > By the way, _require_xfs_io_command "chattr" in xfstests cannot check XFS's > unsupported xflags directly because of the behavior, so we may need to check > them by extra xfs_io -c "lsattr". *nod* Cheers, Dave.
On 2020/9/7 7:01, Dave Chinner wrote: > On Fri, Sep 04, 2020 at 09:14:25AM +0800, Xiao Yang wrote: >> On 2020/9/3 15:46, Dave Chinner wrote: >>> On Thu, Sep 03, 2020 at 11:57:13AM +0800, Xiao Yang wrote: >>>> Current ioctl(FSSETXATTR) ignores unsupported xflags silently >>>> so it is 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 return -EOPNOTSUPP as ext4/f2fs/btrfs does. >>>> >>>> Signed-off-by: Xiao Yang<yangx.jy@cn.fujitsu.com> >>>> Reviewed-by: Darrick J. Wong<darrick.wong@oracle.com> >>>> --- >>>> fs/xfs/xfs_ioctl.c | 12 ++++++++++++ >>>> 1 file changed, 12 insertions(+) >>>> >>>> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c >>>> index 6f22a66777cd..59f9a86f29f7 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; >>> I don't think we can do this as it may break existing applications >>> that have been working on XFS for many, many years that don't >>> correctly initialise fsx_xflags.... >> Hi Dave, >> >> It seems that the only way is to keep the current behavior. :-( > Yes, unfortunately that is the case, but it does follow precedence > set by other syscalls with unchecked flags such as open() - they > mask off unknown flags so they don't do anything, but they do not > return an error if any unknown flag is set. > >> By the way, _require_xfs_io_command "chattr" in xfstests cannot check XFS's >> unsupported xflags directly because of the behavior, so we may need to check >> them by extra xfs_io -c "lsattr". > *nod* Hi Darrick, Dave I had another confusion when trying to add extra xfs_io -c "lsattr" in xfstests: -------------------------------------------------- # xfs_io -f -c "chattr +tPn" file # xfs_io -f -c "lsattr" file ----------------X file # xfs_io -f -c "chattr +E" file xfs_io: cannot set flags on file: Invalid argument -------------------------------------------------- These four flags are invalid for a regular file , but kernel maskes off three flags and returns EINVAL for 'E' flag. kernel can mask off all four flags for a regular file, so is it necessary for 'E' flag to return EINVAL? fs/xfs/xfs_ioctl.c: ------------------------------------------------- 1160 if (S_ISDIR(VFS_I(ip)->i_mode)) { 1161 if (xflags & FS_XFLAG_RTINHERIT) 1162 di_flags |= XFS_DIFLAG_RTINHERIT; 1163 if (xflags & FS_XFLAG_NOSYMLINKS) 1164 di_flags |= XFS_DIFLAG_NOSYMLINKS; 1165 if (xflags & FS_XFLAG_EXTSZINHERIT) 1166 di_flags |= XFS_DIFLAG_EXTSZINHERIT; 1167 if (xflags & FS_XFLAG_PROJINHERIT) 1168 di_flags |= XFS_DIFLAG_PROJINHERIT; ------------------------------------------------- fs/inode.c: ------------------------------------------------- 2356 if ((fa->fsx_xflags & FS_XFLAG_EXTSZINHERIT) && 2357 !S_ISDIR(inode->i_mode)) 2358 return -EINVAL; ------------------------------------------------- I think the behavior of chattr command seems inconsistent/messy. Best Regards, Xiao Yang > Cheers, > > Dave.
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c index 6f22a66777cd..59f9a86f29f7 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;