Re: [PATCH v6 5/6] fs: prepare for extending file_get/setattr()

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, Jun 30, 2025 at 06:20:15PM +0200, Andrey Albershteyn wrote:
> From: Amir Goldstein <amir73il@gmail.com>
> 
> We intend to add support for more xflags to selective filesystems and
> We cannot rely on copy_struct_from_user() to detect this extension.
> 
> In preparation of extending the API, do not allow setting xflags unknown
> by this kernel version.
> 
> Also do not pass the read-only flags and read-only field fsx_nextents to
> filesystem.
> 
> These changes should not affect existing chattr programs that use the
> ioctl to get fsxattr before setting the new values.
> 
> Link: https://lore.kernel.org/linux-fsdevel/20250216164029.20673-4-pali@kernel.org/
> Cc: Pali Rohár <pali@kernel.org>
> Cc: Andrey Albershteyn <aalbersh@redhat.com>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Andrey Albershteyn <aalbersh@kernel.org>
> ---
>  fs/file_attr.c           |  8 +++++++-
>  include/linux/fileattr.h | 20 ++++++++++++++++++++
>  2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/file_attr.c b/fs/file_attr.c
> index 4e85fa00c092..62f08872d4ad 100644
> --- a/fs/file_attr.c
> +++ b/fs/file_attr.c
> @@ -99,9 +99,10 @@ EXPORT_SYMBOL(vfs_fileattr_get);
>  int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa)
>  {
>  	struct fsxattr xfa;
> +	__u32 mask = FS_XFLAGS_MASK;
>  
>  	memset(&xfa, 0, sizeof(xfa));
> -	xfa.fsx_xflags = fa->fsx_xflags;
> +	xfa.fsx_xflags = fa->fsx_xflags & mask;

I wonder, should it be an error if a filesystem sets an fsx_xflags bit
outside of FS_XFLAGS_MASK?  I guess that's one way to prevent
filesystems from overriding the VFS bits. ;)

Though couldn't that be:

	xfa.fsx_xflags = fa->fsx_xflags & FS_XFLAGS_MASK;

instead?  And same below?

>  	xfa.fsx_extsize = fa->fsx_extsize;
>  	xfa.fsx_nextents = fa->fsx_nextents;
>  	xfa.fsx_projid = fa->fsx_projid;
> @@ -118,11 +119,16 @@ static int copy_fsxattr_from_user(struct fileattr *fa,
>  				  struct fsxattr __user *ufa)
>  {
>  	struct fsxattr xfa;
> +	__u32 mask = FS_XFLAGS_MASK;
>  
>  	if (copy_from_user(&xfa, ufa, sizeof(xfa)))
>  		return -EFAULT;
>  
> +	if (xfa.fsx_xflags & ~mask)
> +		return -EINVAL;

I wonder if you want EOPNOTSUPP here?  We don't know how to support
unknown xflags.  OTOH if you all have beaten this to death while I was
out then don't start another round just for me. :P

--D

> +
>  	fileattr_fill_xflags(fa, xfa.fsx_xflags);
> +	fa->fsx_xflags &= ~FS_XFLAG_RDONLY_MASK;
>  	fa->fsx_extsize = xfa.fsx_extsize;
>  	fa->fsx_nextents = xfa.fsx_nextents;
>  	fa->fsx_projid = xfa.fsx_projid;
> diff --git a/include/linux/fileattr.h b/include/linux/fileattr.h
> index 6030d0bf7ad3..e2a2f4ae242d 100644
> --- a/include/linux/fileattr.h
> +++ b/include/linux/fileattr.h
> @@ -14,6 +14,26 @@
>  	 FS_XFLAG_NODUMP | FS_XFLAG_NOATIME | FS_XFLAG_DAX | \
>  	 FS_XFLAG_PROJINHERIT)
>  
> +/* Read-only inode flags */
> +#define FS_XFLAG_RDONLY_MASK \
> +	(FS_XFLAG_PREALLOC | FS_XFLAG_HASATTR)
> +
> +/* Flags to indicate valid value of fsx_ fields */
> +#define FS_XFLAG_VALUES_MASK \
> +	(FS_XFLAG_EXTSIZE | FS_XFLAG_COWEXTSIZE)
> +
> +/* Flags for directories */
> +#define FS_XFLAG_DIRONLY_MASK \
> +	(FS_XFLAG_RTINHERIT | FS_XFLAG_NOSYMLINKS | FS_XFLAG_EXTSZINHERIT)
> +
> +/* Misc settable flags */
> +#define FS_XFLAG_MISC_MASK \
> +	(FS_XFLAG_REALTIME | FS_XFLAG_NODEFRAG | FS_XFLAG_FILESTREAM)
> +
> +#define FS_XFLAGS_MASK \
> +	(FS_XFLAG_COMMON | FS_XFLAG_RDONLY_MASK | FS_XFLAG_VALUES_MASK | \
> +	 FS_XFLAG_DIRONLY_MASK | FS_XFLAG_MISC_MASK)
> +
>  /*
>   * Merged interface for miscellaneous file attributes.  'flags' originates from
>   * ext* and 'fsx_flags' from xfs.  There's some overlap between the two, which
> 
> -- 
> 2.47.2
> 
>