1. Patchew
  2. linux
  3. [PATCH v6 0/6] fs: introduce file_getattr and file_setattr syscalls
  4. View patch

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

Andrey Albershteyn posted 6 patches 3 months, 1 week ago
[PATCH v6 5/6] fs: prepare for extending file_get/setattr()
Posted by Andrey Albershteyn 3 months, 1 week ago 
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;
 	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;
+
 	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
Re: [PATCH v6 5/6] fs: prepare for extending file_get/setattr()
Posted by Darrick J. Wong 3 months, 1 week ago 
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
> 
>
Re: [PATCH v6 5/6] fs: prepare for extending file_get/setattr()
Posted by Amir Goldstein 3 months, 1 week ago 
On Tue, Jul 1, 2025 at 8:31 PM Darrick J. Wong <djwong@kernel.org> wrote:
>
> 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. ;)

I think Pali has a plan on how to ensure that later
when the mask is provided via the API.

>
> Though couldn't that be:
>
>         xfa.fsx_xflags = fa->fsx_xflags & FS_XFLAGS_MASK;
>
> instead?  And same below?
>

Indeed. There is a reason for the var, because the next series
by Pali will use a user provided mask, which defaults to FS_XFLAGS_MASK,
so I left it this way.

I don't see a problem with it keeping as is, but if it bothers you
I guess we can re-add the var later.

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

We have beaten this API almost to death for sure ;)
I don't remember if we discussed this specific aspect,
but I am personally in favor of
EOPNOTSUPP := the fs does not support the set/get operation
EINVAL := some flags provided as value is invalid

For example, if the get API provides you with a mask of the
valid flags that you can set, if you try to set flags outside of
that mask you get EINVAL.

That's my interpretation, but I agree that EOPNOTSUPP can also
make sense in this situation.

Thanks,
Amir.
Re: [PATCH v6 5/6] fs: prepare for extending file_get/setattr()
Posted by Darrick J. Wong 3 months, 1 week ago 
On Tue, Jul 01, 2025 at 09:27:38PM +0200, Amir Goldstein wrote:
> On Tue, Jul 1, 2025 at 8:31 PM Darrick J. Wong <djwong@kernel.org> wrote:
> >
> > 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. ;)
> 
> I think Pali has a plan on how to ensure that later
> when the mask is provided via the API.
> 
> >
> > Though couldn't that be:
> >
> >         xfa.fsx_xflags = fa->fsx_xflags & FS_XFLAGS_MASK;
> >
> > instead?  And same below?
> >
> 
> Indeed. There is a reason for the var, because the next series
> by Pali will use a user provided mask, which defaults to FS_XFLAGS_MASK,
> so I left it this way.
> 
> I don't see a problem with it keeping as is, but if it bothers you
> I guess we can re-add the var later.

Nah, it doesn't bother me that much.

> > >       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
> 
> We have beaten this API almost to death for sure ;)
> I don't remember if we discussed this specific aspect,
> but I am personally in favor of
> EOPNOTSUPP := the fs does not support the set/get operation
> EINVAL := some flags provided as value is invalid
> 
> For example, if the get API provides you with a mask of the
> valid flags that you can set, if you try to set flags outside of
> that mask you get EINVAL.
> 
> That's my interpretation, but I agree that EOPNOTSUPP can also
> make sense in this situation.

<nod> I think I'd rather EOPNOTSUPP for "bits are set that the kernel
doesn't recognize" and EINVAL (or maybe something else like
EPROTONOSUPPORT) for "fs driver will not let you change this bit".
At least for the syscall interface; we probably have to flatten that to
EOPNOTSUPP for both legacy ioctls.

--D

> Thanks,
> Amir.
>
Re: [PATCH v6 5/6] fs: prepare for extending file_get/setattr()
Posted by Pali Rohár 3 months, 1 week ago 
On Tuesday 01 July 2025 12:40:02 Darrick J. Wong wrote:
> On Tue, Jul 01, 2025 at 09:27:38PM +0200, Amir Goldstein wrote:
> > On Tue, Jul 1, 2025 at 8:31 PM Darrick J. Wong <djwong@kernel.org> wrote:
> > >
> > > 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. ;)
> > 
> > I think Pali has a plan on how to ensure that later
> > when the mask is provided via the API.
> > 
> > >
> > > Though couldn't that be:
> > >
> > >         xfa.fsx_xflags = fa->fsx_xflags & FS_XFLAGS_MASK;
> > >
> > > instead?  And same below?
> > >
> > 
> > Indeed. There is a reason for the var, because the next series
> > by Pali will use a user provided mask, which defaults to FS_XFLAGS_MASK,
> > so I left it this way.
> > 
> > I don't see a problem with it keeping as is, but if it bothers you
> > I guess we can re-add the var later.
> 
> Nah, it doesn't bother me that much.
> 
> > > >       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
> > 
> > We have beaten this API almost to death for sure ;)
> > I don't remember if we discussed this specific aspect,
> > but I am personally in favor of
> > EOPNOTSUPP := the fs does not support the set/get operation
> > EINVAL := some flags provided as value is invalid
> > 
> > For example, if the get API provides you with a mask of the
> > valid flags that you can set, if you try to set flags outside of
> > that mask you get EINVAL.
> > 
> > That's my interpretation, but I agree that EOPNOTSUPP can also
> > make sense in this situation.
> 
> <nod> I think I'd rather EOPNOTSUPP for "bits are set that the kernel
> doesn't recognize" and EINVAL (or maybe something else like
> EPROTONOSUPPORT) for "fs driver will not let you change this bit".
> At least for the syscall interface; we probably have to flatten that to
> EOPNOTSUPP for both legacy ioctls.

... and this starting to be complicated if the "fs driver" is network
based (as fs driver can support, but remote server not). See also:
https://lore.kernel.org/linux-fsdevel/20241224160535.pi6nazpugqkhvfns@pali/t/#u

For backup/restore application it would be very useful to distinguish between:
- "kernel does not support flag X"
- "target filesystem does not support flag X"
- "wrong structure was passed / syscall incorrectly called"

third option is bug in application - fatal error. second option is just
a warning for user (sorry, we cannot set NEW FEATURE on FAT32, but if
you would do restore to other fs, it is supported). and first option
happens when you run new application on older kernel version, it is an
recoverable error (or warning to user, but with more important level
then second option as switching to different FS would not help).

Could we return different errnos for these 3 situations?

> --D
> 
> > Thanks,
> > Amir.
> >
Re: [PATCH v6 5/6] fs: prepare for extending file_get/setattr()
Posted by Amir Goldstein 3 months, 1 week ago 
On Tue, Jul 1, 2025 at 9:54 PM Pali Rohár <pali@kernel.org> wrote:
>
> On Tuesday 01 July 2025 12:40:02 Darrick J. Wong wrote:
> > On Tue, Jul 01, 2025 at 09:27:38PM +0200, Amir Goldstein wrote:
> > > On Tue, Jul 1, 2025 at 8:31 PM Darrick J. Wong <djwong@kernel.org> wrote:
> > > >
> > > > 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. ;)
> > >
> > > I think Pali has a plan on how to ensure that later
> > > when the mask is provided via the API.
> > >
> > > >
> > > > Though couldn't that be:
> > > >
> > > >         xfa.fsx_xflags = fa->fsx_xflags & FS_XFLAGS_MASK;
> > > >
> > > > instead?  And same below?
> > > >
> > >
> > > Indeed. There is a reason for the var, because the next series
> > > by Pali will use a user provided mask, which defaults to FS_XFLAGS_MASK,
> > > so I left it this way.
> > >
> > > I don't see a problem with it keeping as is, but if it bothers you
> > > I guess we can re-add the var later.
> >
> > Nah, it doesn't bother me that much.
> >
> > > > >       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
> > >
> > > We have beaten this API almost to death for sure ;)
> > > I don't remember if we discussed this specific aspect,
> > > but I am personally in favor of
> > > EOPNOTSUPP := the fs does not support the set/get operation
> > > EINVAL := some flags provided as value is invalid
> > >
> > > For example, if the get API provides you with a mask of the
> > > valid flags that you can set, if you try to set flags outside of
> > > that mask you get EINVAL.
> > >
> > > That's my interpretation, but I agree that EOPNOTSUPP can also
> > > make sense in this situation.
> >
> > <nod> I think I'd rather EOPNOTSUPP for "bits are set that the kernel
> > doesn't recognize" and EINVAL (or maybe something else like
> > EPROTONOSUPPORT) for "fs driver will not let you change this bit".
> > At least for the syscall interface; we probably have to flatten that to
> > EOPNOTSUPP for both legacy ioctls.

Given the precedents of returning EOPNOTSUPP in xfs_fileattr_set()
and ext4_ioctl_setflags() for flags that cannot be set, I agree.

>
> ... and this starting to be complicated if the "fs driver" is network
> based (as fs driver can support, but remote server not). See also:
> https://lore.kernel.org/linux-fsdevel/20241224160535.pi6nazpugqkhvfns@pali/t/#u
>
> For backup/restore application it would be very useful to distinguish between:
> - "kernel does not support flag X"
> - "target filesystem does not support flag X"
> - "wrong structure was passed / syscall incorrectly called"
>
> third option is bug in application - fatal error. second option is just
> a warning for user (sorry, we cannot set NEW FEATURE on FAT32, but if
> you would do restore to other fs, it is supported). and first option
> happens when you run new application on older kernel version, it is an
> recoverable error (or warning to user, but with more important level
> then second option as switching to different FS would not help).
>
> Could we return different errnos for these 3 situations?

That would be nice, but actually according to your plan
the get API returns the mask of flags supported by the filesystem
(on that specific object even), so userspace in fact has a way to
distinguish between the first two EOPNOTSUPP cases.

Thanks,
Amir.
Re: [PATCH v6 5/6] fs: prepare for extending file_get/setattr()
Posted by Amir Goldstein 3 months, 1 week ago 
On Wed, Jul 2, 2025 at 9:03 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Jul 1, 2025 at 9:54 PM Pali Rohár <pali@kernel.org> wrote:
> >
> > On Tuesday 01 July 2025 12:40:02 Darrick J. Wong wrote:
> > > On Tue, Jul 01, 2025 at 09:27:38PM +0200, Amir Goldstein wrote:
> > > > On Tue, Jul 1, 2025 at 8:31 PM Darrick J. Wong <djwong@kernel.org> wrote:
> > > > >
> > > > > 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. ;)
> > > >
> > > > I think Pali has a plan on how to ensure that later
> > > > when the mask is provided via the API.
> > > >
> > > > >
> > > > > Though couldn't that be:
> > > > >
> > > > >         xfa.fsx_xflags = fa->fsx_xflags & FS_XFLAGS_MASK;
> > > > >
> > > > > instead?  And same below?
> > > > >
> > > >
> > > > Indeed. There is a reason for the var, because the next series
> > > > by Pali will use a user provided mask, which defaults to FS_XFLAGS_MASK,
> > > > so I left it this way.
> > > >
> > > > I don't see a problem with it keeping as is, but if it bothers you
> > > > I guess we can re-add the var later.
> > >
> > > Nah, it doesn't bother me that much.
> > >
> > > > > >       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
> > > >
> > > > We have beaten this API almost to death for sure ;)
> > > > I don't remember if we discussed this specific aspect,
> > > > but I am personally in favor of
> > > > EOPNOTSUPP := the fs does not support the set/get operation
> > > > EINVAL := some flags provided as value is invalid
> > > >
> > > > For example, if the get API provides you with a mask of the
> > > > valid flags that you can set, if you try to set flags outside of
> > > > that mask you get EINVAL.
> > > >
> > > > That's my interpretation, but I agree that EOPNOTSUPP can also
> > > > make sense in this situation.
> > >
> > > <nod> I think I'd rather EOPNOTSUPP for "bits are set that the kernel
> > > doesn't recognize" and EINVAL (or maybe something else like
> > > EPROTONOSUPPORT) for "fs driver will not let you change this bit".
> > > At least for the syscall interface; we probably have to flatten that to
> > > EOPNOTSUPP for both legacy ioctls.
>
> Given the precedents of returning EOPNOTSUPP in xfs_fileattr_set()
> and ext4_ioctl_setflags() for flags that cannot be set, I agree.
>

Wait, I misparsed what you wrote, so I think I "agreed" only to the
first part of your suggestion.

My claim is that unlike the xfs_has_v3inodes() check in
xfs_ioctl_setattr_xflags(),
ext4/f2fs etc return EOPNOTSUPP for various flags depending on supported fs
features (e.g. casefold,dax,encryption), so I think it will be hard to
impose a strict rule
where "fs does not support the feature" returns EINVAL in the syscalls API.

Therefore, I propose to change the code in this patch to
return EOPNOTSUPP for flags that kernel does not support
and with coming changes from Pali, it will also return the same
EOPNOTSUPP for flags that the fs instance does not support.

Christian,

Can you please amend the return value in the following chunk:

@@ -119,11 +120,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 -EOPNOTSUPP;
+

Thanks,
Amir.
Re: [PATCH v6 5/6] fs: prepare for extending file_get/setattr()
Posted by Christian Brauner 3 months, 1 week ago 
> Christian,
> 
> Can you please amend the return value in the following chunk:
> 
> @@ -119,11 +120,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 -EOPNOTSUPP;
> +

Done.
Re: [PATCH v6 5/6] fs: prepare for extending file_get/setattr()
Posted by Jan Kara 3 months, 1 week ago 
On Mon 30-06-25 18:20:15, 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>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

I'd just note that:

> @@ -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;
> +
>  	fileattr_fill_xflags(fa, xfa.fsx_xflags);
> +	fa->fsx_xflags &= ~FS_XFLAG_RDONLY_MASK;

This means that the two flags in FS_XFLAG_RDONLY_MASK cannot easily become
writeable in the future due to this. I think it is a sensible compromise
but I wanted to mention it.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.