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

[Dave Chinner <david@fromorbit.com>]

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