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

["Darrick J. Wong" <darrick.wong@oracle.com>]

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
> > > 
> > > 
> > > 
> > 
> > .
> > 
> 
> 
>