diff mbox series

[v3] xfs: Add check for unsupported xflags

Message ID 20200903035713.60962-1-yangx.jy@cn.fujitsu.com (mailing list archive)
State New
Headers show
Series [v3] xfs: Add check for unsupported xflags | expand

Commit Message

Xiao Yang Sept. 3, 2020, 3:57 a.m. UTC
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(+)

Comments

Dave Chinner Sept. 3, 2020, 7:46 a.m. UTC | #1
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.
Xiao Yang Sept. 4, 2020, 1:14 a.m. UTC | #2
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.
Dave Chinner Sept. 6, 2020, 11:01 p.m. UTC | #3
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.
Xiao Yang Sept. 8, 2020, 7:49 a.m. UTC | #4
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 mbox series

Patch

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;