[RFC PATCH 0/2] [gs]etfsxattrat() followup patches

[RFC PATCH 0/2] [gs]etfsxattrat() followup patches

[Amir Goldstein]

Andrey,

These patches are based on your [gs]etfsxattrat() patcehs [1].

I think that the first patch should be enough to allow Pali to later
extend the API for new flags. Please add it to your patch series.
I have only sanotify tested the ioctls, have not tested the syscalls.

The second patch is an RFC of how the API could be extended to query
filesystems for supported flags and allow userspace to set a subset
of flags and fields.

It plumbs in the fsx_xflags_mask semantics without implementing any
filesystem that reports non-zero mask. Obviously, I did not test this
pluming with no filesystem support, so this is mainly posted as a
reference design or for Pali if he has time to add his filesystem
support patch [2] and test it.

If the semantics of zero mask meaning no mask are not acceptable,
that I had also considered changing FSXATTR_SIZE_VER0 to 20 for the
first release, so initially users will need to pass usize that does
not include fsx_pad, until we decide on the way to extend the API.

Thanks,
Amir.

[1] https://lore.kernel.org/linux-fsdevel/20250321-xattrat-syscall-v4-0-3e82e6fb3264@kernel.org/
[2]https://lore.kernel.org/linux-fsdevel/20250216164029.20673-4-pali@kernel.org/

Amir Goldstein (2):
  fs: prepare for extending [gs]etfsxattrat()
  fs: add support for custom fsx_xflags_mask

 fs/inode.c               |  4 +++-
 fs/ioctl.c               | 46 +++++++++++++++++++++++++++++++++-------
 include/linux/fileattr.h | 23 +++++++++++++++++++-
 include/uapi/linux/fs.h  |  3 ++-
 4 files changed, 65 insertions(+), 11 deletions(-)

-- 
2.34.1

[RFC PATCH 1/2] fs: prepare for extending [gs]etfsxattrat()

[Amir Goldstein]

We intend to add support for more xflags to selective filesystems and
We cannot rely on copy_struct_from_user() to detect this extention.

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>
---
 fs/inode.c               |  4 +++-
 fs/ioctl.c               | 19 +++++++++++++------
 include/linux/fileattr.h | 22 +++++++++++++++++++++-
 3 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index 3cfcb1b9865ea..6c4d08bd53052 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -3049,7 +3049,9 @@ SYSCALL_DEFINE5(setfsxattrat, int, dfd, const char __user *, filename,
 	if (error)
 		return error;
 
-	fsxattr_to_fileattr(&fsx, &fa);
+	error = fsxattr_to_fileattr(&fsx, &fa);
+	if (error)
+		return error;
 
 	name = getname_maybe_null(filename, at_flags);
 	if (!name) {
diff --git a/fs/ioctl.c b/fs/ioctl.c
index 840283d8c4066..b19858db4c432 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -540,8 +540,10 @@ EXPORT_SYMBOL(vfs_fileattr_get);
 
 void fileattr_to_fsxattr(const struct fileattr *fa, struct fsxattr *fsx)
 {
+	__u32 mask = FS_XFALGS_MASK;
+
 	memset(fsx, 0, sizeof(struct fsxattr));
-	fsx->fsx_xflags = fa->fsx_xflags;
+	fsx->fsx_xflags = fa->fsx_xflags & mask;
 	fsx->fsx_extsize = fa->fsx_extsize;
 	fsx->fsx_nextents = fa->fsx_nextents;
 	fsx->fsx_projid = fa->fsx_projid;
@@ -568,13 +570,20 @@ int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa)
 }
 EXPORT_SYMBOL(copy_fsxattr_to_user);
 
-void fsxattr_to_fileattr(const struct fsxattr *fsx, struct fileattr *fa)
+int fsxattr_to_fileattr(const struct fsxattr *fsx, struct fileattr *fa)
 {
+	__u32 mask = FS_XFALGS_MASK;
+
+	if (fsx->fsx_xflags & ~mask)
+		return -EINVAL;
+
 	fileattr_fill_xflags(fa, fsx->fsx_xflags);
+	fa->fsx_xflags &= ~FS_XFLAG_RDONLY_MASK;
 	fa->fsx_extsize = fsx->fsx_extsize;
-	fa->fsx_nextents = fsx->fsx_nextents;
 	fa->fsx_projid = fsx->fsx_projid;
 	fa->fsx_cowextsize = fsx->fsx_cowextsize;
+
+	return 0;
 }
 
 static int copy_fsxattr_from_user(struct fileattr *fa,
@@ -585,9 +594,7 @@ static int copy_fsxattr_from_user(struct fileattr *fa,
 	if (copy_from_user(&xfa, ufa, sizeof(xfa)))
 		return -EFAULT;
 
-	fsxattr_to_fileattr(&xfa, fa);
-
-	return 0;
+	return fsxattr_to_fileattr(&xfa, fa);
 }
 
 /*
diff --git a/include/linux/fileattr.h b/include/linux/fileattr.h
index 31888fa2edf10..f682bfc7749dd 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_XFALGS_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
@@ -35,7 +55,7 @@ struct fileattr {
 
 void fileattr_to_fsxattr(const struct fileattr *fa, struct fsxattr *fsx);
 int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa);
-void fsxattr_to_fileattr(const struct fsxattr *fsx, struct fileattr *fa);
+int fsxattr_to_fileattr(const struct fsxattr *fsx, struct fileattr *fa);
 
 void fileattr_fill_xflags(struct fileattr *fa, u32 xflags);
 void fileattr_fill_flags(struct fileattr *fa, u32 flags);
-- 
2.34.1

[RFC PATCH 2/2] fs: add support for custom fsx_xflags_mask

[Amir Goldstein]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=true, Size: 4740 bytes --]

With getfsxattrat() syscall, filesystem may use this field to report
its supported xflags.  Zero mask value means that supported flags are
not advertized.

With setfsxattrat() syscall, userspace may use this field to declare
which xflags and fields are being set.  Zero mask value means that
all known xflags and fields are being set.

Programs that call getfsxattrat() to fill struct fsxattr before calling
setfsxattrat() will not be affected by this change, but it allows
programs that call setfsxattrat() without calling getfsxattrat() to make
changes to some xflags and fields without knowing or changing the values
of unrelated xflags and fields.

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>
---
 fs/ioctl.c               | 35 +++++++++++++++++++++++++++++------
 include/linux/fileattr.h |  1 +
 include/uapi/linux/fs.h  |  3 ++-
 3 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index b19858db4c432..a4838b3e7de90 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -540,10 +540,13 @@ EXPORT_SYMBOL(vfs_fileattr_get);
 
 void fileattr_to_fsxattr(const struct fileattr *fa, struct fsxattr *fsx)
 {
-	__u32 mask = FS_XFALGS_MASK;
+	/* Filesystem may or may not advertize supported xflags */
+	__u32 fs_mask = fa->fsx_xflags_mask & FS_XFALGS_MASK;
+	__u32 mask = fs_mask ?: FS_XFALGS_MASK;
 
 	memset(fsx, 0, sizeof(struct fsxattr));
 	fsx->fsx_xflags = fa->fsx_xflags & mask;
+	fsx->fsx_xflags_mask = fs_mask;
 	fsx->fsx_extsize = fa->fsx_extsize;
 	fsx->fsx_nextents = fa->fsx_nextents;
 	fsx->fsx_projid = fa->fsx_projid;
@@ -562,6 +565,8 @@ int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa)
 	struct fsxattr xfa;
 
 	fileattr_to_fsxattr(fa, &xfa);
+	/* FS_IOC_FSGETXATTR ioctl does not report supported fsx_xflags_mask */
+	xfa.fsx_xflags_mask = 0;
 
 	if (copy_to_user(ufa, &xfa, sizeof(xfa)))
 		return -EFAULT;
@@ -572,16 +577,30 @@ EXPORT_SYMBOL(copy_fsxattr_to_user);
 
 int fsxattr_to_fileattr(const struct fsxattr *fsx, struct fileattr *fa)
 {
-	__u32 mask = FS_XFALGS_MASK;
+	/* User may or may not provide custom xflags mask */
+	__u32 mask = fsx->fsx_xflags_mask ?: FS_XFALGS_MASK;
 
-	if (fsx->fsx_xflags & ~mask)
+	if ((fsx->fsx_xflags & ~mask) || (mask & ~FS_XFALGS_MASK))
 		return -EINVAL;
 
 	fileattr_fill_xflags(fa, fsx->fsx_xflags);
 	fa->fsx_xflags &= ~FS_XFLAG_RDONLY_MASK;
-	fa->fsx_extsize = fsx->fsx_extsize;
-	fa->fsx_projid = fsx->fsx_projid;
-	fa->fsx_cowextsize = fsx->fsx_cowextsize;
+	fa->fsx_xflags_mask = fsx->fsx_xflags_mask;
+	/*
+	 * If flags mask is specified, we copy the fields value only if the
+	 * relevant flag is set in the mask.
+	 */
+	if (!mask || (mask & (FS_XFLAG_EXTSIZE | FS_XFLAG_EXTSZINHERIT)))
+		fa->fsx_extsize = fsx->fsx_extsize;
+	if (!mask || (mask & FS_XFLAG_COWEXTSIZE))
+		fa->fsx_cowextsize = fsx->fsx_cowextsize;
+	/*
+	 * To save a mask flag (i.e. FS_XFLAG_PROJID), require setting values
+	 * of fsx_projid and FS_XFLAG_PROJINHERIT flag values together.
+	 * For a non-directory, FS_XFLAG_PROJINHERIT flag value should be 0.
+	 */
+	if (!mask || (mask & FS_XFLAG_PROJINHERIT))
+		fa->fsx_projid = fsx->fsx_projid;
 
 	return 0;
 }
@@ -594,6 +613,10 @@ static int copy_fsxattr_from_user(struct fileattr *fa,
 	if (copy_from_user(&xfa, ufa, sizeof(xfa)))
 		return -EFAULT;
 
+	/* FS_IOC_FSSETXATTR ioctl does not support user fsx_xflags_mask */
+	if (xfa.fsx_xflags_mask)
+		return -EINVAL;
+
 	return fsxattr_to_fileattr(&xfa, fa);
 }
 
diff --git a/include/linux/fileattr.h b/include/linux/fileattr.h
index f682bfc7749dd..93102423b7c95 100644
--- a/include/linux/fileattr.h
+++ b/include/linux/fileattr.h
@@ -44,6 +44,7 @@ struct fileattr {
 	u32	flags;		/* flags (FS_IOC_GETFLAGS/FS_IOC_SETFLAGS) */
 	/* struct fsxattr: */
 	u32	fsx_xflags;	/* xflags field value (get/set) */
+	u32	fsx_xflags_mask;/* xflags valid mask (get/set) */
 	u32	fsx_extsize;	/* extsize field value (get/set)*/
 	u32	fsx_nextents;	/* nextents field value (get)	*/
 	u32	fsx_projid;	/* project identifier (get/set) */
diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
index 0ae21596e25a5..5c35c415ca8fa 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -145,7 +145,8 @@ struct fsxattr {
 	__u32		fsx_nextents;	/* nextents field value (get)	*/
 	__u32		fsx_projid;	/* project identifier (get/set) */
 	__u32		fsx_cowextsize;	/* CoW extsize field value (get/set)*/
-	unsigned char	fsx_pad[8];
+	__u32		fsx_xflags_mask;/* xflags valid mask (get/set) */
+	unsigned char	fsx_pad[4];
 };
 
 #define FSXATTR_SIZE_VER0 28
-- 
2.34.1

Re: [RFC PATCH 2/2] fs: add support for custom fsx_xflags_mask

[Amir Goldstein]

On Sat, Mar 29, 2025 at 3:33 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> With getfsxattrat() syscall, filesystem may use this field to report
> its supported xflags.  Zero mask value means that supported flags are
> not advertized.
>
> With setfsxattrat() syscall, userspace may use this field to declare
> which xflags and fields are being set.  Zero mask value means that
> all known xflags and fields are being set.
>
> Programs that call getfsxattrat() to fill struct fsxattr before calling
> setfsxattrat() will not be affected by this change, but it allows
> programs that call setfsxattrat() without calling getfsxattrat() to make
> changes to some xflags and fields without knowing or changing the values
> of unrelated xflags and fields.
>
> 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>
> ---
>  fs/ioctl.c               | 35 +++++++++++++++++++++++++++++------
>  include/linux/fileattr.h |  1 +
>  include/uapi/linux/fs.h  |  3 ++-
>  3 files changed, 32 insertions(+), 7 deletions(-)
>
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index b19858db4c432..a4838b3e7de90 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -540,10 +540,13 @@ EXPORT_SYMBOL(vfs_fileattr_get);
>
>  void fileattr_to_fsxattr(const struct fileattr *fa, struct fsxattr *fsx)
>  {
> -       __u32 mask = FS_XFALGS_MASK;
> +       /* Filesystem may or may not advertize supported xflags */
> +       __u32 fs_mask = fa->fsx_xflags_mask & FS_XFALGS_MASK;
> +       __u32 mask = fs_mask ?: FS_XFALGS_MASK;
>
>         memset(fsx, 0, sizeof(struct fsxattr));
>         fsx->fsx_xflags = fa->fsx_xflags & mask;
> +       fsx->fsx_xflags_mask = fs_mask;
>         fsx->fsx_extsize = fa->fsx_extsize;
>         fsx->fsx_nextents = fa->fsx_nextents;
>         fsx->fsx_projid = fa->fsx_projid;
> @@ -562,6 +565,8 @@ int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa)
>         struct fsxattr xfa;
>
>         fileattr_to_fsxattr(fa, &xfa);
> +       /* FS_IOC_FSGETXATTR ioctl does not report supported fsx_xflags_mask */
> +       xfa.fsx_xflags_mask = 0;
>
>         if (copy_to_user(ufa, &xfa, sizeof(xfa)))
>                 return -EFAULT;
> @@ -572,16 +577,30 @@ EXPORT_SYMBOL(copy_fsxattr_to_user);
>
>  int fsxattr_to_fileattr(const struct fsxattr *fsx, struct fileattr *fa)
>  {
> -       __u32 mask = FS_XFALGS_MASK;
> +       /* User may or may not provide custom xflags mask */
> +       __u32 mask = fsx->fsx_xflags_mask ?: FS_XFALGS_MASK;
>
> -       if (fsx->fsx_xflags & ~mask)
> +       if ((fsx->fsx_xflags & ~mask) || (mask & ~FS_XFALGS_MASK))
>                 return -EINVAL;
>
>         fileattr_fill_xflags(fa, fsx->fsx_xflags);
>         fa->fsx_xflags &= ~FS_XFLAG_RDONLY_MASK;
> -       fa->fsx_extsize = fsx->fsx_extsize;
> -       fa->fsx_projid = fsx->fsx_projid;
> -       fa->fsx_cowextsize = fsx->fsx_cowextsize;
> +       fa->fsx_xflags_mask = fsx->fsx_xflags_mask;
> +       /*
> +        * If flags mask is specified, we copy the fields value only if the
> +        * relevant flag is set in the mask.
> +        */
> +       if (!mask || (mask & (FS_XFLAG_EXTSIZE | FS_XFLAG_EXTSZINHERIT)))
> +               fa->fsx_extsize = fsx->fsx_extsize;
> +       if (!mask || (mask & FS_XFLAG_COWEXTSIZE))
> +               fa->fsx_cowextsize = fsx->fsx_cowextsize;
> +       /*
> +        * To save a mask flag (i.e. FS_XFLAG_PROJID), require setting values
> +        * of fsx_projid and FS_XFLAG_PROJINHERIT flag values together.
> +        * For a non-directory, FS_XFLAG_PROJINHERIT flag value should be 0.
> +        */
> +       if (!mask || (mask & FS_XFLAG_PROJINHERIT))
> +               fa->fsx_projid = fsx->fsx_projid;

Sorry, I ended up initializing the mask without a user provided mask
to FS_XFALGS_MASK, so these (!mask ||) conditions are not needed.

Thanks,
Amir.

Re: [RFC PATCH 2/2] fs: add support for custom fsx_xflags_mask

[Pali Rohár]

On Saturday 29 March 2025 15:43:06 Amir Goldstein wrote:
> On Sat, Mar 29, 2025 at 3:33 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > With getfsxattrat() syscall, filesystem may use this field to report
> > its supported xflags.  Zero mask value means that supported flags are
> > not advertized.
> >
> > With setfsxattrat() syscall, userspace may use this field to declare
> > which xflags and fields are being set.  Zero mask value means that
> > all known xflags and fields are being set.
> >
> > Programs that call getfsxattrat() to fill struct fsxattr before calling
> > setfsxattrat() will not be affected by this change, but it allows
> > programs that call setfsxattrat() without calling getfsxattrat() to make
> > changes to some xflags and fields without knowing or changing the values
> > of unrelated xflags and fields.
> >
> > 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>
> > ---
> >  fs/ioctl.c               | 35 +++++++++++++++++++++++++++++------
> >  include/linux/fileattr.h |  1 +
> >  include/uapi/linux/fs.h  |  3 ++-
> >  3 files changed, 32 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/ioctl.c b/fs/ioctl.c
> > index b19858db4c432..a4838b3e7de90 100644
> > --- a/fs/ioctl.c
> > +++ b/fs/ioctl.c
> > @@ -540,10 +540,13 @@ EXPORT_SYMBOL(vfs_fileattr_get);
> >
> >  void fileattr_to_fsxattr(const struct fileattr *fa, struct fsxattr *fsx)
> >  {
> > -       __u32 mask = FS_XFALGS_MASK;
> > +       /* Filesystem may or may not advertize supported xflags */
> > +       __u32 fs_mask = fa->fsx_xflags_mask & FS_XFALGS_MASK;
> > +       __u32 mask = fs_mask ?: FS_XFALGS_MASK;
> >
> >         memset(fsx, 0, sizeof(struct fsxattr));
> >         fsx->fsx_xflags = fa->fsx_xflags & mask;
> > +       fsx->fsx_xflags_mask = fs_mask;
> >         fsx->fsx_extsize = fa->fsx_extsize;
> >         fsx->fsx_nextents = fa->fsx_nextents;
> >         fsx->fsx_projid = fa->fsx_projid;
> > @@ -562,6 +565,8 @@ int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa)
> >         struct fsxattr xfa;
> >
> >         fileattr_to_fsxattr(fa, &xfa);
> > +       /* FS_IOC_FSGETXATTR ioctl does not report supported fsx_xflags_mask */
> > +       xfa.fsx_xflags_mask = 0;
> >
> >         if (copy_to_user(ufa, &xfa, sizeof(xfa)))
> >                 return -EFAULT;
> > @@ -572,16 +577,30 @@ EXPORT_SYMBOL(copy_fsxattr_to_user);
> >
> >  int fsxattr_to_fileattr(const struct fsxattr *fsx, struct fileattr *fa)
> >  {
> > -       __u32 mask = FS_XFALGS_MASK;
> > +       /* User may or may not provide custom xflags mask */
> > +       __u32 mask = fsx->fsx_xflags_mask ?: FS_XFALGS_MASK;
> >
> > -       if (fsx->fsx_xflags & ~mask)
> > +       if ((fsx->fsx_xflags & ~mask) || (mask & ~FS_XFALGS_MASK))
> >                 return -EINVAL;
> >
> >         fileattr_fill_xflags(fa, fsx->fsx_xflags);
> >         fa->fsx_xflags &= ~FS_XFLAG_RDONLY_MASK;
> > -       fa->fsx_extsize = fsx->fsx_extsize;
> > -       fa->fsx_projid = fsx->fsx_projid;
> > -       fa->fsx_cowextsize = fsx->fsx_cowextsize;
> > +       fa->fsx_xflags_mask = fsx->fsx_xflags_mask;
> > +       /*
> > +        * If flags mask is specified, we copy the fields value only if the
> > +        * relevant flag is set in the mask.
> > +        */
> > +       if (!mask || (mask & (FS_XFLAG_EXTSIZE | FS_XFLAG_EXTSZINHERIT)))
> > +               fa->fsx_extsize = fsx->fsx_extsize;
> > +       if (!mask || (mask & FS_XFLAG_COWEXTSIZE))
> > +               fa->fsx_cowextsize = fsx->fsx_cowextsize;
> > +       /*
> > +        * To save a mask flag (i.e. FS_XFLAG_PROJID), require setting values
> > +        * of fsx_projid and FS_XFLAG_PROJINHERIT flag values together.
> > +        * For a non-directory, FS_XFLAG_PROJINHERIT flag value should be 0.
> > +        */
> > +       if (!mask || (mask & FS_XFLAG_PROJINHERIT))
> > +               fa->fsx_projid = fsx->fsx_projid;
> 
> Sorry, I ended up initializing the mask without a user provided mask
> to FS_XFALGS_MASK, so these (!mask ||) conditions are not needed.
> 
> Thanks,
> Amir.

And there is a typo: FS_XFLAGS_MASK

Re: [RFC PATCH 2/2] fs: add support for custom fsx_xflags_mask

[Amir Goldstein]

On Sat, Mar 29, 2025 at 3:44 PM Pali Rohár <pali@kernel.org> wrote:
>
> On Saturday 29 March 2025 15:43:06 Amir Goldstein wrote:
> > On Sat, Mar 29, 2025 at 3:33 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > With getfsxattrat() syscall, filesystem may use this field to report
> > > its supported xflags.  Zero mask value means that supported flags are
> > > not advertized.
> > >
> > > With setfsxattrat() syscall, userspace may use this field to declare
> > > which xflags and fields are being set.  Zero mask value means that
> > > all known xflags and fields are being set.
> > >
> > > Programs that call getfsxattrat() to fill struct fsxattr before calling
> > > setfsxattrat() will not be affected by this change, but it allows
> > > programs that call setfsxattrat() without calling getfsxattrat() to make
> > > changes to some xflags and fields without knowing or changing the values
> > > of unrelated xflags and fields.
> > >
> > > 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>
> > > ---
> > >  fs/ioctl.c               | 35 +++++++++++++++++++++++++++++------
> > >  include/linux/fileattr.h |  1 +
> > >  include/uapi/linux/fs.h  |  3 ++-
> > >  3 files changed, 32 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/fs/ioctl.c b/fs/ioctl.c
> > > index b19858db4c432..a4838b3e7de90 100644
> > > --- a/fs/ioctl.c
> > > +++ b/fs/ioctl.c
> > > @@ -540,10 +540,13 @@ EXPORT_SYMBOL(vfs_fileattr_get);
> > >
> > >  void fileattr_to_fsxattr(const struct fileattr *fa, struct fsxattr *fsx)
> > >  {
> > > -       __u32 mask = FS_XFALGS_MASK;
> > > +       /* Filesystem may or may not advertize supported xflags */
> > > +       __u32 fs_mask = fa->fsx_xflags_mask & FS_XFALGS_MASK;
> > > +       __u32 mask = fs_mask ?: FS_XFALGS_MASK;
> > >
> > >         memset(fsx, 0, sizeof(struct fsxattr));
> > >         fsx->fsx_xflags = fa->fsx_xflags & mask;
> > > +       fsx->fsx_xflags_mask = fs_mask;
> > >         fsx->fsx_extsize = fa->fsx_extsize;
> > >         fsx->fsx_nextents = fa->fsx_nextents;
> > >         fsx->fsx_projid = fa->fsx_projid;
> > > @@ -562,6 +565,8 @@ int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa)
> > >         struct fsxattr xfa;
> > >
> > >         fileattr_to_fsxattr(fa, &xfa);
> > > +       /* FS_IOC_FSGETXATTR ioctl does not report supported fsx_xflags_mask */
> > > +       xfa.fsx_xflags_mask = 0;
> > >
> > >         if (copy_to_user(ufa, &xfa, sizeof(xfa)))
> > >                 return -EFAULT;
> > > @@ -572,16 +577,30 @@ EXPORT_SYMBOL(copy_fsxattr_to_user);
> > >
> > >  int fsxattr_to_fileattr(const struct fsxattr *fsx, struct fileattr *fa)
> > >  {
> > > -       __u32 mask = FS_XFALGS_MASK;
> > > +       /* User may or may not provide custom xflags mask */
> > > +       __u32 mask = fsx->fsx_xflags_mask ?: FS_XFALGS_MASK;
> > >
> > > -       if (fsx->fsx_xflags & ~mask)
> > > +       if ((fsx->fsx_xflags & ~mask) || (mask & ~FS_XFALGS_MASK))
> > >                 return -EINVAL;
> > >
> > >         fileattr_fill_xflags(fa, fsx->fsx_xflags);
> > >         fa->fsx_xflags &= ~FS_XFLAG_RDONLY_MASK;
> > > -       fa->fsx_extsize = fsx->fsx_extsize;
> > > -       fa->fsx_projid = fsx->fsx_projid;
> > > -       fa->fsx_cowextsize = fsx->fsx_cowextsize;
> > > +       fa->fsx_xflags_mask = fsx->fsx_xflags_mask;
> > > +       /*
> > > +        * If flags mask is specified, we copy the fields value only if the
> > > +        * relevant flag is set in the mask.
> > > +        */
> > > +       if (!mask || (mask & (FS_XFLAG_EXTSIZE | FS_XFLAG_EXTSZINHERIT)))
> > > +               fa->fsx_extsize = fsx->fsx_extsize;
> > > +       if (!mask || (mask & FS_XFLAG_COWEXTSIZE))
> > > +               fa->fsx_cowextsize = fsx->fsx_cowextsize;
> > > +       /*
> > > +        * To save a mask flag (i.e. FS_XFLAG_PROJID), require setting values
> > > +        * of fsx_projid and FS_XFLAG_PROJINHERIT flag values together.
> > > +        * For a non-directory, FS_XFLAG_PROJINHERIT flag value should be 0.
> > > +        */
> > > +       if (!mask || (mask & FS_XFLAG_PROJINHERIT))
> > > +               fa->fsx_projid = fsx->fsx_projid;
> >
> > Sorry, I ended up initializing the mask without a user provided mask
> > to FS_XFALGS_MASK, so these (!mask ||) conditions are not needed.
> >
> > Thanks,
> > Amir.
>
> And there is a typo: FS_XFLAGS_MASK

Oops.
Fixed typo and braino and pushed to
https://github.com/amir73il/linux/commits/fsxattr

Thanks,
Amir.

Re: [RFC PATCH 1/2] fs: prepare for extending [gs]etfsxattrat()

[Andrey Albershteyn]

On 2025-03-29 15:33:11, Amir Goldstein wrote:
> We intend to add support for more xflags to selective filesystems and
> We cannot rely on copy_struct_from_user() to detect this extention.
> 
> 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>
> ---
>  fs/inode.c               |  4 +++-
>  fs/ioctl.c               | 19 +++++++++++++------
>  include/linux/fileattr.h | 22 +++++++++++++++++++++-
>  3 files changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 3cfcb1b9865ea..6c4d08bd53052 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -3049,7 +3049,9 @@ SYSCALL_DEFINE5(setfsxattrat, int, dfd, const char __user *, filename,
>  	if (error)
>  		return error;
>  
> -	fsxattr_to_fileattr(&fsx, &fa);
> +	error = fsxattr_to_fileattr(&fsx, &fa);
> +	if (error)
> +		return error;
>  
>  	name = getname_maybe_null(filename, at_flags);
>  	if (!name) {
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 840283d8c4066..b19858db4c432 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -540,8 +540,10 @@ EXPORT_SYMBOL(vfs_fileattr_get);
>  
>  void fileattr_to_fsxattr(const struct fileattr *fa, struct fsxattr *fsx)
>  {
> +	__u32 mask = FS_XFALGS_MASK;
> +
>  	memset(fsx, 0, sizeof(struct fsxattr));
> -	fsx->fsx_xflags = fa->fsx_xflags;
> +	fsx->fsx_xflags = fa->fsx_xflags & mask;
>  	fsx->fsx_extsize = fa->fsx_extsize;
>  	fsx->fsx_nextents = fa->fsx_nextents;
>  	fsx->fsx_projid = fa->fsx_projid;
> @@ -568,13 +570,20 @@ int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa)
>  }
>  EXPORT_SYMBOL(copy_fsxattr_to_user);
>  
> -void fsxattr_to_fileattr(const struct fsxattr *fsx, struct fileattr *fa)
> +int fsxattr_to_fileattr(const struct fsxattr *fsx, struct fileattr *fa)
>  {
> +	__u32 mask = FS_XFALGS_MASK;
> +
> +	if (fsx->fsx_xflags & ~mask)
> +		return -EINVAL;
> +
>  	fileattr_fill_xflags(fa, fsx->fsx_xflags);
> +	fa->fsx_xflags &= ~FS_XFLAG_RDONLY_MASK;
>  	fa->fsx_extsize = fsx->fsx_extsize;
> -	fa->fsx_nextents = fsx->fsx_nextents;
>  	fa->fsx_projid = fsx->fsx_projid;
>  	fa->fsx_cowextsize = fsx->fsx_cowextsize;
> +
> +	return 0;
>  }
>  
>  static int copy_fsxattr_from_user(struct fileattr *fa,
> @@ -585,9 +594,7 @@ static int copy_fsxattr_from_user(struct fileattr *fa,
>  	if (copy_from_user(&xfa, ufa, sizeof(xfa)))
>  		return -EFAULT;
>  
> -	fsxattr_to_fileattr(&xfa, fa);
> -
> -	return 0;
> +	return fsxattr_to_fileattr(&xfa, fa);
>  }
>  
>  /*
> diff --git a/include/linux/fileattr.h b/include/linux/fileattr.h
> index 31888fa2edf10..f682bfc7749dd 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 */

Maybe it's only me, but this "read-only" is a bit confusing, as
those are not settable get-only flags and not flags of read-only
inode

> +#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_XFALGS_MASK \
> +	(FS_XFLAG_COMMON | FS_XFLAG_RDONLY_MASK | FS_XFLAG_VALUES_MASK | \
> +	 FS_XFLAG_DIRONLY_MASK | FS_XFLAG_MISC_MASK)
> +

I like the splitting but do we want to split flags like this? I can
imagine new flags just getting pushed into _MISK_MASK or these names
just loosing any sense.

>  /*
>   * Merged interface for miscellaneous file attributes.  'flags' originates from
>   * ext* and 'fsx_flags' from xfs.  There's some overlap between the two, which
> @@ -35,7 +55,7 @@ struct fileattr {
>  
>  void fileattr_to_fsxattr(const struct fileattr *fa, struct fsxattr *fsx);
>  int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa);
> -void fsxattr_to_fileattr(const struct fsxattr *fsx, struct fileattr *fa);
> +int fsxattr_to_fileattr(const struct fsxattr *fsx, struct fileattr *fa);
>  
>  void fileattr_fill_xflags(struct fileattr *fa, u32 xflags);
>  void fileattr_fill_flags(struct fileattr *fa, u32 flags);
> -- 
> 2.34.1
> 

Otherwise, this patch looks fine for limiting the interface for now

I will include it in next iteration

-- 
- Andrey

Re: [RFC PATCH 1/2] fs: prepare for extending [gs]etfsxattrat()

[Amir Goldstein]

On Mon, Mar 31, 2025 at 4:43 PM Andrey Albershteyn <aalbersh@redhat.com> wrote:
>
> On 2025-03-29 15:33:11, Amir Goldstein wrote:
> > We intend to add support for more xflags to selective filesystems and
> > We cannot rely on copy_struct_from_user() to detect this extention.
> >
> > 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>
> > ---
> >  fs/inode.c               |  4 +++-
> >  fs/ioctl.c               | 19 +++++++++++++------
> >  include/linux/fileattr.h | 22 +++++++++++++++++++++-
> >  3 files changed, 37 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/inode.c b/fs/inode.c
> > index 3cfcb1b9865ea..6c4d08bd53052 100644
> > --- a/fs/inode.c
> > +++ b/fs/inode.c
> > @@ -3049,7 +3049,9 @@ SYSCALL_DEFINE5(setfsxattrat, int, dfd, const char __user *, filename,
> >       if (error)
> >               return error;
> >
> > -     fsxattr_to_fileattr(&fsx, &fa);
> > +     error = fsxattr_to_fileattr(&fsx, &fa);
> > +     if (error)
> > +             return error;
> >
> >       name = getname_maybe_null(filename, at_flags);
> >       if (!name) {
> > diff --git a/fs/ioctl.c b/fs/ioctl.c
> > index 840283d8c4066..b19858db4c432 100644
> > --- a/fs/ioctl.c
> > +++ b/fs/ioctl.c
> > @@ -540,8 +540,10 @@ EXPORT_SYMBOL(vfs_fileattr_get);
> >
> >  void fileattr_to_fsxattr(const struct fileattr *fa, struct fsxattr *fsx)
> >  {
> > +     __u32 mask = FS_XFALGS_MASK;
> > +
> >       memset(fsx, 0, sizeof(struct fsxattr));
> > -     fsx->fsx_xflags = fa->fsx_xflags;
> > +     fsx->fsx_xflags = fa->fsx_xflags & mask;
> >       fsx->fsx_extsize = fa->fsx_extsize;
> >       fsx->fsx_nextents = fa->fsx_nextents;
> >       fsx->fsx_projid = fa->fsx_projid;
> > @@ -568,13 +570,20 @@ int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa)
> >  }
> >  EXPORT_SYMBOL(copy_fsxattr_to_user);
> >
> > -void fsxattr_to_fileattr(const struct fsxattr *fsx, struct fileattr *fa)
> > +int fsxattr_to_fileattr(const struct fsxattr *fsx, struct fileattr *fa)
> >  {
> > +     __u32 mask = FS_XFALGS_MASK;
> > +
> > +     if (fsx->fsx_xflags & ~mask)
> > +             return -EINVAL;
> > +
> >       fileattr_fill_xflags(fa, fsx->fsx_xflags);
> > +     fa->fsx_xflags &= ~FS_XFLAG_RDONLY_MASK;
> >       fa->fsx_extsize = fsx->fsx_extsize;
> > -     fa->fsx_nextents = fsx->fsx_nextents;
> >       fa->fsx_projid = fsx->fsx_projid;
> >       fa->fsx_cowextsize = fsx->fsx_cowextsize;
> > +
> > +     return 0;
> >  }
> >
> >  static int copy_fsxattr_from_user(struct fileattr *fa,
> > @@ -585,9 +594,7 @@ static int copy_fsxattr_from_user(struct fileattr *fa,
> >       if (copy_from_user(&xfa, ufa, sizeof(xfa)))
> >               return -EFAULT;
> >
> > -     fsxattr_to_fileattr(&xfa, fa);
> > -
> > -     return 0;
> > +     return fsxattr_to_fileattr(&xfa, fa);
> >  }
> >
> >  /*
> > diff --git a/include/linux/fileattr.h b/include/linux/fileattr.h
> > index 31888fa2edf10..f682bfc7749dd 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 */
>
> Maybe it's only me, but this "read-only" is a bit confusing, as
> those are not settable get-only flags and not flags of read-only
> inode
>

I am also not crazy about this name.
I am fine with GETONLY_MASK.

> > +#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_XFALGS_MASK \
> > +     (FS_XFLAG_COMMON | FS_XFLAG_RDONLY_MASK | FS_XFLAG_VALUES_MASK | \
> > +      FS_XFLAG_DIRONLY_MASK | FS_XFLAG_MISC_MASK)
> > +
>
> I like the splitting but do we want to split flags like this? I can
> imagine new flags just getting pushed into _MISK_MASK or these names
> just loosing any sense.
>

I mostly did this for my own sake of order, but I do not mind
if you decide to include this grouping or not. Up to you.
Just don't carry the typo FS_XFALGS_MASK ;)


> >  /*
> >   * Merged interface for miscellaneous file attributes.  'flags' originates from
> >   * ext* and 'fsx_flags' from xfs.  There's some overlap between the two, which
> > @@ -35,7 +55,7 @@ struct fileattr {
> >
> >  void fileattr_to_fsxattr(const struct fileattr *fa, struct fsxattr *fsx);
> >  int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa);
> > -void fsxattr_to_fileattr(const struct fsxattr *fsx, struct fileattr *fa);
> > +int fsxattr_to_fileattr(const struct fsxattr *fsx, struct fileattr *fa);
> >
> >  void fileattr_fill_xflags(struct fileattr *fa, u32 xflags);
> >  void fileattr_fill_flags(struct fileattr *fa, u32 flags);
> > --
> > 2.34.1
> >
>
> Otherwise, this patch looks fine for limiting the interface for now
>
> I will include it in next iteration

Thanks!
Amir.

Re: [RFC PATCH 1/2] fs: prepare for extending [gs]etfsxattrat()

[Dave Chinner]

On Sat, Mar 29, 2025 at 03:33:11PM +0100, Amir Goldstein wrote:
> We intend to add support for more xflags to selective filesystems and
> We cannot rely on copy_struct_from_user() to detect this extention.
> 
> 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.
.....

> +
> +#define FS_XFALGS_MASK \
> +	(FS_XFLAG_COMMON | FS_XFLAG_RDONLY_MASK | FS_XFLAG_VALUES_MASK | \
> +	 FS_XFLAG_DIRONLY_MASK | FS_XFLAG_MISC_MASK)

You might want to fix the obvious typo....

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [RFC PATCH 0/2] [gs]etfsxattrat() followup patches

[Christian Brauner]

I'll do a review of the system calls and the extension here hopefully
this week and see where we're at. Sorry, I'm trying to catch up with a
bunch of stuff and the power management nerd snipe didn't help it...