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

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

On Wed, Sep 02, 2020 at 11:34:58AM +0800, Xiao Yang wrote:
> 于 2020/9/2 11:09, Darrick J. Wong 写道:
> > On Wed, Sep 02, 2020 at 10:41:11AM +0800, Xiao Yang wrote:
> > > On 2020/9/2 0:35, Darrick J. Wong wrote:
> > > > 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.
> > > Hi Darrick,
> > > 
> > > Yes, we have to confirm if XFS supports the specified xflag by both
> > > FSSETXATTR and
> > > FSGETXATT(instead of the single FSSETXATTR) so it is not clear and simple
> > > for user.
> > > This patch just makes ioctl(FSSETXATTR) return -EOPNOTSUPP when XFS doesn't
> > > support
> > > the specified xflag.
> > > Note: ext4/f2fs/btrfs have implemented the behavior.
> > > 
> > > BTW:
> > > With this patch, current '_require_xfs_io_command "chattr"' in xfstests can
> > > check XFS's
> > > supported xflags directly and don't need to check them by extra
> > > ioctl(FSGETXATTR).
> > > > 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.
> > > This patch just tries to implement the second check in XFS.
> > > 
> > > Different filesystems(ext4/f2fs/btrfs/xfs) support different xflags so the
> > > check depends
> > > on these filesystems instead of vfs.  I am not sure why we need to implement
> > > the first
> > > check?(I think the first check seems surplus)

Oops, sorry, I didn't notice this.

The reason for checking both in the VFS and in the fs driver itself is
to ensure that there's at least some checking of the syscall inputs even
if a new fs implementation neglects to check the flags.

(Or the original implementation <cough>.)

> Hi Darrick,
> 
> Do you agree this point that only implements the second check in XFS? :-)

Yes.

> > > > 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.
> > > How about the following patch(i.e. add a check in xfs_ioctl_setattr):
> > > -----------------------------------------------------------
> > > diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> > > index 84bcffa87753..8ac19f55c701 100644
> > > --- a/fs/xfs/libxfs/xfs_fs.h
> > > +++ b/fs/xfs/libxfs/xfs_fs.h
> > > @@ -92,6 +92,14 @@ struct getbmapx {
> > >   #define XFS_FMR_OWN_COW                FMR_OWNER('X', 7) /* cow staging */
> > >   #define XFS_FMR_OWN_DEFECTIVE  FMR_OWNER('X', 8) /* bad blocks */
> > > 
> > > +#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)
> > This is an implementation detail, so you might as well put it right
> > above xfs_ioctl_setattr.
> > 
> > That and xfs_fs.h gets packaged in /usr/include so we don't want to have
> > to support that symbol for userspace programs forever.
> 
> Will put the macro above xfs_ioctl_setattr, as below:
> -------------------------------------------------------------
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 6f22a66777cd..e188e81961bd 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;
> -------------------------------------------------------------
> 
> Best Regards,
> Xiao Yang
> > --D
> > 
> > > +
> > >   /*
> > >    * Structure for XFS_IOC_FSSETDM.
> > >    * For use by backup and restore programs to set the XFS on-disk inode
> > > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > > index 6f22a66777cd..ec5feaa8dec8 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&  ~XFS_SUPPORTED_FS_XFLAGS)
> > > +                return -EOPNOTSUPP;
> > > +
> > >          code = xfs_ioctl_setattr_check_projid(ip, fa);
> > >          if (code)
> > >                  return code;
> > > -----------------------------------------------------------
> > > 
> > > Best Regards,
> > > Xiao Yang
> > > > --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
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > .
> > > > > > 
> > > > > 
> > > > .
> > > > 
> > > 
> > > 
> > 
> > .
> > 
> 
> 
>