* 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: [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: [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 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 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 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 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: [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: 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: [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 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 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: 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: [RFC v2 01/22] kernel/api: introduce kernel API specification framework
From: Sasha Levin @ 2025-07-01 20:50 UTC (permalink / raw)
To: Jonathan Corbet
Cc: Mauro Carvalho Chehab, linux-kernel, linux-doc, linux-api,
workflows, tools, Kate Stewart, Gabriele Paoloni, Chuck Wolber
In-Reply-To: <8734bfspko.fsf@trenco.lwn.net>
On Tue, Jul 01, 2025 at 01:01:27PM -0600, Jonathan Corbet wrote:
>[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
Yup, we all met at OSS and reached the conclusion that we should lean
towards a machine readable spec, which we thought was closer to my
proposal than the kerneldoc work.
However, with your suggestion, I think it makes more sense to go back to
kerneldoc as that can be made machine readable.
>> 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.
Right!
So I have a proof of concept which during the build process creates
.apispec.h which are generated from kerneldoc and contain macros
identical to the ones in my RFC.
Here's an example of sys_mlock() spec:
/**
* sys_mlock - Lock pages in memory
* @start: Starting address of memory range to lock
* @len: Length of memory range to lock in bytes
*
* Locks pages in the specified address range into RAM, preventing them from
* being paged to swap. Requires CAP_IPC_LOCK capability or RLIMIT_MEMLOCK
* resource limit.
*
* long-desc: Locks pages in the specified address range into RAM, preventing
* them from being paged to swap. Requires CAP_IPC_LOCK capability
* or RLIMIT_MEMLOCK resource limit.
* context-flags: KAPI_CTX_PROCESS | KAPI_CTX_SLEEPABLE
* param-type: start, KAPI_TYPE_UINT
* param-flags: start, KAPI_PARAM_IN
* param-constraint-type: start, KAPI_CONSTRAINT_NONE
* param-constraint: start, Rounded down to page boundary
* param-type: len, KAPI_TYPE_UINT
* param-flags: len, KAPI_PARAM_IN
* param-constraint-type: len, KAPI_CONSTRAINT_RANGE
* param-range: len, 0, LONG_MAX
* param-constraint: len, Rounded up to page boundary
* return-type: KAPI_TYPE_INT
* return-check-type: KAPI_RETURN_ERROR_CHECK
* return-success: 0
* error-code: -ENOMEM, ENOMEM, Address range issue,
* Some of the specified range is not mapped, has unmapped gaps,
* or the lock would cause the number of mapped regions to exceed the limit.
* error-code: -EPERM, EPERM, Insufficient privileges,
* The caller is not privileged (no CAP_IPC_LOCK) and RLIMIT_MEMLOCK is 0.
* error-code: -EINVAL, EINVAL, Address overflow,
* The result of the addition start+len was less than start (arithmetic overflow).
* error-code: -EAGAIN, EAGAIN, Some or all memory could not be locked,
* Some or all of the specified address range could not be locked.
* error-code: -EINTR, EINTR, Interrupted by signal,
* The operation was interrupted by a fatal signal before completion.
* error-code: -EFAULT, EFAULT, Bad address,
* The specified address range contains invalid addresses that cannot be accessed.
* since-version: 2.0
* lock: mmap_lock, KAPI_LOCK_RWLOCK
* lock-acquired: true
* lock-released: true
* lock-desc: Process memory map write lock
* signal: FATAL
* signal-direction: KAPI_SIGNAL_RECEIVE
* signal-action: KAPI_SIGNAL_ACTION_RETURN
* signal-condition: Fatal signal pending
* signal-desc: Fatal signals (SIGKILL) can interrupt the operation at two points:
* when acquiring mmap_write_lock_killable() and during page population
* in __mm_populate(). Returns -EINTR. Non-fatal signals do NOT interrupt
* mlock - the operation continues even if SIGINT/SIGTERM are received.
* signal-error: -EINTR
* signal-timing: KAPI_SIGNAL_TIME_DURING
* signal-priority: 0
* signal-interruptible: yes
* signal-state-req: KAPI_SIGNAL_STATE_RUNNING
* examples: mlock(addr, 4096); // Lock one page
* mlock(addr, len); // Lock range of pages
* notes: Memory locks do not stack - multiple calls on the same range can be
* undone by a single munlock. Locks are not inherited by child processes.
* Pages are locked on whole page boundaries. Commonly used by real-time
* applications to prevent page faults during time-critical operations.
* Also used for security to prevent sensitive data (e.g., cryptographic keys)
* from being written to swap. Note: locked pages may still be saved to
* swap during system suspend/hibernate.
*
* Tagged addresses are automatically handled via untagged_addr(). The operation
* occurs in two phases: first VMAs are marked with VM_LOCKED, then pages are
* populated into memory. When checking RLIMIT_MEMLOCK, the kernel optimizes
* by recounting locked memory to avoid double-counting overlapping regions.
* side-effect: KAPI_EFFECT_MODIFY_STATE | KAPI_EFFECT_ALLOC_MEMORY, process memory, Locks pages into physical memory, preventing swapping, reversible=yes
* side-effect: KAPI_EFFECT_MODIFY_STATE, mm->locked_vm, Increases process locked memory counter, reversible=yes
* side-effect: KAPI_EFFECT_ALLOC_MEMORY, physical pages, May allocate and populate page table entries, condition=Pages not already present, reversible=yes
* side-effect: KAPI_EFFECT_MODIFY_STATE | KAPI_EFFECT_ALLOC_MEMORY, page faults, Triggers page faults to bring pages into memory, condition=Pages not already resident
* side-effect: KAPI_EFFECT_MODIFY_STATE, VMA splitting, May split existing VMAs at lock boundaries, condition=Lock range partially overlaps existing VMA
* state-trans: memory pages, swappable, locked in RAM, Pages become non-swappable and pinned in physical memory
* state-trans: VMA flags, unlocked, VM_LOCKED set, Virtual memory area marked as locked
* capability: CAP_IPC_LOCK, KAPI_CAP_BYPASS_CHECK, CAP_IPC_LOCK capability
* capability-allows: Lock unlimited amount of memory (no RLIMIT_MEMLOCK enforcement)
* capability-without: Must respect RLIMIT_MEMLOCK resource limit
* capability-condition: Checked when RLIMIT_MEMLOCK is 0 or locking would exceed limit
* capability-priority: 0
* constraint: RLIMIT_MEMLOCK Resource Limit, The RLIMIT_MEMLOCK soft resource limit specifies the maximum bytes of memory that may be locked into RAM. Unprivileged processes are restricted to this limit. CAP_IPC_LOCK capability allows bypassing this limit entirely. The limit is enforced per-process, not per-user.
* constraint-expr: RLIMIT_MEMLOCK Resource Limit, locked_memory + request_size <= RLIMIT_MEMLOCK || CAP_IPC_LOCK
* constraint: Memory Pressure and OOM, Locking large amounts of memory can cause system-wide memory pressure and potentially trigger the OOM killer. The kernel does not prevent locking memory that would destabilize the system.
* constraint: Special Memory Areas, Some memory types cannot be locked or are silently skipped: VM_IO/VM_PFNMAP areas (device mappings) are skipped; Hugetlb pages are inherently pinned and skipped; DAX mappings are always present in memory and skipped; Secret memory (memfd_secret) mappings are skipped; VM_DROPPABLE memory cannot be locked and is skipped; Gate VMA (kernel entry point) is skipped; VM_LOCKED areas are already locked. These special areas are silently excluded without error.
*
* Context: Process context. May sleep. Takes mmap_lock for write.
*
* Return: 0 on success, negative error code on failure
*/
>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 :)
In the RFC I've sent out I've specced out API from different subsystems
to solicit some feedback on those, but so fair it's been quiet.
I'll resend a "lean" RFC v3 with just the base macro spec infra +
kerneldoc support + "tricker" sched API + "trickier" mm API.
I'm thinking that if it's still quiet in a month or two I'll propose a
talk at LPC around it, or maybe try and feedback/consensus during
maintainer's summit.
But yes, it doesn't make sense to take it in until we have an ack from a
few larger subsystems.
--
Thanks,
Sasha
^ permalink raw reply
* Re: [RFC v2 01/22] kernel/api: introduce kernel API specification framework
From: Jonathan Corbet @ 2025-07-01 21:43 UTC (permalink / raw)
To: Sasha Levin
Cc: Mauro Carvalho Chehab, linux-kernel, linux-doc, linux-api,
workflows, tools, Kate Stewart, Gabriele Paoloni, Chuck Wolber
In-Reply-To: <aGRKIuR6hgW0YLc_@lappy>
Sasha Levin <sashal@kernel.org> writes:
> So I have a proof of concept which during the build process creates
> .apispec.h which are generated from kerneldoc and contain macros
> identical to the ones in my RFC.
>
> Here's an example of sys_mlock() spec:
So I'm getting ahead of the game, but I have to ask some questions...
> /**
> * sys_mlock - Lock pages in memory
> * @start: Starting address of memory range to lock
> * @len: Length of memory range to lock in bytes
> *
> * Locks pages in the specified address range into RAM, preventing them from
> * being paged to swap. Requires CAP_IPC_LOCK capability or RLIMIT_MEMLOCK
> * resource limit.
> *
> * long-desc: Locks pages in the specified address range into RAM, preventing
> * them from being paged to swap. Requires CAP_IPC_LOCK capability
> * or RLIMIT_MEMLOCK resource limit.
Why duplicate the long description?
> * context-flags: KAPI_CTX_PROCESS | KAPI_CTX_SLEEPABLE
> * param-type: start, KAPI_TYPE_UINT
This is something I wondered before; rather than a bunch of lengthy
KAPI_* symbols, why not just say __u64 (or some other familiar type)
here?
> * param-flags: start, KAPI_PARAM_IN
> * param-constraint-type: start, KAPI_CONSTRAINT_NONE
> * param-constraint: start, Rounded down to page boundary
> * param-type: len, KAPI_TYPE_UINT
> * param-flags: len, KAPI_PARAM_IN
> * param-constraint-type: len, KAPI_CONSTRAINT_RANGE
> * param-range: len, 0, LONG_MAX
> * param-constraint: len, Rounded up to page boundary
> * return-type: KAPI_TYPE_INT
> * return-check-type: KAPI_RETURN_ERROR_CHECK
> * return-success: 0
> * error-code: -ENOMEM, ENOMEM, Address range issue,
> * Some of the specified range is not mapped, has unmapped gaps,
> * or the lock would cause the number of mapped regions to exceed the limit.
> * error-code: -EPERM, EPERM, Insufficient privileges,
> * The caller is not privileged (no CAP_IPC_LOCK) and RLIMIT_MEMLOCK is 0.
> * error-code: -EINVAL, EINVAL, Address overflow,
> * The result of the addition start+len was less than start (arithmetic overflow).
> * error-code: -EAGAIN, EAGAIN, Some or all memory could not be locked,
> * Some or all of the specified address range could not be locked.
> * error-code: -EINTR, EINTR, Interrupted by signal,
> * The operation was interrupted by a fatal signal before completion.
> * error-code: -EFAULT, EFAULT, Bad address,
> * The specified address range contains invalid addresses that cannot be accessed.
> * since-version: 2.0
> * lock: mmap_lock, KAPI_LOCK_RWLOCK
> * lock-acquired: true
> * lock-released: true
> * lock-desc: Process memory map write lock
> * signal: FATAL
> * signal-direction: KAPI_SIGNAL_RECEIVE
> * signal-action: KAPI_SIGNAL_ACTION_RETURN
> * signal-condition: Fatal signal pending
> * signal-desc: Fatal signals (SIGKILL) can interrupt the operation at two points:
> * when acquiring mmap_write_lock_killable() and during page population
> * in __mm_populate(). Returns -EINTR. Non-fatal signals do NOT interrupt
> * mlock - the operation continues even if SIGINT/SIGTERM are received.
> * signal-error: -EINTR
> * signal-timing: KAPI_SIGNAL_TIME_DURING
> * signal-priority: 0
> * signal-interruptible: yes
> * signal-state-req: KAPI_SIGNAL_STATE_RUNNING
> * examples: mlock(addr, 4096); // Lock one page
> * mlock(addr, len); // Lock range of pages
> * notes: Memory locks do not stack - multiple calls on the same range can be
> * undone by a single munlock. Locks are not inherited by child processes.
> * Pages are locked on whole page boundaries. Commonly used by real-time
> * applications to prevent page faults during time-critical operations.
> * Also used for security to prevent sensitive data (e.g., cryptographic keys)
> * from being written to swap. Note: locked pages may still be saved to
> * swap during system suspend/hibernate.
> *
> * Tagged addresses are automatically handled via untagged_addr(). The operation
> * occurs in two phases: first VMAs are marked with VM_LOCKED, then pages are
> * populated into memory. When checking RLIMIT_MEMLOCK, the kernel optimizes
> * by recounting locked memory to avoid double-counting overlapping regions.
> * side-effect: KAPI_EFFECT_MODIFY_STATE | KAPI_EFFECT_ALLOC_MEMORY, process memory, Locks pages into physical memory, preventing swapping, reversible=yes
I hope the really long lines starting here aren't the intended way to go...:)
> * side-effect: KAPI_EFFECT_MODIFY_STATE, mm->locked_vm, Increases process locked memory counter, reversible=yes
> * side-effect: KAPI_EFFECT_ALLOC_MEMORY, physical pages, May allocate and populate page table entries, condition=Pages not already present, reversible=yes
> * side-effect: KAPI_EFFECT_MODIFY_STATE | KAPI_EFFECT_ALLOC_MEMORY, page faults, Triggers page faults to bring pages into memory, condition=Pages not already resident
> * side-effect: KAPI_EFFECT_MODIFY_STATE, VMA splitting, May split existing VMAs at lock boundaries, condition=Lock range partially overlaps existing VMA
> * state-trans: memory pages, swappable, locked in RAM, Pages become non-swappable and pinned in physical memory
> * state-trans: VMA flags, unlocked, VM_LOCKED set, Virtual memory area marked as locked
> * capability: CAP_IPC_LOCK, KAPI_CAP_BYPASS_CHECK, CAP_IPC_LOCK capability
> * capability-allows: Lock unlimited amount of memory (no RLIMIT_MEMLOCK enforcement)
> * capability-without: Must respect RLIMIT_MEMLOCK resource limit
> * capability-condition: Checked when RLIMIT_MEMLOCK is 0 or locking would exceed limit
> * capability-priority: 0
> * constraint: RLIMIT_MEMLOCK Resource Limit, The RLIMIT_MEMLOCK soft resource limit specifies the maximum bytes of memory that may be locked into RAM. Unprivileged processes are restricted to this limit. CAP_IPC_LOCK capability allows bypassing this limit entirely. The limit is enforced per-process, not per-user.
> * constraint-expr: RLIMIT_MEMLOCK Resource Limit, locked_memory + request_size <= RLIMIT_MEMLOCK || CAP_IPC_LOCK
> * constraint: Memory Pressure and OOM, Locking large amounts of memory can cause system-wide memory pressure and potentially trigger the OOM killer. The kernel does not prevent locking memory that would destabilize the system.
> * constraint: Special Memory Areas, Some memory types cannot be locked or are silently skipped: VM_IO/VM_PFNMAP areas (device mappings) are skipped; Hugetlb pages are inherently pinned and skipped; DAX mappings are always present in memory and skipped; Secret memory (memfd_secret) mappings are skipped; VM_DROPPABLE memory cannot be locked and is skipped; Gate VMA (kernel entry point) is skipped; VM_LOCKED areas are already locked. These special areas are silently excluded without error.
> *
> * Context: Process context. May sleep. Takes mmap_lock for write.
> *
> * Return: 0 on success, negative error code on failure
Both of these, of course, are much less informative versions of the data
you have put up above; it would be nice to unify them somehow.
Thanks,
jon
^ permalink raw reply
* Re: [RFC v2 01/22] kernel/api: introduce kernel API specification framework
From: Sasha Levin @ 2025-07-01 22:16 UTC (permalink / raw)
To: Jonathan Corbet
Cc: Mauro Carvalho Chehab, linux-kernel, linux-doc, linux-api,
workflows, tools, Kate Stewart, Gabriele Paoloni, Chuck Wolber
In-Reply-To: <87v7obpoxn.fsf@trenco.lwn.net>
On Tue, Jul 01, 2025 at 03:43:32PM -0600, Jonathan Corbet wrote:
>Sasha Levin <sashal@kernel.org> writes:
>
>> So I have a proof of concept which during the build process creates
>> .apispec.h which are generated from kerneldoc and contain macros
>> identical to the ones in my RFC.
>>
>> Here's an example of sys_mlock() spec:
>
>So I'm getting ahead of the game, but I have to ask some questions...
>
>> /**
>> * sys_mlock - Lock pages in memory
>> * @start: Starting address of memory range to lock
>> * @len: Length of memory range to lock in bytes
>> *
>> * Locks pages in the specified address range into RAM, preventing them from
>> * being paged to swap. Requires CAP_IPC_LOCK capability or RLIMIT_MEMLOCK
>> * resource limit.
>> *
>> * long-desc: Locks pages in the specified address range into RAM, preventing
>> * them from being paged to swap. Requires CAP_IPC_LOCK capability
>> * or RLIMIT_MEMLOCK resource limit.
>
>Why duplicate the long description?
Will fix.
>> * context-flags: KAPI_CTX_PROCESS | KAPI_CTX_SLEEPABLE
>> * param-type: start, KAPI_TYPE_UINT
>
>This is something I wondered before; rather than a bunch of lengthy
>KAPI_* symbols, why not just say __u64 (or some other familiar type)
>here?
I think it gets tricky when we got to more complex types. For example,
how do we represent a FD or a (struct sockaddr *)?
With macros, KAPI_TYPE_FD or KAPI_TYPE_SOCKADDR make sense, but
__sockaddr will be a bit confusing (I think).
>> * param-flags: start, KAPI_PARAM_IN
>> * param-constraint-type: start, KAPI_CONSTRAINT_NONE
>> * param-constraint: start, Rounded down to page boundary
>> * param-type: len, KAPI_TYPE_UINT
>> * param-flags: len, KAPI_PARAM_IN
>> * param-constraint-type: len, KAPI_CONSTRAINT_RANGE
>> * param-range: len, 0, LONG_MAX
>> * param-constraint: len, Rounded up to page boundary
>> * return-type: KAPI_TYPE_INT
>> * return-check-type: KAPI_RETURN_ERROR_CHECK
>> * return-success: 0
>> * error-code: -ENOMEM, ENOMEM, Address range issue,
>> * Some of the specified range is not mapped, has unmapped gaps,
>> * or the lock would cause the number of mapped regions to exceed the limit.
>> * error-code: -EPERM, EPERM, Insufficient privileges,
>> * The caller is not privileged (no CAP_IPC_LOCK) and RLIMIT_MEMLOCK is 0.
>> * error-code: -EINVAL, EINVAL, Address overflow,
>> * The result of the addition start+len was less than start (arithmetic overflow).
>> * error-code: -EAGAIN, EAGAIN, Some or all memory could not be locked,
>> * Some or all of the specified address range could not be locked.
>> * error-code: -EINTR, EINTR, Interrupted by signal,
>> * The operation was interrupted by a fatal signal before completion.
>> * error-code: -EFAULT, EFAULT, Bad address,
>> * The specified address range contains invalid addresses that cannot be accessed.
>> * since-version: 2.0
>> * lock: mmap_lock, KAPI_LOCK_RWLOCK
>> * lock-acquired: true
>> * lock-released: true
>> * lock-desc: Process memory map write lock
>> * signal: FATAL
>> * signal-direction: KAPI_SIGNAL_RECEIVE
>> * signal-action: KAPI_SIGNAL_ACTION_RETURN
>> * signal-condition: Fatal signal pending
>> * signal-desc: Fatal signals (SIGKILL) can interrupt the operation at two points:
>> * when acquiring mmap_write_lock_killable() and during page population
>> * in __mm_populate(). Returns -EINTR. Non-fatal signals do NOT interrupt
>> * mlock - the operation continues even if SIGINT/SIGTERM are received.
>> * signal-error: -EINTR
>> * signal-timing: KAPI_SIGNAL_TIME_DURING
>> * signal-priority: 0
>> * signal-interruptible: yes
>> * signal-state-req: KAPI_SIGNAL_STATE_RUNNING
>> * examples: mlock(addr, 4096); // Lock one page
>> * mlock(addr, len); // Lock range of pages
>> * notes: Memory locks do not stack - multiple calls on the same range can be
>> * undone by a single munlock. Locks are not inherited by child processes.
>> * Pages are locked on whole page boundaries. Commonly used by real-time
>> * applications to prevent page faults during time-critical operations.
>> * Also used for security to prevent sensitive data (e.g., cryptographic keys)
>> * from being written to swap. Note: locked pages may still be saved to
>> * swap during system suspend/hibernate.
>> *
>> * Tagged addresses are automatically handled via untagged_addr(). The operation
>> * occurs in two phases: first VMAs are marked with VM_LOCKED, then pages are
>> * populated into memory. When checking RLIMIT_MEMLOCK, the kernel optimizes
>> * by recounting locked memory to avoid double-counting overlapping regions.
>> * side-effect: KAPI_EFFECT_MODIFY_STATE | KAPI_EFFECT_ALLOC_MEMORY, process memory, Locks pages into physical memory, preventing swapping, reversible=yes
>
>I hope the really long lines starting here aren't the intended way to go...:)
I guess that we have two options around more complex blocks like these.
One, the longer lines you've pointed out. They are indeed long and
difficult to read, but they present a relatively static and "not too
interesting" information which users are likely to gloss over.
The other one would look something like:
side-effect: KAPI_EFFECT_MODIFY_STATE
side-effect-type: KAPI_EFFECT_MODIFY_STATE
side-effect-target: mm->locked_vm
side-effect-description: Increases process locked memory counter
side-effect-reversible: yes
Which isn't as long, but it occupies a bunch of vertical real estate
while not being too interesting for most of the readers.
>> * side-effect: KAPI_EFFECT_MODIFY_STATE, mm->locked_vm, Increases process locked memory counter, reversible=yes
>> * side-effect: KAPI_EFFECT_ALLOC_MEMORY, physical pages, May allocate and populate page table entries, condition=Pages not already present, reversible=yes
>> * side-effect: KAPI_EFFECT_MODIFY_STATE | KAPI_EFFECT_ALLOC_MEMORY, page faults, Triggers page faults to bring pages into memory, condition=Pages not already resident
>> * side-effect: KAPI_EFFECT_MODIFY_STATE, VMA splitting, May split existing VMAs at lock boundaries, condition=Lock range partially overlaps existing VMA
>> * state-trans: memory pages, swappable, locked in RAM, Pages become non-swappable and pinned in physical memory
>> * state-trans: VMA flags, unlocked, VM_LOCKED set, Virtual memory area marked as locked
>> * capability: CAP_IPC_LOCK, KAPI_CAP_BYPASS_CHECK, CAP_IPC_LOCK capability
>> * capability-allows: Lock unlimited amount of memory (no RLIMIT_MEMLOCK enforcement)
>> * capability-without: Must respect RLIMIT_MEMLOCK resource limit
>> * capability-condition: Checked when RLIMIT_MEMLOCK is 0 or locking would exceed limit
>> * capability-priority: 0
>> * constraint: RLIMIT_MEMLOCK Resource Limit, The RLIMIT_MEMLOCK soft resource limit specifies the maximum bytes of memory that may be locked into RAM. Unprivileged processes are restricted to this limit. CAP_IPC_LOCK capability allows bypassing this limit entirely. The limit is enforced per-process, not per-user.
>> * constraint-expr: RLIMIT_MEMLOCK Resource Limit, locked_memory + request_size <= RLIMIT_MEMLOCK || CAP_IPC_LOCK
>> * constraint: Memory Pressure and OOM, Locking large amounts of memory can cause system-wide memory pressure and potentially trigger the OOM killer. The kernel does not prevent locking memory that would destabilize the system.
>> * constraint: Special Memory Areas, Some memory types cannot be locked or are silently skipped: VM_IO/VM_PFNMAP areas (device mappings) are skipped; Hugetlb pages are inherently pinned and skipped; DAX mappings are always present in memory and skipped; Secret memory (memfd_secret) mappings are skipped; VM_DROPPABLE memory cannot be locked and is skipped; Gate VMA (kernel entry point) is skipped; VM_LOCKED areas are already locked. These special areas are silently excluded without error.
>> *
>> * Context: Process context. May sleep. Takes mmap_lock for write.
>> *
>> * Return: 0 on success, negative error code on failure
>
>Both of these, of course, are much less informative versions of the data
>you have put up above; it would be nice to unify them somehow.
Ack
--
Thanks,
Sasha
^ permalink raw reply
* Re: [PATCH v6 5/6] fs: prepare for extending file_get/setattr()
From: Amir Goldstein @ 2025-07-02 7:03 UTC (permalink / raw)
To: Pali Rohár
Cc: Darrick J. Wong, 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: <20250701195405.xf27mjknu5bnunue@pali>
On Tue, Jul 1, 2025 at 9:54 PM Pali Rohár <pali@kernel.org> wrote:
>
> On Tuesday 01 July 2025 12:40:02 Darrick J. Wong wrote:
> > On Tue, Jul 01, 2025 at 09:27:38PM +0200, Amir Goldstein wrote:
> > > On Tue, Jul 1, 2025 at 8:31 PM Darrick J. Wong <djwong@kernel.org> wrote:
> > > >
> > > > On Mon, Jun 30, 2025 at 06:20:15PM +0200, Andrey Albershteyn wrote:
> > > > > From: Amir Goldstein <amir73il@gmail.com>
> > > > >
> > > > > We intend to add support for more xflags to selective filesystems and
> > > > > We cannot rely on copy_struct_from_user() to detect this extension.
> > > > >
> > > > > In preparation of extending the API, do not allow setting xflags unknown
> > > > > by this kernel version.
> > > > >
> > > > > Also do not pass the read-only flags and read-only field fsx_nextents to
> > > > > filesystem.
> > > > >
> > > > > These changes should not affect existing chattr programs that use the
> > > > > ioctl to get fsxattr before setting the new values.
> > > > >
> > > > > Link: https://lore.kernel.org/linux-fsdevel/20250216164029.20673-4-pali@kernel.org/
> > > > > Cc: Pali Rohár <pali@kernel.org>
> > > > > Cc: Andrey Albershteyn <aalbersh@redhat.com>
> > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > > > Signed-off-by: Andrey Albershteyn <aalbersh@kernel.org>
> > > > > ---
> > > > > fs/file_attr.c | 8 +++++++-
> > > > > include/linux/fileattr.h | 20 ++++++++++++++++++++
> > > > > 2 files changed, 27 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/fs/file_attr.c b/fs/file_attr.c
> > > > > index 4e85fa00c092..62f08872d4ad 100644
> > > > > --- a/fs/file_attr.c
> > > > > +++ b/fs/file_attr.c
> > > > > @@ -99,9 +99,10 @@ EXPORT_SYMBOL(vfs_fileattr_get);
> > > > > int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa)
> > > > > {
> > > > > struct fsxattr xfa;
> > > > > + __u32 mask = FS_XFLAGS_MASK;
> > > > >
> > > > > memset(&xfa, 0, sizeof(xfa));
> > > > > - xfa.fsx_xflags = fa->fsx_xflags;
> > > > > + xfa.fsx_xflags = fa->fsx_xflags & mask;
> > > >
> > > > I wonder, should it be an error if a filesystem sets an fsx_xflags bit
> > > > outside of FS_XFLAGS_MASK? I guess that's one way to prevent
> > > > filesystems from overriding the VFS bits. ;)
> > >
> > > I think Pali has a plan on how to ensure that later
> > > when the mask is provided via the API.
> > >
> > > >
> > > > Though couldn't that be:
> > > >
> > > > xfa.fsx_xflags = fa->fsx_xflags & FS_XFLAGS_MASK;
> > > >
> > > > instead? And same below?
> > > >
> > >
> > > Indeed. There is a reason for the var, because the next series
> > > by Pali will use a user provided mask, which defaults to FS_XFLAGS_MASK,
> > > so I left it this way.
> > >
> > > I don't see a problem with it keeping as is, but if it bothers you
> > > I guess we can re-add the var later.
> >
> > Nah, it doesn't bother me that much.
> >
> > > > > xfa.fsx_extsize = fa->fsx_extsize;
> > > > > xfa.fsx_nextents = fa->fsx_nextents;
> > > > > xfa.fsx_projid = fa->fsx_projid;
> > > > > @@ -118,11 +119,16 @@ static int copy_fsxattr_from_user(struct fileattr *fa,
> > > > > struct fsxattr __user *ufa)
> > > > > {
> > > > > struct fsxattr xfa;
> > > > > + __u32 mask = FS_XFLAGS_MASK;
> > > > >
> > > > > if (copy_from_user(&xfa, ufa, sizeof(xfa)))
> > > > > return -EFAULT;
> > > > >
> > > > > + if (xfa.fsx_xflags & ~mask)
> > > > > + return -EINVAL;
> > > >
> > > > I wonder if you want EOPNOTSUPP here? We don't know how to support
> > > > unknown xflags. OTOH if you all have beaten this to death while I was
> > > > out then don't start another round just for me. :P
> > >
> > > We have beaten this API almost to death for sure ;)
> > > I don't remember if we discussed this specific aspect,
> > > but I am personally in favor of
> > > EOPNOTSUPP := the fs does not support the set/get operation
> > > EINVAL := some flags provided as value is invalid
> > >
> > > For example, if the get API provides you with a mask of the
> > > valid flags that you can set, if you try to set flags outside of
> > > that mask you get EINVAL.
> > >
> > > That's my interpretation, but I agree that EOPNOTSUPP can also
> > > make sense in this situation.
> >
> > <nod> I think I'd rather EOPNOTSUPP for "bits are set that the kernel
> > doesn't recognize" and EINVAL (or maybe something else like
> > EPROTONOSUPPORT) for "fs driver will not let you change this bit".
> > At least for the syscall interface; we probably have to flatten that to
> > EOPNOTSUPP for both legacy ioctls.
Given the precedents of returning EOPNOTSUPP in xfs_fileattr_set()
and ext4_ioctl_setflags() for flags that cannot be set, I agree.
>
> ... and this starting to be complicated if the "fs driver" is network
> based (as fs driver can support, but remote server not). See also:
> https://lore.kernel.org/linux-fsdevel/20241224160535.pi6nazpugqkhvfns@pali/t/#u
>
> For backup/restore application it would be very useful to distinguish between:
> - "kernel does not support flag X"
> - "target filesystem does not support flag X"
> - "wrong structure was passed / syscall incorrectly called"
>
> third option is bug in application - fatal error. second option is just
> a warning for user (sorry, we cannot set NEW FEATURE on FAT32, but if
> you would do restore to other fs, it is supported). and first option
> happens when you run new application on older kernel version, it is an
> recoverable error (or warning to user, but with more important level
> then second option as switching to different FS would not help).
>
> Could we return different errnos for these 3 situations?
That would be nice, but actually according to your plan
the get API returns the mask of flags supported by the filesystem
(on that specific object even), so userspace in fact has a way to
distinguish between the first two EOPNOTSUPP cases.
Thanks,
Amir.
^ permalink raw reply
* Re: [PATCH v6 2/6] lsm: introduce new hooks for setting/getting inode fsxattr
From: Andrey Albershteyn @ 2025-07-02 8:47 UTC (permalink / raw)
To: Darrick J. Wong
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: <20250701181813.GN10009@frogsfrogsfrogs>
On 2025-07-01 11:18:13, Darrick J. Wong wrote:
> 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?
looks like
> If so, will an out of date security policy
> on a 6.17 kernel now fail to check the new file_[gs]etattr syscalls?
Yeah, probably, not sure if policies can have 'don't allow unknown'
but this is probably will need to be updated in the policy
>
> 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
> >
> >
>
--
- Andrey
^ permalink raw reply
* Re: [PATCH v6 6/6] fs: introduce file_getattr and file_setattr syscalls
From: Amir Goldstein @ 2025-07-02 9:13 UTC (permalink / raw)
To: Christian Brauner
Cc: Andrey Albershteyn, 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: <20250701-bauzaun-riskieren-595464ef81c4@brauner>
> > +/*
> > + * 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. :)
Adding __spare and not verifying that it is zeroed gets us to the
point that we are not able to replace __spare with a real field later.
I suggest to resolve this hole as Darrick and Pali suggested by making it
__u64 fsx_xflags
w.r.t Darrick's comment, I kind of like it that the name for the UAPI
struct (fsxattr)
differs from the name of the kernel internal representation (fileattr), but
I agree that fsx_fileattr does not give a good hint on what it is.
I think that renaming struct fsx_fileattr to struct fsxattr64 along
with changing the
width of fsx_xflags will help reduce the confusion of users.
What do you guys think?
Thanks,
Amir.
^ permalink raw reply
* Re: [PATCH v6 5/6] fs: prepare for extending file_get/setattr()
From: Amir Goldstein @ 2025-07-02 9:48 UTC (permalink / raw)
To: Pali Rohár
Cc: Darrick J. Wong, 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: <CAOQ4uxjZWGz2bqen4F+fkQqZYQjKyufFVky4tOTnwng4D5G4nQ@mail.gmail.com>
On Wed, Jul 2, 2025 at 9:03 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Jul 1, 2025 at 9:54 PM Pali Rohár <pali@kernel.org> wrote:
> >
> > On Tuesday 01 July 2025 12:40:02 Darrick J. Wong wrote:
> > > On Tue, Jul 01, 2025 at 09:27:38PM +0200, Amir Goldstein wrote:
> > > > On Tue, Jul 1, 2025 at 8:31 PM Darrick J. Wong <djwong@kernel.org> wrote:
> > > > >
> > > > > On Mon, Jun 30, 2025 at 06:20:15PM +0200, Andrey Albershteyn wrote:
> > > > > > From: Amir Goldstein <amir73il@gmail.com>
> > > > > >
> > > > > > We intend to add support for more xflags to selective filesystems and
> > > > > > We cannot rely on copy_struct_from_user() to detect this extension.
> > > > > >
> > > > > > In preparation of extending the API, do not allow setting xflags unknown
> > > > > > by this kernel version.
> > > > > >
> > > > > > Also do not pass the read-only flags and read-only field fsx_nextents to
> > > > > > filesystem.
> > > > > >
> > > > > > These changes should not affect existing chattr programs that use the
> > > > > > ioctl to get fsxattr before setting the new values.
> > > > > >
> > > > > > Link: https://lore.kernel.org/linux-fsdevel/20250216164029.20673-4-pali@kernel.org/
> > > > > > Cc: Pali Rohár <pali@kernel.org>
> > > > > > Cc: Andrey Albershteyn <aalbersh@redhat.com>
> > > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > > > > Signed-off-by: Andrey Albershteyn <aalbersh@kernel.org>
> > > > > > ---
> > > > > > fs/file_attr.c | 8 +++++++-
> > > > > > include/linux/fileattr.h | 20 ++++++++++++++++++++
> > > > > > 2 files changed, 27 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/fs/file_attr.c b/fs/file_attr.c
> > > > > > index 4e85fa00c092..62f08872d4ad 100644
> > > > > > --- a/fs/file_attr.c
> > > > > > +++ b/fs/file_attr.c
> > > > > > @@ -99,9 +99,10 @@ EXPORT_SYMBOL(vfs_fileattr_get);
> > > > > > int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa)
> > > > > > {
> > > > > > struct fsxattr xfa;
> > > > > > + __u32 mask = FS_XFLAGS_MASK;
> > > > > >
> > > > > > memset(&xfa, 0, sizeof(xfa));
> > > > > > - xfa.fsx_xflags = fa->fsx_xflags;
> > > > > > + xfa.fsx_xflags = fa->fsx_xflags & mask;
> > > > >
> > > > > I wonder, should it be an error if a filesystem sets an fsx_xflags bit
> > > > > outside of FS_XFLAGS_MASK? I guess that's one way to prevent
> > > > > filesystems from overriding the VFS bits. ;)
> > > >
> > > > I think Pali has a plan on how to ensure that later
> > > > when the mask is provided via the API.
> > > >
> > > > >
> > > > > Though couldn't that be:
> > > > >
> > > > > xfa.fsx_xflags = fa->fsx_xflags & FS_XFLAGS_MASK;
> > > > >
> > > > > instead? And same below?
> > > > >
> > > >
> > > > Indeed. There is a reason for the var, because the next series
> > > > by Pali will use a user provided mask, which defaults to FS_XFLAGS_MASK,
> > > > so I left it this way.
> > > >
> > > > I don't see a problem with it keeping as is, but if it bothers you
> > > > I guess we can re-add the var later.
> > >
> > > Nah, it doesn't bother me that much.
> > >
> > > > > > xfa.fsx_extsize = fa->fsx_extsize;
> > > > > > xfa.fsx_nextents = fa->fsx_nextents;
> > > > > > xfa.fsx_projid = fa->fsx_projid;
> > > > > > @@ -118,11 +119,16 @@ static int copy_fsxattr_from_user(struct fileattr *fa,
> > > > > > struct fsxattr __user *ufa)
> > > > > > {
> > > > > > struct fsxattr xfa;
> > > > > > + __u32 mask = FS_XFLAGS_MASK;
> > > > > >
> > > > > > if (copy_from_user(&xfa, ufa, sizeof(xfa)))
> > > > > > return -EFAULT;
> > > > > >
> > > > > > + if (xfa.fsx_xflags & ~mask)
> > > > > > + return -EINVAL;
> > > > >
> > > > > I wonder if you want EOPNOTSUPP here? We don't know how to support
> > > > > unknown xflags. OTOH if you all have beaten this to death while I was
> > > > > out then don't start another round just for me. :P
> > > >
> > > > We have beaten this API almost to death for sure ;)
> > > > I don't remember if we discussed this specific aspect,
> > > > but I am personally in favor of
> > > > EOPNOTSUPP := the fs does not support the set/get operation
> > > > EINVAL := some flags provided as value is invalid
> > > >
> > > > For example, if the get API provides you with a mask of the
> > > > valid flags that you can set, if you try to set flags outside of
> > > > that mask you get EINVAL.
> > > >
> > > > That's my interpretation, but I agree that EOPNOTSUPP can also
> > > > make sense in this situation.
> > >
> > > <nod> I think I'd rather EOPNOTSUPP for "bits are set that the kernel
> > > doesn't recognize" and EINVAL (or maybe something else like
> > > EPROTONOSUPPORT) for "fs driver will not let you change this bit".
> > > At least for the syscall interface; we probably have to flatten that to
> > > EOPNOTSUPP for both legacy ioctls.
>
> Given the precedents of returning EOPNOTSUPP in xfs_fileattr_set()
> and ext4_ioctl_setflags() for flags that cannot be set, I agree.
>
Wait, I misparsed what you wrote, so I think I "agreed" only to the
first part of your suggestion.
My claim is that unlike the xfs_has_v3inodes() check in
xfs_ioctl_setattr_xflags(),
ext4/f2fs etc return EOPNOTSUPP for various flags depending on supported fs
features (e.g. casefold,dax,encryption), so I think it will be hard to
impose a strict rule
where "fs does not support the feature" returns EINVAL in the syscalls API.
Therefore, I propose to change the code in this patch to
return EOPNOTSUPP for flags that kernel does not support
and with coming changes from Pali, it will also return the same
EOPNOTSUPP for flags that the fs instance does not support.
Christian,
Can you please amend the return value in the following chunk:
@@ -119,11 +120,16 @@ static int copy_fsxattr_from_user(struct fileattr *fa,
struct fsxattr __user *ufa)
{
struct fsxattr xfa;
+ __u32 mask = FS_XFLAGS_MASK;
if (copy_from_user(&xfa, ufa, sizeof(xfa)))
return -EFAULT;
+ if (xfa.fsx_xflags & ~mask)
+ return -EOPNOTSUPP;
+
Thanks,
Amir.
^ permalink raw reply
* [PATCH v18 0/8] fork: Support shadow stacks in clone3()
From: Mark Brown @ 2025-07-02 10:39 UTC (permalink / raw)
To: Rick P. Edgecombe, Deepak Gupta, Szabolcs Nagy, H.J. Lu,
Florian Weimer, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Valentin Schneider, Christian Brauner, Shuah Khan
Cc: linux-kernel, Catalin Marinas, Will Deacon, jannh, bsegall,
Andrew Morton, Yury Khrustalev, Wilco Dijkstra, linux-kselftest,
linux-api, Mark Brown, Kees Cook, Kees Cook, Shuah Khan
The kernel has recently added support for shadow stacks, currently
x86 only using their CET feature but both arm64 and RISC-V have
equivalent features (GCS and Zicfiss respectively), I am actively
working on GCS[1]. With shadow stacks the hardware maintains an
additional stack containing only the return addresses for branch
instructions which is not generally writeable by userspace and ensures
that any returns are to the recorded addresses. This provides some
protection against ROP attacks and making it easier to collect call
stacks. These shadow stacks are allocated in the address space of the
userspace process.
Our API for shadow stacks does not currently offer userspace any
flexiblity for managing the allocation of shadow stacks for newly
created threads, instead the kernel allocates a new shadow stack with
the same size as the normal stack whenever a thread is created with the
feature enabled. The stacks allocated in this way are freed by the
kernel when the thread exits or shadow stacks are disabled for the
thread. This lack of flexibility and control isn't ideal, in the vast
majority of cases the shadow stack will be over allocated and the
implicit allocation and deallocation is not consistent with other
interfaces. As far as I can tell the interface is done in this manner
mainly because the shadow stack patches were in development since before
clone3() was implemented.
Since clone3() is readily extensible let's add support for specifying a
shadow stack when creating a new thread or process, keeping the current
implicit allocation behaviour if one is not specified either with
clone3() or through the use of clone(). The user must provide a shadow
stack pointer, this must point to memory mapped for use as a shadow
stackby map_shadow_stack() with an architecture specified shadow stack
token at the top of the stack.
Yuri Khrustalev has raised questions from the libc side regarding
discoverability of extended clone3() structure sizes[2], this seems like
a general issue with clone3(). There was a suggestion to add a hwcap on
arm64 which isn't ideal but is doable there, though architecture
specific mechanisms would also be needed for x86 (and RISC-V if it's
support gets merged before this does). The idea has, however, had
strong pushback from the architecture maintainers and it is possible to
detect support for this in clone3() by attempting a call with a
misaligned shadow stack pointer specified so no hwcap has been added.
[1] https://lore.kernel.org/linux-arm-kernel/20241001-arm64-gcs-v13-0-222b78d87eee@kernel.org/T/#mc58f97f27461749ccf400ebabf6f9f937116a86b
[2] https://lore.kernel.org/r/aCs65ccRQtJBnZ_5@arm.com
Signed-off-by: Mark Brown <broonie@kernel.org>
---
Changes in v18:
- Rebase onto v6.16-rc3.
- Thanks to pointers from Yuri Khrustalev this version has been tested
on x86 so I have removed the RFT tag.
- Clarify clone3_shadow_stack_valid() comment about the Kconfig check.
- Remove redundant GCSB DSYNCs in arm64 code.
- Fix token validation on x86.
- Link to v17: https://lore.kernel.org/r/20250609-clone3-shadow-stack-v17-0-8840ed97ff6f@kernel.org
Changes in v17:
- Rebase onto v6.16-rc1.
- Link to v16: https://lore.kernel.org/r/20250416-clone3-shadow-stack-v16-0-2ffc9ca3917b@kernel.org
Changes in v16:
- Rebase onto v6.15-rc2.
- Roll in fixes from x86 testing from Rick Edgecombe.
- Rework so that the argument is shadow_stack_token.
- Link to v15: https://lore.kernel.org/r/20250408-clone3-shadow-stack-v15-0-3fa245c6e3be@kernel.org
Changes in v15:
- Rebase onto v6.15-rc1.
- Link to v14: https://lore.kernel.org/r/20250206-clone3-shadow-stack-v14-0-805b53af73b9@kernel.org
Changes in v14:
- Rebase onto v6.14-rc1.
- Link to v13: https://lore.kernel.org/r/20241203-clone3-shadow-stack-v13-0-93b89a81a5ed@kernel.org
Changes in v13:
- Rebase onto v6.13-rc1.
- Link to v12: https://lore.kernel.org/r/20241031-clone3-shadow-stack-v12-0-7183eb8bee17@kernel.org
Changes in v12:
- Add the regular prctl() to the userspace API document since arm64
support is queued in -next.
- Link to v11: https://lore.kernel.org/r/20241005-clone3-shadow-stack-v11-0-2a6a2bd6d651@kernel.org
Changes in v11:
- Rebase onto arm64 for-next/gcs, which is based on v6.12-rc1, and
integrate arm64 support.
- Rework the interface to specify a shadow stack pointer rather than a
base and size like we do for the regular stack.
- Link to v10: https://lore.kernel.org/r/20240821-clone3-shadow-stack-v10-0-06e8797b9445@kernel.org
Changes in v10:
- Integrate fixes & improvements for the x86 implementation from Rick
Edgecombe.
- Require that the shadow stack be VM_WRITE.
- Require that the shadow stack base and size be sizeof(void *) aligned.
- Clean up trailing newline.
- Link to v9: https://lore.kernel.org/r/20240819-clone3-shadow-stack-v9-0-962d74f99464@kernel.org
Changes in v9:
- Pull token validation earlier and report problems with an error return
to parent rather than signal delivery to the child.
- Verify that the top of the supplied shadow stack is VM_SHADOW_STACK.
- Rework token validation to only do the page mapping once.
- Drop no longer needed support for testing for signals in selftest.
- Fix typo in comments.
- Link to v8: https://lore.kernel.org/r/20240808-clone3-shadow-stack-v8-0-0acf37caf14c@kernel.org
Changes in v8:
- Fix token verification with user specified shadow stack.
- Don't track user managed shadow stacks for child processes.
- Link to v7: https://lore.kernel.org/r/20240731-clone3-shadow-stack-v7-0-a9532eebfb1d@kernel.org
Changes in v7:
- Rebase onto v6.11-rc1.
- Typo fixes.
- Link to v6: https://lore.kernel.org/r/20240623-clone3-shadow-stack-v6-0-9ee7783b1fb9@kernel.org
Changes in v6:
- Rebase onto v6.10-rc3.
- Ensure we don't try to free the parent shadow stack in error paths of
x86 arch code.
- Spelling fixes in userspace API document.
- Additional cleanups and improvements to the clone3() tests to support
the shadow stack tests.
- Link to v5: https://lore.kernel.org/r/20240203-clone3-shadow-stack-v5-0-322c69598e4b@kernel.org
Changes in v5:
- Rebase onto v6.8-rc2.
- Rework ABI to have the user allocate the shadow stack memory with
map_shadow_stack() and a token.
- Force inlining of the x86 shadow stack enablement.
- Move shadow stack enablement out into a shared header for reuse by
other tests.
- Link to v4: https://lore.kernel.org/r/20231128-clone3-shadow-stack-v4-0-8b28ffe4f676@kernel.org
Changes in v4:
- Formatting changes.
- Use a define for minimum shadow stack size and move some basic
validation to fork.c.
- Link to v3: https://lore.kernel.org/r/20231120-clone3-shadow-stack-v3-0-a7b8ed3e2acc@kernel.org
Changes in v3:
- Rebase onto v6.7-rc2.
- Remove stale shadow_stack in internal kargs.
- If a shadow stack is specified unconditionally use it regardless of
CLONE_ parameters.
- Force enable shadow stacks in the selftest.
- Update changelogs for RISC-V feature rename.
- Link to v2: https://lore.kernel.org/r/20231114-clone3-shadow-stack-v2-0-b613f8681155@kernel.org
Changes in v2:
- Rebase onto v6.7-rc1.
- Remove ability to provide preallocated shadow stack, just specify the
desired size.
- Link to v1: https://lore.kernel.org/r/20231023-clone3-shadow-stack-v1-0-d867d0b5d4d0@kernel.org
---
Mark Brown (8):
arm64/gcs: Return a success value from gcs_alloc_thread_stack()
Documentation: userspace-api: Add shadow stack API documentation
selftests: Provide helper header for shadow stack testing
fork: Add shadow stack support to clone3()
selftests/clone3: Remove redundant flushes of output streams
selftests/clone3: Factor more of main loop into test_clone3()
selftests/clone3: Allow tests to flag if -E2BIG is a valid error code
selftests/clone3: Test shadow stack support
Documentation/userspace-api/index.rst | 1 +
Documentation/userspace-api/shadow_stack.rst | 44 +++++
arch/arm64/include/asm/gcs.h | 8 +-
arch/arm64/kernel/process.c | 8 +-
arch/arm64/mm/gcs.c | 55 +++++-
arch/x86/include/asm/shstk.h | 11 +-
arch/x86/kernel/process.c | 2 +-
arch/x86/kernel/shstk.c | 53 ++++-
include/asm-generic/cacheflush.h | 11 ++
include/linux/sched/task.h | 17 ++
include/uapi/linux/sched.h | 9 +-
kernel/fork.c | 93 +++++++--
tools/testing/selftests/clone3/clone3.c | 226 ++++++++++++++++++----
tools/testing/selftests/clone3/clone3_selftests.h | 65 ++++++-
tools/testing/selftests/ksft_shstk.h | 98 ++++++++++
15 files changed, 620 insertions(+), 81 deletions(-)
---
base-commit: 86731a2a651e58953fc949573895f2fa6d456841
change-id: 20231019-clone3-shadow-stack-15d40d2bf536
Best regards,
--
Mark Brown <broonie@kernel.org>
^ permalink raw reply
* [PATCH v18 1/8] arm64/gcs: Return a success value from gcs_alloc_thread_stack()
From: Mark Brown @ 2025-07-02 10:39 UTC (permalink / raw)
To: Rick P. Edgecombe, Deepak Gupta, Szabolcs Nagy, H.J. Lu,
Florian Weimer, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Valentin Schneider, Christian Brauner, Shuah Khan
Cc: linux-kernel, Catalin Marinas, Will Deacon, jannh, bsegall,
Andrew Morton, Yury Khrustalev, Wilco Dijkstra, linux-kselftest,
linux-api, Mark Brown, Kees Cook
In-Reply-To: <20250702-clone3-shadow-stack-v18-0-7965d2b694db@kernel.org>
Currently as a result of templating from x86 code gcs_alloc_thread_stack()
returns a pointer as an unsigned int however on arm64 we don't actually use
this pointer value as anything other than a pass/fail flag. Simplify the
interface to just return an int with 0 on success and a negative error code
on failure.
Acked-by: Deepak Gupta <debug@rivosinc.com>
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
arch/arm64/include/asm/gcs.h | 8 ++++----
arch/arm64/kernel/process.c | 8 ++++----
arch/arm64/mm/gcs.c | 8 ++++----
3 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/arch/arm64/include/asm/gcs.h b/arch/arm64/include/asm/gcs.h
index f50660603ecf..d8923b5f03b7 100644
--- a/arch/arm64/include/asm/gcs.h
+++ b/arch/arm64/include/asm/gcs.h
@@ -64,8 +64,8 @@ static inline bool task_gcs_el0_enabled(struct task_struct *task)
void gcs_set_el0_mode(struct task_struct *task);
void gcs_free(struct task_struct *task);
void gcs_preserve_current_state(void);
-unsigned long gcs_alloc_thread_stack(struct task_struct *tsk,
- const struct kernel_clone_args *args);
+int gcs_alloc_thread_stack(struct task_struct *tsk,
+ const struct kernel_clone_args *args);
static inline int gcs_check_locked(struct task_struct *task,
unsigned long new_val)
@@ -91,8 +91,8 @@ static inline bool task_gcs_el0_enabled(struct task_struct *task)
static inline void gcs_set_el0_mode(struct task_struct *task) { }
static inline void gcs_free(struct task_struct *task) { }
static inline void gcs_preserve_current_state(void) { }
-static inline unsigned long gcs_alloc_thread_stack(struct task_struct *tsk,
- const struct kernel_clone_args *args)
+static inline int gcs_alloc_thread_stack(struct task_struct *tsk,
+ const struct kernel_clone_args *args)
{
return -ENOTSUPP;
}
diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
index 5954cec19660..630fbbf95019 100644
--- a/arch/arm64/kernel/process.c
+++ b/arch/arm64/kernel/process.c
@@ -299,7 +299,7 @@ static void flush_gcs(void)
static int copy_thread_gcs(struct task_struct *p,
const struct kernel_clone_args *args)
{
- unsigned long gcs;
+ int ret;
if (!system_supports_gcs())
return 0;
@@ -307,9 +307,9 @@ static int copy_thread_gcs(struct task_struct *p,
p->thread.gcs_base = 0;
p->thread.gcs_size = 0;
- gcs = gcs_alloc_thread_stack(p, args);
- if (IS_ERR_VALUE(gcs))
- return PTR_ERR((void *)gcs);
+ ret = gcs_alloc_thread_stack(p, args);
+ if (ret != 0)
+ return ret;
p->thread.gcs_el0_mode = current->thread.gcs_el0_mode;
p->thread.gcs_el0_locked = current->thread.gcs_el0_locked;
diff --git a/arch/arm64/mm/gcs.c b/arch/arm64/mm/gcs.c
index 5c46ec527b1c..1f633a482558 100644
--- a/arch/arm64/mm/gcs.c
+++ b/arch/arm64/mm/gcs.c
@@ -38,8 +38,8 @@ static unsigned long gcs_size(unsigned long size)
return max(PAGE_SIZE, size);
}
-unsigned long gcs_alloc_thread_stack(struct task_struct *tsk,
- const struct kernel_clone_args *args)
+int gcs_alloc_thread_stack(struct task_struct *tsk,
+ const struct kernel_clone_args *args)
{
unsigned long addr, size;
@@ -59,13 +59,13 @@ unsigned long gcs_alloc_thread_stack(struct task_struct *tsk,
size = gcs_size(size);
addr = alloc_gcs(0, size);
if (IS_ERR_VALUE(addr))
- return addr;
+ return PTR_ERR((void *)addr);
tsk->thread.gcs_base = addr;
tsk->thread.gcs_size = size;
tsk->thread.gcspr_el0 = addr + size - sizeof(u64);
- return addr;
+ return 0;
}
SYSCALL_DEFINE3(map_shadow_stack, unsigned long, addr, unsigned long, size, unsigned int, flags)
--
2.39.5
^ permalink raw reply related
* [PATCH v18 2/8] Documentation: userspace-api: Add shadow stack API documentation
From: Mark Brown @ 2025-07-02 10:39 UTC (permalink / raw)
To: Rick P. Edgecombe, Deepak Gupta, Szabolcs Nagy, H.J. Lu,
Florian Weimer, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Valentin Schneider, Christian Brauner, Shuah Khan
Cc: linux-kernel, Catalin Marinas, Will Deacon, jannh, bsegall,
Andrew Morton, Yury Khrustalev, Wilco Dijkstra, linux-kselftest,
linux-api, Mark Brown, Kees Cook, Kees Cook, Shuah Khan
In-Reply-To: <20250702-clone3-shadow-stack-v18-0-7965d2b694db@kernel.org>
There are a number of architectures with shadow stack features which we are
presenting to userspace with as consistent an API as we can (though there
are some architecture specifics). Especially given that there are some
important considerations for userspace code interacting directly with the
feature let's provide some documentation covering the common aspects.
Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Reviewed-by: Kees Cook <kees@kernel.org>
Tested-by: Kees Cook <kees@kernel.org>
Acked-by: Shuah Khan <skhan@linuxfoundation.org>
Acked-by: Yury Khrustalev <yury.khrustalev@arm.com>
Reviewed-by: Deepak Gupta <debug@rivosinc.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
Documentation/userspace-api/index.rst | 1 +
Documentation/userspace-api/shadow_stack.rst | 44 ++++++++++++++++++++++++++++
2 files changed, 45 insertions(+)
diff --git a/Documentation/userspace-api/index.rst b/Documentation/userspace-api/index.rst
index b8c73be4fb11..0167e59b541e 100644
--- a/Documentation/userspace-api/index.rst
+++ b/Documentation/userspace-api/index.rst
@@ -62,6 +62,7 @@ Everything else
ELF
netlink/index
+ shadow_stack
sysfs-platform_profile
vduse
futex2
diff --git a/Documentation/userspace-api/shadow_stack.rst b/Documentation/userspace-api/shadow_stack.rst
new file mode 100644
index 000000000000..65c665496624
--- /dev/null
+++ b/Documentation/userspace-api/shadow_stack.rst
@@ -0,0 +1,44 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=============
+Shadow Stacks
+=============
+
+Introduction
+============
+
+Several architectures have features which provide backward edge
+control flow protection through a hardware maintained stack, only
+writeable by userspace through very limited operations. This feature
+is referred to as shadow stacks on Linux, on x86 it is part of Intel
+Control Enforcement Technology (CET), on arm64 it is Guarded Control
+Stacks feature (FEAT_GCS) and for RISC-V it is the Zicfiss extension.
+It is expected that this feature will normally be managed by the
+system dynamic linker and libc in ways broadly transparent to
+application code, this document covers interfaces and considerations.
+
+
+Enabling
+========
+
+Shadow stacks default to disabled when a userspace process is
+executed, they can be enabled for the current thread with a syscall:
+
+ - For x86 the ARCH_SHSTK_ENABLE arch_prctl()
+ - For other architectures the PR_SET_SHADOW_STACK_ENABLE prctl()
+
+It is expected that this will normally be done by the dynamic linker.
+Any new threads created by a thread with shadow stacks enabled will
+themselves have shadow stacks enabled.
+
+
+Enablement considerations
+=========================
+
+- Returning from the function that enables shadow stacks without first
+ disabling them will cause a shadow stack exception. This includes
+ any syscall wrapper or other library functions, the syscall will need
+ to be inlined.
+- A lock feature allows userspace to prevent disabling of shadow stacks.
+- Those that change the stack context like longjmp() or use of ucontext
+ changes on signal return will need support from libc.
--
2.39.5
^ permalink raw reply related
* [PATCH v18 3/8] selftests: Provide helper header for shadow stack testing
From: Mark Brown @ 2025-07-02 10:39 UTC (permalink / raw)
To: Rick P. Edgecombe, Deepak Gupta, Szabolcs Nagy, H.J. Lu,
Florian Weimer, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Valentin Schneider, Christian Brauner, Shuah Khan
Cc: linux-kernel, Catalin Marinas, Will Deacon, jannh, bsegall,
Andrew Morton, Yury Khrustalev, Wilco Dijkstra, linux-kselftest,
linux-api, Mark Brown, Kees Cook, Kees Cook, Shuah Khan
In-Reply-To: <20250702-clone3-shadow-stack-v18-0-7965d2b694db@kernel.org>
While almost all users of shadow stacks should be relying on the dynamic
linker and libc to enable the feature there are several low level test
programs where it is useful to enable without any libc support, allowing
testing without full system enablement. This low level testing is helpful
during bringup of the support itself, and also in enabling coverage by
automated testing without needing all system components in the target root
filesystems to have enablement.
Provide a header with helpers for this purpose, intended for use only by
test programs directly exercising shadow stack interfaces.
Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Reviewed-by: Kees Cook <kees@kernel.org>
Tested-by: Kees Cook <kees@kernel.org>
Acked-by: Shuah Khan <skhan@linuxfoundation.org>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
tools/testing/selftests/ksft_shstk.h | 98 ++++++++++++++++++++++++++++++++++++
1 file changed, 98 insertions(+)
diff --git a/tools/testing/selftests/ksft_shstk.h b/tools/testing/selftests/ksft_shstk.h
new file mode 100644
index 000000000000..fecf91218ea5
--- /dev/null
+++ b/tools/testing/selftests/ksft_shstk.h
@@ -0,0 +1,98 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Helpers for shadow stack enablement, this is intended to only be
+ * used by low level test programs directly exercising interfaces for
+ * working with shadow stacks.
+ *
+ * Copyright (C) 2024 ARM Ltd.
+ */
+
+#ifndef __KSFT_SHSTK_H
+#define __KSFT_SHSTK_H
+
+#include <asm/mman.h>
+
+/* This is currently only defined for x86 */
+#ifndef SHADOW_STACK_SET_TOKEN
+#define SHADOW_STACK_SET_TOKEN (1ULL << 0)
+#endif
+
+static bool shadow_stack_enabled;
+
+#ifdef __x86_64__
+#define ARCH_SHSTK_ENABLE 0x5001
+#define ARCH_SHSTK_SHSTK (1ULL << 0)
+
+#define ARCH_PRCTL(arg1, arg2) \
+({ \
+ long _ret; \
+ register long _num asm("eax") = __NR_arch_prctl; \
+ register long _arg1 asm("rdi") = (long)(arg1); \
+ register long _arg2 asm("rsi") = (long)(arg2); \
+ \
+ asm volatile ( \
+ "syscall\n" \
+ : "=a"(_ret) \
+ : "r"(_arg1), "r"(_arg2), \
+ "0"(_num) \
+ : "rcx", "r11", "memory", "cc" \
+ ); \
+ _ret; \
+})
+
+#define ENABLE_SHADOW_STACK
+static __always_inline void enable_shadow_stack(void)
+{
+ int ret = ARCH_PRCTL(ARCH_SHSTK_ENABLE, ARCH_SHSTK_SHSTK);
+ if (ret == 0)
+ shadow_stack_enabled = true;
+}
+
+#endif
+
+#ifdef __aarch64__
+#define PR_SET_SHADOW_STACK_STATUS 75
+# define PR_SHADOW_STACK_ENABLE (1UL << 0)
+
+#define my_syscall2(num, arg1, arg2) \
+({ \
+ register long _num __asm__ ("x8") = (num); \
+ register long _arg1 __asm__ ("x0") = (long)(arg1); \
+ register long _arg2 __asm__ ("x1") = (long)(arg2); \
+ register long _arg3 __asm__ ("x2") = 0; \
+ register long _arg4 __asm__ ("x3") = 0; \
+ register long _arg5 __asm__ ("x4") = 0; \
+ \
+ __asm__ volatile ( \
+ "svc #0\n" \
+ : "=r"(_arg1) \
+ : "r"(_arg1), "r"(_arg2), \
+ "r"(_arg3), "r"(_arg4), \
+ "r"(_arg5), "r"(_num) \
+ : "memory", "cc" \
+ ); \
+ _arg1; \
+})
+
+#define ENABLE_SHADOW_STACK
+static __always_inline void enable_shadow_stack(void)
+{
+ int ret;
+
+ ret = my_syscall2(__NR_prctl, PR_SET_SHADOW_STACK_STATUS,
+ PR_SHADOW_STACK_ENABLE);
+ if (ret == 0)
+ shadow_stack_enabled = true;
+}
+
+#endif
+
+#ifndef __NR_map_shadow_stack
+#define __NR_map_shadow_stack 453
+#endif
+
+#ifndef ENABLE_SHADOW_STACK
+static inline void enable_shadow_stack(void) { }
+#endif
+
+#endif
--
2.39.5
^ permalink raw reply related
* [PATCH v18 4/8] fork: Add shadow stack support to clone3()
From: Mark Brown @ 2025-07-02 10:39 UTC (permalink / raw)
To: Rick P. Edgecombe, Deepak Gupta, Szabolcs Nagy, H.J. Lu,
Florian Weimer, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Valentin Schneider, Christian Brauner, Shuah Khan
Cc: linux-kernel, Catalin Marinas, Will Deacon, jannh, bsegall,
Andrew Morton, Yury Khrustalev, Wilco Dijkstra, linux-kselftest,
linux-api, Mark Brown, Kees Cook
In-Reply-To: <20250702-clone3-shadow-stack-v18-0-7965d2b694db@kernel.org>
Unlike with the normal stack there is no API for configuring the shadow
stack for a new thread, instead the kernel will dynamically allocate a
new shadow stack with the same size as the normal stack. This appears to
be due to the shadow stack series having been in development since
before the more extensible clone3() was added rather than anything more
deliberate.
Add a parameter to clone3() specifying a shadow stack pointer to use
for the new thread, this is inconsistent with the way we specify the
normal stack but during review concerns were expressed about having to
identify where the shadow stack pointer should be placed especially in
cases where the shadow stack has been previously active. If no shadow
stack is specified then the existing implicit allocation behaviour is
maintained.
If a shadow stack pointer is specified then it is required to have an
architecture defined token placed on the stack, this will be consumed by
the new task, the shadow stack is specified by pointing to this token. If
no valid token is present then this will be reported with -EINVAL. This
token prevents new threads being created pointing at the shadow stack of
an existing running thread. On architectures with support for userspace
pivoting of shadow stacks it is expected that the same format and placement
of tokens will be used, this is the case for arm64 and x86.
If the architecture does not support shadow stacks the shadow stack
pointer must be not be specified, architectures that do support the
feature are expected to enforce the same requirement on individual
systems that lack shadow stack support.
Update the existing arm64 and x86 implementations to pay attention to
the newly added arguments, in order to maintain compatibility we use the
existing behaviour if no shadow stack is specified. Since we are now
using more fields from the kernel_clone_args we pass that into the
shadow stack code rather than individual fields.
Portions of the x86 architecture code were written by Rick Edgecombe.
Acked-by: Yury Khrustalev <yury.khrustalev@arm.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
arch/arm64/mm/gcs.c | 47 +++++++++++++++++++-
arch/x86/include/asm/shstk.h | 11 +++--
arch/x86/kernel/process.c | 2 +-
arch/x86/kernel/shstk.c | 53 ++++++++++++++++++++---
include/asm-generic/cacheflush.h | 11 +++++
include/linux/sched/task.h | 17 ++++++++
include/uapi/linux/sched.h | 9 ++--
kernel/fork.c | 93 ++++++++++++++++++++++++++++++++++------
8 files changed, 217 insertions(+), 26 deletions(-)
diff --git a/arch/arm64/mm/gcs.c b/arch/arm64/mm/gcs.c
index 1f633a482558..884ae663ba96 100644
--- a/arch/arm64/mm/gcs.c
+++ b/arch/arm64/mm/gcs.c
@@ -43,8 +43,23 @@ int gcs_alloc_thread_stack(struct task_struct *tsk,
{
unsigned long addr, size;
- if (!system_supports_gcs())
+ if (!system_supports_gcs()) {
+ if (args->shadow_stack_token)
+ return -EINVAL;
+
return 0;
+ }
+
+ /*
+ * If the user specified a GCS then use it, otherwise fall
+ * back to a default allocation strategy. Validation is done
+ * in arch_shstk_validate_clone().
+ */
+ if (args->shadow_stack_token) {
+ tsk->thread.gcs_base = 0;
+ tsk->thread.gcs_size = 0;
+ return 0;
+ }
if (!task_gcs_el0_enabled(tsk))
return 0;
@@ -68,6 +83,36 @@ int gcs_alloc_thread_stack(struct task_struct *tsk,
return 0;
}
+static bool gcs_consume_token(struct vm_area_struct *vma, struct page *page,
+ unsigned long user_addr)
+{
+ u64 expected = GCS_CAP(user_addr);
+ u64 *token = page_address(page) + offset_in_page(user_addr);
+
+ if (!cmpxchg_to_user_page(vma, page, user_addr, token, expected, 0))
+ return false;
+ set_page_dirty_lock(page);
+
+ return true;
+}
+
+int arch_shstk_validate_clone(struct task_struct *tsk,
+ struct vm_area_struct *vma,
+ struct page *page,
+ struct kernel_clone_args *args)
+{
+ unsigned long gcspr_el0;
+ int ret = 0;
+
+ gcspr_el0 = args->shadow_stack_token;
+ if (!gcs_consume_token(vma, page, gcspr_el0))
+ return -EINVAL;
+
+ tsk->thread.gcspr_el0 = gcspr_el0 + sizeof(u64);
+
+ return ret;
+}
+
SYSCALL_DEFINE3(map_shadow_stack, unsigned long, addr, unsigned long, size, unsigned int, flags)
{
unsigned long alloc_size;
diff --git a/arch/x86/include/asm/shstk.h b/arch/x86/include/asm/shstk.h
index ba6f2fe43848..827e983430aa 100644
--- a/arch/x86/include/asm/shstk.h
+++ b/arch/x86/include/asm/shstk.h
@@ -6,6 +6,7 @@
#include <linux/types.h>
struct task_struct;
+struct kernel_clone_args;
struct ksignal;
#ifdef CONFIG_X86_USER_SHADOW_STACK
@@ -16,8 +17,8 @@ struct thread_shstk {
long shstk_prctl(struct task_struct *task, int option, unsigned long arg2);
void reset_thread_features(void);
-unsigned long shstk_alloc_thread_stack(struct task_struct *p, unsigned long clone_flags,
- unsigned long stack_size);
+unsigned long shstk_alloc_thread_stack(struct task_struct *p,
+ const struct kernel_clone_args *args);
void shstk_free(struct task_struct *p);
int setup_signal_shadow_stack(struct ksignal *ksig);
int restore_signal_shadow_stack(void);
@@ -28,8 +29,10 @@ static inline long shstk_prctl(struct task_struct *task, int option,
unsigned long arg2) { return -EINVAL; }
static inline void reset_thread_features(void) {}
static inline unsigned long shstk_alloc_thread_stack(struct task_struct *p,
- unsigned long clone_flags,
- unsigned long stack_size) { return 0; }
+ const struct kernel_clone_args *args)
+{
+ return 0;
+}
static inline void shstk_free(struct task_struct *p) {}
static inline int setup_signal_shadow_stack(struct ksignal *ksig) { return 0; }
static inline int restore_signal_shadow_stack(void) { return 0; }
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 704883c21f3a..56bf3394360a 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -209,7 +209,7 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args)
* is disabled, new_ssp will remain 0, and fpu_clone() will know not to
* update it.
*/
- new_ssp = shstk_alloc_thread_stack(p, clone_flags, args->stack_size);
+ new_ssp = shstk_alloc_thread_stack(p, args);
if (IS_ERR_VALUE(new_ssp))
return PTR_ERR((void *)new_ssp);
diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
index 2ddf23387c7e..88ca9eaebc96 100644
--- a/arch/x86/kernel/shstk.c
+++ b/arch/x86/kernel/shstk.c
@@ -191,18 +191,61 @@ void reset_thread_features(void)
current->thread.features_locked = 0;
}
-unsigned long shstk_alloc_thread_stack(struct task_struct *tsk, unsigned long clone_flags,
- unsigned long stack_size)
+int arch_shstk_validate_clone(struct task_struct *t,
+ struct vm_area_struct *vma,
+ struct page *page,
+ struct kernel_clone_args *args)
+{
+ /*
+ * SSP is aligned, so reserved bits and mode bit are a zero, just mark
+ * the token 64-bit.
+ */
+ void *maddr = page_address(page);
+ unsigned long token;
+ int offset;
+ u64 expected;
+
+ token = args->shadow_stack_token;
+ expected = (token + SS_FRAME_SIZE) | BIT(0);
+ offset = offset_in_page(token);
+
+ if (!cmpxchg_to_user_page(vma, page, token, (unsigned long *)(maddr + offset),
+ expected, 0))
+ return -EINVAL;
+ set_page_dirty_lock(page);
+
+ return 0;
+}
+
+unsigned long shstk_alloc_thread_stack(struct task_struct *tsk,
+ const struct kernel_clone_args *args)
{
struct thread_shstk *shstk = &tsk->thread.shstk;
+ unsigned long clone_flags = args->flags;
unsigned long addr, size;
/*
* If shadow stack is not enabled on the new thread, skip any
- * switch to a new shadow stack.
+ * implicit switch to a new shadow stack and reject attempts to
+ * explicitly specify one.
*/
- if (!features_enabled(ARCH_SHSTK_SHSTK))
+ if (!features_enabled(ARCH_SHSTK_SHSTK)) {
+ if (args->shadow_stack_token)
+ return (unsigned long)ERR_PTR(-EINVAL);
+
return 0;
+ }
+
+ /*
+ * If the user specified a shadow stack then use it, otherwise
+ * fall back to a default allocation strategy. Validation is
+ * done in arch_shstk_validate_clone().
+ */
+ if (args->shadow_stack_token) {
+ shstk->base = 0;
+ shstk->size = 0;
+ return args->shadow_stack_token + 8;
+ }
/*
* For CLONE_VFORK the child will share the parents shadow stack.
@@ -222,7 +265,7 @@ unsigned long shstk_alloc_thread_stack(struct task_struct *tsk, unsigned long cl
if (!(clone_flags & CLONE_VM))
return 0;
- size = adjust_shstk_size(stack_size);
+ size = adjust_shstk_size(args->stack_size);
addr = alloc_shstk(0, size, 0, false);
if (IS_ERR_VALUE(addr))
return addr;
diff --git a/include/asm-generic/cacheflush.h b/include/asm-generic/cacheflush.h
index 7ee8a179d103..96cc0c7a5c90 100644
--- a/include/asm-generic/cacheflush.h
+++ b/include/asm-generic/cacheflush.h
@@ -124,4 +124,15 @@ static inline void flush_cache_vunmap(unsigned long start, unsigned long end)
} while (0)
#endif
+#ifndef cmpxchg_to_user_page
+#define cmpxchg_to_user_page(vma, page, vaddr, ptr, old, new) \
+({ \
+ bool ret; \
+ \
+ ret = try_cmpxchg(ptr, &old, new); \
+ flush_icache_user_page(vma, page, vaddr, sizeof(*ptr)); \
+ ret; \
+})
+#endif
+
#endif /* _ASM_GENERIC_CACHEFLUSH_H */
diff --git a/include/linux/sched/task.h b/include/linux/sched/task.h
index ca1db4b92c32..c34f3cb68822 100644
--- a/include/linux/sched/task.h
+++ b/include/linux/sched/task.h
@@ -16,6 +16,7 @@ struct task_struct;
struct rusage;
union thread_union;
struct css_set;
+struct vm_area_struct;
/* All the bits taken by the old clone syscall. */
#define CLONE_LEGACY_FLAGS 0xffffffffULL
@@ -44,6 +45,7 @@ struct kernel_clone_args {
struct cgroup *cgrp;
struct css_set *cset;
unsigned int kill_seq;
+ unsigned long shadow_stack_token;
};
/*
@@ -237,4 +239,19 @@ static inline void task_unlock(struct task_struct *p)
DEFINE_GUARD(task_lock, struct task_struct *, task_lock(_T), task_unlock(_T))
+#ifdef CONFIG_ARCH_HAS_USER_SHADOW_STACK
+int arch_shstk_validate_clone(struct task_struct *p,
+ struct vm_area_struct *vma,
+ struct page *page,
+ struct kernel_clone_args *args);
+#else
+static inline int arch_shstk_validate_clone(struct task_struct *p,
+ struct vm_area_struct *vma,
+ struct page *page,
+ struct kernel_clone_args *args)
+{
+ return 0;
+}
+#endif
+
#endif /* _LINUX_SCHED_TASK_H */
diff --git a/include/uapi/linux/sched.h b/include/uapi/linux/sched.h
index 359a14cc76a4..9cf5c419e109 100644
--- a/include/uapi/linux/sched.h
+++ b/include/uapi/linux/sched.h
@@ -84,6 +84,7 @@
* kernel's limit of nested PID namespaces.
* @cgroup: If CLONE_INTO_CGROUP is specified set this to
* a file descriptor for the cgroup.
+ * @shadow_stack_token: Pointer to shadow stack token at top of stack.
*
* The structure is versioned by size and thus extensible.
* New struct members must go at the end of the struct and
@@ -101,12 +102,14 @@ struct clone_args {
__aligned_u64 set_tid;
__aligned_u64 set_tid_size;
__aligned_u64 cgroup;
+ __aligned_u64 shadow_stack_token;
};
#endif
-#define CLONE_ARGS_SIZE_VER0 64 /* sizeof first published struct */
-#define CLONE_ARGS_SIZE_VER1 80 /* sizeof second published struct */
-#define CLONE_ARGS_SIZE_VER2 88 /* sizeof third published struct */
+#define CLONE_ARGS_SIZE_VER0 64 /* sizeof first published struct */
+#define CLONE_ARGS_SIZE_VER1 80 /* sizeof second published struct */
+#define CLONE_ARGS_SIZE_VER2 88 /* sizeof third published struct */
+#define CLONE_ARGS_SIZE_VER3 96 /* sizeof fourth published struct */
/*
* Scheduling policies
diff --git a/kernel/fork.c b/kernel/fork.c
index 1ee8eb11f38b..728f9b037853 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1902,6 +1902,51 @@ static bool need_futex_hash_allocate_default(u64 clone_flags)
return true;
}
+static int shstk_validate_clone(struct task_struct *p,
+ struct kernel_clone_args *args)
+{
+ struct mm_struct *mm;
+ struct vm_area_struct *vma;
+ struct page *page;
+ unsigned long addr;
+ int ret;
+
+ if (!IS_ENABLED(CONFIG_ARCH_HAS_USER_SHADOW_STACK))
+ return 0;
+
+ if (!args->shadow_stack_token)
+ return 0;
+
+ mm = get_task_mm(p);
+ if (!mm)
+ return -EFAULT;
+
+ mmap_read_lock(mm);
+
+ addr = untagged_addr_remote(mm, args->shadow_stack_token);
+ page = get_user_page_vma_remote(mm, addr, FOLL_FORCE | FOLL_WRITE,
+ &vma);
+ if (IS_ERR(page)) {
+ ret = -EFAULT;
+ goto out;
+ }
+
+ if (!(vma->vm_flags & VM_SHADOW_STACK) ||
+ !(vma->vm_flags & VM_WRITE)) {
+ ret = -EFAULT;
+ goto out_page;
+ }
+
+ ret = arch_shstk_validate_clone(p, vma, page, args);
+
+out_page:
+ put_page(page);
+out:
+ mmap_read_unlock(mm);
+ mmput(mm);
+ return ret;
+}
+
/*
* This creates a new process as a copy of the old one,
* but does not actually start it yet.
@@ -2176,6 +2221,9 @@ __latent_entropy struct task_struct *copy_process(
if (retval)
goto bad_fork_cleanup_namespaces;
retval = copy_thread(p, args);
+ if (retval)
+ goto bad_fork_cleanup_io;
+ retval = shstk_validate_clone(p, args);
if (retval)
goto bad_fork_cleanup_io;
@@ -2757,7 +2805,9 @@ noinline static int copy_clone_args_from_user(struct kernel_clone_args *kargs,
CLONE_ARGS_SIZE_VER1);
BUILD_BUG_ON(offsetofend(struct clone_args, cgroup) !=
CLONE_ARGS_SIZE_VER2);
- BUILD_BUG_ON(sizeof(struct clone_args) != CLONE_ARGS_SIZE_VER2);
+ BUILD_BUG_ON(offsetofend(struct clone_args, shadow_stack_token) !=
+ CLONE_ARGS_SIZE_VER3);
+ BUILD_BUG_ON(sizeof(struct clone_args) != CLONE_ARGS_SIZE_VER3);
if (unlikely(usize > PAGE_SIZE))
return -E2BIG;
@@ -2790,16 +2840,17 @@ noinline static int copy_clone_args_from_user(struct kernel_clone_args *kargs,
return -EINVAL;
*kargs = (struct kernel_clone_args){
- .flags = args.flags,
- .pidfd = u64_to_user_ptr(args.pidfd),
- .child_tid = u64_to_user_ptr(args.child_tid),
- .parent_tid = u64_to_user_ptr(args.parent_tid),
- .exit_signal = args.exit_signal,
- .stack = args.stack,
- .stack_size = args.stack_size,
- .tls = args.tls,
- .set_tid_size = args.set_tid_size,
- .cgroup = args.cgroup,
+ .flags = args.flags,
+ .pidfd = u64_to_user_ptr(args.pidfd),
+ .child_tid = u64_to_user_ptr(args.child_tid),
+ .parent_tid = u64_to_user_ptr(args.parent_tid),
+ .exit_signal = args.exit_signal,
+ .stack = args.stack,
+ .stack_size = args.stack_size,
+ .tls = args.tls,
+ .set_tid_size = args.set_tid_size,
+ .cgroup = args.cgroup,
+ .shadow_stack_token = args.shadow_stack_token,
};
if (args.set_tid &&
@@ -2840,6 +2891,24 @@ static inline bool clone3_stack_valid(struct kernel_clone_args *kargs)
return true;
}
+/**
+ * clone3_shadow_stack_valid - check and prepare shadow stack
+ * @kargs: kernel clone args
+ *
+ * Verify that shadow stacks are only enabled if supported.
+ */
+static inline bool clone3_shadow_stack_valid(struct kernel_clone_args *kargs)
+{
+ if (!kargs->shadow_stack_token)
+ return true;
+
+ if (!IS_ALIGNED(kargs->shadow_stack_token, sizeof(void *)))
+ return false;
+
+ /* Fail if the kernel wasn't built with shadow stacks */
+ return IS_ENABLED(CONFIG_ARCH_HAS_USER_SHADOW_STACK);
+}
+
static bool clone3_args_valid(struct kernel_clone_args *kargs)
{
/* Verify that no unknown flags are passed along. */
@@ -2862,7 +2931,7 @@ static bool clone3_args_valid(struct kernel_clone_args *kargs)
kargs->exit_signal)
return false;
- if (!clone3_stack_valid(kargs))
+ if (!clone3_stack_valid(kargs) || !clone3_shadow_stack_valid(kargs))
return false;
return true;
--
2.39.5
^ permalink raw reply related
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