* [PATCH v3] xfs: Add check for unsupported xflags
@ 2020-09-03 3:57 Xiao Yang
2020-09-03 7:46 ` Dave Chinner
0 siblings, 1 reply; 5+ messages in thread
From: Xiao Yang @ 2020-09-03 3:57 UTC (permalink / raw)
To: linux-xfs; +Cc: darrick.wong, ira.weiny, Xiao Yang
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;
+
code = xfs_ioctl_setattr_check_projid(ip, fa);
if (code)
return code;
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v3] xfs: Add check for unsupported xflags
2020-09-03 3:57 [PATCH v3] xfs: Add check for unsupported xflags Xiao Yang
@ 2020-09-03 7:46 ` Dave Chinner
2020-09-04 1:14 ` Xiao Yang
0 siblings, 1 reply; 5+ messages in thread
From: Dave Chinner @ 2020-09-03 7:46 UTC (permalink / raw)
To: Xiao Yang; +Cc: linux-xfs, darrick.wong, ira.weiny
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.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] xfs: Add check for unsupported xflags
2020-09-03 7:46 ` Dave Chinner
@ 2020-09-04 1:14 ` Xiao Yang
2020-09-06 23:01 ` Dave Chinner
0 siblings, 1 reply; 5+ messages in thread
From: Xiao Yang @ 2020-09-04 1:14 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs, darrick.wong, ira.weiny
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] xfs: Add check for unsupported xflags
2020-09-04 1:14 ` Xiao Yang
@ 2020-09-06 23:01 ` Dave Chinner
2020-09-08 7:49 ` Xiao Yang
0 siblings, 1 reply; 5+ messages in thread
From: Dave Chinner @ 2020-09-06 23:01 UTC (permalink / raw)
To: Xiao Yang; +Cc: linux-xfs, darrick.wong, ira.weiny
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.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] xfs: Add check for unsupported xflags
2020-09-06 23:01 ` Dave Chinner
@ 2020-09-08 7:49 ` Xiao Yang
0 siblings, 0 replies; 5+ messages in thread
From: Xiao Yang @ 2020-09-08 7:49 UTC (permalink / raw)
To: Dave Chinner, darrick.wong; +Cc: linux-xfs, ira.weiny
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-09-08 7:49 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-09-03 3:57 [PATCH v3] xfs: Add check for unsupported xflags Xiao Yang
2020-09-03 7:46 ` Dave Chinner
2020-09-04 1:14 ` Xiao Yang
2020-09-06 23:01 ` Dave Chinner
2020-09-08 7:49 ` Xiao Yang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox