* Re: [PATCH v6 5/6] fs: prepare for extending file_get/setattr()
From: Pali Rohár @ 2025-07-01 19:54 UTC (permalink / raw)
To: Darrick J. Wong
Cc: Amir Goldstein, Andrey Albershteyn, Arnd Bergmann,
Casey Schaufler, Christian Brauner, Jan Kara, Paul Moore,
linux-api, linux-fsdevel, linux-kernel, linux-xfs, selinux,
Andrey Albershteyn
In-Reply-To: <20250701194002.GS10009@frogsfrogsfrogs>
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.
> >
^ permalink raw reply
* Re: [PATCH v6 5/6] fs: prepare for extending file_get/setattr()
From: Darrick J. Wong @ 2025-07-01 19:40 UTC (permalink / raw)
To: Amir Goldstein
Cc: Andrey Albershteyn, Arnd Bergmann, Casey Schaufler,
Christian Brauner, Jan Kara, Pali Rohár, Paul Moore,
linux-api, linux-fsdevel, linux-kernel, linux-xfs, selinux,
Andrey Albershteyn
In-Reply-To: <CAOQ4uxiCpGcZ7V8OqssP2xKsN0ZiAO7mQ_1Qt705BrcHeSPmBg@mail.gmail.com>
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.
>
^ permalink raw reply
* Re: [PATCH v6 5/6] fs: prepare for extending file_get/setattr()
From: Amir Goldstein @ 2025-07-01 19:27 UTC (permalink / raw)
To: Darrick J. Wong
Cc: Andrey Albershteyn, Arnd Bergmann, Casey Schaufler,
Christian Brauner, Jan Kara, Pali Rohár, Paul Moore,
linux-api, linux-fsdevel, linux-kernel, linux-xfs, selinux,
Andrey Albershteyn
In-Reply-To: <20250701183105.GP10009@frogsfrogsfrogs>
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.
^ permalink raw reply
* Re: [PATCH v6 6/6] fs: introduce file_getattr and file_setattr syscalls
From: Pali Rohár @ 2025-07-01 19:17 UTC (permalink / raw)
To: Darrick J. Wong
Cc: Andrey Albershteyn, Amir Goldstein, Arnd Bergmann,
Casey Schaufler, Christian Brauner, Jan Kara, Paul Moore,
linux-api, linux-fsdevel, linux-kernel, linux-xfs, selinux,
Andrey Albershteyn
In-Reply-To: <20250701190857.GR10009@frogsfrogsfrogs>
On Tuesday 01 July 2025 12:08:57 Darrick J. Wong wrote:
> On Tue, Jul 01, 2025 at 08:54:57PM +0200, Pali Rohár wrote:
> > On Tuesday 01 July 2025 11:43:17 Darrick J. Wong wrote:
> > > On Mon, Jun 30, 2025 at 06:20:16PM +0200, Andrey Albershteyn wrote:
> > > > diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> > > > index 0098b0ce8ccb..0784f2033ba4 100644
> > > > --- a/include/uapi/linux/fs.h
> > > > +++ b/include/uapi/linux/fs.h
> > > > @@ -148,6 +148,24 @@ struct fsxattr {
> > > > unsigned char fsx_pad[8];
> > > > };
> > > >
> > > > +/*
> > > > + * Variable size structure for file_[sg]et_attr().
> > > > + *
> > > > + * Note. This is alternative to the structure 'struct fileattr'/'struct fsxattr'.
> > > > + * As this structure is passed to/from userspace with its size, this can
> > > > + * be versioned based on the size.
> > > > + */
> > > > +struct fsx_fileattr {
> > > > + __u32 fsx_xflags; /* xflags field value (get/set) */
> > >
> > > Should this to be __u64 from the start? Seeing as (a) this struct is
> > > not already a multiple of 8 bytes and (b) it's likely that we'll have to
> > > add a u64 field at some point. That would also address brauner's
> > > comment about padding.
> >
> > Hello!
> >
> > As I have already mentioned, after this syscall API/ABI is finished, I'm
> > planning to prepare patches for changing just selected fields / flags by
> > introducing a new mask field, and support for additional flags used by
> > existing filesystems (like windows flags).
> >
> > My idea is extending this structure for a new "u32 fsx_xflags_mask"
> > and new "u32 fsx_xflags2" + "u32 fsx_xflags2_mask". (field names are
> > just examples).
> >
> > So in case you are extending the structure now, please consider if it
> > makes sense to add all members, so we do not have to define 2 or 3
> > structure versions in near feature.
> >
> > Your idea of __u64 for fsx_xflags means that it will already cover the
> > "u32 fsx_xflags2" field.
>
> Ah, ok, so that work *is* still coming. :)
Yes. I'm just waiting until this patch series is accepted.
In past I have already sent RFC patches to the list which modifies the
existing ioctl interface. So you can look at it if you want :-)
> Are you still planning to add masks for xflags bits that are clearable
> and settable? i.e.
>
> __u64 fa_xflags; /* state */
> ...
> <end of V0 structure>
>
> __u64 fa_xflags_mask; /* bits for setattr to examine */
> __u64 fa_xflags_clearable; /* clearable bits */
> __u64 fa_xflags_settable; /* settable bits */
>
> I think it's easier just to define u64 in the V0 structure and then add
> the three new fields in V1. What do you think?
I wanted the interface which would allow to atomically change specified
bit/flag without the need for get-modify-set. And I think that this
would not work as the fa_xflags requires the state.
My idea is following:
__u64 fa_xflags;
...
<end of V0 structure>
__u64 fa_xflags_mask;
The fa_xflags_mask will specify which bits from the fa_xflags and from
other fa_* fields in V0 struct are going to be changed.
> --D
>
> > > --D
> > >
> > > > + __u32 fsx_extsize; /* extsize field value (get/set)*/
> > > > + __u32 fsx_nextents; /* nextents field value (get) */
> > > > + __u32 fsx_projid; /* project identifier (get/set) */
> > > > + __u32 fsx_cowextsize; /* CoW extsize field value (get/set) */
> > > > +};
> > > > +
> > > > +#define FSX_FILEATTR_SIZE_VER0 20
> > > > +#define FSX_FILEATTR_SIZE_LATEST FSX_FILEATTR_SIZE_VER0
> > > > +
> > > > /*
> > > > * Flags for the fsx_xflags field
> > > > */
> > > > diff --git a/scripts/syscall.tbl b/scripts/syscall.tbl
> > > > index 580b4e246aec..d1ae5e92c615 100644
> > > > --- a/scripts/syscall.tbl
> > > > +++ b/scripts/syscall.tbl
> > > > @@ -408,3 +408,5 @@
> > > > 465 common listxattrat sys_listxattrat
> > > > 466 common removexattrat sys_removexattrat
> > > > 467 common open_tree_attr sys_open_tree_attr
> > > > +468 common file_getattr sys_file_getattr
> > > > +469 common file_setattr sys_file_setattr
> > > >
> > > > --
> > > > 2.47.2
> > > >
> > > >
> >
^ permalink raw reply
* Re: [PATCH v6 6/6] fs: introduce file_getattr and file_setattr syscalls
From: Darrick J. Wong @ 2025-07-01 19:08 UTC (permalink / raw)
To: Pali Rohár
Cc: Andrey Albershteyn, Amir Goldstein, Arnd Bergmann,
Casey Schaufler, Christian Brauner, Jan Kara, Paul Moore,
linux-api, linux-fsdevel, linux-kernel, linux-xfs, selinux,
Andrey Albershteyn
In-Reply-To: <20250701185457.jvbwhiiihdauymrg@pali>
On Tue, Jul 01, 2025 at 08:54:57PM +0200, Pali Rohár wrote:
> On Tuesday 01 July 2025 11:43:17 Darrick J. Wong wrote:
> > On Mon, Jun 30, 2025 at 06:20:16PM +0200, Andrey Albershteyn wrote:
> > > diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> > > index 0098b0ce8ccb..0784f2033ba4 100644
> > > --- a/include/uapi/linux/fs.h
> > > +++ b/include/uapi/linux/fs.h
> > > @@ -148,6 +148,24 @@ struct fsxattr {
> > > unsigned char fsx_pad[8];
> > > };
> > >
> > > +/*
> > > + * Variable size structure for file_[sg]et_attr().
> > > + *
> > > + * Note. This is alternative to the structure 'struct fileattr'/'struct fsxattr'.
> > > + * As this structure is passed to/from userspace with its size, this can
> > > + * be versioned based on the size.
> > > + */
> > > +struct fsx_fileattr {
> > > + __u32 fsx_xflags; /* xflags field value (get/set) */
> >
> > Should this to be __u64 from the start? Seeing as (a) this struct is
> > not already a multiple of 8 bytes and (b) it's likely that we'll have to
> > add a u64 field at some point. That would also address brauner's
> > comment about padding.
>
> Hello!
>
> As I have already mentioned, after this syscall API/ABI is finished, I'm
> planning to prepare patches for changing just selected fields / flags by
> introducing a new mask field, and support for additional flags used by
> existing filesystems (like windows flags).
>
> My idea is extending this structure for a new "u32 fsx_xflags_mask"
> and new "u32 fsx_xflags2" + "u32 fsx_xflags2_mask". (field names are
> just examples).
>
> So in case you are extending the structure now, please consider if it
> makes sense to add all members, so we do not have to define 2 or 3
> structure versions in near feature.
>
> Your idea of __u64 for fsx_xflags means that it will already cover the
> "u32 fsx_xflags2" field.
Ah, ok, so that work *is* still coming. :)
Are you still planning to add masks for xflags bits that are clearable
and settable? i.e.
__u64 fa_xflags; /* state */
...
<end of V0 structure>
__u64 fa_xflags_mask; /* bits for setattr to examine */
__u64 fa_xflags_clearable; /* clearable bits */
__u64 fa_xflags_settable; /* settable bits */
I think it's easier just to define u64 in the V0 structure and then add
the three new fields in V1. What do you think?
--D
> > --D
> >
> > > + __u32 fsx_extsize; /* extsize field value (get/set)*/
> > > + __u32 fsx_nextents; /* nextents field value (get) */
> > > + __u32 fsx_projid; /* project identifier (get/set) */
> > > + __u32 fsx_cowextsize; /* CoW extsize field value (get/set) */
> > > +};
> > > +
> > > +#define FSX_FILEATTR_SIZE_VER0 20
> > > +#define FSX_FILEATTR_SIZE_LATEST FSX_FILEATTR_SIZE_VER0
> > > +
> > > /*
> > > * Flags for the fsx_xflags field
> > > */
> > > diff --git a/scripts/syscall.tbl b/scripts/syscall.tbl
> > > index 580b4e246aec..d1ae5e92c615 100644
> > > --- a/scripts/syscall.tbl
> > > +++ b/scripts/syscall.tbl
> > > @@ -408,3 +408,5 @@
> > > 465 common listxattrat sys_listxattrat
> > > 466 common removexattrat sys_removexattrat
> > > 467 common open_tree_attr sys_open_tree_attr
> > > +468 common file_getattr sys_file_getattr
> > > +469 common file_setattr sys_file_setattr
> > >
> > > --
> > > 2.47.2
> > >
> > >
>
^ permalink raw reply
* Re: [RFC v2 01/22] kernel/api: introduce kernel API specification framework
From: Jonathan Corbet @ 2025-07-01 19:01 UTC (permalink / raw)
To: Sasha Levin, Mauro Carvalho Chehab
Cc: linux-kernel, linux-doc, linux-api, workflows, tools,
Kate Stewart, Gabriele Paoloni, Chuck Wolber
In-Reply-To: <aGPvR-Mj6aR4Y8B5@lappy>
[Adding some of the ELISA folks, who are working in a related area and
might have thoughts on this. You can find the patch series under
discussion at:
https://lore.kernel.org/all/20250624180742.5795-1-sashal@kernel.org
]
Sasha Levin <sashal@kernel.org> writes:
> 1. Add new section patterns to doc_sect regex in to include API
> specification sections: api-type, api-version, param-type, param-flags,
> param-constraint, error-code, capability, signal, lock-req, since...
Easily enough done - you can never have too many regexes :)
> 2. Create new output module (scripts/lib/kdoc/kdoc_apispec.py?) to
> generate C macro invocations from parsed data.
>
> Which will generate output like:
>
> DEFINE_KERNEL_API_SPEC(function_name)
> KAPI_DESCRIPTION("...")
> KAPI_PARAM(0, "name", "type", "desc")
> KAPI_PARAM_TYPE(KAPI_TYPE_INT)
> KAPI_PARAM_FLAGS(KAPI_PARAM_IN)
> KAPI_PARAM_END
> KAPI_END_SPEC
Also shouldn't be all that hard.
> 3. And then via makefile we can:
> - Generate API specs from kerneldoc comments
> - Include generated specs conditionally based on CONFIG_KERNEL_API_SPEC
>
> Allowing us to just have these in the relevant source files:
> #ifdef CONFIG_KERNEL_API_SPEC
> #include "socket.apispec.h"
> #endif
...seems like it should work.
> In theory, all of that will let us have something like the following in
> kerneldoc:
>
> - @api-type: syscall
> - @api-version: 1
> - @context-flags: KAPI_CTX_PROCESS | KAPI_CTX_SLEEPABLE
> - @param-type: family, KAPI_TYPE_INT
> - @param-flags: family, KAPI_PARAM_IN
> - @param-range: family, 0, 45
> - @param-mask: type, SOCK_TYPE_MASK | SOCK_CLOEXEC | SOCK_NONBLOCK
> - @error-code: -EAFNOSUPPORT, "Address family not supported"
> - @error-condition: -EAFNOSUPPORT, "family < 0 || family >= NPROTO"
> - @capability: CAP_NET_RAW, KAPI_CAP_GRANT_PERMISSION
> - @capability-allows: CAP_NET_RAW, "Create SOCK_RAW sockets"
> - @since: 2.0
> - @return-type: KAPI_TYPE_FD
> - @return-check: KAPI_RETURN_ERROR_CHECK
>
> How does it sound? I'm pretty excited about the possiblity to align this
> with kerneldoc. Please poke holes in the plan :)
I think we could do it without all the @signs. We'd also want to see
how well we could integrate that information with the minimal structure
we already have: getting the return-value information into the Returns:
section, for example, and tying the parameter constraints to the
parameter descriptions we already have.
The other thing I would really like to see, to the extent we can, is
that a bunch of patches adding all this data to the source will actually
be accepted by the relevant maintainers. It would be a shame to get all
this infrastructure into place, then have things stall out due to
maintainer pushback. Maybe you should start by annotating the
scheduler-related system calls; if that works the rest should be a piece
of cake :)
Thanks,
jon
^ permalink raw reply
* Re: [PATCH v6 6/6] fs: introduce file_getattr and file_setattr syscalls
From: Pali Rohár @ 2025-07-01 18:54 UTC (permalink / raw)
To: Darrick J. Wong
Cc: Andrey Albershteyn, Amir Goldstein, Arnd Bergmann,
Casey Schaufler, Christian Brauner, Jan Kara, Paul Moore,
linux-api, linux-fsdevel, linux-kernel, linux-xfs, selinux,
Andrey Albershteyn
In-Reply-To: <20250701184317.GQ10009@frogsfrogsfrogs>
On Tuesday 01 July 2025 11:43:17 Darrick J. Wong wrote:
> On Mon, Jun 30, 2025 at 06:20:16PM +0200, Andrey Albershteyn wrote:
> > diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> > index 0098b0ce8ccb..0784f2033ba4 100644
> > --- a/include/uapi/linux/fs.h
> > +++ b/include/uapi/linux/fs.h
> > @@ -148,6 +148,24 @@ struct fsxattr {
> > unsigned char fsx_pad[8];
> > };
> >
> > +/*
> > + * Variable size structure for file_[sg]et_attr().
> > + *
> > + * Note. This is alternative to the structure 'struct fileattr'/'struct fsxattr'.
> > + * As this structure is passed to/from userspace with its size, this can
> > + * be versioned based on the size.
> > + */
> > +struct fsx_fileattr {
> > + __u32 fsx_xflags; /* xflags field value (get/set) */
>
> Should this to be __u64 from the start? Seeing as (a) this struct is
> not already a multiple of 8 bytes and (b) it's likely that we'll have to
> add a u64 field at some point. That would also address brauner's
> comment about padding.
Hello!
As I have already mentioned, after this syscall API/ABI is finished, I'm
planning to prepare patches for changing just selected fields / flags by
introducing a new mask field, and support for additional flags used by
existing filesystems (like windows flags).
My idea is extending this structure for a new "u32 fsx_xflags_mask"
and new "u32 fsx_xflags2" + "u32 fsx_xflags2_mask". (field names are
just examples).
So in case you are extending the structure now, please consider if it
makes sense to add all members, so we do not have to define 2 or 3
structure versions in near feature.
Your idea of __u64 for fsx_xflags means that it will already cover the
"u32 fsx_xflags2" field.
> --D
>
> > + __u32 fsx_extsize; /* extsize field value (get/set)*/
> > + __u32 fsx_nextents; /* nextents field value (get) */
> > + __u32 fsx_projid; /* project identifier (get/set) */
> > + __u32 fsx_cowextsize; /* CoW extsize field value (get/set) */
> > +};
> > +
> > +#define FSX_FILEATTR_SIZE_VER0 20
> > +#define FSX_FILEATTR_SIZE_LATEST FSX_FILEATTR_SIZE_VER0
> > +
> > /*
> > * Flags for the fsx_xflags field
> > */
> > diff --git a/scripts/syscall.tbl b/scripts/syscall.tbl
> > index 580b4e246aec..d1ae5e92c615 100644
> > --- a/scripts/syscall.tbl
> > +++ b/scripts/syscall.tbl
> > @@ -408,3 +408,5 @@
> > 465 common listxattrat sys_listxattrat
> > 466 common removexattrat sys_removexattrat
> > 467 common open_tree_attr sys_open_tree_attr
> > +468 common file_getattr sys_file_getattr
> > +469 common file_setattr sys_file_setattr
> >
> > --
> > 2.47.2
> >
> >
^ permalink raw reply
* Re: [PATCH v6 6/6] fs: introduce file_getattr and file_setattr syscalls
From: Darrick J. Wong @ 2025-07-01 18:43 UTC (permalink / raw)
To: Andrey Albershteyn
Cc: Amir Goldstein, Arnd Bergmann, Casey Schaufler, Christian Brauner,
Jan Kara, Pali Rohár, Paul Moore, linux-api, linux-fsdevel,
linux-kernel, linux-xfs, selinux, Andrey Albershteyn
In-Reply-To: <20250630-xattrat-syscall-v6-6-c4e3bc35227b@kernel.org>
On Mon, Jun 30, 2025 at 06:20:16PM +0200, Andrey Albershteyn wrote:
> From: Andrey Albershteyn <aalbersh@redhat.com>
>
> Introduce file_getattr() and file_setattr() syscalls to manipulate inode
> extended attributes. The syscalls takes pair of file descriptor and
> pathname. Then it operates on inode opened accroding to openat()
> semantics. The struct fsx_fileattr is passed to obtain/change extended
> attributes.
>
> This is an alternative to FS_IOC_FSSETXATTR ioctl with a difference
> that file don't need to be open as we can reference it with a path
> instead of fd. By having this we can manipulated inode extended
> attributes not only on regular files but also on special ones. This
> is not possible with FS_IOC_FSSETXATTR ioctl as with special files
> we can not call ioctl() directly on the filesystem inode using fd.
>
> This patch adds two new syscalls which allows userspace to get/set
> extended inode attributes on special files by using parent directory
> and a path - *at() like syscall.
>
> CC: linux-api@vger.kernel.org
> CC: linux-fsdevel@vger.kernel.org
> CC: linux-xfs@vger.kernel.org
> Signed-off-by: Andrey Albershteyn <aalbersh@kernel.org>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> ---
<snip syscall table>
> diff --git a/fs/file_attr.c b/fs/file_attr.c
> index 62f08872d4ad..fda9d847eee5 100644
> --- a/fs/file_attr.c
> +++ b/fs/file_attr.c
> @@ -3,6 +3,10 @@
> #include <linux/security.h>
> #include <linux/fscrypt.h>
> #include <linux/fileattr.h>
> +#include <linux/syscalls.h>
> +#include <linux/namei.h>
> +
> +#include "internal.h"
>
> /**
> * fileattr_fill_xflags - initialize fileattr with xflags
> @@ -89,6 +93,19 @@ int vfs_fileattr_get(struct dentry *dentry, struct fileattr *fa)
> }
> EXPORT_SYMBOL(vfs_fileattr_get);
>
> +static void fileattr_to_fsx_fileattr(const struct fileattr *fa,
> + struct fsx_fileattr *fsx)
Er... "fsx_fileattr" is the struct that the system call uses?
That's a little confusing considering that xfs already has a
xfs_fill_fsxattr function that actually fills a struct fileattr.
That could be renamed xfs_fill_fileattr.
I dunno. There's a part of me that would really rather that the
file_getattr and file_setattr syscalls operate on a struct file_attr.
More whining/bikeshedding to come.
<snip stuff that looks ok to me>
<<well, I still dislike the CLASS(fd, fd)(fd) syntax...>>
> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> index 0098b0ce8ccb..0784f2033ba4 100644
> --- a/include/uapi/linux/fs.h
> +++ b/include/uapi/linux/fs.h
> @@ -148,6 +148,24 @@ struct fsxattr {
> unsigned char fsx_pad[8];
> };
>
> +/*
> + * Variable size structure for file_[sg]et_attr().
> + *
> + * Note. This is alternative to the structure 'struct fileattr'/'struct fsxattr'.
> + * As this structure is passed to/from userspace with its size, this can
> + * be versioned based on the size.
> + */
> +struct fsx_fileattr {
> + __u32 fsx_xflags; /* xflags field value (get/set) */
Should this to be __u64 from the start? Seeing as (a) this struct is
not already a multiple of 8 bytes and (b) it's likely that we'll have to
add a u64 field at some point. That would also address brauner's
comment about padding.
--D
> + __u32 fsx_extsize; /* extsize field value (get/set)*/
> + __u32 fsx_nextents; /* nextents field value (get) */
> + __u32 fsx_projid; /* project identifier (get/set) */
> + __u32 fsx_cowextsize; /* CoW extsize field value (get/set) */
> +};
> +
> +#define FSX_FILEATTR_SIZE_VER0 20
> +#define FSX_FILEATTR_SIZE_LATEST FSX_FILEATTR_SIZE_VER0
> +
> /*
> * Flags for the fsx_xflags field
> */
> diff --git a/scripts/syscall.tbl b/scripts/syscall.tbl
> index 580b4e246aec..d1ae5e92c615 100644
> --- a/scripts/syscall.tbl
> +++ b/scripts/syscall.tbl
> @@ -408,3 +408,5 @@
> 465 common listxattrat sys_listxattrat
> 466 common removexattrat sys_removexattrat
> 467 common open_tree_attr sys_open_tree_attr
> +468 common file_getattr sys_file_getattr
> +469 common file_setattr sys_file_setattr
>
> --
> 2.47.2
>
>
^ permalink raw reply
* Re: [PATCH v6 5/6] fs: prepare for extending file_get/setattr()
From: Darrick J. Wong @ 2025-07-01 18:31 UTC (permalink / raw)
To: Andrey Albershteyn
Cc: Amir Goldstein, Arnd Bergmann, Casey Schaufler, Christian Brauner,
Jan Kara, Pali Rohár, Paul Moore, linux-api, linux-fsdevel,
linux-kernel, linux-xfs, selinux, Andrey Albershteyn
In-Reply-To: <20250630-xattrat-syscall-v6-5-c4e3bc35227b@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
>
>
^ permalink raw reply
* Re: [PATCH v6 4/6] fs: make vfs_fileattr_[get|set] return -EOPNOSUPP
From: Darrick J. Wong @ 2025-07-01 18:18 UTC (permalink / raw)
To: Andrey Albershteyn
Cc: Amir Goldstein, Arnd Bergmann, Casey Schaufler, Christian Brauner,
Jan Kara, Pali Rohár, Paul Moore, linux-api, linux-fsdevel,
linux-kernel, linux-xfs, selinux, Andrey Albershteyn
In-Reply-To: <20250630-xattrat-syscall-v6-4-c4e3bc35227b@kernel.org>
On Mon, Jun 30, 2025 at 06:20:14PM +0200, Andrey Albershteyn wrote:
> Future patches will add new syscalls which use these functions. As
> this interface won't be used for ioctls only, the EOPNOSUPP is more
> appropriate return code.
>
> This patch converts return code from ENOIOCTLCMD to EOPNOSUPP for
> vfs_fileattr_get and vfs_fileattr_set. To save old behavior translate
> EOPNOSUPP back for current users - overlayfs, encryptfs and fs/ioctl.c.
>
> Signed-off-by: Andrey Albershteyn <aalbersh@kernel.org>
With EOPNOSUPP -> EOPNOTSUPP corrected,
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
--D
> ---
> fs/ecryptfs/inode.c | 8 +++++++-
> fs/file_attr.c | 12 ++++++++++--
> fs/overlayfs/inode.c | 2 +-
> 3 files changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
> index 493d7f194956..a55c1375127f 100644
> --- a/fs/ecryptfs/inode.c
> +++ b/fs/ecryptfs/inode.c
> @@ -1126,7 +1126,13 @@ static int ecryptfs_removexattr(struct dentry *dentry, struct inode *inode,
>
> static int ecryptfs_fileattr_get(struct dentry *dentry, struct fileattr *fa)
> {
> - return vfs_fileattr_get(ecryptfs_dentry_to_lower(dentry), fa);
> + int rc;
> +
> + rc = vfs_fileattr_get(ecryptfs_dentry_to_lower(dentry), fa);
> + if (rc == -EOPNOTSUPP)
> + rc = -ENOIOCTLCMD;
> +
> + return rc;
> }
>
> static int ecryptfs_fileattr_set(struct mnt_idmap *idmap,
> diff --git a/fs/file_attr.c b/fs/file_attr.c
> index be62d97cc444..4e85fa00c092 100644
> --- a/fs/file_attr.c
> +++ b/fs/file_attr.c
> @@ -79,7 +79,7 @@ int vfs_fileattr_get(struct dentry *dentry, struct fileattr *fa)
> int error;
>
> if (!inode->i_op->fileattr_get)
> - return -ENOIOCTLCMD;
> + return -EOPNOTSUPP;
>
> error = security_inode_file_getattr(dentry, fa);
> if (error)
> @@ -229,7 +229,7 @@ int vfs_fileattr_set(struct mnt_idmap *idmap, struct dentry *dentry,
> int err;
>
> if (!inode->i_op->fileattr_set)
> - return -ENOIOCTLCMD;
> + return -EOPNOTSUPP;
>
> if (!inode_owner_or_capable(idmap, inode))
> return -EPERM;
> @@ -271,6 +271,8 @@ int ioctl_getflags(struct file *file, unsigned int __user *argp)
> int err;
>
> err = vfs_fileattr_get(file->f_path.dentry, &fa);
> + if (err == -EOPNOTSUPP)
> + err = -ENOIOCTLCMD;
> if (!err)
> err = put_user(fa.flags, argp);
> return err;
> @@ -292,6 +294,8 @@ int ioctl_setflags(struct file *file, unsigned int __user *argp)
> fileattr_fill_flags(&fa, flags);
> err = vfs_fileattr_set(idmap, dentry, &fa);
> mnt_drop_write_file(file);
> + if (err == -EOPNOTSUPP)
> + err = -ENOIOCTLCMD;
> }
> }
> return err;
> @@ -304,6 +308,8 @@ int ioctl_fsgetxattr(struct file *file, void __user *argp)
> int err;
>
> err = vfs_fileattr_get(file->f_path.dentry, &fa);
> + if (err == -EOPNOTSUPP)
> + err = -ENOIOCTLCMD;
> if (!err)
> err = copy_fsxattr_to_user(&fa, argp);
>
> @@ -324,6 +330,8 @@ int ioctl_fssetxattr(struct file *file, void __user *argp)
> if (!err) {
> err = vfs_fileattr_set(idmap, dentry, &fa);
> mnt_drop_write_file(file);
> + if (err == -EOPNOTSUPP)
> + err = -ENOIOCTLCMD;
> }
> }
> return err;
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 6f0e15f86c21..096d44712bb1 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -721,7 +721,7 @@ int ovl_real_fileattr_get(const struct path *realpath, struct fileattr *fa)
> return err;
>
> err = vfs_fileattr_get(realpath->dentry, fa);
> - if (err == -ENOIOCTLCMD)
> + if (err == -EOPNOTSUPP)
> err = -ENOTTY;
> return err;
> }
>
> --
> 2.47.2
>
>
^ permalink raw reply
* Re: [PATCH v6 2/6] lsm: introduce new hooks for setting/getting inode fsxattr
From: Darrick J. Wong @ 2025-07-01 18:18 UTC (permalink / raw)
To: Andrey Albershteyn
Cc: Amir Goldstein, Arnd Bergmann, Casey Schaufler, Christian Brauner,
Jan Kara, Pali Rohár, Paul Moore, linux-api, linux-fsdevel,
linux-kernel, linux-xfs, selinux, Andrey Albershteyn
In-Reply-To: <20250630-xattrat-syscall-v6-2-c4e3bc35227b@kernel.org>
On Mon, Jun 30, 2025 at 06:20:12PM +0200, Andrey Albershteyn wrote:
> Introduce new hooks for setting and getting filesystem extended
> attributes on inode (FS_IOC_FSGETXATTR).
>
> Cc: selinux@vger.kernel.org
> Cc: Paul Moore <paul@paul-moore.com>
>
> Acked-by: Paul Moore <paul@paul-moore.com>
> Signed-off-by: Andrey Albershteyn <aalbersh@kernel.org>
I wonder, were FS_IOC_FS[GS]ETXATTR already covered by the
security_file_ioctl hook? If so, will an out of date security policy
on a 6.17 kernel now fail to check the new file_[gs]etattr syscalls?
Though AFAICT the future of managing these "extra" file attributes is
the system call so it's probably appropriate to have an explicit
callout to LSMs.
Acked-by: "Darrick J. Wong" <djwong@kernel.org>
--D
> ---
> fs/file_attr.c | 19 ++++++++++++++++---
> include/linux/lsm_hook_defs.h | 2 ++
> include/linux/security.h | 16 ++++++++++++++++
> security/security.c | 30 ++++++++++++++++++++++++++++++
> 4 files changed, 64 insertions(+), 3 deletions(-)
>
> diff --git a/fs/file_attr.c b/fs/file_attr.c
> index 2910b7047721..be62d97cc444 100644
> --- a/fs/file_attr.c
> +++ b/fs/file_attr.c
> @@ -76,10 +76,15 @@ EXPORT_SYMBOL(fileattr_fill_flags);
> int vfs_fileattr_get(struct dentry *dentry, struct fileattr *fa)
> {
> struct inode *inode = d_inode(dentry);
> + int error;
>
> if (!inode->i_op->fileattr_get)
> return -ENOIOCTLCMD;
>
> + error = security_inode_file_getattr(dentry, fa);
> + if (error)
> + return error;
> +
> return inode->i_op->fileattr_get(dentry, fa);
> }
> EXPORT_SYMBOL(vfs_fileattr_get);
> @@ -242,12 +247,20 @@ int vfs_fileattr_set(struct mnt_idmap *idmap, struct dentry *dentry,
> } else {
> fa->flags |= old_ma.flags & ~FS_COMMON_FL;
> }
> +
> err = fileattr_set_prepare(inode, &old_ma, fa);
> - if (!err)
> - err = inode->i_op->fileattr_set(idmap, dentry, fa);
> + if (err)
> + goto out;
> + err = security_inode_file_setattr(dentry, fa);
> + if (err)
> + goto out;
> + err = inode->i_op->fileattr_set(idmap, dentry, fa);
> + if (err)
> + goto out;
> }
> +
> +out:
> inode_unlock(inode);
> -
> return err;
> }
> EXPORT_SYMBOL(vfs_fileattr_set);
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index bf3bbac4e02a..9600a4350e79 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -157,6 +157,8 @@ LSM_HOOK(int, 0, inode_removexattr, struct mnt_idmap *idmap,
> struct dentry *dentry, const char *name)
> LSM_HOOK(void, LSM_RET_VOID, inode_post_removexattr, struct dentry *dentry,
> const char *name)
> +LSM_HOOK(int, 0, inode_file_setattr, struct dentry *dentry, struct fileattr *fa)
> +LSM_HOOK(int, 0, inode_file_getattr, struct dentry *dentry, struct fileattr *fa)
> LSM_HOOK(int, 0, inode_set_acl, struct mnt_idmap *idmap,
> struct dentry *dentry, const char *acl_name, struct posix_acl *kacl)
> LSM_HOOK(void, LSM_RET_VOID, inode_post_set_acl, struct dentry *dentry,
> diff --git a/include/linux/security.h b/include/linux/security.h
> index dba349629229..9ed0d0e0c81f 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -451,6 +451,10 @@ int security_inode_listxattr(struct dentry *dentry);
> int security_inode_removexattr(struct mnt_idmap *idmap,
> struct dentry *dentry, const char *name);
> void security_inode_post_removexattr(struct dentry *dentry, const char *name);
> +int security_inode_file_setattr(struct dentry *dentry,
> + struct fileattr *fa);
> +int security_inode_file_getattr(struct dentry *dentry,
> + struct fileattr *fa);
> int security_inode_need_killpriv(struct dentry *dentry);
> int security_inode_killpriv(struct mnt_idmap *idmap, struct dentry *dentry);
> int security_inode_getsecurity(struct mnt_idmap *idmap,
> @@ -1052,6 +1056,18 @@ static inline void security_inode_post_removexattr(struct dentry *dentry,
> const char *name)
> { }
>
> +static inline int security_inode_file_setattr(struct dentry *dentry,
> + struct fileattr *fa)
> +{
> + return 0;
> +}
> +
> +static inline int security_inode_file_getattr(struct dentry *dentry,
> + struct fileattr *fa)
> +{
> + return 0;
> +}
> +
> static inline int security_inode_need_killpriv(struct dentry *dentry)
> {
> return cap_inode_need_killpriv(dentry);
> diff --git a/security/security.c b/security/security.c
> index 596d41818577..711b4de40b8d 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2622,6 +2622,36 @@ void security_inode_post_removexattr(struct dentry *dentry, const char *name)
> call_void_hook(inode_post_removexattr, dentry, name);
> }
>
> +/**
> + * security_inode_file_setattr() - check if setting fsxattr is allowed
> + * @dentry: file to set filesystem extended attributes on
> + * @fa: extended attributes to set on the inode
> + *
> + * Called when file_setattr() syscall or FS_IOC_FSSETXATTR ioctl() is called on
> + * inode
> + *
> + * Return: Returns 0 if permission is granted.
> + */
> +int security_inode_file_setattr(struct dentry *dentry, struct fileattr *fa)
> +{
> + return call_int_hook(inode_file_setattr, dentry, fa);
> +}
> +
> +/**
> + * security_inode_file_getattr() - check if retrieving fsxattr is allowed
> + * @dentry: file to retrieve filesystem extended attributes from
> + * @fa: extended attributes to get
> + *
> + * Called when file_getattr() syscall or FS_IOC_FSGETXATTR ioctl() is called on
> + * inode
> + *
> + * Return: Returns 0 if permission is granted.
> + */
> +int security_inode_file_getattr(struct dentry *dentry, struct fileattr *fa)
> +{
> + return call_int_hook(inode_file_getattr, dentry, fa);
> +}
> +
> /**
> * security_inode_need_killpriv() - Check if security_inode_killpriv() required
> * @dentry: associated dentry
>
> --
> 2.47.2
>
>
^ permalink raw reply
* Re: [PATCH v6 1/6] fs: split fileattr related helpers into separate file
From: Darrick J. Wong @ 2025-07-01 18:13 UTC (permalink / raw)
To: Andrey Albershteyn
Cc: Amir Goldstein, Arnd Bergmann, Casey Schaufler, Christian Brauner,
Jan Kara, Pali Rohár, Paul Moore, linux-api, linux-fsdevel,
linux-kernel, linux-xfs, selinux, Andrey Albershteyn
In-Reply-To: <20250630-xattrat-syscall-v6-1-c4e3bc35227b@kernel.org>
On Mon, Jun 30, 2025 at 06:20:11PM +0200, Andrey Albershteyn wrote:
> From: Andrey Albershteyn <aalbersh@kernel.org>
>
> This patch moves function related to file extended attributes
> manipulations to separate file. Refactoring only.
>
> Signed-off-by: Andrey Albershteyn <aalbersh@kernel.org>
Seems fine to me to move that to a separate file.
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
--D
> ---
> fs/Makefile | 3 +-
> fs/file_attr.c | 318 +++++++++++++++++++++++++++++++++++++++++++++++
> fs/ioctl.c | 309 ---------------------------------------------
> include/linux/fileattr.h | 4 +
> 4 files changed, 324 insertions(+), 310 deletions(-)
>
> diff --git a/fs/Makefile b/fs/Makefile
> index 79c08b914c47..334654f9584b 100644
> --- a/fs/Makefile
> +++ b/fs/Makefile
> @@ -15,7 +15,8 @@ obj-y := open.o read_write.o file_table.o super.o \
> pnode.o splice.o sync.o utimes.o d_path.o \
> stack.o fs_struct.o statfs.o fs_pin.o nsfs.o \
> fs_types.o fs_context.o fs_parser.o fsopen.o init.o \
> - kernel_read_file.o mnt_idmapping.o remap_range.o pidfs.o
> + kernel_read_file.o mnt_idmapping.o remap_range.o pidfs.o \
> + file_attr.o
>
> obj-$(CONFIG_BUFFER_HEAD) += buffer.o mpage.o
> obj-$(CONFIG_PROC_FS) += proc_namespace.o
> diff --git a/fs/file_attr.c b/fs/file_attr.c
> new file mode 100644
> index 000000000000..2910b7047721
> --- /dev/null
> +++ b/fs/file_attr.c
> @@ -0,0 +1,318 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/fs.h>
> +#include <linux/security.h>
> +#include <linux/fscrypt.h>
> +#include <linux/fileattr.h>
> +
> +/**
> + * fileattr_fill_xflags - initialize fileattr with xflags
> + * @fa: fileattr pointer
> + * @xflags: FS_XFLAG_* flags
> + *
> + * Set ->fsx_xflags, ->fsx_valid and ->flags (translated xflags). All
> + * other fields are zeroed.
> + */
> +void fileattr_fill_xflags(struct fileattr *fa, u32 xflags)
> +{
> + memset(fa, 0, sizeof(*fa));
> + fa->fsx_valid = true;
> + fa->fsx_xflags = xflags;
> + if (fa->fsx_xflags & FS_XFLAG_IMMUTABLE)
> + fa->flags |= FS_IMMUTABLE_FL;
> + if (fa->fsx_xflags & FS_XFLAG_APPEND)
> + fa->flags |= FS_APPEND_FL;
> + if (fa->fsx_xflags & FS_XFLAG_SYNC)
> + fa->flags |= FS_SYNC_FL;
> + if (fa->fsx_xflags & FS_XFLAG_NOATIME)
> + fa->flags |= FS_NOATIME_FL;
> + if (fa->fsx_xflags & FS_XFLAG_NODUMP)
> + fa->flags |= FS_NODUMP_FL;
> + if (fa->fsx_xflags & FS_XFLAG_DAX)
> + fa->flags |= FS_DAX_FL;
> + if (fa->fsx_xflags & FS_XFLAG_PROJINHERIT)
> + fa->flags |= FS_PROJINHERIT_FL;
> +}
> +EXPORT_SYMBOL(fileattr_fill_xflags);
> +
> +/**
> + * fileattr_fill_flags - initialize fileattr with flags
> + * @fa: fileattr pointer
> + * @flags: FS_*_FL flags
> + *
> + * Set ->flags, ->flags_valid and ->fsx_xflags (translated flags).
> + * All other fields are zeroed.
> + */
> +void fileattr_fill_flags(struct fileattr *fa, u32 flags)
> +{
> + memset(fa, 0, sizeof(*fa));
> + fa->flags_valid = true;
> + fa->flags = flags;
> + if (fa->flags & FS_SYNC_FL)
> + fa->fsx_xflags |= FS_XFLAG_SYNC;
> + if (fa->flags & FS_IMMUTABLE_FL)
> + fa->fsx_xflags |= FS_XFLAG_IMMUTABLE;
> + if (fa->flags & FS_APPEND_FL)
> + fa->fsx_xflags |= FS_XFLAG_APPEND;
> + if (fa->flags & FS_NODUMP_FL)
> + fa->fsx_xflags |= FS_XFLAG_NODUMP;
> + if (fa->flags & FS_NOATIME_FL)
> + fa->fsx_xflags |= FS_XFLAG_NOATIME;
> + if (fa->flags & FS_DAX_FL)
> + fa->fsx_xflags |= FS_XFLAG_DAX;
> + if (fa->flags & FS_PROJINHERIT_FL)
> + fa->fsx_xflags |= FS_XFLAG_PROJINHERIT;
> +}
> +EXPORT_SYMBOL(fileattr_fill_flags);
> +
> +/**
> + * vfs_fileattr_get - retrieve miscellaneous file attributes
> + * @dentry: the object to retrieve from
> + * @fa: fileattr pointer
> + *
> + * Call i_op->fileattr_get() callback, if exists.
> + *
> + * Return: 0 on success, or a negative error on failure.
> + */
> +int vfs_fileattr_get(struct dentry *dentry, struct fileattr *fa)
> +{
> + struct inode *inode = d_inode(dentry);
> +
> + if (!inode->i_op->fileattr_get)
> + return -ENOIOCTLCMD;
> +
> + return inode->i_op->fileattr_get(dentry, fa);
> +}
> +EXPORT_SYMBOL(vfs_fileattr_get);
> +
> +/**
> + * copy_fsxattr_to_user - copy fsxattr to userspace.
> + * @fa: fileattr pointer
> + * @ufa: fsxattr user pointer
> + *
> + * Return: 0 on success, or -EFAULT on failure.
> + */
> +int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa)
> +{
> + struct fsxattr xfa;
> +
> + memset(&xfa, 0, sizeof(xfa));
> + xfa.fsx_xflags = fa->fsx_xflags;
> + xfa.fsx_extsize = fa->fsx_extsize;
> + xfa.fsx_nextents = fa->fsx_nextents;
> + xfa.fsx_projid = fa->fsx_projid;
> + xfa.fsx_cowextsize = fa->fsx_cowextsize;
> +
> + if (copy_to_user(ufa, &xfa, sizeof(xfa)))
> + return -EFAULT;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(copy_fsxattr_to_user);
> +
> +static int copy_fsxattr_from_user(struct fileattr *fa,
> + struct fsxattr __user *ufa)
> +{
> + struct fsxattr xfa;
> +
> + if (copy_from_user(&xfa, ufa, sizeof(xfa)))
> + return -EFAULT;
> +
> + fileattr_fill_xflags(fa, xfa.fsx_xflags);
> + fa->fsx_extsize = xfa.fsx_extsize;
> + fa->fsx_nextents = xfa.fsx_nextents;
> + fa->fsx_projid = xfa.fsx_projid;
> + fa->fsx_cowextsize = xfa.fsx_cowextsize;
> +
> + return 0;
> +}
> +
> +/*
> + * Generic function to check FS_IOC_FSSETXATTR/FS_IOC_SETFLAGS values and reject
> + * any invalid configurations.
> + *
> + * Note: must be called with inode lock held.
> + */
> +static int fileattr_set_prepare(struct inode *inode,
> + const struct fileattr *old_ma,
> + struct fileattr *fa)
> +{
> + int err;
> +
> + /*
> + * The IMMUTABLE and APPEND_ONLY flags can only be changed by
> + * the relevant capability.
> + */
> + if ((fa->flags ^ old_ma->flags) & (FS_APPEND_FL | FS_IMMUTABLE_FL) &&
> + !capable(CAP_LINUX_IMMUTABLE))
> + return -EPERM;
> +
> + err = fscrypt_prepare_setflags(inode, old_ma->flags, fa->flags);
> + if (err)
> + return err;
> +
> + /*
> + * Project Quota ID state is only allowed to change from within the init
> + * namespace. Enforce that restriction only if we are trying to change
> + * the quota ID state. Everything else is allowed in user namespaces.
> + */
> + if (current_user_ns() != &init_user_ns) {
> + if (old_ma->fsx_projid != fa->fsx_projid)
> + return -EINVAL;
> + if ((old_ma->fsx_xflags ^ fa->fsx_xflags) &
> + FS_XFLAG_PROJINHERIT)
> + return -EINVAL;
> + } else {
> + /*
> + * Caller is allowed to change the project ID. If it is being
> + * changed, make sure that the new value is valid.
> + */
> + if (old_ma->fsx_projid != fa->fsx_projid &&
> + !projid_valid(make_kprojid(&init_user_ns, fa->fsx_projid)))
> + return -EINVAL;
> + }
> +
> + /* Check extent size hints. */
> + if ((fa->fsx_xflags & FS_XFLAG_EXTSIZE) && !S_ISREG(inode->i_mode))
> + return -EINVAL;
> +
> + if ((fa->fsx_xflags & FS_XFLAG_EXTSZINHERIT) &&
> + !S_ISDIR(inode->i_mode))
> + return -EINVAL;
> +
> + if ((fa->fsx_xflags & FS_XFLAG_COWEXTSIZE) &&
> + !S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
> + return -EINVAL;
> +
> + /*
> + * It is only valid to set the DAX flag on regular files and
> + * directories on filesystems.
> + */
> + if ((fa->fsx_xflags & FS_XFLAG_DAX) &&
> + !(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)))
> + return -EINVAL;
> +
> + /* Extent size hints of zero turn off the flags. */
> + if (fa->fsx_extsize == 0)
> + fa->fsx_xflags &= ~(FS_XFLAG_EXTSIZE | FS_XFLAG_EXTSZINHERIT);
> + if (fa->fsx_cowextsize == 0)
> + fa->fsx_xflags &= ~FS_XFLAG_COWEXTSIZE;
> +
> + return 0;
> +}
> +
> +/**
> + * vfs_fileattr_set - change miscellaneous file attributes
> + * @idmap: idmap of the mount
> + * @dentry: the object to change
> + * @fa: fileattr pointer
> + *
> + * After verifying permissions, call i_op->fileattr_set() callback, if
> + * exists.
> + *
> + * Verifying attributes involves retrieving current attributes with
> + * i_op->fileattr_get(), this also allows initializing attributes that have
> + * not been set by the caller to current values. Inode lock is held
> + * thoughout to prevent racing with another instance.
> + *
> + * Return: 0 on success, or a negative error on failure.
> + */
> +int vfs_fileattr_set(struct mnt_idmap *idmap, struct dentry *dentry,
> + struct fileattr *fa)
> +{
> + struct inode *inode = d_inode(dentry);
> + struct fileattr old_ma = {};
> + int err;
> +
> + if (!inode->i_op->fileattr_set)
> + return -ENOIOCTLCMD;
> +
> + if (!inode_owner_or_capable(idmap, inode))
> + return -EPERM;
> +
> + inode_lock(inode);
> + err = vfs_fileattr_get(dentry, &old_ma);
> + if (!err) {
> + /* initialize missing bits from old_ma */
> + if (fa->flags_valid) {
> + fa->fsx_xflags |= old_ma.fsx_xflags & ~FS_XFLAG_COMMON;
> + fa->fsx_extsize = old_ma.fsx_extsize;
> + fa->fsx_nextents = old_ma.fsx_nextents;
> + fa->fsx_projid = old_ma.fsx_projid;
> + fa->fsx_cowextsize = old_ma.fsx_cowextsize;
> + } else {
> + fa->flags |= old_ma.flags & ~FS_COMMON_FL;
> + }
> + err = fileattr_set_prepare(inode, &old_ma, fa);
> + if (!err)
> + err = inode->i_op->fileattr_set(idmap, dentry, fa);
> + }
> + inode_unlock(inode);
> +
> + return err;
> +}
> +EXPORT_SYMBOL(vfs_fileattr_set);
> +
> +int ioctl_getflags(struct file *file, unsigned int __user *argp)
> +{
> + struct fileattr fa = { .flags_valid = true }; /* hint only */
> + int err;
> +
> + err = vfs_fileattr_get(file->f_path.dentry, &fa);
> + if (!err)
> + err = put_user(fa.flags, argp);
> + return err;
> +}
> +EXPORT_SYMBOL(ioctl_getflags);
> +
> +int ioctl_setflags(struct file *file, unsigned int __user *argp)
> +{
> + struct mnt_idmap *idmap = file_mnt_idmap(file);
> + struct dentry *dentry = file->f_path.dentry;
> + struct fileattr fa;
> + unsigned int flags;
> + int err;
> +
> + err = get_user(flags, argp);
> + if (!err) {
> + err = mnt_want_write_file(file);
> + if (!err) {
> + fileattr_fill_flags(&fa, flags);
> + err = vfs_fileattr_set(idmap, dentry, &fa);
> + mnt_drop_write_file(file);
> + }
> + }
> + return err;
> +}
> +EXPORT_SYMBOL(ioctl_setflags);
> +
> +int ioctl_fsgetxattr(struct file *file, void __user *argp)
> +{
> + struct fileattr fa = { .fsx_valid = true }; /* hint only */
> + int err;
> +
> + err = vfs_fileattr_get(file->f_path.dentry, &fa);
> + if (!err)
> + err = copy_fsxattr_to_user(&fa, argp);
> +
> + return err;
> +}
> +EXPORT_SYMBOL(ioctl_fsgetxattr);
> +
> +int ioctl_fssetxattr(struct file *file, void __user *argp)
> +{
> + struct mnt_idmap *idmap = file_mnt_idmap(file);
> + struct dentry *dentry = file->f_path.dentry;
> + struct fileattr fa;
> + int err;
> +
> + err = copy_fsxattr_from_user(&fa, argp);
> + if (!err) {
> + err = mnt_want_write_file(file);
> + if (!err) {
> + err = vfs_fileattr_set(idmap, dentry, &fa);
> + mnt_drop_write_file(file);
> + }
> + }
> + return err;
> +}
> +EXPORT_SYMBOL(ioctl_fssetxattr);
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 69107a245b4c..0248cb8db2d3 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -453,315 +453,6 @@ static int ioctl_file_dedupe_range(struct file *file,
> return ret;
> }
>
> -/**
> - * fileattr_fill_xflags - initialize fileattr with xflags
> - * @fa: fileattr pointer
> - * @xflags: FS_XFLAG_* flags
> - *
> - * Set ->fsx_xflags, ->fsx_valid and ->flags (translated xflags). All
> - * other fields are zeroed.
> - */
> -void fileattr_fill_xflags(struct fileattr *fa, u32 xflags)
> -{
> - memset(fa, 0, sizeof(*fa));
> - fa->fsx_valid = true;
> - fa->fsx_xflags = xflags;
> - if (fa->fsx_xflags & FS_XFLAG_IMMUTABLE)
> - fa->flags |= FS_IMMUTABLE_FL;
> - if (fa->fsx_xflags & FS_XFLAG_APPEND)
> - fa->flags |= FS_APPEND_FL;
> - if (fa->fsx_xflags & FS_XFLAG_SYNC)
> - fa->flags |= FS_SYNC_FL;
> - if (fa->fsx_xflags & FS_XFLAG_NOATIME)
> - fa->flags |= FS_NOATIME_FL;
> - if (fa->fsx_xflags & FS_XFLAG_NODUMP)
> - fa->flags |= FS_NODUMP_FL;
> - if (fa->fsx_xflags & FS_XFLAG_DAX)
> - fa->flags |= FS_DAX_FL;
> - if (fa->fsx_xflags & FS_XFLAG_PROJINHERIT)
> - fa->flags |= FS_PROJINHERIT_FL;
> -}
> -EXPORT_SYMBOL(fileattr_fill_xflags);
> -
> -/**
> - * fileattr_fill_flags - initialize fileattr with flags
> - * @fa: fileattr pointer
> - * @flags: FS_*_FL flags
> - *
> - * Set ->flags, ->flags_valid and ->fsx_xflags (translated flags).
> - * All other fields are zeroed.
> - */
> -void fileattr_fill_flags(struct fileattr *fa, u32 flags)
> -{
> - memset(fa, 0, sizeof(*fa));
> - fa->flags_valid = true;
> - fa->flags = flags;
> - if (fa->flags & FS_SYNC_FL)
> - fa->fsx_xflags |= FS_XFLAG_SYNC;
> - if (fa->flags & FS_IMMUTABLE_FL)
> - fa->fsx_xflags |= FS_XFLAG_IMMUTABLE;
> - if (fa->flags & FS_APPEND_FL)
> - fa->fsx_xflags |= FS_XFLAG_APPEND;
> - if (fa->flags & FS_NODUMP_FL)
> - fa->fsx_xflags |= FS_XFLAG_NODUMP;
> - if (fa->flags & FS_NOATIME_FL)
> - fa->fsx_xflags |= FS_XFLAG_NOATIME;
> - if (fa->flags & FS_DAX_FL)
> - fa->fsx_xflags |= FS_XFLAG_DAX;
> - if (fa->flags & FS_PROJINHERIT_FL)
> - fa->fsx_xflags |= FS_XFLAG_PROJINHERIT;
> -}
> -EXPORT_SYMBOL(fileattr_fill_flags);
> -
> -/**
> - * vfs_fileattr_get - retrieve miscellaneous file attributes
> - * @dentry: the object to retrieve from
> - * @fa: fileattr pointer
> - *
> - * Call i_op->fileattr_get() callback, if exists.
> - *
> - * Return: 0 on success, or a negative error on failure.
> - */
> -int vfs_fileattr_get(struct dentry *dentry, struct fileattr *fa)
> -{
> - struct inode *inode = d_inode(dentry);
> -
> - if (!inode->i_op->fileattr_get)
> - return -ENOIOCTLCMD;
> -
> - return inode->i_op->fileattr_get(dentry, fa);
> -}
> -EXPORT_SYMBOL(vfs_fileattr_get);
> -
> -/**
> - * copy_fsxattr_to_user - copy fsxattr to userspace.
> - * @fa: fileattr pointer
> - * @ufa: fsxattr user pointer
> - *
> - * Return: 0 on success, or -EFAULT on failure.
> - */
> -int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa)
> -{
> - struct fsxattr xfa;
> -
> - memset(&xfa, 0, sizeof(xfa));
> - xfa.fsx_xflags = fa->fsx_xflags;
> - xfa.fsx_extsize = fa->fsx_extsize;
> - xfa.fsx_nextents = fa->fsx_nextents;
> - xfa.fsx_projid = fa->fsx_projid;
> - xfa.fsx_cowextsize = fa->fsx_cowextsize;
> -
> - if (copy_to_user(ufa, &xfa, sizeof(xfa)))
> - return -EFAULT;
> -
> - return 0;
> -}
> -EXPORT_SYMBOL(copy_fsxattr_to_user);
> -
> -static int copy_fsxattr_from_user(struct fileattr *fa,
> - struct fsxattr __user *ufa)
> -{
> - struct fsxattr xfa;
> -
> - if (copy_from_user(&xfa, ufa, sizeof(xfa)))
> - return -EFAULT;
> -
> - fileattr_fill_xflags(fa, xfa.fsx_xflags);
> - fa->fsx_extsize = xfa.fsx_extsize;
> - fa->fsx_nextents = xfa.fsx_nextents;
> - fa->fsx_projid = xfa.fsx_projid;
> - fa->fsx_cowextsize = xfa.fsx_cowextsize;
> -
> - return 0;
> -}
> -
> -/*
> - * Generic function to check FS_IOC_FSSETXATTR/FS_IOC_SETFLAGS values and reject
> - * any invalid configurations.
> - *
> - * Note: must be called with inode lock held.
> - */
> -static int fileattr_set_prepare(struct inode *inode,
> - const struct fileattr *old_ma,
> - struct fileattr *fa)
> -{
> - int err;
> -
> - /*
> - * The IMMUTABLE and APPEND_ONLY flags can only be changed by
> - * the relevant capability.
> - */
> - if ((fa->flags ^ old_ma->flags) & (FS_APPEND_FL | FS_IMMUTABLE_FL) &&
> - !capable(CAP_LINUX_IMMUTABLE))
> - return -EPERM;
> -
> - err = fscrypt_prepare_setflags(inode, old_ma->flags, fa->flags);
> - if (err)
> - return err;
> -
> - /*
> - * Project Quota ID state is only allowed to change from within the init
> - * namespace. Enforce that restriction only if we are trying to change
> - * the quota ID state. Everything else is allowed in user namespaces.
> - */
> - if (current_user_ns() != &init_user_ns) {
> - if (old_ma->fsx_projid != fa->fsx_projid)
> - return -EINVAL;
> - if ((old_ma->fsx_xflags ^ fa->fsx_xflags) &
> - FS_XFLAG_PROJINHERIT)
> - return -EINVAL;
> - } else {
> - /*
> - * Caller is allowed to change the project ID. If it is being
> - * changed, make sure that the new value is valid.
> - */
> - if (old_ma->fsx_projid != fa->fsx_projid &&
> - !projid_valid(make_kprojid(&init_user_ns, fa->fsx_projid)))
> - return -EINVAL;
> - }
> -
> - /* Check extent size hints. */
> - if ((fa->fsx_xflags & FS_XFLAG_EXTSIZE) && !S_ISREG(inode->i_mode))
> - return -EINVAL;
> -
> - if ((fa->fsx_xflags & FS_XFLAG_EXTSZINHERIT) &&
> - !S_ISDIR(inode->i_mode))
> - return -EINVAL;
> -
> - if ((fa->fsx_xflags & FS_XFLAG_COWEXTSIZE) &&
> - !S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
> - return -EINVAL;
> -
> - /*
> - * It is only valid to set the DAX flag on regular files and
> - * directories on filesystems.
> - */
> - if ((fa->fsx_xflags & FS_XFLAG_DAX) &&
> - !(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)))
> - return -EINVAL;
> -
> - /* Extent size hints of zero turn off the flags. */
> - if (fa->fsx_extsize == 0)
> - fa->fsx_xflags &= ~(FS_XFLAG_EXTSIZE | FS_XFLAG_EXTSZINHERIT);
> - if (fa->fsx_cowextsize == 0)
> - fa->fsx_xflags &= ~FS_XFLAG_COWEXTSIZE;
> -
> - return 0;
> -}
> -
> -/**
> - * vfs_fileattr_set - change miscellaneous file attributes
> - * @idmap: idmap of the mount
> - * @dentry: the object to change
> - * @fa: fileattr pointer
> - *
> - * After verifying permissions, call i_op->fileattr_set() callback, if
> - * exists.
> - *
> - * Verifying attributes involves retrieving current attributes with
> - * i_op->fileattr_get(), this also allows initializing attributes that have
> - * not been set by the caller to current values. Inode lock is held
> - * thoughout to prevent racing with another instance.
> - *
> - * Return: 0 on success, or a negative error on failure.
> - */
> -int vfs_fileattr_set(struct mnt_idmap *idmap, struct dentry *dentry,
> - struct fileattr *fa)
> -{
> - struct inode *inode = d_inode(dentry);
> - struct fileattr old_ma = {};
> - int err;
> -
> - if (!inode->i_op->fileattr_set)
> - return -ENOIOCTLCMD;
> -
> - if (!inode_owner_or_capable(idmap, inode))
> - return -EPERM;
> -
> - inode_lock(inode);
> - err = vfs_fileattr_get(dentry, &old_ma);
> - if (!err) {
> - /* initialize missing bits from old_ma */
> - if (fa->flags_valid) {
> - fa->fsx_xflags |= old_ma.fsx_xflags & ~FS_XFLAG_COMMON;
> - fa->fsx_extsize = old_ma.fsx_extsize;
> - fa->fsx_nextents = old_ma.fsx_nextents;
> - fa->fsx_projid = old_ma.fsx_projid;
> - fa->fsx_cowextsize = old_ma.fsx_cowextsize;
> - } else {
> - fa->flags |= old_ma.flags & ~FS_COMMON_FL;
> - }
> - err = fileattr_set_prepare(inode, &old_ma, fa);
> - if (!err)
> - err = inode->i_op->fileattr_set(idmap, dentry, fa);
> - }
> - inode_unlock(inode);
> -
> - return err;
> -}
> -EXPORT_SYMBOL(vfs_fileattr_set);
> -
> -static int ioctl_getflags(struct file *file, unsigned int __user *argp)
> -{
> - struct fileattr fa = { .flags_valid = true }; /* hint only */
> - int err;
> -
> - err = vfs_fileattr_get(file->f_path.dentry, &fa);
> - if (!err)
> - err = put_user(fa.flags, argp);
> - return err;
> -}
> -
> -static int ioctl_setflags(struct file *file, unsigned int __user *argp)
> -{
> - struct mnt_idmap *idmap = file_mnt_idmap(file);
> - struct dentry *dentry = file->f_path.dentry;
> - struct fileattr fa;
> - unsigned int flags;
> - int err;
> -
> - err = get_user(flags, argp);
> - if (!err) {
> - err = mnt_want_write_file(file);
> - if (!err) {
> - fileattr_fill_flags(&fa, flags);
> - err = vfs_fileattr_set(idmap, dentry, &fa);
> - mnt_drop_write_file(file);
> - }
> - }
> - return err;
> -}
> -
> -static int ioctl_fsgetxattr(struct file *file, void __user *argp)
> -{
> - struct fileattr fa = { .fsx_valid = true }; /* hint only */
> - int err;
> -
> - err = vfs_fileattr_get(file->f_path.dentry, &fa);
> - if (!err)
> - err = copy_fsxattr_to_user(&fa, argp);
> -
> - return err;
> -}
> -
> -static int ioctl_fssetxattr(struct file *file, void __user *argp)
> -{
> - struct mnt_idmap *idmap = file_mnt_idmap(file);
> - struct dentry *dentry = file->f_path.dentry;
> - struct fileattr fa;
> - int err;
> -
> - err = copy_fsxattr_from_user(&fa, argp);
> - if (!err) {
> - err = mnt_want_write_file(file);
> - if (!err) {
> - err = vfs_fileattr_set(idmap, dentry, &fa);
> - mnt_drop_write_file(file);
> - }
> - }
> - return err;
> -}
> -
> static int ioctl_getfsuuid(struct file *file, void __user *argp)
> {
> struct super_block *sb = file_inode(file)->i_sb;
> diff --git a/include/linux/fileattr.h b/include/linux/fileattr.h
> index 47c05a9851d0..6030d0bf7ad3 100644
> --- a/include/linux/fileattr.h
> +++ b/include/linux/fileattr.h
> @@ -55,5 +55,9 @@ static inline bool fileattr_has_fsx(const struct fileattr *fa)
> int vfs_fileattr_get(struct dentry *dentry, struct fileattr *fa);
> int vfs_fileattr_set(struct mnt_idmap *idmap, struct dentry *dentry,
> struct fileattr *fa);
> +int ioctl_getflags(struct file *file, unsigned int __user *argp);
> +int ioctl_setflags(struct file *file, unsigned int __user *argp);
> +int ioctl_fsgetxattr(struct file *file, void __user *argp);
> +int ioctl_fssetxattr(struct file *file, void __user *argp);
>
> #endif /* _LINUX_FILEATTR_H */
>
> --
> 2.47.2
>
>
^ permalink raw reply
* Re: [RFC v2 01/22] kernel/api: introduce kernel API specification framework
From: Mauro Carvalho Chehab @ 2025-07-01 15:25 UTC (permalink / raw)
To: Sasha Levin
Cc: Jonathan Corbet, linux-kernel, linux-doc, linux-api, workflows,
tools
In-Reply-To: <aGPvR-Mj6aR4Y8B5@lappy>
Em Tue, 1 Jul 2025 10:23:03 -0400
Sasha Levin <sashal@kernel.org> escreveu:
> On Tue, Jul 01, 2025 at 12:20:58AM +0200, Mauro Carvalho Chehab wrote:
> >Em Mon, 30 Jun 2025 13:53:55 -0600
> >Jonathan Corbet <corbet@lwn.net> escreveu:
> >
> >> Sasha Levin <sashal@kernel.org> writes:
> >>
> >> > Add a comprehensive framework for formally documenting kernel APIs with
> >> > inline specifications. This framework provides:
> >> >
> >> > - Structured API documentation with parameter specifications, return
> >> > values, error conditions, and execution context requirements
> >> > - Runtime validation capabilities for debugging (CONFIG_KAPI_RUNTIME_CHECKS)
> >> > - Export of specifications via debugfs for tooling integration
> >> > - Support for both internal kernel APIs and system calls
> >> >
> >> > The framework stores specifications in a dedicated ELF section and
> >> > provides infrastructure for:
> >> > - Compile-time validation of specifications
> >> > - Runtime querying of API documentation
> >> > - Machine-readable export formats
> >> > - Integration with existing SYSCALL_DEFINE macros
> >> >
> >> > This commit introduces the core infrastructure without modifying any
> >> > existing APIs. Subsequent patches will add specifications to individual
> >> > subsystems.
> >> >
> >> > Signed-off-by: Sasha Levin <sashal@kernel.org>
> >> > ---
> >> > Documentation/admin-guide/kernel-api-spec.rst | 507 ++++++
> >>
> >> You need to add that file to index.rst in that directory or it won't be
> >> pulled into the docs build.
> >>
> >> Wouldn't it be nice to integrate all this stuff with out existing
> >> kerneldoc mechanism...? :)
> >
> >+1
> >
> >Having two different mechanisms (kapi and kerneldoc) makes a lot harder
> >to maintain kAPI.
>
> I hated the idea of not reusing kerneldoc.
>
> My concern with kerneldoc was that I can't manipulate the
> information it stores in the context of a kernel build. So for example,
> I wasn't sure how I can expose information stored within kerneldoc via
> debugfs on a running system (or how I can store it within the vmlinux
> for later extraction from the binary built kernel).
>
> I did some research based on your proposal, and I think I was incorrect
> with the assumption above. I suppose we could do something like the
> following:
>
> 1. Add new section patterns to doc_sect regex in to include API
> specification sections: api-type, api-version, param-type, param-flags,
> param-constraint, error-code, capability, signal, lock-req, since...
>
> 2. Create new output module (scripts/lib/kdoc/kdoc_apispec.py?) to
> generate C macro invocations from parsed data.
>
> Which will generate output like:
>
> DEFINE_KERNEL_API_SPEC(function_name)
> KAPI_DESCRIPTION("...")
> KAPI_PARAM(0, "name", "type", "desc")
> KAPI_PARAM_TYPE(KAPI_TYPE_INT)
> KAPI_PARAM_FLAGS(KAPI_PARAM_IN)
> KAPI_PARAM_END
> KAPI_END_SPEC
> 3. And then via makefile we can:
> - Generate API specs from kerneldoc comments
> - Include generated specs conditionally based on CONFIG_KERNEL_API_SPEC
>
> Allowing us to just have these in the relevant source files:
> #ifdef CONFIG_KERNEL_API_SPEC
> #include "socket.apispec.h"
> #endif
>
>
> In theory, all of that will let us have something like the following in
> kerneldoc:
>
> - @api-type: syscall
> - @api-version: 1
> - @context-flags: KAPI_CTX_PROCESS | KAPI_CTX_SLEEPABLE
> - @param-type: family, KAPI_TYPE_INT
> - @param-flags: family, KAPI_PARAM_IN
> - @param-range: family, 0, 45
> - @param-mask: type, SOCK_TYPE_MASK | SOCK_CLOEXEC | SOCK_NONBLOCK
> - @error-code: -EAFNOSUPPORT, "Address family not supported"
> - @error-condition: -EAFNOSUPPORT, "family < 0 || family >= NPROTO"
> - @capability: CAP_NET_RAW, KAPI_CAP_GRANT_PERMISSION
> - @capability-allows: CAP_NET_RAW, "Create SOCK_RAW sockets"
> - @since: 2.0
> - @return-type: KAPI_TYPE_FD
> - @return-check: KAPI_RETURN_ERROR_CHECK
>
> How does it sound? I'm pretty excited about the possiblity to align this
> with kerneldoc. Please poke holes in the plan :)
Sounds like a plan!
We did something somewhat similar on IGT.
The python classes there were written with the goal to document
tests, so its examples are related to test docs, but I wrote it
to be generic.
There, all fields comes form a JSON file like this:
https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/intel/xe_test_config.json?ref_type=heads
which describes what fields will be used. It also lists file
patterns that will use it. The fields allow hierarchical
grouping, with could be interesting for some types of fields.
From the json example (I dropped the optional field description
from the example, to make it cleaner):
"Category": {
"Mega feature": {
"Sub-category": {},
}
...
"Test category": {},
"Issue": {},
...
The hierarchical part is useful to properly order kapi content
without the need to add multiple Sphinx markups to manually reorder
the output inside the .rst files.
(*) I would avoid hardcoding the fields/structures, as eventually
we may need more flexibility to add fields and/or having some
fields that are specific, for instance, to debugfs or sysfs.
The python class it uses is at:
https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/scripts/test_list.py?ref_type=heads
and caller is at:
https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/scripts/igt_doc.py?ref_type=heads
Eventually you may find something useful there. If so, feel free to
pick from it.
Regards,
Mauro
^ permalink raw reply
* Re: [RFC v2 00/22] Kernel API specification framework
From: Sasha Levin @ 2025-07-01 14:54 UTC (permalink / raw)
To: Jake Edge; +Cc: linux-kernel, linux-doc, linux-api, workflows, tools
In-Reply-To: <87qzz0aaw1.fsf@pelicano.edge2.net>
On Mon, Jun 30, 2025 at 07:43:42PM -0700, Jake Edge wrote:
>
>Hi Sasha,
>
>On Tue, Jun 24 2025 14:07 -0400, Sasha Levin <sashal@kernel.org> wrote:
>
>> Hey folks,
>>
>> This is a second attempt at a "Kernel API Specification" framework,
>> addressing the feedback from the initial RFC and expanding the scope
>> to include sysfs attribute specifications.
>
>In light of your talk at OSS last week [1] (for non-subscribers [2]), I
>am wondering if any of this code has been written by coding LLMs. It
>seems like the kind of unpleasant boilerplate that they are said to be
>good at generating, but also seems like an enormous blob of "code" to
>review. What is the status of this specification in that regard?
Hey Jake!
The macro definitions were done mostly manually: it ended up being
more of a copy/paste/replace exercise to get all the different macros in
place (which, yes, ended up being a huge blob).
For the syscall/ioctl/sysfs APIs I used to demonstrate the
infrastructure, I started with defining the basic spec skeleton manually
based on our existing docs and code review, but then had LLMs extend it
based on it's review of the code.
If we do proceed with something along the lines of this spec, I can see
LLMs being useful at reviewing incoming code changes and alerting us of
required updates/changes to the spec (or, alerting us that we're
breaking the spec). Think of something like AUTOSEL but for
classification of commits that affect the userspace API.
The tools/kapi/ code is mostly mostly LLM generated.
--
Thanks,
Sasha
^ permalink raw reply
* Re: [RFC v2 01/22] kernel/api: introduce kernel API specification framework
From: Sasha Levin @ 2025-07-01 14:23 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Jonathan Corbet, linux-kernel, linux-doc, linux-api, workflows,
tools
In-Reply-To: <20250701002058.1cae5a7e@foz.lan>
On Tue, Jul 01, 2025 at 12:20:58AM +0200, Mauro Carvalho Chehab wrote:
>Em Mon, 30 Jun 2025 13:53:55 -0600
>Jonathan Corbet <corbet@lwn.net> escreveu:
>
>> Sasha Levin <sashal@kernel.org> writes:
>>
>> > Add a comprehensive framework for formally documenting kernel APIs with
>> > inline specifications. This framework provides:
>> >
>> > - Structured API documentation with parameter specifications, return
>> > values, error conditions, and execution context requirements
>> > - Runtime validation capabilities for debugging (CONFIG_KAPI_RUNTIME_CHECKS)
>> > - Export of specifications via debugfs for tooling integration
>> > - Support for both internal kernel APIs and system calls
>> >
>> > The framework stores specifications in a dedicated ELF section and
>> > provides infrastructure for:
>> > - Compile-time validation of specifications
>> > - Runtime querying of API documentation
>> > - Machine-readable export formats
>> > - Integration with existing SYSCALL_DEFINE macros
>> >
>> > This commit introduces the core infrastructure without modifying any
>> > existing APIs. Subsequent patches will add specifications to individual
>> > subsystems.
>> >
>> > Signed-off-by: Sasha Levin <sashal@kernel.org>
>> > ---
>> > Documentation/admin-guide/kernel-api-spec.rst | 507 ++++++
>>
>> You need to add that file to index.rst in that directory or it won't be
>> pulled into the docs build.
>>
>> Wouldn't it be nice to integrate all this stuff with out existing
>> kerneldoc mechanism...? :)
>
>+1
>
>Having two different mechanisms (kapi and kerneldoc) makes a lot harder
>to maintain kAPI.
I hated the idea of not reusing kerneldoc.
My concern with kerneldoc was that I can't manipulate the
information it stores in the context of a kernel build. So for example,
I wasn't sure how I can expose information stored within kerneldoc via
debugfs on a running system (or how I can store it within the vmlinux
for later extraction from the binary built kernel).
I did some research based on your proposal, and I think I was incorrect
with the assumption above. I suppose we could do something like the
following:
1. Add new section patterns to doc_sect regex in to include API
specification sections: api-type, api-version, param-type, param-flags,
param-constraint, error-code, capability, signal, lock-req, since...
2. Create new output module (scripts/lib/kdoc/kdoc_apispec.py?) to
generate C macro invocations from parsed data.
Which will generate output like:
DEFINE_KERNEL_API_SPEC(function_name)
KAPI_DESCRIPTION("...")
KAPI_PARAM(0, "name", "type", "desc")
KAPI_PARAM_TYPE(KAPI_TYPE_INT)
KAPI_PARAM_FLAGS(KAPI_PARAM_IN)
KAPI_PARAM_END
KAPI_END_SPEC
3. And then via makefile we can:
- Generate API specs from kerneldoc comments
- Include generated specs conditionally based on CONFIG_KERNEL_API_SPEC
Allowing us to just have these in the relevant source files:
#ifdef CONFIG_KERNEL_API_SPEC
#include "socket.apispec.h"
#endif
In theory, all of that will let us have something like the following in
kerneldoc:
- @api-type: syscall
- @api-version: 1
- @context-flags: KAPI_CTX_PROCESS | KAPI_CTX_SLEEPABLE
- @param-type: family, KAPI_TYPE_INT
- @param-flags: family, KAPI_PARAM_IN
- @param-range: family, 0, 45
- @param-mask: type, SOCK_TYPE_MASK | SOCK_CLOEXEC | SOCK_NONBLOCK
- @error-code: -EAFNOSUPPORT, "Address family not supported"
- @error-condition: -EAFNOSUPPORT, "family < 0 || family >= NPROTO"
- @capability: CAP_NET_RAW, KAPI_CAP_GRANT_PERMISSION
- @capability-allows: CAP_NET_RAW, "Create SOCK_RAW sockets"
- @since: 2.0
- @return-type: KAPI_TYPE_FD
- @return-check: KAPI_RETURN_ERROR_CHECK
How does it sound? I'm pretty excited about the possiblity to align this
with kerneldoc. Please poke holes in the plan :)
--
Thanks,
Sasha
^ permalink raw reply
* Re: [PATCH v6 4/6] fs: make vfs_fileattr_[get|set] return -EOPNOSUPP
From: Amir Goldstein @ 2025-07-01 14:16 UTC (permalink / raw)
To: Jan Kara
Cc: Andrey Albershteyn, Arnd Bergmann, Casey Schaufler,
Christian Brauner, Pali Rohár, Paul Moore, linux-api,
linux-fsdevel, linux-kernel, linux-xfs, selinux,
Andrey Albershteyn
In-Reply-To: <ozxxm5fglq5kuoiteqr34wghaqhxgue4kshz2jnnk7oopmhxk6@a2lo6weivsyz>
[-- Attachment #1: Type: text/plain, Size: 2222 bytes --]
On Tue, Jul 1, 2025 at 2:51 PM Jan Kara <jack@suse.cz> wrote:
>
> On Tue 01-07-25 08:05:45, Amir Goldstein wrote:
> > On Mon, Jun 30, 2025 at 6:20 PM Andrey Albershteyn <aalbersh@redhat.com> wrote:
> > >
> > > Future patches will add new syscalls which use these functions. As
> > > this interface won't be used for ioctls only, the EOPNOSUPP is more
> > > appropriate return code.
> > >
> > > This patch converts return code from ENOIOCTLCMD to EOPNOSUPP for
> > > vfs_fileattr_get and vfs_fileattr_set. To save old behavior translate
> > > EOPNOSUPP back for current users - overlayfs, encryptfs and fs/ioctl.c.
> > >
> > > Signed-off-by: Andrey Albershteyn <aalbersh@kernel.org>
> ...
> > > --- a/fs/overlayfs/inode.c
> > > +++ b/fs/overlayfs/inode.c
> > > @@ -721,7 +721,7 @@ int ovl_real_fileattr_get(const struct path *realpath, struct fileattr *fa)
> > > return err;
> > >
> > > err = vfs_fileattr_get(realpath->dentry, fa);
> > > - if (err == -ENOIOCTLCMD)
> > > + if (err == -EOPNOTSUPP)
> > > err = -ENOTTY;
> > > return err;
> > > }
> >
> > That's the wrong way, because it hides the desired -EOPNOTSUPP
> > return code from ovl_fileattr_get().
> >
> > The conversion to -ENOTTY was done for
> > 5b0a414d06c3 ("ovl: fix filattr copy-up failure"),
> > so please do this instead:
> >
> > --- a/fs/overlayfs/inode.c
> > +++ b/fs/overlayfs/inode.c
> > @@ -722,7 +722,7 @@ int ovl_real_fileattr_get(const struct path
> > *realpath, struct fileattr *fa)
> >
> > err = vfs_fileattr_get(realpath->dentry, fa);
> > if (err == -ENOIOCTLCMD)
> > - err = -ENOTTY;
> > + err = -EOPNOTSUPP;
>
> Is this really needed? AFAICS nobody returns ENOIOCTLCMD after this
> patch...
you are right it is not needed
Attaching the patch with missing bits of fuse and overlayfs to make this
conversion complete.
Christian, please squash my patch
and afterward make sure there is no conversion remaining in
ovl_real_fileattr_get() as well as in ecryptfs_fileattr_get()
Both those helpers should return the value they
got from vfs_fileattr_get() as is.
Thanks,
Amir.
[-- Attachment #2: 0001-fuse-return-EOPNOTSUPP-from-fileattr_-gs-et-instead-.patch --]
[-- Type: text/x-patch, Size: 2017 bytes --]
From 85d097f639518670c57827513b02f497950071de Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@gmail.com>
Date: Tue, 1 Jul 2025 16:06:44 +0200
Subject: [PATCH] fuse: return -EOPNOTSUPP from ->fileattr_[gs]et() instead of
-ENOTTY
As part of changing calling convenstion of ->fileattr_[gs]et()
to return -EOPNOTSUPP and fix related overlayfs code.
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
fs/fuse/ioctl.c | 4 ++++
fs/overlayfs/copy_up.c | 2 +-
fs/overlayfs/inode.c | 2 +-
3 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/fs/fuse/ioctl.c b/fs/fuse/ioctl.c
index 2d9abf48828f..f2692f7d5932 100644
--- a/fs/fuse/ioctl.c
+++ b/fs/fuse/ioctl.c
@@ -536,6 +536,8 @@ int fuse_fileattr_get(struct dentry *dentry, struct fileattr *fa)
cleanup:
fuse_priv_ioctl_cleanup(inode, ff);
+ if (err == -ENOTTY)
+ err = -EOPNOTSUPP;
return err;
}
@@ -572,5 +574,7 @@ int fuse_fileattr_set(struct mnt_idmap *idmap,
cleanup:
fuse_priv_ioctl_cleanup(inode, ff);
+ if (err == -ENOTTY)
+ err = -EOPNOTSUPP;
return err;
}
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index d7310fcf3888..2c646b7076d0 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -178,7 +178,7 @@ static int ovl_copy_fileattr(struct inode *inode, const struct path *old,
err = ovl_real_fileattr_get(old, &oldfa);
if (err) {
/* Ntfs-3g returns -EINVAL for "no fileattr support" */
- if (err == -ENOTTY || err == -EINVAL)
+ if (err == -EOPNOTSUPP || err == -EINVAL)
return 0;
pr_warn("failed to retrieve lower fileattr (%pd2, err=%i)\n",
old->dentry, err);
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 6f0e15f86c21..92754749f316 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -722,7 +722,7 @@ int ovl_real_fileattr_get(const struct path *realpath, struct fileattr *fa)
err = vfs_fileattr_get(realpath->dentry, fa);
if (err == -ENOIOCTLCMD)
- err = -ENOTTY;
+ err = -EOPNOTSUPP;
return err;
}
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v6 6/6] fs: introduce file_getattr and file_setattr syscalls
From: Jan Kara @ 2025-07-01 13:24 UTC (permalink / raw)
To: Andrey Albershteyn
Cc: Amir Goldstein, Arnd Bergmann, Casey Schaufler, Christian Brauner,
Jan Kara, Pali Rohár, Paul Moore, linux-api, linux-fsdevel,
linux-kernel, linux-xfs, selinux, Andrey Albershteyn
In-Reply-To: <20250630-xattrat-syscall-v6-6-c4e3bc35227b@kernel.org>
On Mon 30-06-25 18:20:16, Andrey Albershteyn wrote:
> From: Andrey Albershteyn <aalbersh@redhat.com>
>
> Introduce file_getattr() and file_setattr() syscalls to manipulate inode
> extended attributes. The syscalls takes pair of file descriptor and
> pathname. Then it operates on inode opened accroding to openat()
^^^ according
> semantics. The struct fsx_fileattr is passed to obtain/change extended
> attributes.
>
> This is an alternative to FS_IOC_FSSETXATTR ioctl with a difference
> that file don't need to be open as we can reference it with a path
^^^ doesn't
> instead of fd. By having this we can manipulated inode extended
> attributes not only on regular files but also on special ones. This
> is not possible with FS_IOC_FSSETXATTR ioctl as with special files
> we can not call ioctl() directly on the filesystem inode using fd.
>
> This patch adds two new syscalls which allows userspace to get/set
> extended inode attributes on special files by using parent directory
> and a path - *at() like syscall.
>
> CC: linux-api@vger.kernel.org
> CC: linux-fsdevel@vger.kernel.org
> CC: linux-xfs@vger.kernel.org
> Signed-off-by: Andrey Albershteyn <aalbersh@kernel.org>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
There's possible NULL ptr deref bug below (2x) that's easy to fix. Once
done feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
> @@ -343,3 +377,117 @@ int ioctl_fssetxattr(struct file *file, void __user *argp)
> return err;
> }
> EXPORT_SYMBOL(ioctl_fssetxattr);
> +
> +SYSCALL_DEFINE5(file_getattr, int, dfd, const char __user *, filename,
> + struct fsx_fileattr __user *, ufsx, size_t, usize,
> + unsigned int, at_flags)
> +{
> + struct fileattr fa;
> + struct path filepath __free(path_put) = {};
> + int error;
> + unsigned int lookup_flags = 0;
> + struct filename *name __free(putname) = NULL;
> + struct fsx_fileattr fsx;
> +
> + BUILD_BUG_ON(sizeof(struct fsx_fileattr) < FSX_FILEATTR_SIZE_VER0);
> + BUILD_BUG_ON(sizeof(struct fsx_fileattr) != FSX_FILEATTR_SIZE_LATEST);
> +
> + if ((at_flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
> + return -EINVAL;
> +
> + if (!(at_flags & AT_SYMLINK_NOFOLLOW))
> + lookup_flags |= LOOKUP_FOLLOW;
> +
> + if (usize > PAGE_SIZE)
> + return -E2BIG;
> +
> + if (usize < FSX_FILEATTR_SIZE_VER0)
> + return -EINVAL;
> +
> + name = getname_maybe_null(filename, at_flags);
> + if (IS_ERR(name))
> + return PTR_ERR(name);
> +
> + if (!name && dfd >= 0) {
> + CLASS(fd, f)(dfd);
> +
> + filepath = fd_file(f)->f_path;
If dfd is not correct fd, then this will dereference NULL AFAICT. I think
you need here:
if (fd_empty(f))
return -EBADF;
> + path_get(&filepath);
> + } else {
> + error = filename_lookup(dfd, name, lookup_flags, &filepath,
> + NULL);
> + if (error)
> + return error;
> + }
> +
> + error = vfs_fileattr_get(filepath.dentry, &fa);
> + if (error)
> + return error;
> +
> + fileattr_to_fsx_fileattr(&fa, &fsx);
> + error = copy_struct_to_user(ufsx, usize, &fsx,
> + sizeof(struct fsx_fileattr), NULL);
> +
> + return error;
> +}
> +
> +SYSCALL_DEFINE5(file_setattr, int, dfd, const char __user *, filename,
> + struct fsx_fileattr __user *, ufsx, size_t, usize,
> + unsigned int, at_flags)
> +{
> + struct fileattr fa;
> + struct path filepath __free(path_put) = {};
> + int error;
> + unsigned int lookup_flags = 0;
> + struct filename *name __free(putname) = NULL;
> + struct fsx_fileattr fsx;
> +
> + BUILD_BUG_ON(sizeof(struct fsx_fileattr) < FSX_FILEATTR_SIZE_VER0);
> + BUILD_BUG_ON(sizeof(struct fsx_fileattr) != FSX_FILEATTR_SIZE_LATEST);
> +
> + if ((at_flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
> + return -EINVAL;
> +
> + if (!(at_flags & AT_SYMLINK_NOFOLLOW))
> + lookup_flags |= LOOKUP_FOLLOW;
> +
> + if (usize > PAGE_SIZE)
> + return -E2BIG;
> +
> + if (usize < FSX_FILEATTR_SIZE_VER0)
> + return -EINVAL;
> +
> + error = copy_struct_from_user(&fsx, sizeof(struct fsx_fileattr), ufsx,
> + usize);
> + if (error)
> + return error;
> +
> + error = fsx_fileattr_to_fileattr(&fsx, &fa);
> + if (error)
> + return error;
> +
> + name = getname_maybe_null(filename, at_flags);
> + if (IS_ERR(name))
> + return PTR_ERR(name);
> +
> + if (!name && dfd >= 0) {
> + CLASS(fd, f)(dfd);
> +
Same comment here as above.
> + filepath = fd_file(f)->f_path;
> + path_get(&filepath);
> + } else {
> + error = filename_lookup(dfd, name, lookup_flags, &filepath,
> + NULL);
> + if (error)
> + return error;
> + }
> +
> + error = mnt_want_write(filepath.mnt);
> + if (!error) {
> + error = vfs_fileattr_set(mnt_idmap(filepath.mnt),
> + filepath.dentry, &fa);
> + mnt_drop_write(filepath.mnt);
> + }
> +
> + return error;
> +}
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH v6 5/6] fs: prepare for extending file_get/setattr()
From: Jan Kara @ 2025-07-01 13:06 UTC (permalink / raw)
To: Andrey Albershteyn
Cc: Amir Goldstein, Arnd Bergmann, Casey Schaufler, Christian Brauner,
Jan Kara, Pali Rohár, Paul Moore, linux-api, linux-fsdevel,
linux-kernel, linux-xfs, selinux, Andrey Albershteyn
In-Reply-To: <20250630-xattrat-syscall-v6-5-c4e3bc35227b@kernel.org>
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
^ permalink raw reply
* Re: [PATCH v6 4/6] fs: make vfs_fileattr_[get|set] return -EOPNOSUPP
From: Jan Kara @ 2025-07-01 12:52 UTC (permalink / raw)
To: Andrey Albershteyn
Cc: Amir Goldstein, Arnd Bergmann, Casey Schaufler, Christian Brauner,
Jan Kara, Pali Rohár, Paul Moore, linux-api, linux-fsdevel,
linux-kernel, linux-xfs, selinux, Andrey Albershteyn
In-Reply-To: <20250630-xattrat-syscall-v6-4-c4e3bc35227b@kernel.org>
On Mon 30-06-25 18:20:14, Andrey Albershteyn wrote:
> Future patches will add new syscalls which use these functions. As
> this interface won't be used for ioctls only, the EOPNOSUPP is more
> appropriate return code.
>
> This patch converts return code from ENOIOCTLCMD to EOPNOSUPP for
> vfs_fileattr_get and vfs_fileattr_set. To save old behavior translate
> EOPNOSUPP back for current users - overlayfs, encryptfs and fs/ioctl.c.
>
> Signed-off-by: Andrey Albershteyn <aalbersh@kernel.org>
Modulo the small nits already pointed out this looks good to me. Feel free
to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH v6 4/6] fs: make vfs_fileattr_[get|set] return -EOPNOSUPP
From: Jan Kara @ 2025-07-01 12:51 UTC (permalink / raw)
To: Amir Goldstein
Cc: Andrey Albershteyn, Arnd Bergmann, Casey Schaufler,
Christian Brauner, Jan Kara, Pali Rohár, Paul Moore,
linux-api, linux-fsdevel, linux-kernel, linux-xfs, selinux,
Andrey Albershteyn
In-Reply-To: <CAOQ4uxgbeMEqx7FtBc3KnrCjOHHRniSjBPLzk7_S9SjYKcY_ag@mail.gmail.com>
On Tue 01-07-25 08:05:45, Amir Goldstein wrote:
> On Mon, Jun 30, 2025 at 6:20 PM Andrey Albershteyn <aalbersh@redhat.com> wrote:
> >
> > Future patches will add new syscalls which use these functions. As
> > this interface won't be used for ioctls only, the EOPNOSUPP is more
> > appropriate return code.
> >
> > This patch converts return code from ENOIOCTLCMD to EOPNOSUPP for
> > vfs_fileattr_get and vfs_fileattr_set. To save old behavior translate
> > EOPNOSUPP back for current users - overlayfs, encryptfs and fs/ioctl.c.
> >
> > Signed-off-by: Andrey Albershteyn <aalbersh@kernel.org>
...
> > --- a/fs/overlayfs/inode.c
> > +++ b/fs/overlayfs/inode.c
> > @@ -721,7 +721,7 @@ int ovl_real_fileattr_get(const struct path *realpath, struct fileattr *fa)
> > return err;
> >
> > err = vfs_fileattr_get(realpath->dentry, fa);
> > - if (err == -ENOIOCTLCMD)
> > + if (err == -EOPNOTSUPP)
> > err = -ENOTTY;
> > return err;
> > }
>
> That's the wrong way, because it hides the desired -EOPNOTSUPP
> return code from ovl_fileattr_get().
>
> The conversion to -ENOTTY was done for
> 5b0a414d06c3 ("ovl: fix filattr copy-up failure"),
> so please do this instead:
>
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -722,7 +722,7 @@ int ovl_real_fileattr_get(const struct path
> *realpath, struct fileattr *fa)
>
> err = vfs_fileattr_get(realpath->dentry, fa);
> if (err == -ENOIOCTLCMD)
> - err = -ENOTTY;
> + err = -EOPNOTSUPP;
Is this really needed? AFAICS nobody returns ENOIOCTLCMD after this
patch...
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH v6 2/6] lsm: introduce new hooks for setting/getting inode fsxattr
From: Jan Kara @ 2025-07-01 12:39 UTC (permalink / raw)
To: Andrey Albershteyn
Cc: Amir Goldstein, Arnd Bergmann, Casey Schaufler, Christian Brauner,
Jan Kara, Pali Rohár, Paul Moore, linux-api, linux-fsdevel,
linux-kernel, linux-xfs, selinux, Andrey Albershteyn
In-Reply-To: <20250630-xattrat-syscall-v6-2-c4e3bc35227b@kernel.org>
On Mon 30-06-25 18:20:12, Andrey Albershteyn wrote:
> Introduce new hooks for setting and getting filesystem extended
> attributes on inode (FS_IOC_FSGETXATTR).
>
> Cc: selinux@vger.kernel.org
> Cc: Paul Moore <paul@paul-moore.com>
>
> Acked-by: Paul Moore <paul@paul-moore.com>
> Signed-off-by: Andrey Albershteyn <aalbersh@kernel.org>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> fs/file_attr.c | 19 ++++++++++++++++---
> include/linux/lsm_hook_defs.h | 2 ++
> include/linux/security.h | 16 ++++++++++++++++
> security/security.c | 30 ++++++++++++++++++++++++++++++
> 4 files changed, 64 insertions(+), 3 deletions(-)
>
> diff --git a/fs/file_attr.c b/fs/file_attr.c
> index 2910b7047721..be62d97cc444 100644
> --- a/fs/file_attr.c
> +++ b/fs/file_attr.c
> @@ -76,10 +76,15 @@ EXPORT_SYMBOL(fileattr_fill_flags);
> int vfs_fileattr_get(struct dentry *dentry, struct fileattr *fa)
> {
> struct inode *inode = d_inode(dentry);
> + int error;
>
> if (!inode->i_op->fileattr_get)
> return -ENOIOCTLCMD;
>
> + error = security_inode_file_getattr(dentry, fa);
> + if (error)
> + return error;
> +
> return inode->i_op->fileattr_get(dentry, fa);
> }
> EXPORT_SYMBOL(vfs_fileattr_get);
> @@ -242,12 +247,20 @@ int vfs_fileattr_set(struct mnt_idmap *idmap, struct dentry *dentry,
> } else {
> fa->flags |= old_ma.flags & ~FS_COMMON_FL;
> }
> +
> err = fileattr_set_prepare(inode, &old_ma, fa);
> - if (!err)
> - err = inode->i_op->fileattr_set(idmap, dentry, fa);
> + if (err)
> + goto out;
> + err = security_inode_file_setattr(dentry, fa);
> + if (err)
> + goto out;
> + err = inode->i_op->fileattr_set(idmap, dentry, fa);
> + if (err)
> + goto out;
> }
> +
> +out:
> inode_unlock(inode);
> -
> return err;
> }
> EXPORT_SYMBOL(vfs_fileattr_set);
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index bf3bbac4e02a..9600a4350e79 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -157,6 +157,8 @@ LSM_HOOK(int, 0, inode_removexattr, struct mnt_idmap *idmap,
> struct dentry *dentry, const char *name)
> LSM_HOOK(void, LSM_RET_VOID, inode_post_removexattr, struct dentry *dentry,
> const char *name)
> +LSM_HOOK(int, 0, inode_file_setattr, struct dentry *dentry, struct fileattr *fa)
> +LSM_HOOK(int, 0, inode_file_getattr, struct dentry *dentry, struct fileattr *fa)
> LSM_HOOK(int, 0, inode_set_acl, struct mnt_idmap *idmap,
> struct dentry *dentry, const char *acl_name, struct posix_acl *kacl)
> LSM_HOOK(void, LSM_RET_VOID, inode_post_set_acl, struct dentry *dentry,
> diff --git a/include/linux/security.h b/include/linux/security.h
> index dba349629229..9ed0d0e0c81f 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -451,6 +451,10 @@ int security_inode_listxattr(struct dentry *dentry);
> int security_inode_removexattr(struct mnt_idmap *idmap,
> struct dentry *dentry, const char *name);
> void security_inode_post_removexattr(struct dentry *dentry, const char *name);
> +int security_inode_file_setattr(struct dentry *dentry,
> + struct fileattr *fa);
> +int security_inode_file_getattr(struct dentry *dentry,
> + struct fileattr *fa);
> int security_inode_need_killpriv(struct dentry *dentry);
> int security_inode_killpriv(struct mnt_idmap *idmap, struct dentry *dentry);
> int security_inode_getsecurity(struct mnt_idmap *idmap,
> @@ -1052,6 +1056,18 @@ static inline void security_inode_post_removexattr(struct dentry *dentry,
> const char *name)
> { }
>
> +static inline int security_inode_file_setattr(struct dentry *dentry,
> + struct fileattr *fa)
> +{
> + return 0;
> +}
> +
> +static inline int security_inode_file_getattr(struct dentry *dentry,
> + struct fileattr *fa)
> +{
> + return 0;
> +}
> +
> static inline int security_inode_need_killpriv(struct dentry *dentry)
> {
> return cap_inode_need_killpriv(dentry);
> diff --git a/security/security.c b/security/security.c
> index 596d41818577..711b4de40b8d 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2622,6 +2622,36 @@ void security_inode_post_removexattr(struct dentry *dentry, const char *name)
> call_void_hook(inode_post_removexattr, dentry, name);
> }
>
> +/**
> + * security_inode_file_setattr() - check if setting fsxattr is allowed
> + * @dentry: file to set filesystem extended attributes on
> + * @fa: extended attributes to set on the inode
> + *
> + * Called when file_setattr() syscall or FS_IOC_FSSETXATTR ioctl() is called on
> + * inode
> + *
> + * Return: Returns 0 if permission is granted.
> + */
> +int security_inode_file_setattr(struct dentry *dentry, struct fileattr *fa)
> +{
> + return call_int_hook(inode_file_setattr, dentry, fa);
> +}
> +
> +/**
> + * security_inode_file_getattr() - check if retrieving fsxattr is allowed
> + * @dentry: file to retrieve filesystem extended attributes from
> + * @fa: extended attributes to get
> + *
> + * Called when file_getattr() syscall or FS_IOC_FSGETXATTR ioctl() is called on
> + * inode
> + *
> + * Return: Returns 0 if permission is granted.
> + */
> +int security_inode_file_getattr(struct dentry *dentry, struct fileattr *fa)
> +{
> + return call_int_hook(inode_file_getattr, dentry, fa);
> +}
> +
> /**
> * security_inode_need_killpriv() - Check if security_inode_killpriv() required
> * @dentry: associated dentry
>
> --
> 2.47.2
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH v6 1/6] fs: split fileattr related helpers into separate file
From: Jan Kara @ 2025-07-01 12:38 UTC (permalink / raw)
To: Andrey Albershteyn
Cc: Amir Goldstein, Arnd Bergmann, Casey Schaufler, Christian Brauner,
Jan Kara, Pali Rohár, Paul Moore, linux-api, linux-fsdevel,
linux-kernel, linux-xfs, selinux, Andrey Albershteyn
In-Reply-To: <20250630-xattrat-syscall-v6-1-c4e3bc35227b@kernel.org>
On Mon 30-06-25 18:20:11, Andrey Albershteyn wrote:
> From: Andrey Albershteyn <aalbersh@kernel.org>
>
> This patch moves function related to file extended attributes
> manipulations to separate file. Refactoring only.
>
> Signed-off-by: Andrey Albershteyn <aalbersh@kernel.org>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> fs/Makefile | 3 +-
> fs/file_attr.c | 318 +++++++++++++++++++++++++++++++++++++++++++++++
> fs/ioctl.c | 309 ---------------------------------------------
> include/linux/fileattr.h | 4 +
> 4 files changed, 324 insertions(+), 310 deletions(-)
>
> diff --git a/fs/Makefile b/fs/Makefile
> index 79c08b914c47..334654f9584b 100644
> --- a/fs/Makefile
> +++ b/fs/Makefile
> @@ -15,7 +15,8 @@ obj-y := open.o read_write.o file_table.o super.o \
> pnode.o splice.o sync.o utimes.o d_path.o \
> stack.o fs_struct.o statfs.o fs_pin.o nsfs.o \
> fs_types.o fs_context.o fs_parser.o fsopen.o init.o \
> - kernel_read_file.o mnt_idmapping.o remap_range.o pidfs.o
> + kernel_read_file.o mnt_idmapping.o remap_range.o pidfs.o \
> + file_attr.o
>
> obj-$(CONFIG_BUFFER_HEAD) += buffer.o mpage.o
> obj-$(CONFIG_PROC_FS) += proc_namespace.o
> diff --git a/fs/file_attr.c b/fs/file_attr.c
> new file mode 100644
> index 000000000000..2910b7047721
> --- /dev/null
> +++ b/fs/file_attr.c
> @@ -0,0 +1,318 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/fs.h>
> +#include <linux/security.h>
> +#include <linux/fscrypt.h>
> +#include <linux/fileattr.h>
> +
> +/**
> + * fileattr_fill_xflags - initialize fileattr with xflags
> + * @fa: fileattr pointer
> + * @xflags: FS_XFLAG_* flags
> + *
> + * Set ->fsx_xflags, ->fsx_valid and ->flags (translated xflags). All
> + * other fields are zeroed.
> + */
> +void fileattr_fill_xflags(struct fileattr *fa, u32 xflags)
> +{
> + memset(fa, 0, sizeof(*fa));
> + fa->fsx_valid = true;
> + fa->fsx_xflags = xflags;
> + if (fa->fsx_xflags & FS_XFLAG_IMMUTABLE)
> + fa->flags |= FS_IMMUTABLE_FL;
> + if (fa->fsx_xflags & FS_XFLAG_APPEND)
> + fa->flags |= FS_APPEND_FL;
> + if (fa->fsx_xflags & FS_XFLAG_SYNC)
> + fa->flags |= FS_SYNC_FL;
> + if (fa->fsx_xflags & FS_XFLAG_NOATIME)
> + fa->flags |= FS_NOATIME_FL;
> + if (fa->fsx_xflags & FS_XFLAG_NODUMP)
> + fa->flags |= FS_NODUMP_FL;
> + if (fa->fsx_xflags & FS_XFLAG_DAX)
> + fa->flags |= FS_DAX_FL;
> + if (fa->fsx_xflags & FS_XFLAG_PROJINHERIT)
> + fa->flags |= FS_PROJINHERIT_FL;
> +}
> +EXPORT_SYMBOL(fileattr_fill_xflags);
> +
> +/**
> + * fileattr_fill_flags - initialize fileattr with flags
> + * @fa: fileattr pointer
> + * @flags: FS_*_FL flags
> + *
> + * Set ->flags, ->flags_valid and ->fsx_xflags (translated flags).
> + * All other fields are zeroed.
> + */
> +void fileattr_fill_flags(struct fileattr *fa, u32 flags)
> +{
> + memset(fa, 0, sizeof(*fa));
> + fa->flags_valid = true;
> + fa->flags = flags;
> + if (fa->flags & FS_SYNC_FL)
> + fa->fsx_xflags |= FS_XFLAG_SYNC;
> + if (fa->flags & FS_IMMUTABLE_FL)
> + fa->fsx_xflags |= FS_XFLAG_IMMUTABLE;
> + if (fa->flags & FS_APPEND_FL)
> + fa->fsx_xflags |= FS_XFLAG_APPEND;
> + if (fa->flags & FS_NODUMP_FL)
> + fa->fsx_xflags |= FS_XFLAG_NODUMP;
> + if (fa->flags & FS_NOATIME_FL)
> + fa->fsx_xflags |= FS_XFLAG_NOATIME;
> + if (fa->flags & FS_DAX_FL)
> + fa->fsx_xflags |= FS_XFLAG_DAX;
> + if (fa->flags & FS_PROJINHERIT_FL)
> + fa->fsx_xflags |= FS_XFLAG_PROJINHERIT;
> +}
> +EXPORT_SYMBOL(fileattr_fill_flags);
> +
> +/**
> + * vfs_fileattr_get - retrieve miscellaneous file attributes
> + * @dentry: the object to retrieve from
> + * @fa: fileattr pointer
> + *
> + * Call i_op->fileattr_get() callback, if exists.
> + *
> + * Return: 0 on success, or a negative error on failure.
> + */
> +int vfs_fileattr_get(struct dentry *dentry, struct fileattr *fa)
> +{
> + struct inode *inode = d_inode(dentry);
> +
> + if (!inode->i_op->fileattr_get)
> + return -ENOIOCTLCMD;
> +
> + return inode->i_op->fileattr_get(dentry, fa);
> +}
> +EXPORT_SYMBOL(vfs_fileattr_get);
> +
> +/**
> + * copy_fsxattr_to_user - copy fsxattr to userspace.
> + * @fa: fileattr pointer
> + * @ufa: fsxattr user pointer
> + *
> + * Return: 0 on success, or -EFAULT on failure.
> + */
> +int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa)
> +{
> + struct fsxattr xfa;
> +
> + memset(&xfa, 0, sizeof(xfa));
> + xfa.fsx_xflags = fa->fsx_xflags;
> + xfa.fsx_extsize = fa->fsx_extsize;
> + xfa.fsx_nextents = fa->fsx_nextents;
> + xfa.fsx_projid = fa->fsx_projid;
> + xfa.fsx_cowextsize = fa->fsx_cowextsize;
> +
> + if (copy_to_user(ufa, &xfa, sizeof(xfa)))
> + return -EFAULT;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(copy_fsxattr_to_user);
> +
> +static int copy_fsxattr_from_user(struct fileattr *fa,
> + struct fsxattr __user *ufa)
> +{
> + struct fsxattr xfa;
> +
> + if (copy_from_user(&xfa, ufa, sizeof(xfa)))
> + return -EFAULT;
> +
> + fileattr_fill_xflags(fa, xfa.fsx_xflags);
> + fa->fsx_extsize = xfa.fsx_extsize;
> + fa->fsx_nextents = xfa.fsx_nextents;
> + fa->fsx_projid = xfa.fsx_projid;
> + fa->fsx_cowextsize = xfa.fsx_cowextsize;
> +
> + return 0;
> +}
> +
> +/*
> + * Generic function to check FS_IOC_FSSETXATTR/FS_IOC_SETFLAGS values and reject
> + * any invalid configurations.
> + *
> + * Note: must be called with inode lock held.
> + */
> +static int fileattr_set_prepare(struct inode *inode,
> + const struct fileattr *old_ma,
> + struct fileattr *fa)
> +{
> + int err;
> +
> + /*
> + * The IMMUTABLE and APPEND_ONLY flags can only be changed by
> + * the relevant capability.
> + */
> + if ((fa->flags ^ old_ma->flags) & (FS_APPEND_FL | FS_IMMUTABLE_FL) &&
> + !capable(CAP_LINUX_IMMUTABLE))
> + return -EPERM;
> +
> + err = fscrypt_prepare_setflags(inode, old_ma->flags, fa->flags);
> + if (err)
> + return err;
> +
> + /*
> + * Project Quota ID state is only allowed to change from within the init
> + * namespace. Enforce that restriction only if we are trying to change
> + * the quota ID state. Everything else is allowed in user namespaces.
> + */
> + if (current_user_ns() != &init_user_ns) {
> + if (old_ma->fsx_projid != fa->fsx_projid)
> + return -EINVAL;
> + if ((old_ma->fsx_xflags ^ fa->fsx_xflags) &
> + FS_XFLAG_PROJINHERIT)
> + return -EINVAL;
> + } else {
> + /*
> + * Caller is allowed to change the project ID. If it is being
> + * changed, make sure that the new value is valid.
> + */
> + if (old_ma->fsx_projid != fa->fsx_projid &&
> + !projid_valid(make_kprojid(&init_user_ns, fa->fsx_projid)))
> + return -EINVAL;
> + }
> +
> + /* Check extent size hints. */
> + if ((fa->fsx_xflags & FS_XFLAG_EXTSIZE) && !S_ISREG(inode->i_mode))
> + return -EINVAL;
> +
> + if ((fa->fsx_xflags & FS_XFLAG_EXTSZINHERIT) &&
> + !S_ISDIR(inode->i_mode))
> + return -EINVAL;
> +
> + if ((fa->fsx_xflags & FS_XFLAG_COWEXTSIZE) &&
> + !S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
> + return -EINVAL;
> +
> + /*
> + * It is only valid to set the DAX flag on regular files and
> + * directories on filesystems.
> + */
> + if ((fa->fsx_xflags & FS_XFLAG_DAX) &&
> + !(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)))
> + return -EINVAL;
> +
> + /* Extent size hints of zero turn off the flags. */
> + if (fa->fsx_extsize == 0)
> + fa->fsx_xflags &= ~(FS_XFLAG_EXTSIZE | FS_XFLAG_EXTSZINHERIT);
> + if (fa->fsx_cowextsize == 0)
> + fa->fsx_xflags &= ~FS_XFLAG_COWEXTSIZE;
> +
> + return 0;
> +}
> +
> +/**
> + * vfs_fileattr_set - change miscellaneous file attributes
> + * @idmap: idmap of the mount
> + * @dentry: the object to change
> + * @fa: fileattr pointer
> + *
> + * After verifying permissions, call i_op->fileattr_set() callback, if
> + * exists.
> + *
> + * Verifying attributes involves retrieving current attributes with
> + * i_op->fileattr_get(), this also allows initializing attributes that have
> + * not been set by the caller to current values. Inode lock is held
> + * thoughout to prevent racing with another instance.
> + *
> + * Return: 0 on success, or a negative error on failure.
> + */
> +int vfs_fileattr_set(struct mnt_idmap *idmap, struct dentry *dentry,
> + struct fileattr *fa)
> +{
> + struct inode *inode = d_inode(dentry);
> + struct fileattr old_ma = {};
> + int err;
> +
> + if (!inode->i_op->fileattr_set)
> + return -ENOIOCTLCMD;
> +
> + if (!inode_owner_or_capable(idmap, inode))
> + return -EPERM;
> +
> + inode_lock(inode);
> + err = vfs_fileattr_get(dentry, &old_ma);
> + if (!err) {
> + /* initialize missing bits from old_ma */
> + if (fa->flags_valid) {
> + fa->fsx_xflags |= old_ma.fsx_xflags & ~FS_XFLAG_COMMON;
> + fa->fsx_extsize = old_ma.fsx_extsize;
> + fa->fsx_nextents = old_ma.fsx_nextents;
> + fa->fsx_projid = old_ma.fsx_projid;
> + fa->fsx_cowextsize = old_ma.fsx_cowextsize;
> + } else {
> + fa->flags |= old_ma.flags & ~FS_COMMON_FL;
> + }
> + err = fileattr_set_prepare(inode, &old_ma, fa);
> + if (!err)
> + err = inode->i_op->fileattr_set(idmap, dentry, fa);
> + }
> + inode_unlock(inode);
> +
> + return err;
> +}
> +EXPORT_SYMBOL(vfs_fileattr_set);
> +
> +int ioctl_getflags(struct file *file, unsigned int __user *argp)
> +{
> + struct fileattr fa = { .flags_valid = true }; /* hint only */
> + int err;
> +
> + err = vfs_fileattr_get(file->f_path.dentry, &fa);
> + if (!err)
> + err = put_user(fa.flags, argp);
> + return err;
> +}
> +EXPORT_SYMBOL(ioctl_getflags);
> +
> +int ioctl_setflags(struct file *file, unsigned int __user *argp)
> +{
> + struct mnt_idmap *idmap = file_mnt_idmap(file);
> + struct dentry *dentry = file->f_path.dentry;
> + struct fileattr fa;
> + unsigned int flags;
> + int err;
> +
> + err = get_user(flags, argp);
> + if (!err) {
> + err = mnt_want_write_file(file);
> + if (!err) {
> + fileattr_fill_flags(&fa, flags);
> + err = vfs_fileattr_set(idmap, dentry, &fa);
> + mnt_drop_write_file(file);
> + }
> + }
> + return err;
> +}
> +EXPORT_SYMBOL(ioctl_setflags);
> +
> +int ioctl_fsgetxattr(struct file *file, void __user *argp)
> +{
> + struct fileattr fa = { .fsx_valid = true }; /* hint only */
> + int err;
> +
> + err = vfs_fileattr_get(file->f_path.dentry, &fa);
> + if (!err)
> + err = copy_fsxattr_to_user(&fa, argp);
> +
> + return err;
> +}
> +EXPORT_SYMBOL(ioctl_fsgetxattr);
> +
> +int ioctl_fssetxattr(struct file *file, void __user *argp)
> +{
> + struct mnt_idmap *idmap = file_mnt_idmap(file);
> + struct dentry *dentry = file->f_path.dentry;
> + struct fileattr fa;
> + int err;
> +
> + err = copy_fsxattr_from_user(&fa, argp);
> + if (!err) {
> + err = mnt_want_write_file(file);
> + if (!err) {
> + err = vfs_fileattr_set(idmap, dentry, &fa);
> + mnt_drop_write_file(file);
> + }
> + }
> + return err;
> +}
> +EXPORT_SYMBOL(ioctl_fssetxattr);
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 69107a245b4c..0248cb8db2d3 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -453,315 +453,6 @@ static int ioctl_file_dedupe_range(struct file *file,
> return ret;
> }
>
> -/**
> - * fileattr_fill_xflags - initialize fileattr with xflags
> - * @fa: fileattr pointer
> - * @xflags: FS_XFLAG_* flags
> - *
> - * Set ->fsx_xflags, ->fsx_valid and ->flags (translated xflags). All
> - * other fields are zeroed.
> - */
> -void fileattr_fill_xflags(struct fileattr *fa, u32 xflags)
> -{
> - memset(fa, 0, sizeof(*fa));
> - fa->fsx_valid = true;
> - fa->fsx_xflags = xflags;
> - if (fa->fsx_xflags & FS_XFLAG_IMMUTABLE)
> - fa->flags |= FS_IMMUTABLE_FL;
> - if (fa->fsx_xflags & FS_XFLAG_APPEND)
> - fa->flags |= FS_APPEND_FL;
> - if (fa->fsx_xflags & FS_XFLAG_SYNC)
> - fa->flags |= FS_SYNC_FL;
> - if (fa->fsx_xflags & FS_XFLAG_NOATIME)
> - fa->flags |= FS_NOATIME_FL;
> - if (fa->fsx_xflags & FS_XFLAG_NODUMP)
> - fa->flags |= FS_NODUMP_FL;
> - if (fa->fsx_xflags & FS_XFLAG_DAX)
> - fa->flags |= FS_DAX_FL;
> - if (fa->fsx_xflags & FS_XFLAG_PROJINHERIT)
> - fa->flags |= FS_PROJINHERIT_FL;
> -}
> -EXPORT_SYMBOL(fileattr_fill_xflags);
> -
> -/**
> - * fileattr_fill_flags - initialize fileattr with flags
> - * @fa: fileattr pointer
> - * @flags: FS_*_FL flags
> - *
> - * Set ->flags, ->flags_valid and ->fsx_xflags (translated flags).
> - * All other fields are zeroed.
> - */
> -void fileattr_fill_flags(struct fileattr *fa, u32 flags)
> -{
> - memset(fa, 0, sizeof(*fa));
> - fa->flags_valid = true;
> - fa->flags = flags;
> - if (fa->flags & FS_SYNC_FL)
> - fa->fsx_xflags |= FS_XFLAG_SYNC;
> - if (fa->flags & FS_IMMUTABLE_FL)
> - fa->fsx_xflags |= FS_XFLAG_IMMUTABLE;
> - if (fa->flags & FS_APPEND_FL)
> - fa->fsx_xflags |= FS_XFLAG_APPEND;
> - if (fa->flags & FS_NODUMP_FL)
> - fa->fsx_xflags |= FS_XFLAG_NODUMP;
> - if (fa->flags & FS_NOATIME_FL)
> - fa->fsx_xflags |= FS_XFLAG_NOATIME;
> - if (fa->flags & FS_DAX_FL)
> - fa->fsx_xflags |= FS_XFLAG_DAX;
> - if (fa->flags & FS_PROJINHERIT_FL)
> - fa->fsx_xflags |= FS_XFLAG_PROJINHERIT;
> -}
> -EXPORT_SYMBOL(fileattr_fill_flags);
> -
> -/**
> - * vfs_fileattr_get - retrieve miscellaneous file attributes
> - * @dentry: the object to retrieve from
> - * @fa: fileattr pointer
> - *
> - * Call i_op->fileattr_get() callback, if exists.
> - *
> - * Return: 0 on success, or a negative error on failure.
> - */
> -int vfs_fileattr_get(struct dentry *dentry, struct fileattr *fa)
> -{
> - struct inode *inode = d_inode(dentry);
> -
> - if (!inode->i_op->fileattr_get)
> - return -ENOIOCTLCMD;
> -
> - return inode->i_op->fileattr_get(dentry, fa);
> -}
> -EXPORT_SYMBOL(vfs_fileattr_get);
> -
> -/**
> - * copy_fsxattr_to_user - copy fsxattr to userspace.
> - * @fa: fileattr pointer
> - * @ufa: fsxattr user pointer
> - *
> - * Return: 0 on success, or -EFAULT on failure.
> - */
> -int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa)
> -{
> - struct fsxattr xfa;
> -
> - memset(&xfa, 0, sizeof(xfa));
> - xfa.fsx_xflags = fa->fsx_xflags;
> - xfa.fsx_extsize = fa->fsx_extsize;
> - xfa.fsx_nextents = fa->fsx_nextents;
> - xfa.fsx_projid = fa->fsx_projid;
> - xfa.fsx_cowextsize = fa->fsx_cowextsize;
> -
> - if (copy_to_user(ufa, &xfa, sizeof(xfa)))
> - return -EFAULT;
> -
> - return 0;
> -}
> -EXPORT_SYMBOL(copy_fsxattr_to_user);
> -
> -static int copy_fsxattr_from_user(struct fileattr *fa,
> - struct fsxattr __user *ufa)
> -{
> - struct fsxattr xfa;
> -
> - if (copy_from_user(&xfa, ufa, sizeof(xfa)))
> - return -EFAULT;
> -
> - fileattr_fill_xflags(fa, xfa.fsx_xflags);
> - fa->fsx_extsize = xfa.fsx_extsize;
> - fa->fsx_nextents = xfa.fsx_nextents;
> - fa->fsx_projid = xfa.fsx_projid;
> - fa->fsx_cowextsize = xfa.fsx_cowextsize;
> -
> - return 0;
> -}
> -
> -/*
> - * Generic function to check FS_IOC_FSSETXATTR/FS_IOC_SETFLAGS values and reject
> - * any invalid configurations.
> - *
> - * Note: must be called with inode lock held.
> - */
> -static int fileattr_set_prepare(struct inode *inode,
> - const struct fileattr *old_ma,
> - struct fileattr *fa)
> -{
> - int err;
> -
> - /*
> - * The IMMUTABLE and APPEND_ONLY flags can only be changed by
> - * the relevant capability.
> - */
> - if ((fa->flags ^ old_ma->flags) & (FS_APPEND_FL | FS_IMMUTABLE_FL) &&
> - !capable(CAP_LINUX_IMMUTABLE))
> - return -EPERM;
> -
> - err = fscrypt_prepare_setflags(inode, old_ma->flags, fa->flags);
> - if (err)
> - return err;
> -
> - /*
> - * Project Quota ID state is only allowed to change from within the init
> - * namespace. Enforce that restriction only if we are trying to change
> - * the quota ID state. Everything else is allowed in user namespaces.
> - */
> - if (current_user_ns() != &init_user_ns) {
> - if (old_ma->fsx_projid != fa->fsx_projid)
> - return -EINVAL;
> - if ((old_ma->fsx_xflags ^ fa->fsx_xflags) &
> - FS_XFLAG_PROJINHERIT)
> - return -EINVAL;
> - } else {
> - /*
> - * Caller is allowed to change the project ID. If it is being
> - * changed, make sure that the new value is valid.
> - */
> - if (old_ma->fsx_projid != fa->fsx_projid &&
> - !projid_valid(make_kprojid(&init_user_ns, fa->fsx_projid)))
> - return -EINVAL;
> - }
> -
> - /* Check extent size hints. */
> - if ((fa->fsx_xflags & FS_XFLAG_EXTSIZE) && !S_ISREG(inode->i_mode))
> - return -EINVAL;
> -
> - if ((fa->fsx_xflags & FS_XFLAG_EXTSZINHERIT) &&
> - !S_ISDIR(inode->i_mode))
> - return -EINVAL;
> -
> - if ((fa->fsx_xflags & FS_XFLAG_COWEXTSIZE) &&
> - !S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
> - return -EINVAL;
> -
> - /*
> - * It is only valid to set the DAX flag on regular files and
> - * directories on filesystems.
> - */
> - if ((fa->fsx_xflags & FS_XFLAG_DAX) &&
> - !(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)))
> - return -EINVAL;
> -
> - /* Extent size hints of zero turn off the flags. */
> - if (fa->fsx_extsize == 0)
> - fa->fsx_xflags &= ~(FS_XFLAG_EXTSIZE | FS_XFLAG_EXTSZINHERIT);
> - if (fa->fsx_cowextsize == 0)
> - fa->fsx_xflags &= ~FS_XFLAG_COWEXTSIZE;
> -
> - return 0;
> -}
> -
> -/**
> - * vfs_fileattr_set - change miscellaneous file attributes
> - * @idmap: idmap of the mount
> - * @dentry: the object to change
> - * @fa: fileattr pointer
> - *
> - * After verifying permissions, call i_op->fileattr_set() callback, if
> - * exists.
> - *
> - * Verifying attributes involves retrieving current attributes with
> - * i_op->fileattr_get(), this also allows initializing attributes that have
> - * not been set by the caller to current values. Inode lock is held
> - * thoughout to prevent racing with another instance.
> - *
> - * Return: 0 on success, or a negative error on failure.
> - */
> -int vfs_fileattr_set(struct mnt_idmap *idmap, struct dentry *dentry,
> - struct fileattr *fa)
> -{
> - struct inode *inode = d_inode(dentry);
> - struct fileattr old_ma = {};
> - int err;
> -
> - if (!inode->i_op->fileattr_set)
> - return -ENOIOCTLCMD;
> -
> - if (!inode_owner_or_capable(idmap, inode))
> - return -EPERM;
> -
> - inode_lock(inode);
> - err = vfs_fileattr_get(dentry, &old_ma);
> - if (!err) {
> - /* initialize missing bits from old_ma */
> - if (fa->flags_valid) {
> - fa->fsx_xflags |= old_ma.fsx_xflags & ~FS_XFLAG_COMMON;
> - fa->fsx_extsize = old_ma.fsx_extsize;
> - fa->fsx_nextents = old_ma.fsx_nextents;
> - fa->fsx_projid = old_ma.fsx_projid;
> - fa->fsx_cowextsize = old_ma.fsx_cowextsize;
> - } else {
> - fa->flags |= old_ma.flags & ~FS_COMMON_FL;
> - }
> - err = fileattr_set_prepare(inode, &old_ma, fa);
> - if (!err)
> - err = inode->i_op->fileattr_set(idmap, dentry, fa);
> - }
> - inode_unlock(inode);
> -
> - return err;
> -}
> -EXPORT_SYMBOL(vfs_fileattr_set);
> -
> -static int ioctl_getflags(struct file *file, unsigned int __user *argp)
> -{
> - struct fileattr fa = { .flags_valid = true }; /* hint only */
> - int err;
> -
> - err = vfs_fileattr_get(file->f_path.dentry, &fa);
> - if (!err)
> - err = put_user(fa.flags, argp);
> - return err;
> -}
> -
> -static int ioctl_setflags(struct file *file, unsigned int __user *argp)
> -{
> - struct mnt_idmap *idmap = file_mnt_idmap(file);
> - struct dentry *dentry = file->f_path.dentry;
> - struct fileattr fa;
> - unsigned int flags;
> - int err;
> -
> - err = get_user(flags, argp);
> - if (!err) {
> - err = mnt_want_write_file(file);
> - if (!err) {
> - fileattr_fill_flags(&fa, flags);
> - err = vfs_fileattr_set(idmap, dentry, &fa);
> - mnt_drop_write_file(file);
> - }
> - }
> - return err;
> -}
> -
> -static int ioctl_fsgetxattr(struct file *file, void __user *argp)
> -{
> - struct fileattr fa = { .fsx_valid = true }; /* hint only */
> - int err;
> -
> - err = vfs_fileattr_get(file->f_path.dentry, &fa);
> - if (!err)
> - err = copy_fsxattr_to_user(&fa, argp);
> -
> - return err;
> -}
> -
> -static int ioctl_fssetxattr(struct file *file, void __user *argp)
> -{
> - struct mnt_idmap *idmap = file_mnt_idmap(file);
> - struct dentry *dentry = file->f_path.dentry;
> - struct fileattr fa;
> - int err;
> -
> - err = copy_fsxattr_from_user(&fa, argp);
> - if (!err) {
> - err = mnt_want_write_file(file);
> - if (!err) {
> - err = vfs_fileattr_set(idmap, dentry, &fa);
> - mnt_drop_write_file(file);
> - }
> - }
> - return err;
> -}
> -
> static int ioctl_getfsuuid(struct file *file, void __user *argp)
> {
> struct super_block *sb = file_inode(file)->i_sb;
> diff --git a/include/linux/fileattr.h b/include/linux/fileattr.h
> index 47c05a9851d0..6030d0bf7ad3 100644
> --- a/include/linux/fileattr.h
> +++ b/include/linux/fileattr.h
> @@ -55,5 +55,9 @@ static inline bool fileattr_has_fsx(const struct fileattr *fa)
> int vfs_fileattr_get(struct dentry *dentry, struct fileattr *fa);
> int vfs_fileattr_set(struct mnt_idmap *idmap, struct dentry *dentry,
> struct fileattr *fa);
> +int ioctl_getflags(struct file *file, unsigned int __user *argp);
> +int ioctl_setflags(struct file *file, unsigned int __user *argp);
> +int ioctl_fsgetxattr(struct file *file, void __user *argp);
> +int ioctl_fssetxattr(struct file *file, void __user *argp);
>
> #endif /* _LINUX_FILEATTR_H */
>
> --
> 2.47.2
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH v6 6/6] fs: introduce file_getattr and file_setattr syscalls
From: Christian Brauner @ 2025-07-01 12:34 UTC (permalink / raw)
To: Andrey Albershteyn
Cc: Amir Goldstein, Arnd Bergmann, Casey Schaufler, Jan Kara,
Pali Rohár, Paul Moore, linux-api, linux-fsdevel,
linux-kernel, linux-xfs, selinux, Andrey Albershteyn
In-Reply-To: <20250630-xattrat-syscall-v6-6-c4e3bc35227b@kernel.org>
On Mon, Jun 30, 2025 at 06:20:16PM +0200, Andrey Albershteyn wrote:
> From: Andrey Albershteyn <aalbersh@redhat.com>
>
> Introduce file_getattr() and file_setattr() syscalls to manipulate inode
> extended attributes. The syscalls takes pair of file descriptor and
> pathname. Then it operates on inode opened accroding to openat()
> semantics. The struct fsx_fileattr is passed to obtain/change extended
> attributes.
>
> This is an alternative to FS_IOC_FSSETXATTR ioctl with a difference
> that file don't need to be open as we can reference it with a path
> instead of fd. By having this we can manipulated inode extended
> attributes not only on regular files but also on special ones. This
> is not possible with FS_IOC_FSSETXATTR ioctl as with special files
> we can not call ioctl() directly on the filesystem inode using fd.
>
> This patch adds two new syscalls which allows userspace to get/set
> extended inode attributes on special files by using parent directory
> and a path - *at() like syscall.
>
> CC: linux-api@vger.kernel.org
> CC: linux-fsdevel@vger.kernel.org
> CC: linux-xfs@vger.kernel.org
> Signed-off-by: Andrey Albershteyn <aalbersh@kernel.org>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> ---
> arch/alpha/kernel/syscalls/syscall.tbl | 2 +
> arch/arm/tools/syscall.tbl | 2 +
> arch/arm64/tools/syscall_32.tbl | 2 +
> arch/m68k/kernel/syscalls/syscall.tbl | 2 +
> arch/microblaze/kernel/syscalls/syscall.tbl | 2 +
> arch/mips/kernel/syscalls/syscall_n32.tbl | 2 +
> arch/mips/kernel/syscalls/syscall_n64.tbl | 2 +
> arch/mips/kernel/syscalls/syscall_o32.tbl | 2 +
> arch/parisc/kernel/syscalls/syscall.tbl | 2 +
> arch/powerpc/kernel/syscalls/syscall.tbl | 2 +
> arch/s390/kernel/syscalls/syscall.tbl | 2 +
> arch/sh/kernel/syscalls/syscall.tbl | 2 +
> arch/sparc/kernel/syscalls/syscall.tbl | 2 +
> arch/x86/entry/syscalls/syscall_32.tbl | 2 +
> arch/x86/entry/syscalls/syscall_64.tbl | 2 +
> arch/xtensa/kernel/syscalls/syscall.tbl | 2 +
> fs/file_attr.c | 148 ++++++++++++++++++++++++++++
> include/linux/syscalls.h | 6 ++
> include/uapi/asm-generic/unistd.h | 8 +-
> include/uapi/linux/fs.h | 18 ++++
> scripts/syscall.tbl | 2 +
> 21 files changed, 213 insertions(+), 1 deletion(-)
>
> diff --git a/arch/alpha/kernel/syscalls/syscall.tbl b/arch/alpha/kernel/syscalls/syscall.tbl
> index 2dd6340de6b4..16dca28ebf17 100644
> --- a/arch/alpha/kernel/syscalls/syscall.tbl
> +++ b/arch/alpha/kernel/syscalls/syscall.tbl
> @@ -507,3 +507,5 @@
> 575 common listxattrat sys_listxattrat
> 576 common removexattrat sys_removexattrat
> 577 common open_tree_attr sys_open_tree_attr
> +578 common file_getattr sys_file_getattr
> +579 common file_setattr sys_file_setattr
> diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl
> index 27c1d5ebcd91..b07e699aaa3c 100644
> --- a/arch/arm/tools/syscall.tbl
> +++ b/arch/arm/tools/syscall.tbl
> @@ -482,3 +482,5 @@
> 465 common listxattrat sys_listxattrat
> 466 common removexattrat sys_removexattrat
> 467 common open_tree_attr sys_open_tree_attr
> +468 common file_getattr sys_file_getattr
> +469 common file_setattr sys_file_setattr
> diff --git a/arch/arm64/tools/syscall_32.tbl b/arch/arm64/tools/syscall_32.tbl
> index 0765b3a8d6d6..8d9088bc577d 100644
> --- a/arch/arm64/tools/syscall_32.tbl
> +++ b/arch/arm64/tools/syscall_32.tbl
> @@ -479,3 +479,5 @@
> 465 common listxattrat sys_listxattrat
> 466 common removexattrat sys_removexattrat
> 467 common open_tree_attr sys_open_tree_attr
> +468 common file_getattr sys_file_getattr
> +469 common file_setattr sys_file_setattr
> diff --git a/arch/m68k/kernel/syscalls/syscall.tbl b/arch/m68k/kernel/syscalls/syscall.tbl
> index 9fe47112c586..f41d38dfbf13 100644
> --- a/arch/m68k/kernel/syscalls/syscall.tbl
> +++ b/arch/m68k/kernel/syscalls/syscall.tbl
> @@ -467,3 +467,5 @@
> 465 common listxattrat sys_listxattrat
> 466 common removexattrat sys_removexattrat
> 467 common open_tree_attr sys_open_tree_attr
> +468 common file_getattr sys_file_getattr
> +469 common file_setattr sys_file_setattr
> diff --git a/arch/microblaze/kernel/syscalls/syscall.tbl b/arch/microblaze/kernel/syscalls/syscall.tbl
> index 7b6e97828e55..580af574fe73 100644
> --- a/arch/microblaze/kernel/syscalls/syscall.tbl
> +++ b/arch/microblaze/kernel/syscalls/syscall.tbl
> @@ -473,3 +473,5 @@
> 465 common listxattrat sys_listxattrat
> 466 common removexattrat sys_removexattrat
> 467 common open_tree_attr sys_open_tree_attr
> +468 common file_getattr sys_file_getattr
> +469 common file_setattr sys_file_setattr
> diff --git a/arch/mips/kernel/syscalls/syscall_n32.tbl b/arch/mips/kernel/syscalls/syscall_n32.tbl
> index aa70e371bb54..d824ffe9a014 100644
> --- a/arch/mips/kernel/syscalls/syscall_n32.tbl
> +++ b/arch/mips/kernel/syscalls/syscall_n32.tbl
> @@ -406,3 +406,5 @@
> 465 n32 listxattrat sys_listxattrat
> 466 n32 removexattrat sys_removexattrat
> 467 n32 open_tree_attr sys_open_tree_attr
> +468 n32 file_getattr sys_file_getattr
> +469 n32 file_setattr sys_file_setattr
> diff --git a/arch/mips/kernel/syscalls/syscall_n64.tbl b/arch/mips/kernel/syscalls/syscall_n64.tbl
> index 1e8c44c7b614..7a7049c2c307 100644
> --- a/arch/mips/kernel/syscalls/syscall_n64.tbl
> +++ b/arch/mips/kernel/syscalls/syscall_n64.tbl
> @@ -382,3 +382,5 @@
> 465 n64 listxattrat sys_listxattrat
> 466 n64 removexattrat sys_removexattrat
> 467 n64 open_tree_attr sys_open_tree_attr
> +468 n64 file_getattr sys_file_getattr
> +469 n64 file_setattr sys_file_setattr
> diff --git a/arch/mips/kernel/syscalls/syscall_o32.tbl b/arch/mips/kernel/syscalls/syscall_o32.tbl
> index 114a5a1a6230..d330274f0601 100644
> --- a/arch/mips/kernel/syscalls/syscall_o32.tbl
> +++ b/arch/mips/kernel/syscalls/syscall_o32.tbl
> @@ -455,3 +455,5 @@
> 465 o32 listxattrat sys_listxattrat
> 466 o32 removexattrat sys_removexattrat
> 467 o32 open_tree_attr sys_open_tree_attr
> +468 o32 file_getattr sys_file_getattr
> +469 o32 file_setattr sys_file_setattr
> diff --git a/arch/parisc/kernel/syscalls/syscall.tbl b/arch/parisc/kernel/syscalls/syscall.tbl
> index 94df3cb957e9..88a788a7b18d 100644
> --- a/arch/parisc/kernel/syscalls/syscall.tbl
> +++ b/arch/parisc/kernel/syscalls/syscall.tbl
> @@ -466,3 +466,5 @@
> 465 common listxattrat sys_listxattrat
> 466 common removexattrat sys_removexattrat
> 467 common open_tree_attr sys_open_tree_attr
> +468 common file_getattr sys_file_getattr
> +469 common file_setattr sys_file_setattr
> diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
> index 9a084bdb8926..b453e80dfc00 100644
> --- a/arch/powerpc/kernel/syscalls/syscall.tbl
> +++ b/arch/powerpc/kernel/syscalls/syscall.tbl
> @@ -558,3 +558,5 @@
> 465 common listxattrat sys_listxattrat
> 466 common removexattrat sys_removexattrat
> 467 common open_tree_attr sys_open_tree_attr
> +468 common file_getattr sys_file_getattr
> +469 common file_setattr sys_file_setattr
> diff --git a/arch/s390/kernel/syscalls/syscall.tbl b/arch/s390/kernel/syscalls/syscall.tbl
> index a4569b96ef06..8a6744d658db 100644
> --- a/arch/s390/kernel/syscalls/syscall.tbl
> +++ b/arch/s390/kernel/syscalls/syscall.tbl
> @@ -470,3 +470,5 @@
> 465 common listxattrat sys_listxattrat sys_listxattrat
> 466 common removexattrat sys_removexattrat sys_removexattrat
> 467 common open_tree_attr sys_open_tree_attr sys_open_tree_attr
> +468 common file_getattr sys_file_getattr sys_file_getattr
> +469 common file_setattr sys_file_setattr sys_file_setattr
> diff --git a/arch/sh/kernel/syscalls/syscall.tbl b/arch/sh/kernel/syscalls/syscall.tbl
> index 52a7652fcff6..5e9c9eff5539 100644
> --- a/arch/sh/kernel/syscalls/syscall.tbl
> +++ b/arch/sh/kernel/syscalls/syscall.tbl
> @@ -471,3 +471,5 @@
> 465 common listxattrat sys_listxattrat
> 466 common removexattrat sys_removexattrat
> 467 common open_tree_attr sys_open_tree_attr
> +468 common file_getattr sys_file_getattr
> +469 common file_setattr sys_file_setattr
> diff --git a/arch/sparc/kernel/syscalls/syscall.tbl b/arch/sparc/kernel/syscalls/syscall.tbl
> index 83e45eb6c095..ebb7d06d1044 100644
> --- a/arch/sparc/kernel/syscalls/syscall.tbl
> +++ b/arch/sparc/kernel/syscalls/syscall.tbl
> @@ -513,3 +513,5 @@
> 465 common listxattrat sys_listxattrat
> 466 common removexattrat sys_removexattrat
> 467 common open_tree_attr sys_open_tree_attr
> +468 common file_getattr sys_file_getattr
> +469 common file_setattr sys_file_setattr
> diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
> index ac007ea00979..4877e16da69a 100644
> --- a/arch/x86/entry/syscalls/syscall_32.tbl
> +++ b/arch/x86/entry/syscalls/syscall_32.tbl
> @@ -473,3 +473,5 @@
> 465 i386 listxattrat sys_listxattrat
> 466 i386 removexattrat sys_removexattrat
> 467 i386 open_tree_attr sys_open_tree_attr
> +468 i386 file_getattr sys_file_getattr
> +469 i386 file_setattr sys_file_setattr
> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
> index cfb5ca41e30d..92cf0fe2291e 100644
> --- a/arch/x86/entry/syscalls/syscall_64.tbl
> +++ b/arch/x86/entry/syscalls/syscall_64.tbl
> @@ -391,6 +391,8 @@
> 465 common listxattrat sys_listxattrat
> 466 common removexattrat sys_removexattrat
> 467 common open_tree_attr sys_open_tree_attr
> +468 common file_getattr sys_file_getattr
> +469 common file_setattr sys_file_setattr
>
> #
> # Due to a historical design error, certain syscalls are numbered differently
> diff --git a/arch/xtensa/kernel/syscalls/syscall.tbl b/arch/xtensa/kernel/syscalls/syscall.tbl
> index f657a77314f8..374e4cb788d8 100644
> --- a/arch/xtensa/kernel/syscalls/syscall.tbl
> +++ b/arch/xtensa/kernel/syscalls/syscall.tbl
> @@ -438,3 +438,5 @@
> 465 common listxattrat sys_listxattrat
> 466 common removexattrat sys_removexattrat
> 467 common open_tree_attr sys_open_tree_attr
> +468 common file_getattr sys_file_getattr
> +469 common file_setattr sys_file_setattr
> diff --git a/fs/file_attr.c b/fs/file_attr.c
> index 62f08872d4ad..fda9d847eee5 100644
> --- a/fs/file_attr.c
> +++ b/fs/file_attr.c
> @@ -3,6 +3,10 @@
> #include <linux/security.h>
> #include <linux/fscrypt.h>
> #include <linux/fileattr.h>
> +#include <linux/syscalls.h>
> +#include <linux/namei.h>
> +
> +#include "internal.h"
>
> /**
> * fileattr_fill_xflags - initialize fileattr with xflags
> @@ -89,6 +93,19 @@ int vfs_fileattr_get(struct dentry *dentry, struct fileattr *fa)
> }
> EXPORT_SYMBOL(vfs_fileattr_get);
>
> +static void fileattr_to_fsx_fileattr(const struct fileattr *fa,
> + struct fsx_fileattr *fsx)
> +{
> + __u32 mask = FS_XFLAGS_MASK;
> +
> + memset(fsx, 0, sizeof(struct fsx_fileattr));
Fwiw, what also works is:
*fsx = (struct fsx_fileattr){
.fsx_xflags = fa->fsx_xflags & mask,
.fsx_extsize = fa->fsx_extsize,
.fsx_nextents = fa->fsx_nextents,
.fsx_projid = fa->fsx_projid,
.fsx_cowextsize = fa->fsx_cowextsize,
}
avoiding the memset(). Anyway, all minor nits.
> + 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;
> + fsx->fsx_cowextsize = fa->fsx_cowextsize;
> +}
> +
> /**
> * copy_fsxattr_to_user - copy fsxattr to userspace.
> * @fa: fileattr pointer
> @@ -115,6 +132,23 @@ int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa)
> }
> EXPORT_SYMBOL(copy_fsxattr_to_user);
>
> +static int fsx_fileattr_to_fileattr(const struct fsx_fileattr *fsx,
> + struct fileattr *fa)
> +{
> + __u32 mask = FS_XFLAGS_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_projid = fsx->fsx_projid;
> + fa->fsx_cowextsize = fsx->fsx_cowextsize;
> +
> + return 0;
> +}
> +
> static int copy_fsxattr_from_user(struct fileattr *fa,
> struct fsxattr __user *ufa)
> {
> @@ -343,3 +377,117 @@ int ioctl_fssetxattr(struct file *file, void __user *argp)
> return err;
> }
> EXPORT_SYMBOL(ioctl_fssetxattr);
> +
> +SYSCALL_DEFINE5(file_getattr, int, dfd, const char __user *, filename,
> + struct fsx_fileattr __user *, ufsx, size_t, usize,
> + unsigned int, at_flags)
> +{
> + struct fileattr fa;
> + struct path filepath __free(path_put) = {};
> + int error;
> + unsigned int lookup_flags = 0;
> + struct filename *name __free(putname) = NULL;
Fwiw, cleanup guards should always be grouped together at the top like:
struct path filepath __free(path_put) = {};
struct filename *name __free(putname) = NULL;
struct fileattr fa;
int error;
unsigned int lookup_flags = 0;
This makes it easy to spot them when reading a function with multiple
variables on top.
> + struct fsx_fileattr fsx;
> + struct fsx_fileattr fsx;
> +
> + BUILD_BUG_ON(sizeof(struct fsx_fileattr) < FSX_FILEATTR_SIZE_VER0);
> + BUILD_BUG_ON(sizeof(struct fsx_fileattr) != FSX_FILEATTR_SIZE_LATEST);
> +
> + if ((at_flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
> + return -EINVAL;
> +
> + if (!(at_flags & AT_SYMLINK_NOFOLLOW))
> + lookup_flags |= LOOKUP_FOLLOW;
> +
> + if (usize > PAGE_SIZE)
> + return -E2BIG;
> +
> + if (usize < FSX_FILEATTR_SIZE_VER0)
> + return -EINVAL;
> +
> + name = getname_maybe_null(filename, at_flags);
> + if (IS_ERR(name))
> + return PTR_ERR(name);
> +
> + if (!name && dfd >= 0) {
> + CLASS(fd, f)(dfd);
> +
> + filepath = fd_file(f)->f_path;
> + path_get(&filepath);
> + } else {
> + error = filename_lookup(dfd, name, lookup_flags, &filepath,
> + NULL);
> + if (error)
> + return error;
> + }
> +
> + error = vfs_fileattr_get(filepath.dentry, &fa);
> + if (error)
> + return error;
> +
> + fileattr_to_fsx_fileattr(&fa, &fsx);
> + error = copy_struct_to_user(ufsx, usize, &fsx,
> + sizeof(struct fsx_fileattr), NULL);
> +
> + return error;
> +}
> +
> +SYSCALL_DEFINE5(file_setattr, int, dfd, const char __user *, filename,
> + struct fsx_fileattr __user *, ufsx, size_t, usize,
> + unsigned int, at_flags)
> +{
> + struct fileattr fa;
> + struct path filepath __free(path_put) = {};
> + int error;
> + unsigned int lookup_flags = 0;
> + struct filename *name __free(putname) = NULL;
> + struct fsx_fileattr fsx;
> +
> + BUILD_BUG_ON(sizeof(struct fsx_fileattr) < FSX_FILEATTR_SIZE_VER0);
> + BUILD_BUG_ON(sizeof(struct fsx_fileattr) != FSX_FILEATTR_SIZE_LATEST);
> +
> + if ((at_flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
> + return -EINVAL;
> +
> + if (!(at_flags & AT_SYMLINK_NOFOLLOW))
> + lookup_flags |= LOOKUP_FOLLOW;
> +
> + if (usize > PAGE_SIZE)
> + return -E2BIG;
> +
> + if (usize < FSX_FILEATTR_SIZE_VER0)
> + return -EINVAL;
> +
> + error = copy_struct_from_user(&fsx, sizeof(struct fsx_fileattr), ufsx,
> + usize);
> + if (error)
> + return error;
> +
> + error = fsx_fileattr_to_fileattr(&fsx, &fa);
> + if (error)
> + return error;
> +
> + name = getname_maybe_null(filename, at_flags);
> + if (IS_ERR(name))
> + return PTR_ERR(name);
> +
> + if (!name && dfd >= 0) {
> + CLASS(fd, f)(dfd);
> +
> + filepath = fd_file(f)->f_path;
> + path_get(&filepath);
> + } else {
> + error = filename_lookup(dfd, name, lookup_flags, &filepath,
> + NULL);
> + if (error)
> + return error;
> + }
> +
> + error = mnt_want_write(filepath.mnt);
> + if (!error) {
> + error = vfs_fileattr_set(mnt_idmap(filepath.mnt),
> + filepath.dentry, &fa);
> + mnt_drop_write(filepath.mnt);
> + }
Note-to-self: I really want scoped_guard()s for mnt_want_write() going forward...
> +
> + return error;
> +}
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index e5603cc91963..179acbe28fec 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -371,6 +371,12 @@ asmlinkage long sys_removexattrat(int dfd, const char __user *path,
> asmlinkage long sys_lremovexattr(const char __user *path,
> const char __user *name);
> asmlinkage long sys_fremovexattr(int fd, const char __user *name);
> +asmlinkage long sys_file_getattr(int dfd, const char __user *filename,
> + struct fsx_fileattr __user *ufsx, size_t usize,
> + unsigned int at_flags);
> +asmlinkage long sys_file_setattr(int dfd, const char __user *filename,
> + struct fsx_fileattr __user *ufsx, size_t usize,
> + unsigned int at_flags);
> asmlinkage long sys_getcwd(char __user *buf, unsigned long size);
> asmlinkage long sys_eventfd2(unsigned int count, int flags);
> asmlinkage long sys_epoll_create1(int flags);
> diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
> index 2892a45023af..04e0077fb4c9 100644
> --- a/include/uapi/asm-generic/unistd.h
> +++ b/include/uapi/asm-generic/unistd.h
> @@ -852,8 +852,14 @@ __SYSCALL(__NR_removexattrat, sys_removexattrat)
> #define __NR_open_tree_attr 467
> __SYSCALL(__NR_open_tree_attr, sys_open_tree_attr)
>
> +/* fs/inode.c */
> +#define __NR_file_getattr 468
> +__SYSCALL(__NR_file_getattr, sys_file_getattr)
> +#define __NR_file_setattr 469
> +__SYSCALL(__NR_file_setattr, sys_file_setattr)
> +
> #undef __NR_syscalls
> -#define __NR_syscalls 468
> +#define __NR_syscalls 470
>
> /*
> * 32 bit systems traditionally used different
> diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
> index 0098b0ce8ccb..0784f2033ba4 100644
> --- a/include/uapi/linux/fs.h
> +++ b/include/uapi/linux/fs.h
> @@ -148,6 +148,24 @@ struct fsxattr {
> unsigned char fsx_pad[8];
> };
>
> +/*
> + * Variable size structure for file_[sg]et_attr().
> + *
> + * Note. This is alternative to the structure 'struct fileattr'/'struct fsxattr'.
> + * As this structure is passed to/from userspace with its size, this can
> + * be versioned based on the size.
> + */
> +struct fsx_fileattr {
> + __u32 fsx_xflags; /* xflags field value (get/set) */
> + __u32 fsx_extsize; /* extsize field value (get/set)*/
> + __u32 fsx_nextents; /* nextents field value (get) */
> + __u32 fsx_projid; /* project identifier (get/set) */
> + __u32 fsx_cowextsize; /* CoW extsize field value (get/set) */
This misses a:
__u32 __spare;
so there's no holes in the struct. :)
> +};
> +
> +#define FSX_FILEATTR_SIZE_VER0 20
> +#define FSX_FILEATTR_SIZE_LATEST FSX_FILEATTR_SIZE_VER0
> +
> /*
> * Flags for the fsx_xflags field
> */
> diff --git a/scripts/syscall.tbl b/scripts/syscall.tbl
> index 580b4e246aec..d1ae5e92c615 100644
> --- a/scripts/syscall.tbl
> +++ b/scripts/syscall.tbl
> @@ -408,3 +408,5 @@
> 465 common listxattrat sys_listxattrat
> 466 common removexattrat sys_removexattrat
> 467 common open_tree_attr sys_open_tree_attr
> +468 common file_getattr sys_file_getattr
> +469 common file_setattr sys_file_setattr
>
> --
> 2.47.2
>
^ permalink raw reply
* Re: [PATCH v6 0/6] fs: introduce file_getattr and file_setattr syscalls
From: Christian Brauner @ 2025-07-01 12:29 UTC (permalink / raw)
To: Andrey Albershteyn
Cc: Amir Goldstein, Arnd Bergmann, Casey Schaufler, Jan Kara,
Pali Rohár, Paul Moore, linux-api, linux-fsdevel,
linux-kernel, linux-xfs, selinux, Andrey Albershteyn
In-Reply-To: <20250630-xattrat-syscall-v6-0-c4e3bc35227b@kernel.org>
On Mon, Jun 30, 2025 at 06:20:10PM +0200, Andrey Albershteyn wrote:
> This patchset introduced two new syscalls file_getattr() and
> file_setattr(). These syscalls are similar to FS_IOC_FSSETXATTR ioctl()
> except they use *at() semantics. Therefore, there's no need to open the
> file to get a fd.
>
> These syscalls allow userspace to set filesystem inode attributes on
> special files. One of the usage examples is XFS quota projects.
>
> XFS has project quotas which could be attached to a directory. All
> new inodes in these directories inherit project ID set on parent
> directory.
>
> The project is created from userspace by opening and calling
> FS_IOC_FSSETXATTR on each inode. This is not possible for special
> files such as FIFO, SOCK, BLK etc. Therefore, some inodes are left
> with empty project ID. Those inodes then are not shown in the quota
> accounting but still exist in the directory. This is not critical but in
> the case when special files are created in the directory with already
> existing project quota, these new inodes inherit extended attributes.
> This creates a mix of special files with and without attributes.
> Moreover, special files with attributes don't have a possibility to
> become clear or change the attributes. This, in turn, prevents userspace
> from re-creating quota project on these existing files.
Only small nits I'm going to comment on that I can fix myself.
Otherwise looks great.
^ permalink raw reply
* Re: [PATCH v4 3/3] AppArmor: add support for lsm_config_self_policy and lsm_config_system_policy
From: Tetsuo Handa @ 2025-07-01 10:01 UTC (permalink / raw)
To: Maxime Bélair, linux-security-module
Cc: john.johansen, paul, jmorris, serge, mic, kees,
stephen.smalley.work, casey, takedakn, song, rdunlap, linux-api,
apparmor, linux-kernel
In-Reply-To: <20250701091904.395837-4-maxime.belair@canonical.com>
On 2025/07/01 18:17, Maxime Bélair wrote:
> +static int apparmor_lsm_config_self_policy(u32 lsm_id, u32 op, void __user *buf,
> + size_t size, u32 flags)
> +{
> + char *name;
> + long name_size;
> + int ret;
> +
> + if (op != LSM_POLICY_LOAD || flags)
> + return -EOPNOTSUPP;
> + if (size > AA_PROFILE_NAME_MAX_SIZE)
> + return -E2BIG;
> +
> + name = kmalloc(size, GFP_KERNEL);
> + if (!name)
> + return -ENOMEM;
> +
> +
> + name_size = strncpy_from_user(name, buf, size);
> + if (name_size < 0) {
> + kfree(name);
> + return name_size;
> + }
> +
> + ret = aa_change_profile(name, AA_CHANGE_STACK);
If size == 0, name == ZERO_SIZE_PTR and name_size == 0.
Then, aa_change_profile() will oops due to ZERO_SIZE_PTR deref.
> +
> + kfree(name);
> +
> + return ret;
> +}
> +
> +/**
> + * apparmor_lsm_config_system_policy - Load or replace a system policy
> + * @lsm_id: AppArmor ID (LSM_ID_APPARMOR). Unused here
> + * @op: operation to perform. Currently, only LSM_POLICY_LOAD is supported
> + * @buf: user-supplied buffer in the form "<ns>\0<policy>"
> + * <ns> is the namespace to load the policy into (empty string for root)
> + * <policy> is the policy to load
> + * @size: size of @buf
> + * @flags: reserved for future uses; must be zero
> + *
> + * Returns: 0 on success, negative value on error
> + */
> +static int apparmor_lsm_config_system_policy(u32 lsm_id, u32 op, void __user *buf,
> + size_t size, u32 flags)
> +{
> + loff_t pos = 0; // Partial writing is not currently supported
> + char name[AA_PROFILE_NAME_MAX_SIZE];
> + long name_size;
> +
> + if (op != LSM_POLICY_LOAD || flags)
> + return -EOPNOTSUPP;
> + if (size > AA_PROFILE_MAX_SIZE)
> + return -E2BIG;
> +
> + name_size = strncpy_from_user(name, buf, AA_PROFILE_NAME_MAX_SIZE);
> + if (name_size < 0)
> + return name_size;
> + else if (name_size == AA_PROFILE_NAME_MAX_SIZE)
> + return -E2BIG;
> +
> + return aa_profile_load_ns_name(name, name_size, buf + name_size + 1,
> + size - name_size - 1, &pos);
If size == 0 and *name == '\0', name_size == 0. Then, size will be -1 at aa_profile_load_ns_name()
and WARN_ON_ONCE() in __kvmalloc_node_noprof() from kvzalloc() from policy_update() will trigger?
You need more stricter checks to verify that @buf is in the form "<ns>\0<policy>".
strncpy_from_user() should not try to read more than @size bytes.
> +}
> +
> +
Also, in [PATCH v4 2/3], why do you need lines below?
These functions are supposed to be called via only syscalls, aren't these?
+EXPORT_SYMBOL(security_lsm_config_self_policy);
+EXPORT_SYMBOL(security_lsm_config_system_policy);
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox