* Re: [RFC v2 00/22] Kernel API specification framework
From: Jake Edge @ 2025-07-01 2:43 UTC (permalink / raw)
To: Sasha Levin; +Cc: linux-kernel, linux-doc, linux-api, workflows, tools
In-Reply-To: <20250624180742.5795-1-sashal@kernel.org>
Hi Sasha,
On Tue, Jun 24 2025 14:07 -0400, Sasha Levin <sashal@kernel.org> wrote:
> Hey folks,
>
> This is a second attempt at a "Kernel API Specification" framework,
> addressing the feedback from the initial RFC and expanding the scope
> to include sysfs attribute specifications.
In light of your talk at OSS last week [1] (for non-subscribers [2]), I
am wondering if any of this code has been written by coding LLMs. It
seems like the kind of unpleasant boilerplate that they are said to be
good at generating, but also seems like an enormous blob of "code" to
review. What is the status of this specification in that regard?
thanks!
jake
[1] https://lwn.net/Articles/1026558/
[2] https://lwn.net/SubscriberLink/1026558/914fa4ec5964b0c5/
--
Jake Edge - LWN - jake@lwn.net - https://lwn.net
^ permalink raw reply
* Re: [RFC v2 01/22] kernel/api: introduce kernel API specification framework
From: Mauro Carvalho Chehab @ 2025-06-30 22:20 UTC (permalink / raw)
To: Jonathan Corbet
Cc: Sasha Levin, linux-kernel, linux-doc, linux-api, workflows, tools
In-Reply-To: <874ivxuht8.fsf@trenco.lwn.net>
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.
Also, IGT (a testing tool for DRM subsystem) used to have a macro
based documentation system. It got outdated with time, as people
ends forgetting to update the macros when changing the code.
Also, sometimes we want to add some rich text there, with graphs,
tables, ...
More important than that: people end not remembering to add such macros.
As kerneldoc markups are similar to Doxygen and normal C comments,
it is more likely that people will remember.
So, IMO the best would be to use kerneldoc syntax there, letting
Kerneldoc Sphinx extension handling it for docs, while having
tools to implement the other features you mentioned.
Thanks,
Mauro
^ permalink raw reply
* Re: [RFC v2 01/22] kernel/api: introduce kernel API specification framework
From: Jonathan Corbet @ 2025-06-30 19:53 UTC (permalink / raw)
To: Sasha Levin, linux-kernel
Cc: linux-doc, linux-api, workflows, tools, Sasha Levin
In-Reply-To: <20250624180742.5795-2-sashal@kernel.org>
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...? :)
Thanks,
jon
^ permalink raw reply
* Re: [PATCH v6 4/6] fs: make vfs_fileattr_[get|set] return -EOPNOSUPP
From: Pali Rohár @ 2025-06-30 18:05 UTC (permalink / raw)
To: Andrey Albershteyn
Cc: 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: <20250630-xattrat-syscall-v6-4-c4e3bc35227b@kernel.org>
nit: typo in commit subject and description: Missing T in EOPNO*T*SUPP.
But please do not resend whole patch series just because of this.
That is not needed.
On Monday 30 June 2025 18:20:14 Andrey Albershteyn wrote:
> Future patches will add new syscalls which use these functions. As
> this interface won't be used for ioctls only, the EOPNOSUPP is more
> appropriate return code.
>
> This patch converts return code from ENOIOCTLCMD to EOPNOSUPP for
> vfs_fileattr_get and vfs_fileattr_set. To save old behavior translate
> EOPNOSUPP back for current users - overlayfs, encryptfs and fs/ioctl.c.
>
> Signed-off-by: Andrey Albershteyn <aalbersh@kernel.org>
> ---
> 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
* [PATCH v6 6/6] fs: introduce file_getattr and file_setattr syscalls
From: Andrey Albershteyn @ 2025-06-30 16:20 UTC (permalink / raw)
To: Amir Goldstein, Arnd Bergmann, Casey Schaufler, Christian Brauner,
Jan Kara, Pali Rohár, Paul Moore
Cc: linux-api, linux-fsdevel, linux-kernel, linux-xfs, selinux,
Andrey Albershteyn, Andrey Albershteyn
In-Reply-To: <20250630-xattrat-syscall-v6-0-c4e3bc35227b@kernel.org>
From: Andrey Albershteyn <aalbersh@redhat.com>
Introduce file_getattr() and file_setattr() syscalls to manipulate inode
extended attributes. The syscalls takes pair of file descriptor and
pathname. Then it operates on inode opened accroding to openat()
semantics. The struct fsx_fileattr is passed to obtain/change extended
attributes.
This is an alternative to FS_IOC_FSSETXATTR ioctl with a difference
that file don't need to be open as we can reference it with a path
instead of fd. By having this we can manipulated inode extended
attributes not only on regular files but also on special ones. This
is not possible with FS_IOC_FSSETXATTR ioctl as with special files
we can not call ioctl() directly on the filesystem inode using fd.
This patch adds two new syscalls which allows userspace to get/set
extended inode attributes on special files by using parent directory
and a path - *at() like syscall.
CC: linux-api@vger.kernel.org
CC: linux-fsdevel@vger.kernel.org
CC: linux-xfs@vger.kernel.org
Signed-off-by: Andrey Albershteyn <aalbersh@kernel.org>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
arch/alpha/kernel/syscalls/syscall.tbl | 2 +
arch/arm/tools/syscall.tbl | 2 +
arch/arm64/tools/syscall_32.tbl | 2 +
arch/m68k/kernel/syscalls/syscall.tbl | 2 +
arch/microblaze/kernel/syscalls/syscall.tbl | 2 +
arch/mips/kernel/syscalls/syscall_n32.tbl | 2 +
arch/mips/kernel/syscalls/syscall_n64.tbl | 2 +
arch/mips/kernel/syscalls/syscall_o32.tbl | 2 +
arch/parisc/kernel/syscalls/syscall.tbl | 2 +
arch/powerpc/kernel/syscalls/syscall.tbl | 2 +
arch/s390/kernel/syscalls/syscall.tbl | 2 +
arch/sh/kernel/syscalls/syscall.tbl | 2 +
arch/sparc/kernel/syscalls/syscall.tbl | 2 +
arch/x86/entry/syscalls/syscall_32.tbl | 2 +
arch/x86/entry/syscalls/syscall_64.tbl | 2 +
arch/xtensa/kernel/syscalls/syscall.tbl | 2 +
fs/file_attr.c | 148 ++++++++++++++++++++++++++++
include/linux/syscalls.h | 6 ++
include/uapi/asm-generic/unistd.h | 8 +-
include/uapi/linux/fs.h | 18 ++++
scripts/syscall.tbl | 2 +
21 files changed, 213 insertions(+), 1 deletion(-)
diff --git a/arch/alpha/kernel/syscalls/syscall.tbl b/arch/alpha/kernel/syscalls/syscall.tbl
index 2dd6340de6b4..16dca28ebf17 100644
--- a/arch/alpha/kernel/syscalls/syscall.tbl
+++ b/arch/alpha/kernel/syscalls/syscall.tbl
@@ -507,3 +507,5 @@
575 common listxattrat sys_listxattrat
576 common removexattrat sys_removexattrat
577 common open_tree_attr sys_open_tree_attr
+578 common file_getattr sys_file_getattr
+579 common file_setattr sys_file_setattr
diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl
index 27c1d5ebcd91..b07e699aaa3c 100644
--- a/arch/arm/tools/syscall.tbl
+++ b/arch/arm/tools/syscall.tbl
@@ -482,3 +482,5 @@
465 common listxattrat sys_listxattrat
466 common removexattrat sys_removexattrat
467 common open_tree_attr sys_open_tree_attr
+468 common file_getattr sys_file_getattr
+469 common file_setattr sys_file_setattr
diff --git a/arch/arm64/tools/syscall_32.tbl b/arch/arm64/tools/syscall_32.tbl
index 0765b3a8d6d6..8d9088bc577d 100644
--- a/arch/arm64/tools/syscall_32.tbl
+++ b/arch/arm64/tools/syscall_32.tbl
@@ -479,3 +479,5 @@
465 common listxattrat sys_listxattrat
466 common removexattrat sys_removexattrat
467 common open_tree_attr sys_open_tree_attr
+468 common file_getattr sys_file_getattr
+469 common file_setattr sys_file_setattr
diff --git a/arch/m68k/kernel/syscalls/syscall.tbl b/arch/m68k/kernel/syscalls/syscall.tbl
index 9fe47112c586..f41d38dfbf13 100644
--- a/arch/m68k/kernel/syscalls/syscall.tbl
+++ b/arch/m68k/kernel/syscalls/syscall.tbl
@@ -467,3 +467,5 @@
465 common listxattrat sys_listxattrat
466 common removexattrat sys_removexattrat
467 common open_tree_attr sys_open_tree_attr
+468 common file_getattr sys_file_getattr
+469 common file_setattr sys_file_setattr
diff --git a/arch/microblaze/kernel/syscalls/syscall.tbl b/arch/microblaze/kernel/syscalls/syscall.tbl
index 7b6e97828e55..580af574fe73 100644
--- a/arch/microblaze/kernel/syscalls/syscall.tbl
+++ b/arch/microblaze/kernel/syscalls/syscall.tbl
@@ -473,3 +473,5 @@
465 common listxattrat sys_listxattrat
466 common removexattrat sys_removexattrat
467 common open_tree_attr sys_open_tree_attr
+468 common file_getattr sys_file_getattr
+469 common file_setattr sys_file_setattr
diff --git a/arch/mips/kernel/syscalls/syscall_n32.tbl b/arch/mips/kernel/syscalls/syscall_n32.tbl
index aa70e371bb54..d824ffe9a014 100644
--- a/arch/mips/kernel/syscalls/syscall_n32.tbl
+++ b/arch/mips/kernel/syscalls/syscall_n32.tbl
@@ -406,3 +406,5 @@
465 n32 listxattrat sys_listxattrat
466 n32 removexattrat sys_removexattrat
467 n32 open_tree_attr sys_open_tree_attr
+468 n32 file_getattr sys_file_getattr
+469 n32 file_setattr sys_file_setattr
diff --git a/arch/mips/kernel/syscalls/syscall_n64.tbl b/arch/mips/kernel/syscalls/syscall_n64.tbl
index 1e8c44c7b614..7a7049c2c307 100644
--- a/arch/mips/kernel/syscalls/syscall_n64.tbl
+++ b/arch/mips/kernel/syscalls/syscall_n64.tbl
@@ -382,3 +382,5 @@
465 n64 listxattrat sys_listxattrat
466 n64 removexattrat sys_removexattrat
467 n64 open_tree_attr sys_open_tree_attr
+468 n64 file_getattr sys_file_getattr
+469 n64 file_setattr sys_file_setattr
diff --git a/arch/mips/kernel/syscalls/syscall_o32.tbl b/arch/mips/kernel/syscalls/syscall_o32.tbl
index 114a5a1a6230..d330274f0601 100644
--- a/arch/mips/kernel/syscalls/syscall_o32.tbl
+++ b/arch/mips/kernel/syscalls/syscall_o32.tbl
@@ -455,3 +455,5 @@
465 o32 listxattrat sys_listxattrat
466 o32 removexattrat sys_removexattrat
467 o32 open_tree_attr sys_open_tree_attr
+468 o32 file_getattr sys_file_getattr
+469 o32 file_setattr sys_file_setattr
diff --git a/arch/parisc/kernel/syscalls/syscall.tbl b/arch/parisc/kernel/syscalls/syscall.tbl
index 94df3cb957e9..88a788a7b18d 100644
--- a/arch/parisc/kernel/syscalls/syscall.tbl
+++ b/arch/parisc/kernel/syscalls/syscall.tbl
@@ -466,3 +466,5 @@
465 common listxattrat sys_listxattrat
466 common removexattrat sys_removexattrat
467 common open_tree_attr sys_open_tree_attr
+468 common file_getattr sys_file_getattr
+469 common file_setattr sys_file_setattr
diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
index 9a084bdb8926..b453e80dfc00 100644
--- a/arch/powerpc/kernel/syscalls/syscall.tbl
+++ b/arch/powerpc/kernel/syscalls/syscall.tbl
@@ -558,3 +558,5 @@
465 common listxattrat sys_listxattrat
466 common removexattrat sys_removexattrat
467 common open_tree_attr sys_open_tree_attr
+468 common file_getattr sys_file_getattr
+469 common file_setattr sys_file_setattr
diff --git a/arch/s390/kernel/syscalls/syscall.tbl b/arch/s390/kernel/syscalls/syscall.tbl
index a4569b96ef06..8a6744d658db 100644
--- a/arch/s390/kernel/syscalls/syscall.tbl
+++ b/arch/s390/kernel/syscalls/syscall.tbl
@@ -470,3 +470,5 @@
465 common listxattrat sys_listxattrat sys_listxattrat
466 common removexattrat sys_removexattrat sys_removexattrat
467 common open_tree_attr sys_open_tree_attr sys_open_tree_attr
+468 common file_getattr sys_file_getattr sys_file_getattr
+469 common file_setattr sys_file_setattr sys_file_setattr
diff --git a/arch/sh/kernel/syscalls/syscall.tbl b/arch/sh/kernel/syscalls/syscall.tbl
index 52a7652fcff6..5e9c9eff5539 100644
--- a/arch/sh/kernel/syscalls/syscall.tbl
+++ b/arch/sh/kernel/syscalls/syscall.tbl
@@ -471,3 +471,5 @@
465 common listxattrat sys_listxattrat
466 common removexattrat sys_removexattrat
467 common open_tree_attr sys_open_tree_attr
+468 common file_getattr sys_file_getattr
+469 common file_setattr sys_file_setattr
diff --git a/arch/sparc/kernel/syscalls/syscall.tbl b/arch/sparc/kernel/syscalls/syscall.tbl
index 83e45eb6c095..ebb7d06d1044 100644
--- a/arch/sparc/kernel/syscalls/syscall.tbl
+++ b/arch/sparc/kernel/syscalls/syscall.tbl
@@ -513,3 +513,5 @@
465 common listxattrat sys_listxattrat
466 common removexattrat sys_removexattrat
467 common open_tree_attr sys_open_tree_attr
+468 common file_getattr sys_file_getattr
+469 common file_setattr sys_file_setattr
diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
index ac007ea00979..4877e16da69a 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -473,3 +473,5 @@
465 i386 listxattrat sys_listxattrat
466 i386 removexattrat sys_removexattrat
467 i386 open_tree_attr sys_open_tree_attr
+468 i386 file_getattr sys_file_getattr
+469 i386 file_setattr sys_file_setattr
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index cfb5ca41e30d..92cf0fe2291e 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -391,6 +391,8 @@
465 common listxattrat sys_listxattrat
466 common removexattrat sys_removexattrat
467 common open_tree_attr sys_open_tree_attr
+468 common file_getattr sys_file_getattr
+469 common file_setattr sys_file_setattr
#
# Due to a historical design error, certain syscalls are numbered differently
diff --git a/arch/xtensa/kernel/syscalls/syscall.tbl b/arch/xtensa/kernel/syscalls/syscall.tbl
index f657a77314f8..374e4cb788d8 100644
--- a/arch/xtensa/kernel/syscalls/syscall.tbl
+++ b/arch/xtensa/kernel/syscalls/syscall.tbl
@@ -438,3 +438,5 @@
465 common listxattrat sys_listxattrat
466 common removexattrat sys_removexattrat
467 common open_tree_attr sys_open_tree_attr
+468 common file_getattr sys_file_getattr
+469 common file_setattr sys_file_setattr
diff --git a/fs/file_attr.c b/fs/file_attr.c
index 62f08872d4ad..fda9d847eee5 100644
--- a/fs/file_attr.c
+++ b/fs/file_attr.c
@@ -3,6 +3,10 @@
#include <linux/security.h>
#include <linux/fscrypt.h>
#include <linux/fileattr.h>
+#include <linux/syscalls.h>
+#include <linux/namei.h>
+
+#include "internal.h"
/**
* fileattr_fill_xflags - initialize fileattr with xflags
@@ -89,6 +93,19 @@ int vfs_fileattr_get(struct dentry *dentry, struct fileattr *fa)
}
EXPORT_SYMBOL(vfs_fileattr_get);
+static void fileattr_to_fsx_fileattr(const struct fileattr *fa,
+ struct fsx_fileattr *fsx)
+{
+ __u32 mask = FS_XFLAGS_MASK;
+
+ memset(fsx, 0, sizeof(struct fsx_fileattr));
+ fsx->fsx_xflags = fa->fsx_xflags & mask;
+ fsx->fsx_extsize = fa->fsx_extsize;
+ fsx->fsx_nextents = fa->fsx_nextents;
+ fsx->fsx_projid = fa->fsx_projid;
+ fsx->fsx_cowextsize = fa->fsx_cowextsize;
+}
+
/**
* copy_fsxattr_to_user - copy fsxattr to userspace.
* @fa: fileattr pointer
@@ -115,6 +132,23 @@ int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa)
}
EXPORT_SYMBOL(copy_fsxattr_to_user);
+static int fsx_fileattr_to_fileattr(const struct fsx_fileattr *fsx,
+ struct fileattr *fa)
+{
+ __u32 mask = FS_XFLAGS_MASK;
+
+ if (fsx->fsx_xflags & ~mask)
+ return -EINVAL;
+
+ fileattr_fill_xflags(fa, fsx->fsx_xflags);
+ fa->fsx_xflags &= ~FS_XFLAG_RDONLY_MASK;
+ fa->fsx_extsize = fsx->fsx_extsize;
+ fa->fsx_projid = fsx->fsx_projid;
+ fa->fsx_cowextsize = fsx->fsx_cowextsize;
+
+ return 0;
+}
+
static int copy_fsxattr_from_user(struct fileattr *fa,
struct fsxattr __user *ufa)
{
@@ -343,3 +377,117 @@ int ioctl_fssetxattr(struct file *file, void __user *argp)
return err;
}
EXPORT_SYMBOL(ioctl_fssetxattr);
+
+SYSCALL_DEFINE5(file_getattr, int, dfd, const char __user *, filename,
+ struct fsx_fileattr __user *, ufsx, size_t, usize,
+ unsigned int, at_flags)
+{
+ struct fileattr fa;
+ struct path filepath __free(path_put) = {};
+ int error;
+ unsigned int lookup_flags = 0;
+ struct filename *name __free(putname) = NULL;
+ struct fsx_fileattr fsx;
+
+ BUILD_BUG_ON(sizeof(struct fsx_fileattr) < FSX_FILEATTR_SIZE_VER0);
+ BUILD_BUG_ON(sizeof(struct fsx_fileattr) != FSX_FILEATTR_SIZE_LATEST);
+
+ if ((at_flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
+ return -EINVAL;
+
+ if (!(at_flags & AT_SYMLINK_NOFOLLOW))
+ lookup_flags |= LOOKUP_FOLLOW;
+
+ if (usize > PAGE_SIZE)
+ return -E2BIG;
+
+ if (usize < FSX_FILEATTR_SIZE_VER0)
+ return -EINVAL;
+
+ name = getname_maybe_null(filename, at_flags);
+ if (IS_ERR(name))
+ return PTR_ERR(name);
+
+ if (!name && dfd >= 0) {
+ CLASS(fd, f)(dfd);
+
+ filepath = fd_file(f)->f_path;
+ path_get(&filepath);
+ } else {
+ error = filename_lookup(dfd, name, lookup_flags, &filepath,
+ NULL);
+ if (error)
+ return error;
+ }
+
+ error = vfs_fileattr_get(filepath.dentry, &fa);
+ if (error)
+ return error;
+
+ fileattr_to_fsx_fileattr(&fa, &fsx);
+ error = copy_struct_to_user(ufsx, usize, &fsx,
+ sizeof(struct fsx_fileattr), NULL);
+
+ return error;
+}
+
+SYSCALL_DEFINE5(file_setattr, int, dfd, const char __user *, filename,
+ struct fsx_fileattr __user *, ufsx, size_t, usize,
+ unsigned int, at_flags)
+{
+ struct fileattr fa;
+ struct path filepath __free(path_put) = {};
+ int error;
+ unsigned int lookup_flags = 0;
+ struct filename *name __free(putname) = NULL;
+ struct fsx_fileattr fsx;
+
+ BUILD_BUG_ON(sizeof(struct fsx_fileattr) < FSX_FILEATTR_SIZE_VER0);
+ BUILD_BUG_ON(sizeof(struct fsx_fileattr) != FSX_FILEATTR_SIZE_LATEST);
+
+ if ((at_flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
+ return -EINVAL;
+
+ if (!(at_flags & AT_SYMLINK_NOFOLLOW))
+ lookup_flags |= LOOKUP_FOLLOW;
+
+ if (usize > PAGE_SIZE)
+ return -E2BIG;
+
+ if (usize < FSX_FILEATTR_SIZE_VER0)
+ return -EINVAL;
+
+ error = copy_struct_from_user(&fsx, sizeof(struct fsx_fileattr), ufsx,
+ usize);
+ if (error)
+ return error;
+
+ error = fsx_fileattr_to_fileattr(&fsx, &fa);
+ if (error)
+ return error;
+
+ name = getname_maybe_null(filename, at_flags);
+ if (IS_ERR(name))
+ return PTR_ERR(name);
+
+ if (!name && dfd >= 0) {
+ CLASS(fd, f)(dfd);
+
+ filepath = fd_file(f)->f_path;
+ path_get(&filepath);
+ } else {
+ error = filename_lookup(dfd, name, lookup_flags, &filepath,
+ NULL);
+ if (error)
+ return error;
+ }
+
+ error = mnt_want_write(filepath.mnt);
+ if (!error) {
+ error = vfs_fileattr_set(mnt_idmap(filepath.mnt),
+ filepath.dentry, &fa);
+ mnt_drop_write(filepath.mnt);
+ }
+
+ return error;
+}
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index e5603cc91963..179acbe28fec 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -371,6 +371,12 @@ asmlinkage long sys_removexattrat(int dfd, const char __user *path,
asmlinkage long sys_lremovexattr(const char __user *path,
const char __user *name);
asmlinkage long sys_fremovexattr(int fd, const char __user *name);
+asmlinkage long sys_file_getattr(int dfd, const char __user *filename,
+ struct fsx_fileattr __user *ufsx, size_t usize,
+ unsigned int at_flags);
+asmlinkage long sys_file_setattr(int dfd, const char __user *filename,
+ struct fsx_fileattr __user *ufsx, size_t usize,
+ unsigned int at_flags);
asmlinkage long sys_getcwd(char __user *buf, unsigned long size);
asmlinkage long sys_eventfd2(unsigned int count, int flags);
asmlinkage long sys_epoll_create1(int flags);
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index 2892a45023af..04e0077fb4c9 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -852,8 +852,14 @@ __SYSCALL(__NR_removexattrat, sys_removexattrat)
#define __NR_open_tree_attr 467
__SYSCALL(__NR_open_tree_attr, sys_open_tree_attr)
+/* fs/inode.c */
+#define __NR_file_getattr 468
+__SYSCALL(__NR_file_getattr, sys_file_getattr)
+#define __NR_file_setattr 469
+__SYSCALL(__NR_file_setattr, sys_file_setattr)
+
#undef __NR_syscalls
-#define __NR_syscalls 468
+#define __NR_syscalls 470
/*
* 32 bit systems traditionally used different
diff --git a/include/uapi/linux/fs.h b/include/uapi/linux/fs.h
index 0098b0ce8ccb..0784f2033ba4 100644
--- a/include/uapi/linux/fs.h
+++ b/include/uapi/linux/fs.h
@@ -148,6 +148,24 @@ struct fsxattr {
unsigned char fsx_pad[8];
};
+/*
+ * Variable size structure for file_[sg]et_attr().
+ *
+ * Note. This is alternative to the structure 'struct fileattr'/'struct fsxattr'.
+ * As this structure is passed to/from userspace with its size, this can
+ * be versioned based on the size.
+ */
+struct fsx_fileattr {
+ __u32 fsx_xflags; /* xflags field value (get/set) */
+ __u32 fsx_extsize; /* extsize field value (get/set)*/
+ __u32 fsx_nextents; /* nextents field value (get) */
+ __u32 fsx_projid; /* project identifier (get/set) */
+ __u32 fsx_cowextsize; /* CoW extsize field value (get/set) */
+};
+
+#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 related
* [PATCH v6 5/6] fs: prepare for extending file_get/setattr()
From: Andrey Albershteyn @ 2025-06-30 16:20 UTC (permalink / raw)
To: Amir Goldstein, Arnd Bergmann, Casey Schaufler, Christian Brauner,
Jan Kara, Pali Rohár, Paul Moore
Cc: linux-api, linux-fsdevel, linux-kernel, linux-xfs, selinux,
Andrey Albershteyn, Andrey Albershteyn
In-Reply-To: <20250630-xattrat-syscall-v6-0-c4e3bc35227b@kernel.org>
From: Amir Goldstein <amir73il@gmail.com>
We intend to add support for more xflags to selective filesystems and
We cannot rely on copy_struct_from_user() to detect this extension.
In preparation of extending the API, do not allow setting xflags unknown
by this kernel version.
Also do not pass the read-only flags and read-only field fsx_nextents to
filesystem.
These changes should not affect existing chattr programs that use the
ioctl to get fsxattr before setting the new values.
Link: https://lore.kernel.org/linux-fsdevel/20250216164029.20673-4-pali@kernel.org/
Cc: Pali Rohár <pali@kernel.org>
Cc: Andrey Albershteyn <aalbersh@redhat.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Andrey Albershteyn <aalbersh@kernel.org>
---
fs/file_attr.c | 8 +++++++-
include/linux/fileattr.h | 20 ++++++++++++++++++++
2 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/fs/file_attr.c b/fs/file_attr.c
index 4e85fa00c092..62f08872d4ad 100644
--- a/fs/file_attr.c
+++ b/fs/file_attr.c
@@ -99,9 +99,10 @@ EXPORT_SYMBOL(vfs_fileattr_get);
int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa)
{
struct fsxattr xfa;
+ __u32 mask = FS_XFLAGS_MASK;
memset(&xfa, 0, sizeof(xfa));
- xfa.fsx_xflags = fa->fsx_xflags;
+ xfa.fsx_xflags = fa->fsx_xflags & mask;
xfa.fsx_extsize = fa->fsx_extsize;
xfa.fsx_nextents = fa->fsx_nextents;
xfa.fsx_projid = fa->fsx_projid;
@@ -118,11 +119,16 @@ static int copy_fsxattr_from_user(struct fileattr *fa,
struct fsxattr __user *ufa)
{
struct fsxattr xfa;
+ __u32 mask = FS_XFLAGS_MASK;
if (copy_from_user(&xfa, ufa, sizeof(xfa)))
return -EFAULT;
+ if (xfa.fsx_xflags & ~mask)
+ return -EINVAL;
+
fileattr_fill_xflags(fa, xfa.fsx_xflags);
+ fa->fsx_xflags &= ~FS_XFLAG_RDONLY_MASK;
fa->fsx_extsize = xfa.fsx_extsize;
fa->fsx_nextents = xfa.fsx_nextents;
fa->fsx_projid = xfa.fsx_projid;
diff --git a/include/linux/fileattr.h b/include/linux/fileattr.h
index 6030d0bf7ad3..e2a2f4ae242d 100644
--- a/include/linux/fileattr.h
+++ b/include/linux/fileattr.h
@@ -14,6 +14,26 @@
FS_XFLAG_NODUMP | FS_XFLAG_NOATIME | FS_XFLAG_DAX | \
FS_XFLAG_PROJINHERIT)
+/* Read-only inode flags */
+#define FS_XFLAG_RDONLY_MASK \
+ (FS_XFLAG_PREALLOC | FS_XFLAG_HASATTR)
+
+/* Flags to indicate valid value of fsx_ fields */
+#define FS_XFLAG_VALUES_MASK \
+ (FS_XFLAG_EXTSIZE | FS_XFLAG_COWEXTSIZE)
+
+/* Flags for directories */
+#define FS_XFLAG_DIRONLY_MASK \
+ (FS_XFLAG_RTINHERIT | FS_XFLAG_NOSYMLINKS | FS_XFLAG_EXTSZINHERIT)
+
+/* Misc settable flags */
+#define FS_XFLAG_MISC_MASK \
+ (FS_XFLAG_REALTIME | FS_XFLAG_NODEFRAG | FS_XFLAG_FILESTREAM)
+
+#define FS_XFLAGS_MASK \
+ (FS_XFLAG_COMMON | FS_XFLAG_RDONLY_MASK | FS_XFLAG_VALUES_MASK | \
+ FS_XFLAG_DIRONLY_MASK | FS_XFLAG_MISC_MASK)
+
/*
* Merged interface for miscellaneous file attributes. 'flags' originates from
* ext* and 'fsx_flags' from xfs. There's some overlap between the two, which
--
2.47.2
^ permalink raw reply related
* [PATCH v6 4/6] fs: make vfs_fileattr_[get|set] return -EOPNOSUPP
From: Andrey Albershteyn @ 2025-06-30 16:20 UTC (permalink / raw)
To: Amir Goldstein, Arnd Bergmann, Casey Schaufler, Christian Brauner,
Jan Kara, Pali Rohár, Paul Moore
Cc: linux-api, linux-fsdevel, linux-kernel, linux-xfs, selinux,
Andrey Albershteyn
In-Reply-To: <20250630-xattrat-syscall-v6-0-c4e3bc35227b@kernel.org>
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>
---
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 related
* [PATCH v6 3/6] selinux: implement inode_file_[g|s]etattr hooks
From: Andrey Albershteyn @ 2025-06-30 16:20 UTC (permalink / raw)
To: Amir Goldstein, Arnd Bergmann, Casey Schaufler, Christian Brauner,
Jan Kara, Pali Rohár, Paul Moore
Cc: linux-api, linux-fsdevel, linux-kernel, linux-xfs, selinux,
Andrey Albershteyn
In-Reply-To: <20250630-xattrat-syscall-v6-0-c4e3bc35227b@kernel.org>
These hooks are called on inode extended attribute retrieval/change.
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>
---
security/selinux/hooks.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 595ceb314aeb..be7aca2269fa 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3480,6 +3480,18 @@ static int selinux_inode_removexattr(struct mnt_idmap *idmap,
return -EACCES;
}
+static int selinux_inode_file_setattr(struct dentry *dentry,
+ struct fileattr *fa)
+{
+ return dentry_has_perm(current_cred(), dentry, FILE__SETATTR);
+}
+
+static int selinux_inode_file_getattr(struct dentry *dentry,
+ struct fileattr *fa)
+{
+ return dentry_has_perm(current_cred(), dentry, FILE__GETATTR);
+}
+
static int selinux_path_notify(const struct path *path, u64 mask,
unsigned int obj_type)
{
@@ -7350,6 +7362,8 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = {
LSM_HOOK_INIT(inode_getxattr, selinux_inode_getxattr),
LSM_HOOK_INIT(inode_listxattr, selinux_inode_listxattr),
LSM_HOOK_INIT(inode_removexattr, selinux_inode_removexattr),
+ LSM_HOOK_INIT(inode_file_getattr, selinux_inode_file_getattr),
+ LSM_HOOK_INIT(inode_file_setattr, selinux_inode_file_setattr),
LSM_HOOK_INIT(inode_set_acl, selinux_inode_set_acl),
LSM_HOOK_INIT(inode_get_acl, selinux_inode_get_acl),
LSM_HOOK_INIT(inode_remove_acl, selinux_inode_remove_acl),
--
2.47.2
^ permalink raw reply related
* [PATCH v6 2/6] lsm: introduce new hooks for setting/getting inode fsxattr
From: Andrey Albershteyn @ 2025-06-30 16:20 UTC (permalink / raw)
To: Amir Goldstein, Arnd Bergmann, Casey Schaufler, Christian Brauner,
Jan Kara, Pali Rohár, Paul Moore
Cc: linux-api, linux-fsdevel, linux-kernel, linux-xfs, selinux,
Andrey Albershteyn
In-Reply-To: <20250630-xattrat-syscall-v6-0-c4e3bc35227b@kernel.org>
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>
---
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 related
* [PATCH v6 1/6] fs: split fileattr related helpers into separate file
From: Andrey Albershteyn @ 2025-06-30 16:20 UTC (permalink / raw)
To: Amir Goldstein, Arnd Bergmann, Casey Schaufler, Christian Brauner,
Jan Kara, Pali Rohár, Paul Moore
Cc: linux-api, linux-fsdevel, linux-kernel, linux-xfs, selinux,
Andrey Albershteyn
In-Reply-To: <20250630-xattrat-syscall-v6-0-c4e3bc35227b@kernel.org>
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>
---
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 related
* [PATCH v6 0/6] fs: introduce file_getattr and file_setattr syscalls
From: Andrey Albershteyn @ 2025-06-30 16:20 UTC (permalink / raw)
To: Amir Goldstein, Arnd Bergmann, Casey Schaufler, Christian Brauner,
Jan Kara, Pali Rohár, Paul Moore
Cc: linux-api, linux-fsdevel, linux-kernel, linux-xfs, selinux,
Andrey Albershteyn, Andrey Albershteyn
This patchset introduced two new syscalls file_getattr() and
file_setattr(). These syscalls are similar to FS_IOC_FSSETXATTR ioctl()
except they use *at() semantics. Therefore, there's no need to open the
file to get a fd.
These syscalls allow userspace to set filesystem inode attributes on
special files. One of the usage examples is XFS quota projects.
XFS has project quotas which could be attached to a directory. All
new inodes in these directories inherit project ID set on parent
directory.
The project is created from userspace by opening and calling
FS_IOC_FSSETXATTR on each inode. This is not possible for special
files such as FIFO, SOCK, BLK etc. Therefore, some inodes are left
with empty project ID. Those inodes then are not shown in the quota
accounting but still exist in the directory. This is not critical but in
the case when special files are created in the directory with already
existing project quota, these new inodes inherit extended attributes.
This creates a mix of special files with and without attributes.
Moreover, special files with attributes don't have a possibility to
become clear or change the attributes. This, in turn, prevents userspace
from re-creating quota project on these existing files.
An xfstests test generic/766 with basic coverage is at:
https://github.com/alberand/xfstests/commits/b4/file-attr/
NAME
file_getattr/file_setattr - get/set filesystem inode attributes
SYNOPSIS
#include <sys/syscall.h> /* Definition of SYS_* constants */
#include <unistd.h>
long syscall(SYS_file_getattr, int dirfd, const char *pathname,
struct fsx_fileattr *fsx, size_t size,
unsigned int at_flags);
long syscall(SYS_file_setattr, int dirfd, const char *pathname,
struct fsx_fileattr *fsx, size_t size,
unsigned int at_flags);
Note: glibc doesn't provide for file_getattr()/file_setattr(),
use syscall(2) instead.
DESCRIPTION
The file_getattr()/file_setattr() are used to set extended file
attributes. These syscalls take dirfd in conjunction with the
pathname argument. The syscall then operates on inode opened
according to openat(2) semantics.
This is an alternative to FS_IOC_FSGETXATTR/FS_IOC_FSSETXATTR
ioctl with a difference that file don't need to be open as file
can be referenced with a path instead of fd. By having this one
can manipulated filesystem inode attributes not only on regular
files but also on special ones. This is not possible with
FS_IOC_FSSETXATTR ioctl as ioctl() can not be called on special
files directly for the filesystem inode.
at_flags can be set to AT_SYMLINK_NOFOLLOW or AT_EMPTY_PATH.
RETURN VALUE
On success, 0 is returned. On error, -1 is returned, and errno
is set to indicate the error.
ERRORS
EINVAL Invalid at_flag specified (only
AT_SYMLINK_NOFOLLOW and AT_EMPTY_PATH is
supported).
EINVAL Size was smaller than any known version of
struct fsx_fileattr.
EINVAL Invalid combination of parameters provided in
fsx_fileattr for this type of file.
E2BIG Size of input argument struct fsx_fileattr
is too big.
EBADF Invalid file descriptor was provided.
EPERM No permission to change this file.
EOPNOTSUPP Filesystem does not support setting attributes
on this type of inode
HISTORY
Added in Linux 6.16.
EXAMPLE
Create directory and file "mkdir ./dir && touch ./dir/foo" and then
execute the following program:
#include <fcntl.h>
#include <errno.h>
#include <string.h>
#include <linux/fs.h>
#include <stdio.h>
#include <sys/syscall.h>
#include <unistd.h>
#if !defined(SYS_file_getattr) && defined(__x86_64__)
#define SYS_file_getattr 468
#define SYS_file_setattr 469
struct fsx_fileattr {
__u32 fsx_xflags;
__u32 fsx_extsize;
__u32 fsx_nextents;
__u32 fsx_projid;
__u32 fsx_cowextsize;
};
#endif
int
main(int argc, char **argv) {
int dfd;
int error;
struct fsx_fileattr fsx;
dfd = open("./dir", O_RDONLY);
if (dfd == -1) {
printf("can not open ./dir");
return dfd;
}
error = syscall(SYS_file_getattr, dfd, "./foo", &fsx,
sizeof(struct fsx_fileattr), 0);
if (error) {
printf("can not call SYS_file_getattr: %s",
strerror(errno));
return error;
}
printf("./dir/foo flags: %d\n", fsx.fsx_xflags);
fsx.fsx_xflags |= FS_XFLAG_NODUMP;
error = syscall(SYS_file_setattr, dfd, "./foo", &fsx,
sizeof(struct fsx_fileattr), 0);
if (error) {
printf("can not call SYS_file_setattr: %s",
strerror(errno));
return error;
}
printf("./dir/foo flags: %d\n", fsx.fsx_xflags);
return error;
}
SEE ALSO
ioctl(2), ioctl_iflags(2), ioctl_xfs_fsgetxattr(2), openat(2)
---
Changes in v6:
- Update cover letter example and docs
- Applied __free() attribute for syscall stack objects
- Introduced struct fsx_fileattr
- Replace 'struct fsxattr' with 'struct fsx_fileattr'
- Add helper to fill in fsx_fileattr from fileattr
- Dropped copy_fsx_to_user() header declaration
- Link to v5: https://lore.kernel.org/r/20250513-xattrat-syscall-v5-0-22bb9c6c767f@kernel.org
Changes in v5:
- Remove setting of LOOKUP_EMPTY flags which does not have any effect
- Return -ENOSUPP from vfs_fileattr_set()
- Add fsxattr masking (by Amir)
- Fix UAF issue dentry
- Fix getname_maybe_null() issue with NULL path
- Implement file_getattr/file_setattr hooks
- Return LSM return code from file_setattr
- Rename from getfsxattrat/setfsxattrat to file_getattr/file_setattr
- Link to v4: https://lore.kernel.org/r/20250321-xattrat-syscall-v4-0-3e82e6fb3264@kernel.org
Changes in v4:
- Use getname_maybe_null() for correct handling of dfd + path semantic
- Remove restriction for special files on which flags are allowed
- Utilize copy_struct_from_user() for better future compatibility
- Add draft man page to cover letter
- Convert -ENOIOCTLCMD to -EOPNOSUPP as more appropriate for syscall
- Add missing __user to header declaration of syscalls
- Link to v3: https://lore.kernel.org/r/20250211-xattrat-syscall-v3-1-a07d15f898b2@kernel.org
Changes in v3:
- Remove unnecessary "dfd is dir" check as it checked in user_path_at()
- Remove unnecessary "same filesystem" check
- Use CLASS() instead of directly calling fdget/fdput
- Link to v2: https://lore.kernel.org/r/20250122-xattrat-syscall-v2-1-5b360d4fbcb2@kernel.org
v1:
https://lore.kernel.org/linuxppc-dev/20250109174540.893098-1-aalbersh@kernel.org/
Previous discussion:
https://lore.kernel.org/linux-xfs/20240520164624.665269-2-aalbersh@redhat.com/
---
Amir Goldstein (1):
fs: prepare for extending file_get/setattr()
Andrey Albershteyn (5):
fs: split fileattr related helpers into separate file
lsm: introduce new hooks for setting/getting inode fsxattr
selinux: implement inode_file_[g|s]etattr hooks
fs: make vfs_fileattr_[get|set] return -EOPNOSUPP
fs: introduce file_getattr and file_setattr syscalls
arch/alpha/kernel/syscalls/syscall.tbl | 2 +
arch/arm/tools/syscall.tbl | 2 +
arch/arm64/tools/syscall_32.tbl | 2 +
arch/m68k/kernel/syscalls/syscall.tbl | 2 +
arch/microblaze/kernel/syscalls/syscall.tbl | 2 +
arch/mips/kernel/syscalls/syscall_n32.tbl | 2 +
arch/mips/kernel/syscalls/syscall_n64.tbl | 2 +
arch/mips/kernel/syscalls/syscall_o32.tbl | 2 +
arch/parisc/kernel/syscalls/syscall.tbl | 2 +
arch/powerpc/kernel/syscalls/syscall.tbl | 2 +
arch/s390/kernel/syscalls/syscall.tbl | 2 +
arch/sh/kernel/syscalls/syscall.tbl | 2 +
arch/sparc/kernel/syscalls/syscall.tbl | 2 +
arch/x86/entry/syscalls/syscall_32.tbl | 2 +
arch/x86/entry/syscalls/syscall_64.tbl | 2 +
arch/xtensa/kernel/syscalls/syscall.tbl | 2 +
fs/Makefile | 3 +-
fs/ecryptfs/inode.c | 8 +-
fs/file_attr.c | 493 ++++++++++++++++++++++++++++
fs/ioctl.c | 309 -----------------
fs/overlayfs/inode.c | 2 +-
include/linux/fileattr.h | 24 ++
include/linux/lsm_hook_defs.h | 2 +
include/linux/security.h | 16 +
include/linux/syscalls.h | 6 +
include/uapi/asm-generic/unistd.h | 8 +-
include/uapi/linux/fs.h | 18 +
scripts/syscall.tbl | 2 +
security/security.c | 30 ++
security/selinux/hooks.c | 14 +
30 files changed, 654 insertions(+), 313 deletions(-)
---
base-commit: d0b3b7b22dfa1f4b515fd3a295b3fd958f9e81af
change-id: 20250114-xattrat-syscall-6a1136d2db59
Best regards,
--
Andrey Albershteyn <aalbersh@kernel.org>
^ permalink raw reply
* Re: [RFC 00/19] Kernel API Specification Framework
From: Sasha Levin @ 2025-06-30 14:27 UTC (permalink / raw)
To: Dmitry Vyukov; +Cc: kees, elver, linux-api, linux-kernel, tools, workflows
In-Reply-To: <CACT4Y+ZB45ovD0hX3xX_yTUVSRDc1UCXnVDB57jxyWPPc7k=MA@mail.gmail.com>
On Fri, Jun 27, 2025 at 08:23:41AM +0200, Dmitry Vyukov wrote:
>On Thu, 26 Jun 2025 at 18:23, Sasha Levin <sashal@kernel.org> wrote:
>>
>> On Thu, Jun 26, 2025 at 10:37:33AM +0200, Dmitry Vyukov wrote:
>> >On Thu, 26 Jun 2025 at 10:32, Dmitry Vyukov <dvyukov@google.com> wrote:
>> >>
>> >> On Wed, 25 Jun 2025 at 17:55, Sasha Levin <sashal@kernel.org> wrote:
>> >> >
>> >> > On Wed, Jun 25, 2025 at 10:52:46AM +0200, Dmitry Vyukov wrote:
>> >> > >On Tue, 24 Jun 2025 at 22:04, Sasha Levin <sashal@kernel.org> wrote:
>> >> > >
>> >> > >> >6. What's the goal of validation of the input arguments?
>> >> > >> >Kernel code must do this validation anyway, right.
>> >> > >> >Any non-trivial validation is hard, e.g. even for open the validation function
>> >> > >> >for file name would need to have access to flags and check file precense for
>> >> > >> >some flags combinations. That may add significant amount of non-trivial code
>> >> > >> >that duplicates main syscall logic, and that logic may also have bugs and
>> >> > >> >memory leaks.
>> >> > >>
>> >> > >> Mostly to catch divergence from the spec: think of a scenario where
>> >> > >> someone added a new param/flag/etc but forgot to update the spec - this
>> >> > >> will help catch it.
>> >> > >
>> >> > >How exactly is this supposed to work?
>> >> > >Even if we run with a unit test suite, a test suite may include some
>> >> > >incorrect inputs to check for error conditions. The framework will
>> >> > >report violations on these incorrect inputs. These are not bugs in the
>> >> > >API specifications, nor in the test suite (read false positives).
>> >> >
>> >> > Right now it would be something along the lines of the test checking for
>> >> > an expected failure message in dmesg, something along the lines of:
>> >> >
>> >> > https://github.com/linux-test-project/ltp/blob/0c99c7915f029d32de893b15b0a213ff3de210af/testcases/commands/sysctl/sysctl02.sh#L67
>> >> >
>> >> > I'm not opposed to coming up with a better story...
>> >
>> >If the goal of validation is just indirectly validating correctness of
>> >the specification itself, then I would look for other ways of
>> >validating correctness of the spec.
>> >Either removing duplication between specification and actual code
>> >(i.e. generating it from SYSCALL_DEFINE, or the other way around) ,
>> >then spec is correct by construction. Or, cross-validating it with
>> >info automatically extracted from the source (using
>> >clang/dwarf/pahole).
>> >This would be more scalable (O(1) work, rather than thousands more
>> >manually written tests).
>> >
>> >> Oh, you mean special tests for this framework (rather than existing tests).
>> >> I don't think this is going to work in practice. Besides writing all
>> >> these specifications, we will also need to write dozens of tests per
>> >> each specification (e.g. for each fd arg one needs at least 3 tests:
>> >> -1, valid fd, inclid fd; an enum may need 5 various inputs of
>> >> something; let alone netlink specifications).
>>
>> I didn't mean just for the framework: being able to specify the APIs in
>> machine readable format will enable us to automatically generate
>> exhaustive tests for each such API.
>>
>> I've been playing with the kapi tool (see last patch) which already
>> supports different formatters. Right now it outputs human readable
>> output, but I have proof-of-concept code that outputs testcases for
>> specced APIs.
>>
>> The dream here is to be able to automatically generate
>> hundreds/thousands of tests for each API in an automated fashion, and
>> verify the results with:
>>
>> 1. Simply checking expected return value.
>>
>> 2. Checking that the actual action happened (i.e. we called close(fd),
>> verify that `fd` is really closed).
>>
>> 3. Check for side effects (i.e. close(fd) isn't supposed to allocate
>> memory - verify that it didn't allocate memory).
>>
>> 4. Code coverage: our tests are supposed to cover 100% of the code in
>> that APIs call chain, do we have code that didn't run (missing/incorrect
>> specs).
>
>
>This is all good. I was asking the argument verification part of the
>framework. Is it required for any of this? How?
Specifications without enforcement are just documentation :)
In my mind, there are a few reasons we want this:
1. For folks coding against the kernel, it's a way for them to know that
the code they're writing fits within the spec of the kernel's API.
2. Enforcement around kernel changes: think of a scenario where a flag
is added to a syscall - the author of that change will have to also
update the spec because otherwise the verification layer will complain
about the new flag. This helps prevent divergence between the code and
the spec.
3. Extra layer of security: we can choose to enable this as an
additional layer to protect us from missing checks in our userspace
facing API.
--
Thanks,
Sasha
^ permalink raw reply
* Re: [PATCH v5 3/7] futex: Use explicit sizes for compat_exit_robust_list
From: kernel test robot @ 2025-06-28 14:27 UTC (permalink / raw)
To: André Almeida, Thomas Gleixner, Ingo Molnar, Peter Zijlstra,
Darren Hart, Davidlohr Bueso, Shuah Khan, Arnd Bergmann,
Sebastian Andrzej Siewior, Waiman Long
Cc: llvm, oe-kbuild-all, linux-kernel, linux-kselftest, linux-api,
kernel-dev, André Almeida
In-Reply-To: <20250626-tonyk-robust_futex-v5-3-179194dbde8f@igalia.com>
Hi André,
kernel test robot noticed the following build warnings:
[auto build test WARNING on a24cc6ce1933eade12aa2b9859de0fcd2dac2c06]
url: https://github.com/intel-lab-lkp/linux/commits/Andr-Almeida/selftests-futex-Add-ASSERT_-macros/20250627-011636
base: a24cc6ce1933eade12aa2b9859de0fcd2dac2c06
patch link: https://lore.kernel.org/r/20250626-tonyk-robust_futex-v5-3-179194dbde8f%40igalia.com
patch subject: [PATCH v5 3/7] futex: Use explicit sizes for compat_exit_robust_list
config: arm-randconfig-003-20250627 (https://download.01.org/0day-ci/archive/20250628/202506282104.ThReVuLD-lkp@intel.com/config)
compiler: clang version 21.0.0git (https://github.com/llvm/llvm-project e04c938cc08a90ae60440ce22d072ebc69d67ee8)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250628/202506282104.ThReVuLD-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506282104.ThReVuLD-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from net/sched/sch_qfq.c:13:
In file included from include/linux/netdevice.h:44:
In file included from include/uapi/linux/neighbour.h:6:
In file included from include/linux/netlink.h:9:
In file included from include/net/scm.h:13:
In file included from include/net/compat.h:8:
>> include/linux/compat.h:665:35: warning: declaration of 'struct robust_list_head32' will not be visible outside of this function [-Wvisibility]
665 | compat_sys_set_robust_list(struct robust_list_head32 __user *head,
| ^
1 warning generated.
--
In file included from net/sched/sch_multiq.c:15:
In file included from include/net/netlink.h:6:
In file included from include/linux/netlink.h:9:
In file included from include/net/scm.h:13:
In file included from include/net/compat.h:8:
>> include/linux/compat.h:665:35: warning: declaration of 'struct robust_list_head32' will not be visible outside of this function [-Wvisibility]
665 | compat_sys_set_robust_list(struct robust_list_head32 __user *head,
| ^
net/sched/sch_multiq.c:36:6: warning: variable 'err' set but not used [-Wunused-but-set-variable]
36 | int err;
| ^
2 warnings generated.
vim +665 include/linux/compat.h
621
622 #ifdef __ARCH_WANT_COMPAT_SYS_PWRITEV64
623 asmlinkage long compat_sys_pwritev64(unsigned long fd,
624 const struct iovec __user *vec,
625 unsigned long vlen, loff_t pos);
626 #endif
627 asmlinkage long compat_sys_sendfile(int out_fd, int in_fd,
628 compat_off_t __user *offset, compat_size_t count);
629 asmlinkage long compat_sys_sendfile64(int out_fd, int in_fd,
630 compat_loff_t __user *offset, compat_size_t count);
631 asmlinkage long compat_sys_pselect6_time32(int n, compat_ulong_t __user *inp,
632 compat_ulong_t __user *outp,
633 compat_ulong_t __user *exp,
634 struct old_timespec32 __user *tsp,
635 void __user *sig);
636 asmlinkage long compat_sys_pselect6_time64(int n, compat_ulong_t __user *inp,
637 compat_ulong_t __user *outp,
638 compat_ulong_t __user *exp,
639 struct __kernel_timespec __user *tsp,
640 void __user *sig);
641 asmlinkage long compat_sys_ppoll_time32(struct pollfd __user *ufds,
642 unsigned int nfds,
643 struct old_timespec32 __user *tsp,
644 const compat_sigset_t __user *sigmask,
645 compat_size_t sigsetsize);
646 asmlinkage long compat_sys_ppoll_time64(struct pollfd __user *ufds,
647 unsigned int nfds,
648 struct __kernel_timespec __user *tsp,
649 const compat_sigset_t __user *sigmask,
650 compat_size_t sigsetsize);
651 asmlinkage long compat_sys_signalfd4(int ufd,
652 const compat_sigset_t __user *sigmask,
653 compat_size_t sigsetsize, int flags);
654 asmlinkage long compat_sys_newfstatat(unsigned int dfd,
655 const char __user *filename,
656 struct compat_stat __user *statbuf,
657 int flag);
658 asmlinkage long compat_sys_newfstat(unsigned int fd,
659 struct compat_stat __user *statbuf);
660 /* No generic prototype for sync_file_range and sync_file_range2 */
661 asmlinkage long compat_sys_waitid(int, compat_pid_t,
662 struct compat_siginfo __user *, int,
663 struct compat_rusage __user *);
664 asmlinkage long
> 665 compat_sys_set_robust_list(struct robust_list_head32 __user *head,
666 compat_size_t len);
667 asmlinkage long
668 compat_sys_get_robust_list(int pid, compat_uptr_t __user *head_ptr,
669 compat_size_t __user *len_ptr);
670 asmlinkage long compat_sys_getitimer(int which,
671 struct old_itimerval32 __user *it);
672 asmlinkage long compat_sys_setitimer(int which,
673 struct old_itimerval32 __user *in,
674 struct old_itimerval32 __user *out);
675 asmlinkage long compat_sys_kexec_load(compat_ulong_t entry,
676 compat_ulong_t nr_segments,
677 struct compat_kexec_segment __user *,
678 compat_ulong_t flags);
679 asmlinkage long compat_sys_timer_create(clockid_t which_clock,
680 struct compat_sigevent __user *timer_event_spec,
681 timer_t __user *created_timer_id);
682 asmlinkage long compat_sys_ptrace(compat_long_t request, compat_long_t pid,
683 compat_long_t addr, compat_long_t data);
684 asmlinkage long compat_sys_sched_setaffinity(compat_pid_t pid,
685 unsigned int len,
686 compat_ulong_t __user *user_mask_ptr);
687 asmlinkage long compat_sys_sched_getaffinity(compat_pid_t pid,
688 unsigned int len,
689 compat_ulong_t __user *user_mask_ptr);
690 asmlinkage long compat_sys_sigaltstack(const compat_stack_t __user *uss_ptr,
691 compat_stack_t __user *uoss_ptr);
692 asmlinkage long compat_sys_rt_sigsuspend(compat_sigset_t __user *unewset,
693 compat_size_t sigsetsize);
694 #ifndef CONFIG_ODD_RT_SIGACTION
695 asmlinkage long compat_sys_rt_sigaction(int,
696 const struct compat_sigaction __user *,
697 struct compat_sigaction __user *,
698 compat_size_t);
699 #endif
700 asmlinkage long compat_sys_rt_sigprocmask(int how, compat_sigset_t __user *set,
701 compat_sigset_t __user *oset,
702 compat_size_t sigsetsize);
703 asmlinkage long compat_sys_rt_sigpending(compat_sigset_t __user *uset,
704 compat_size_t sigsetsize);
705 asmlinkage long compat_sys_rt_sigtimedwait_time32(compat_sigset_t __user *uthese,
706 struct compat_siginfo __user *uinfo,
707 struct old_timespec32 __user *uts, compat_size_t sigsetsize);
708 asmlinkage long compat_sys_rt_sigtimedwait_time64(compat_sigset_t __user *uthese,
709 struct compat_siginfo __user *uinfo,
710 struct __kernel_timespec __user *uts, compat_size_t sigsetsize);
711 asmlinkage long compat_sys_rt_sigqueueinfo(compat_pid_t pid, int sig,
712 struct compat_siginfo __user *uinfo);
713 /* No generic prototype for rt_sigreturn */
714 asmlinkage long compat_sys_times(struct compat_tms __user *tbuf);
715 asmlinkage long compat_sys_getrlimit(unsigned int resource,
716 struct compat_rlimit __user *rlim);
717 asmlinkage long compat_sys_setrlimit(unsigned int resource,
718 struct compat_rlimit __user *rlim);
719 asmlinkage long compat_sys_getrusage(int who, struct compat_rusage __user *ru);
720 asmlinkage long compat_sys_gettimeofday(struct old_timeval32 __user *tv,
721 struct timezone __user *tz);
722 asmlinkage long compat_sys_settimeofday(struct old_timeval32 __user *tv,
723 struct timezone __user *tz);
724 asmlinkage long compat_sys_sysinfo(struct compat_sysinfo __user *info);
725 asmlinkage long compat_sys_mq_open(const char __user *u_name,
726 int oflag, compat_mode_t mode,
727 struct compat_mq_attr __user *u_attr);
728 asmlinkage long compat_sys_mq_notify(mqd_t mqdes,
729 const struct compat_sigevent __user *u_notification);
730 asmlinkage long compat_sys_mq_getsetattr(mqd_t mqdes,
731 const struct compat_mq_attr __user *u_mqstat,
732 struct compat_mq_attr __user *u_omqstat);
733 asmlinkage long compat_sys_msgctl(int first, int second, void __user *uptr);
734 asmlinkage long compat_sys_msgrcv(int msqid, compat_uptr_t msgp,
735 compat_ssize_t msgsz, compat_long_t msgtyp, int msgflg);
736 asmlinkage long compat_sys_msgsnd(int msqid, compat_uptr_t msgp,
737 compat_ssize_t msgsz, int msgflg);
738 asmlinkage long compat_sys_semctl(int semid, int semnum, int cmd, int arg);
739 asmlinkage long compat_sys_shmctl(int first, int second, void __user *uptr);
740 asmlinkage long compat_sys_shmat(int shmid, compat_uptr_t shmaddr, int shmflg);
741 asmlinkage long compat_sys_recvfrom(int fd, void __user *buf, compat_size_t len,
742 unsigned flags, struct sockaddr __user *addr,
743 int __user *addrlen);
744 asmlinkage long compat_sys_sendmsg(int fd, struct compat_msghdr __user *msg,
745 unsigned flags);
746 asmlinkage long compat_sys_recvmsg(int fd, struct compat_msghdr __user *msg,
747 unsigned int flags);
748 /* No generic prototype for readahead */
749 asmlinkage long compat_sys_keyctl(u32 option,
750 u32 arg2, u32 arg3, u32 arg4, u32 arg5);
751 asmlinkage long compat_sys_execve(const char __user *filename, const compat_uptr_t __user *argv,
752 const compat_uptr_t __user *envp);
753 /* No generic prototype for fadvise64_64 */
754 /* CONFIG_MMU only */
755 asmlinkage long compat_sys_rt_tgsigqueueinfo(compat_pid_t tgid,
756 compat_pid_t pid, int sig,
757 struct compat_siginfo __user *uinfo);
758 asmlinkage long compat_sys_recvmmsg_time64(int fd, struct compat_mmsghdr __user *mmsg,
759 unsigned vlen, unsigned int flags,
760 struct __kernel_timespec __user *timeout);
761 asmlinkage long compat_sys_recvmmsg_time32(int fd, struct compat_mmsghdr __user *mmsg,
762 unsigned vlen, unsigned int flags,
763 struct old_timespec32 __user *timeout);
764 asmlinkage long compat_sys_wait4(compat_pid_t pid,
765 compat_uint_t __user *stat_addr, int options,
766 struct compat_rusage __user *ru);
767 asmlinkage long compat_sys_fanotify_mark(int, unsigned int, __u32, __u32,
768 int, const char __user *);
769 asmlinkage long compat_sys_open_by_handle_at(int mountdirfd,
770 struct file_handle __user *handle,
771 int flags);
772 asmlinkage long compat_sys_sendmmsg(int fd, struct compat_mmsghdr __user *mmsg,
773 unsigned vlen, unsigned int flags);
774 asmlinkage long compat_sys_execveat(int dfd, const char __user *filename,
775 const compat_uptr_t __user *argv,
776 const compat_uptr_t __user *envp, int flags);
777 asmlinkage ssize_t compat_sys_preadv2(compat_ulong_t fd,
778 const struct iovec __user *vec,
779 compat_ulong_t vlen, u32 pos_low, u32 pos_high, rwf_t flags);
780 asmlinkage ssize_t compat_sys_pwritev2(compat_ulong_t fd,
781 const struct iovec __user *vec,
782 compat_ulong_t vlen, u32 pos_low, u32 pos_high, rwf_t flags);
783 #ifdef __ARCH_WANT_COMPAT_SYS_PREADV64V2
784 asmlinkage long compat_sys_preadv64v2(unsigned long fd,
785 const struct iovec __user *vec,
786 unsigned long vlen, loff_t pos, rwf_t flags);
787 #endif
788
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH RFT v17 4/8] fork: Add shadow stack support to clone3()
From: Mark Brown @ 2025-06-27 21:31 UTC (permalink / raw)
To: Catalin Marinas
Cc: 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,
linux-kernel, Will Deacon, jannh, Yury Khrustalev, Wilco Dijkstra,
linux-kselftest, linux-api, Kees Cook
In-Reply-To: <aF7SpWSKfjEFTHBk@arm.com>
[-- Attachment #1: Type: text/plain, Size: 3836 bytes --]
On Fri, Jun 27, 2025 at 06:19:33PM +0100, Catalin Marinas wrote:
> On Mon, Jun 09, 2025 at 01:54:05PM +0100, Mark Brown wrote:
> > + /* Ensure that a token written as a result of a pivot is visible */
> > + gcsb_dsync();
> > + gcspr_el0 = args->shadow_stack_token;
> > + if (!gcs_consume_token(vma, page, gcspr_el0))
> > + return -EINVAL;
> > +
> > + tsk->thread.gcspr_el0 = gcspr_el0 + sizeof(u64);
> > +
> > + /* Ensure that our token consumption visible */
> > + gcsb_dsync();
> What are the scenarios where we need the barriers? We have one via
> map_shadow_stack() that would cover the first one. IIUC, GCSSS2 also
> generates a GCSB event (or maybe I got it all wrong; I need to read the
> spec).
I think now that gcs_consume_token() does a cmpxchg they're redundant,
your analysis covers the first one (anything that puts a valid token
in memory should have a barrier) and now gcs_consume_token() does a
cmpxchg the second one should also be redundant thanks to R_FZRGP. It
would be good if someone double checked though.
Originally gcs_consume_token() was using regular accesses as for the
example in DDI0487 L.a K3.3 and was tried on two addresses, I missed
dropping the barriers when changing to a cmpxchg.
> > +static int shstk_validate_clone(struct task_struct *p,
> > + struct kernel_clone_args *args)
> > +{
> > + mmap_read_lock(mm);
> > + addr = untagged_addr_remote(mm, args->shadow_stack_token);
> I think down the line, get_user_page_vma_remote() already does an
> untagged_addr_remote(). But it does it after the vma look-up, so we
> still need the untagging early.
> That said, would we ever allowed a tagged pointer for the shadow stack?
For arm64 you can architecturally use tags as per G_HMJHM. I_WBHHX says
that GCS accesses are tag unchecked, but tags are used on GCSSS1 as per
I_MGLTC and I_MBHFS. We'll need new ABI to allow userspace to get a
PROT_MTE GCS though, I'd planned on extending map_shadow_stack() for
that, and adding handling in the token validation here.
There's also the fact that the untagging should be very cheap in the
context of what we're doing so it seems sensible to just have it,
especially generic code which applies to all arches.
> > +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;
> > +
> > + /*
> > + * The architecture must check support on the specific
> > + * machine.
> > + */
> > + return IS_ENABLED(CONFIG_ARCH_HAS_USER_SHADOW_STACK);
> I don't understand the comment here. It implies some kind of fallback
> for further arch checks but it's just a return.
Here we're just doing initial triage that a shadow stack could possibly
be valid, the check here is there to fail if there's one specified but
there is no support in the kernel (eg, for architectures that don't have
the feature at all like arm32). The comment is trying to say that we're
not attempting to validate that we can actually use shadow stacks on the
current system, just that the support exists in the kernel. I'll reword
the comment, it's not clear.
> BTW, clone3_stack_valid() has an access_ok() check as well. Shall we add
> it here? That's where the size would have come in handy but IIUC the
> decision was to drop it (fine by me, just validate that the token is
> accessible).
AIUI the main reason for doing that for the normal stack is to report an
error before we actually start the thread and have it fault trying to
access an invalid stack since we don't otherwise look at the memory,
like you say with shadow stacks we'll consume the token before we start
the new thread so we get the equivalent error reporting as part of that.
I don't think the extra check would buy us much.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v5 1/7] selftests/futex: Add ASSERT_ macros
From: André Almeida @ 2025-06-27 20:23 UTC (permalink / raw)
To: Thomas Gleixner, Shuah Khan
Cc: Davidlohr Bueso, Peter Zijlstra, Arnd Bergmann, linux-kernel,
linux-kselftest, Sebastian Andrzej Siewior, linux-api, kernel-dev,
Darren Hart, Ingo Molnar, Waiman Long
In-Reply-To: <87ecv6p364.ffs@tglx>
Em 26/06/2025 19:07, Thomas Gleixner escreveu:
> On Thu, Jun 26 2025 at 14:11, André Almeida wrote:
>
>> Create ASSERT_{EQ, NE, TRUE, FALSE} macros to make test creation easier.
>
> What's so futex special about this that it can't use the same muck in
>
> tools/testing/selftests/kselftest_harness.h
>
My previous version of this test used kselftest_harness.h, but Shuah
request to keep consistency and don't use this header, giving that the
rest of futex test doesn't use it:
https://lore.kernel.org/lkml/fe02f42b-7ba8-4a3b-a86c-2a4a7942fd3b@linuxfoundation.org/
> or at least share the implementation in some way?
>
> Thanks,
>
> tglx
^ permalink raw reply
* Re: [PATCH RFT v17 4/8] fork: Add shadow stack support to clone3()
From: Catalin Marinas @ 2025-06-27 17:19 UTC (permalink / raw)
To: Mark Brown
Cc: 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,
linux-kernel, Will Deacon, jannh, Yury Khrustalev, Wilco Dijkstra,
linux-kselftest, linux-api, Kees Cook
In-Reply-To: <20250609-clone3-shadow-stack-v17-4-8840ed97ff6f@kernel.org>
On Mon, Jun 09, 2025 at 01:54:05PM +0100, Mark Brown wrote:
> +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;
> +
> + /* Ensure that a token written as a result of a pivot is visible */
> + gcsb_dsync();
> + gcspr_el0 = args->shadow_stack_token;
> + if (!gcs_consume_token(vma, page, gcspr_el0))
> + return -EINVAL;
> +
> + tsk->thread.gcspr_el0 = gcspr_el0 + sizeof(u64);
> +
> + /* Ensure that our token consumption visible */
> + gcsb_dsync();
> +
> + return ret;
> +}
What are the scenarios where we need the barriers? We have one via
map_shadow_stack() that would cover the first one. IIUC, GCSSS2 also
generates a GCSB event (or maybe I got it all wrong; I need to read the
spec).
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 1ee8eb11f38b..89c19996235d 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);
I think down the line, get_user_page_vma_remote() already does an
untagged_addr_remote(). But it does it after the vma look-up, so we
still need the untagging early.
That said, would we ever allowed a tagged pointer for the shadow stack?
> @@ -2840,6 +2891,27 @@ 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;
> +
> + /*
> + * The architecture must check support on the specific
> + * machine.
> + */
> + return IS_ENABLED(CONFIG_ARCH_HAS_USER_SHADOW_STACK);
I don't understand the comment here. It implies some kind of fallback
for further arch checks but it's just a return.
BTW, clone3_stack_valid() has an access_ok() check as well. Shall we add
it here? That's where the size would have come in handy but IIUC the
decision was to drop it (fine by me, just validate that the token is
accessible).
--
Catalin
^ permalink raw reply
* Re: [PATCH v5 7/7] selftests: futex: Expand robust list test for the new interface
From: Thomas Gleixner @ 2025-06-27 12:48 UTC (permalink / raw)
To: André Almeida, Ingo Molnar, Peter Zijlstra, Darren Hart,
Davidlohr Bueso, Shuah Khan, Arnd Bergmann,
Sebastian Andrzej Siewior, Waiman Long
Cc: linux-kernel, linux-kselftest, linux-api, kernel-dev,
André Almeida
In-Reply-To: <20250626-tonyk-robust_futex-v5-7-179194dbde8f@igalia.com>
On Thu, Jun 26 2025 at 14:11, André Almeida wrote:
> Expand the current robust list test for the new set_robust_list2
> syscall. Create an option to make it possible to run the same tests
> using the new syscall, and also add two new relevant test: test long
> lists (bigger than ROBUST_LIST_LIMIT) and for unaligned addresses.
>
> Signed-off-by: André Almeida <andrealmeid@igalia.com>
> ---
> .../selftests/futex/functional/robust_list.c | 160 ++++++++++++++++++++-
> 1 file changed, 156 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/futex/functional/robust_list.c b/tools/testing/selftests/futex/functional/robust_list.c
> index 42690b2440fd29a9b12c46f67f9645ccc93d1147..004ad79ff6171c411fd47e699e3c38889544218e 100644
> --- a/tools/testing/selftests/futex/functional/robust_list.c
> +++ b/tools/testing/selftests/futex/functional/robust_list.c
> @@ -35,16 +35,45 @@
> #include <stddef.h>
> #include <sys/mman.h>
> #include <sys/wait.h>
> +#include <stdint.h>
>
> #define STACK_SIZE (1024 * 1024)
>
> #define FUTEX_TIMEOUT 3
>
> +#define SYS_set_robust_list2 468
> +
> +enum robust_list2_type {
> + ROBUST_LIST_32BIT,
> + ROBUST_LIST_64BIT,
> +};
Why can't this use an updated header?
> +
> static pthread_barrier_t barrier, barrier2;
>
> +bool robust2 = false;
global because ....
> int set_robust_list(struct robust_list_head *head, size_t len)
> {
> - return syscall(SYS_set_robust_list, head, len);
> + int ret, flags;
> +
> + if (!robust2) {
> + return syscall(SYS_set_robust_list, head, len);
> + }
Pointless brackets.
> + if (sizeof(head) == 8)
> + flags = ROBUST_LIST_64BIT;
> + else
> + flags = ROBUST_LIST_32BIT;
> +
> + /*
> + * We act as we have just one list here. We try to use the first slot,
> + * but if it hasn't been alocated yet we allocate it.
> + */
> + ret = syscall(SYS_set_robust_list2, head, 0, flags);
> + if (ret == -1 && errno == ENOENT)
> + ret = syscall(SYS_set_robust_list2, head, -1, flags);
What the heck is this?
> + return ret;
> }
>
> int get_robust_list(int pid, struct robust_list_head **head, size_t *len_ptr)
> @@ -246,6 +275,11 @@ static void test_set_robust_list_invalid_size(void)
> size_t head_size = sizeof(struct robust_list_head);
> int ret;
>
> + if (robust2) {
> + ksft_test_result_skip("This test is only for old robust interface\n");
Why is it invoked in the first place?
> + return;
> + }
> +
> ret = set_robust_list(&head, head_size);
> ASSERT_EQ(ret, 0);
>
> @@ -321,6 +355,11 @@ static void test_get_robust_list_child(void)
> struct robust_list_head head, *get_head;
> size_t len_ptr;
>
> + if (robust2) {
> + ksft_test_result_skip("Not implemented in the new robust interface\n");
For the very wrong reasons.
> + return;
> + }
> +
> ret = pthread_barrier_init(&barrier, NULL, 2);
> ret = pthread_barrier_init(&barrier2, NULL, 2);
> ASSERT_EQ(ret, 0);
> @@ -332,7 +371,7 @@ static void test_get_robust_list_child(void)
>
> ret = get_robust_list(tid, &get_head, &len_ptr);
> ASSERT_EQ(ret, 0);
> - ASSERT_EQ(&head, get_head);
> + ASSERT_EQ(get_head, &head);
ROTFL
>
> pthread_barrier_wait(&barrier2);
>
> @@ -507,11 +546,119 @@ static void test_circular_list(void)
> ksft_test_result_pass("%s\n", __func__);
> }
>
> +#define ROBUST_LIST_LIMIT 2048
> +#define CHILD_LIST_LIMIT (ROBUST_LIST_LIMIT + 10)
> +
> +static int child_robust_list_limit(void *arg)
> +{
> + struct lock_struct *locks;
> + struct robust_list *list;
> + struct robust_list_head head;
> + int ret, i;
> +
> + locks = (struct lock_struct *) arg;
> +
> + ret = set_list(&head);
> + if (ret)
> + ksft_test_result_fail("set_list error\n");
Yet again the same broken crap.
> + /*
> + * Create a very long list of locks
> + */
> + head.list.next = &locks[0].list;
> +
> + list = head.list.next;
> + for (i = 0; i < CHILD_LIST_LIMIT - 1; i++) {
> + list->next = &locks[i+1].list;
> + list = list->next;
> + }
> + list->next = &head.list;
> +
> + /*
> + * Grab the lock in the last one, and die without releasing it
> + */
> + mutex_lock(&locks[CHILD_LIST_LIMIT], &head, false);
> + pthread_barrier_wait(&barrier);
> +
> + sleep(1);
> +
> + return 0;
> +}
> +
> +/*
> + * The old robust list used to have a limit of 2048 items from the kernel side.
> + * After this limit the kernel stops walking the list and ignore the other
ignores
> + * futexes, causing deadlocks.
> + *
> + * For the new interface, test if we can wait for a list of more than 2048
> + * elements.
> + */
> +static void test_robust_list_limit(void)
> +{
> + struct lock_struct locks[CHILD_LIST_LIMIT + 1];
> + _Atomic(unsigned int) *futex = &locks[CHILD_LIST_LIMIT].futex;
> + struct robust_list_head head;
> + int ret;
> +
> + if (!robust2) {
> + ksft_test_result_skip("This test is only for new robust interface\n");
> + return;
> + }
> +
> + *futex = 0;
> +
> + ret = set_list(&head);
> + ASSERT_EQ(ret, 0);
> +
> + ret = pthread_barrier_init(&barrier, NULL, 2);
> + ASSERT_EQ(ret, 0);
> +
> + create_child(child_robust_list_limit, locks);
> +
> + /*
> + * After the child thread creates the very long list of locks, wait on
> + * the last one.
> + */
> + pthread_barrier_wait(&barrier);
> + ret = mutex_lock(&locks[CHILD_LIST_LIMIT], &head, false);
> +
> + if (ret != 0)
> + printf("futex wait returned %d\n", errno);
> + ASSERT_EQ(ret, 0);
lalala.
> +
> + ASSERT_TRUE(*futex | FUTEX_OWNER_DIED);
Copy and pasta does not make it more correct.
> + wait(NULL);
> + pthread_barrier_destroy(&barrier);
> +
> + ksft_test_result_pass("%s\n", __func__);
> +}
> +
> +/*
> + * The kernel should refuse an unaligned head pointer
> + */
> +static void test_unaligned_address(void)
> +{
> + struct robust_list_head head, *h;
> + int ret;
> +
> + if (!robust2) {
> + ksft_test_result_skip("This test is only for new robust interface\n");
> + return;
> + }
> +
> + h = (struct robust_list_head *) ((uintptr_t) &head + 1);
> + ret = set_list(h);
> + ASSERT_EQ(ret, -1);
> + ASSERT_EQ(errno, EINVAL);
> +}
> +
> void usage(char *prog)
> {
> printf("Usage: %s\n", prog);
> printf(" -c Use color\n");
> printf(" -h Display this help message\n");
> + printf(" -n Use robust2 syscall\n");
Right. We need a command line option to guarantee that the test is not
executed by bots...
> printf(" -v L Verbosity level: %d=QUIET %d=CRITICAL %d=INFO\n",
> VQUIET, VCRITICAL, VINFO);
> }
> @@ -520,7 +667,7 @@ int main(int argc, char *argv[])
> {
> int c;
>
> - while ((c = getopt(argc, argv, "cht:v:")) != -1) {
> + while ((c = getopt(argc, argv, "chnt:v:")) != -1) {
> switch (c) {
> case 'c':
> log_color(1);
> @@ -531,6 +678,9 @@ int main(int argc, char *argv[])
> case 'v':
> log_verbosity(atoi(optarg));
> break;
> + case 'n':
> + robust2 = true;
> + break;
> default:
> usage(basename(argv[0]));
> exit(1);
> @@ -538,7 +688,7 @@ int main(int argc, char *argv[])
> }
>
> ksft_print_header();
> - ksft_set_plan(7);
> + ksft_set_plan(8);
>
Just check whether the new syscall is implemented and then set the
number of tests accordingly.
> test_robustness();
>
> @@ -548,6 +698,8 @@ int main(int argc, char *argv[])
> test_set_list_op_pending();
> test_robust_list_multiple_elements();
> test_circular_list();
> + test_robust_list_limit();
> + test_unaligned_address();
and then do:
test_robustness();
....
test_circular_list();
if (has_robust) {
robust2 = true;
test_robustness();
...
test_circular_list();
test_robust_list_limit();
test_unaligned_address();
}
or something like that.
Time for a stiff drink....
^ permalink raw reply
* Re: [PATCH v5 5/7] futex: Remove the limit of elements for sys_set_robust_list2 lists
From: Thomas Gleixner @ 2025-06-27 12:22 UTC (permalink / raw)
To: André Almeida, Ingo Molnar, Peter Zijlstra, Darren Hart,
Davidlohr Bueso, Shuah Khan, Arnd Bergmann,
Sebastian Andrzej Siewior, Waiman Long
Cc: linux-kernel, linux-kselftest, linux-api, kernel-dev,
André Almeida
In-Reply-To: <20250626-tonyk-robust_futex-v5-5-179194dbde8f@igalia.com>
On Thu, Jun 26 2025 at 14:11, André Almeida wrote:
> Remove the limit of ROBUST_LIST_LIMIT elements that a robust list can
> have, for the ones created with the new interface. This is done by
With which new interface?
> overwritten the list as it's proceeded in a way that we avoid circular
overwriting each processed list entry to point at ...., which eliminates
a potential circular list.
> lists.
>
> For the old interface, we keep the limited behavior to avoid changing
s/we//
> the API.
Which API would be violated?
Overwriting the dying tasks robust list entries is not violating any
ABI. The task's memory is on the way to be destroyed.
> Signed-off-by: André Almeida <andrealmeid@igalia.com>
> ---
> kernel/futex/core.c | 37 +++++++++++++++++++++++++++++--------
> 1 file changed, 29 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/futex/core.c b/kernel/futex/core.c
> index 1049f8ef3ce3c611b3be0ca12df34a98f710121d..942b66facdea16cd7be2235d95c2bbbae8d7cc63 100644
> --- a/kernel/futex/core.c
> +++ b/kernel/futex/core.c
> @@ -1152,7 +1152,8 @@ static inline int fetch_robust_entry(struct robust_list __user **entry,
> * We silently return on any sign of list-walking problem.
> */
> static void exit_robust_list64(struct task_struct *curr,
> - struct robust_list_head __user *head)
> + struct robust_list_head __user *head,
> + bool destroyable)
> {
> struct robust_list __user *entry, *next_entry, *pending;
> unsigned int limit = ROBUST_LIST_LIMIT, pi, pip;
> @@ -1196,13 +1197,17 @@ static void exit_robust_list64(struct task_struct *curr,
> }
> if (rc)
> return;
> - entry = next_entry;
> - pi = next_pi;
> +
> /*
> * Avoid excessively long or circular lists:
> */
> - if (!--limit)
> + if (!destroyable && !--limit)
> break;
> + else
> + put_user(&head->list, &entry->next);
Unchecked put_user() with zero explanation what it actually does.
> +
> + entry = next_entry;
> + pi = next_pi;
>
> cond_resched();
> }
> @@ -1214,7 +1219,8 @@ static void exit_robust_list64(struct task_struct *curr,
> }
> #else
> static void exit_robust_list64(struct task_struct *curr,
> - struct robust_list_head __user *head)
> + struct robust_list_head __user *head,
> + bool destroyable)
> {
> pr_warn("32bit kernel should not allow ROBUST_LIST_64BIT");
> }
> @@ -1252,7 +1258,8 @@ fetch_robust_entry32(u32 *uentry, struct robust_list __user **entry,
> * We silently return on any sign of list-walking problem.
> */
> static void exit_robust_list32(struct task_struct *curr,
> - struct robust_list_head32 __user *head)
> + struct robust_list_head32 __user *head,
> + bool destroyable)
> {
> struct robust_list __user *entry, *next_entry, *pending;
> unsigned int limit = ROBUST_LIST_LIMIT, pi, pip;
So this get's a destroyable argument as well, but no implementation?
> @@ -1474,10 +1481,19 @@ static void exit_pi_state_list(struct task_struct *curr)
> static inline void exit_pi_state_list(struct task_struct *curr) { }
> #endif
>
> +/*
> + * futex_cleanup - After the task exists, process the robust lists
> + *
> + * Walk through the linked list, parsing robust lists and freeing the
> + * allocated lists. Lists created with the set_robust_list2 don't have a limit
> + * for sizing and can be destroyed while we walk on it to avoid circular list.
> + */
> static void futex_cleanup(struct task_struct *tsk)
> {
> struct robust_list2_entry *curr, *n;
> struct list_head *list2 = &tsk->robust_list2;
> + bool destroyable = true;
> + int i = 0;
>
> /*
> * Walk through the linked list, parsing robust lists and freeing the
> @@ -1485,15 +1501,20 @@ static void futex_cleanup(struct task_struct *tsk)
> */
> if (unlikely(!list_empty(list2))) {
> list_for_each_entry_safe(curr, n, list2, list) {
> + destroyable = true;
> + if (tsk->robust_list_index == i)
> + destroyable = false;
Oh well.....
^ permalink raw reply
* Re: [PATCH v5 4/7] futex: Create set_robust_list2
From: Thomas Gleixner @ 2025-06-27 12:06 UTC (permalink / raw)
To: André Almeida, Ingo Molnar, Peter Zijlstra, Darren Hart,
Davidlohr Bueso, Shuah Khan, Arnd Bergmann,
Sebastian Andrzej Siewior, Waiman Long
Cc: linux-kernel, linux-kselftest, linux-api, kernel-dev,
André Almeida
In-Reply-To: <20250626-tonyk-robust_futex-v5-4-179194dbde8f@igalia.com>
On Thu, Jun 26 2025 at 14:11, André Almeida wrote:
> Create a new robust_list() syscall. The current syscall can't be
> expanded to cover the following use case, so a new one is needed. This
> new syscall allows users to set multiple robust lists per process and to
> have either 32bit or 64bit pointers in the list.
>
> * Interface
>
> This is the proposed interface:
What's proposed about this interface once it hits the tree? Just get rid
of this line completely. The paragraph decribes the interface
implemented by this patch, no?
If you want to make proposals, then do so in a RFC cover letter or
somewhere else.
> long set_robust_list2(void *head, int index, unsigned int flags)
>
> `head` is the head of the userspace struct robust_list_head, just as old
> set_robust_list(). It needs to be a void pointer since it can point to a
> normal robust_list_head or a compat_robust_list_head.
>
> `flags` can be used for defining the list type:
>
> enum robust_list_type {
> ROBUST_LIST_32BIT,
> ROBUST_LIST_64BIT,
> };
>
> `index` is the index in the internal robust_list's linked list (the
> naming starts to get confusing, I reckon).
Yes
> If `index == -1`, that means that user wants to set a new robust_list,
> and the kernel will append it in the end of the list, assign a new
> index and return this index to the user. If `index >= 0`, that means
> that user wants to re-set `*head` of an already existing list
> (similarly to what happens when you call set_robust_list() twice with
> different `*head`).
I'm lost in this word salad by now.
> If `index` is out of range, or it points to a non-existing robust_list,
> or if the internal list is full, an error is returned.
>
> Unaligned `head` addresses are refused by the kernel with -EINVAL.
>
> User cannot remove lists.
A registered list cannot be removed.
But wording aside, I see that a once created list entry cannot be
removed, but it can be set to NULL, which means it is invalidated, no?
> * Implementation
>
> The old syscall's set/get_robust_list() are converted to use the linked
s/old/original/
> list as well. When using only the old syscalls user shouldn't any
user space
also 'shouldn't any difference' does not qualify as a parseable
sentence.
Aside of that this patch doing five things at once is unreviewable.
> difference as the internal code will handle the linked list insertion as
> usual. When mixing old and new interfaces users should be aware that one
> of the elements of the list was created by another syscall and they
> should have special care handling this element index.
That's a guaranteed recipe for disaster.
> #ifdef CONFIG_FUTEX
> +/*
> + * This is an entry of a linked list of robust lists.
> + *
> + * @head: can point to a 64bit list or a 32bit list
> + * @list_type: determine the size of the futex pointers in the list
> + * @index: the index of this entry in the list
> + * @list: linked list element
> + */
> +struct robust_list2_entry {
> + void __user *head;
> + enum robust_list2_type list_type;
> + unsigned int index;
> + struct list_head list;
> +};
I'm quite sure that I pointed you to
https://www.kernel.org/doc/html/latest/process/maintainer-tip.html
before, no?
> +
> enum {
> FUTEX_STATE_OK,
> FUTEX_STATE_EXITING,
> @@ -74,10 +89,11 @@ enum {
>
> static inline void futex_init_task(struct task_struct *tsk)
> {
> - tsk->robust_list = NULL;
> + tsk->robust_list_index = -1;
Can we have proper defines for this instead of '-1'. NULL for a pointer
makes obviously sense as it is well defined, but '-1' is just a number
pulled out of thin air and has no meaning at all.
> #ifdef CONFIG_COMPAT
> - tsk->compat_robust_list = NULL;
> + tsk->compat_robust_list_index = -1;
> #endif
> diff --git a/include/uapi/linux/futex.h b/include/uapi/linux/futex.h
> index 7e2744ec89336a260e89883e95222eda199eeb7f..a8d8d4b1306ba56041046647bfb12146cf1ae4cb 100644
> --- a/include/uapi/linux/futex.h
> +++ b/include/uapi/linux/futex.h
> @@ -153,6 +153,16 @@ struct robust_list_head {
> struct robust_list __user *list_op_pending;
> };
>
> +#define ROBUST_LISTS_PER_TASK 10
> +
> +enum robust_list2_type {
> + ROBUST_LIST_32BIT,
> + ROBUST_LIST_64BIT,
> +};
> +
> +#define ROBUST_LIST_TYPE_MASK (ROBUST_LIST_32BIT | ROBUST_LIST_64BIT)
enum is a numerical value and ROBUST_LIST_32BIT is obviously 0.
Defining the mask out if these two values is surely creative and works
by chance, but seriously?
> +long do_set_robust_list2(struct robust_list_head __user *head,
> + int index, unsigned int type)
This expects a type defined in an named enum, So why is @type unsigned int?
> +{
> + struct list_head *list2 = ¤t->robust_list2;
> + struct robust_list2_entry *prev, *new = NULL;
> +
> + if (index == -1) {
> + if (list_empty(list2)) {
> + index = 0;
> + } else {
> + prev = list_last_entry(list2, struct robust_list2_entry, list);
> + index = prev->index + 1;
> + }
> +
> + if (index >= ROBUST_LISTS_PER_TASK)
> + return -EINVAL;
> +
> + new = kmalloc(sizeof(struct robust_list2_entry), GFP_KERNEL);
> + if (!new)
> + return -ENOMEM;
> +
> + list_add_tail(&new->list, list2);
> + new->index = index;
Right. Make the entry visible _before_ initializing it.
What's worse is that the list operation is done without any form of
serialization against a concurrent invocation of get_robust_list(),
which can operate on a task which is not current. IOW, you created an
inviting unpriviledged security hole.
Aside of that, this list magic is completely overengineered. Why?
- The amount of lists is limited to 10 for whatever reason as the
changelog is silent about it.
- The memory required for storing 10 robust list heads is 10 *
sizeof(unsigned long), i.e. 80 bytes on a 64bit machine
- The magic list head and the two index variables for the legacy
interfaces consume 20 bytes unconditionally in each task struct.
- Each allocated robust_list2_entry consumes 32 bytes of memory
So the first robust list setup amounts already to a total of 20 + 32 =
52 bytes, right? Any subsequent robust list installment exceeds 80 bytes
obviously.
This list handling is just overly complex for absolutely zero
value. Worse, it results in incomprehensible code and bugs like the
above.
It's not rocket science to make this non-obfuscated for both the user
side API and the kernel implementation. Something like this:
enum robust_list_cmd {
FUTEX_ROBUST_LIST_CMD_CREATE_64,
FUTEX_ROBUST_LIST_CMD_CREATE_32,
FUTEX_ROBUST_LIST_CMD_MODIFY_64,
FUTEX_ROBUST_LIST_CMD_MODIFY_32,
FUTEX_ROBUST_LIST_CMD_USER_MAX,
// Kernel internal, rejected for user space
FUTEX_ROBUST_LIST_SET_NATIVE = 128,
FUTEX_ROBUST_LIST_SET_COMPAT,
};
#define FUTEX_ROBUST_LIST_NATIVE_IDX 0
#define FUTEX_ROBUST_LIST_COMPAT_IDX 1
#define FUTEX_ROBUST_LIST2_IDX 2
#define FUTEX_ROBUST_LIST_MAX_IDX 8
#define FUTEX_ROBUST_LIST_ENTRY_INUSE 0x1UL
#define FUTEX_ROBUST_LIST_ENTRY_64BIT 0x2UL
#define FUTEX_ROBUST_LIST_ENTRY_MASK (~0x3UL)
static long futex_robust_list_create(uintptr_t head, enum robust_list_cmd cmd)
{
uintptr_t entry = FUTEX_ROBUST_LIST_ENTRY_VALID;
uintptr_t *rl = current->futex_robust_lists;
unsigned int index;
if (!rl) {
rl = kcalloc(ROBUST_LISTS_PER_TASK, sizeof(*rl), ....);
....
scoped_guard(mutex, ¤t->futex_exit_mutex)
current->futex_robust_lists = rl;
}
switch (cmd) {
case FUTEX_ROBUST_LIST_CREATE_64:
if (!IS_ENABLED(CONFIG_64BIT))
return -EINVAL;
entry |= FUTEX_ROBUST_LIST_ENTRY_64BIT;
fallthrough;
case FUTEX_ROBUST_LIST_CREATE_32:
// Search for a empty slot
for (index = FUTEX_ROBUST_LIST2_IDX; index < FUTEX_ROBUST_LIST_MAX_IDX; index++) {
if (!rl[index])
break;
}
return -ENOSPC;
case FUTEX_ROBUST_LIST_SET_NATIVE:
index = FUTEX_ROBUST_LIST_LEGACY_IDX;
if (IS_ENABLED(CONFIG_64BIT))
entry |= FUTEX_ROBUST_LIST_ENTRY_64BIT;
break;
case FUTEX_ROBUST_LIST_SET_COMPAT:
if (!IS_ENABLED(CONFIG_64BIT))
return -EINVAL;
index = FUTEX_ROBUST_LIST_COMPAT_IDX;
break;
default:
return -EINVAL;
}
entry |= head;
scoped_guard(mutex, ¤t->futex_exit_mutex)
rl[index] = entry;
return index;
}
static long futex_robust_list_modify(uintptr_t head, enum robust_list_cmd cmd, unsigned int index)
{
uintptr_t *rl = current->futex_robust_lists;
unsigned long entry;
if (index >= FUTEX_ROBUST_LIST_MAX_IDX)
return -EINVAL;
if (!rl || !rl[index])
return -ENOENT;
entry = rl[index];
switch (cmd) {
case FUTEX_ROBUST_LIST_MODIFY_32:
if (entry & FUTEX_ROBUST_LIST_ENTRY_64BIT)
return -EINVAL;
break;
case FUTEX_ROBUST_LIST_MODIFY_64:
if (!IS_ENABLED(CONFIG_64BIT))
return -EINVAL;
if (!(entry & FUTEX_ROBUST_LIST_ENTRY_64BIT))
return -EINVAL;
break;
default:
return -EINVAL;
}
entry &= FUTEX_ROBUST_LIST_ENTRY_MASK;
entry |= head;
scoped_guard(mutex, ¤t->futex_exit_mutex)
rl[index] = entry;
return index;
}
There is no confusion of index numbers and user space does not have to
care about a situation where an older libc uses the original syscall and
the application itself uses the new API. The two reserved entries are
just worth that.
The per task struct footprint is exactly one pointer for everything and
the allocation of 28 extra bytes for installing it once is definitely
not a problem at all.
get_robust_list() can safely access the array with the mutex held...
See?
> @@ -1414,25 +1476,28 @@ static inline void exit_pi_state_list(struct task_struct *curr) { }
>
> static void futex_cleanup(struct task_struct *tsk)
> {
> -#ifdef CONFIG_64BIT
> - if (unlikely(tsk->robust_list)) {
> - exit_robust_list64(tsk, tsk->robust_list);
> - tsk->robust_list = NULL;
> - }
> -#else
> - if (unlikely(tsk->robust_list)) {
> - exit_robust_list32(tsk,
> - (struct robust_list_head32 __user *) tsk->robust_list);
> - tsk->robust_list = NULL;
> - }
> -#endif
> + struct robust_list2_entry *curr, *n;
> + struct list_head *list2 = &tsk->robust_list2;
>
> -#ifdef CONFIG_COMPAT
> - if (unlikely(tsk->compat_robust_list)) {
> - exit_robust_list32(tsk, tsk->compat_robust_list);
> - tsk->compat_robust_list = NULL;
> + /*
> + * Walk through the linked list, parsing robust lists and freeing the
> + * allocated lists
> + */
> + if (unlikely(!list_empty(list2))) {
> + list_for_each_entry_safe(curr, n, list2, list) {
> + if (curr->head != NULL) {
> + if (curr->list_type == ROBUST_LIST_64BIT)
> + exit_robust_list64(tsk, curr->head);
> + else if (curr->list_type == ROBUST_LIST_32BIT)
> + exit_robust_list32(tsk, curr->head);
Unknown type is silently ignored?
> + curr->head = NULL;
> + }
> + list_del_init(&curr->list);
> + kfree(curr);
> + }
> }
> -#endif
> +
> + tsk->robust_list_index = -1;
Also why has this to be crammed into futex_cleanup()? Just because it
can and makes it more unreabable by deep indentation levels?
if (unlikely(tsk->futex_robust_lists))
exit_robust_lists(tsk);
and
static void exit_robust_lists(struct task_struct *tsk)
{
uintptr_t *rl = tsk->futex_robust_lists;
tsk->futex_robust_lists = NULL;
for (unsigned int idx = 0; idx < ROBUST_LISTS_PER_TASK; idx++) {
uintptr_t entry = rl[idx];
if (!(entry & FUTEX_ROBUST_LIST_ENTRY_MASK))
continue;
if (entry & FUTEX_ROBUST_LIST_ENTRY_32BIT) {
struct robust_list_head32 __user *head;
entry &= FUTEX_ROBUST_LIST_ENTRY_MASK;
head = (__force struct robust_list_head32 __user *)entry;
exit_robust_list32(tsk, head);
} else {
struct robust_list_head __user *head;
entry &= FUTEX_ROBUST_LIST_ENTRY_MASK;
head = (__force struct robust_list_head __user *)entry;
exit_robust_list64(tsk, head);
}
kfree(rl);
}
Is too readable, right?
> +#ifdef CONFIG_64BIT
> +static inline int robust_list_native_type(void)
> +{
> + return ROBUST_LIST_64BIT;
> +}
> +#else
> +static inline int robust_list_native_type(void)
> +{
> + return ROBUST_LIST_32BIT;
> +}
> +#endif
More ifdeffery is better
return IS_ENABLED(CONFIG_64BIT) ? ....;
> /**
> * sys_set_robust_list() - Set the robust-futex list head of a task
> * @head: pointer to the list-head
> @@ -28,17 +40,63 @@
> SYSCALL_DEFINE2(set_robust_list, struct robust_list_head __user *, head,
> size_t, len)
> {
> + unsigned int type = robust_list_native_type();
Right. enum returned from an inline as int and then assigned to an
unsigned int for consistency....
> + int ret;
> +
> /*
> * The kernel knows only one size for now:
> */
> if (unlikely(len != sizeof(*head)))
> return -EINVAL;
>
> - current->robust_list = head;
> + ret = do_set_robust_list2(head, current->robust_list_index, type);
> + if (ret < 0)
> + return ret;
> +
> + current->robust_list_index = ret;
return futex_robust_list_create((__force uintptr)head, FUTEX_ROBUST_LIST_SET_LEGACY);
> return 0;
> }
>
> +#define ROBUST_LIST_FLAGS ROBUST_LIST_TYPE_MASK
> +
> +/*
> + * sys_set_robust_list2()
> + *
> + * When index == -1, create a new list for user. When index >= 0, try to find
> + * the corresponding list and re-set the head there.
> + *
> + * Return values:
> + * >= 0: success, index of the robust list
> + * -EINVAL: invalid flags, invalid index
> + * -ENOENT: requested index no where to be found
> + * -ENOMEM: error allocating new list
> + * -ESRCH: too many allocated lists
-ESRCH really makes no sense. -ENOSPC is what you want, no?
Aside of that, the comment is not valid kernel doc format even if it
pretends to be,
> + */
> +SYSCALL_DEFINE3(set_robust_list2, struct robust_list_head __user *, head,
> + int, index, unsigned int, flags)
> +{
> + unsigned int type;
> +
> + type = flags & ROBUST_LIST_TYPE_MASK;
> +
> + if (index < -1 || index >= ROBUST_LISTS_PER_TASK)
> + return -EINVAL;
> +
> + if ((flags & ~ROBUST_LIST_FLAGS) != 0)
> + return -EINVAL;
> +
> + if (((uintptr_t) head % sizeof(u32)) != 0)
> + return -EINVAL;
A 64-bit robust list head should be 64-bit aligned, no?
> +#ifndef CONFIG_64BIT
Sigh.
> + if (type == ROBUST_LIST_64BIT)
> + return -EINVAL;
> +#endif
> +
> + return do_set_robust_list2(head, index, type);
uintptr entry = (__force uintptr_t)head;
unsigned int align = sizeof(u32);
switch (cmd) {
case FUTEX_ROBUST_LIST_CREATE_64:
if (!IS_ENABLED(CONFIG_64BIT))
return -ENOTSUPP;
align = sizeof(u64);
fallthrough;
case FUTEX_ROBUST_LIST_CREATE_32:
if (entry % align)
return -EINVAL;
return futex_robust_list_create(entry, cmd);
case FUTEX_ROBUST_LIST_MODIFY_64:
if (!IS_ENABLED(CONFIG_64BIT))
return -ENOTSUPP;
align = sizeof(u64);
fallthrough;
case FUTEX_ROBUST_LIST_MODIFY_32:
if (entry % align)
return -EINVAL;
return futex_robust_list_modify(entry, cmd, index);
default:
return -EINVAL;
Or something like that.
> +}
> +
> /**
> * sys_get_robust_list() - Get the robust-futex list head of a task
> * @pid: pid of the process [zero for current task]
> @@ -52,6 +110,7 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
> struct robust_list_head __user *head;
> unsigned long ret;
> struct task_struct *p;
> + int index;
>
> rcu_read_lock();
>
> @@ -68,9 +127,11 @@ SYSCALL_DEFINE3(get_robust_list, int, pid,
> if (!ptrace_may_access(p, PTRACE_MODE_READ_REALCREDS))
> goto err_unlock;
>
> - head = p->robust_list;
> + index = p->robust_list_index;
> rcu_read_unlock();
>
> + head = get_robust_list2(index, p);
Aside of the unprotected list walk, this adds a user exploitable exit
race on top. @p can go away immediately after rcu_read_unlock(), no?
Impressive...
With the above this becomes:
uintptr_t get_robust_list_at(pid_t pid, unsigned int idx)
{
struct task_struct *p;
uintptr_t entry;
scoped_guard(rcu) {
p = !pid ? current : find_task_by_vpid(pid);
if (!p)
return -ESRCH;
if (!ptrace_may_access(p, PTRACE_MODE_READ_REALCREDS))
return -EPERM;
get_task_struct(p);
}
scoped_guard(mutex, &p->futex_exit_mutex) {
uintptr_t *rl = p->futex_robust_lists;
entry = rl ? rl[idx] : NULL;
entry &= FUTEX_ROBUST_LIST_ENTRY_MASK;
}
put_task_struct(p);
return entry;
get_robust_list()
{
uintptr_t entry = get_robust_list_at(pid, FUTEX_ROBUST_LIST_NATIVE_IDX);
struct robust_list_head __user *head;
head = (__force struct robust_list_head __user *)entry;
if (put_user(sizeof(*head), len_ptr))
return -EFAULT;
return put_user(head, head_ptr);
}
Or something along those lines. The scoped_guard(rcu) cleanup want's to
be in a preparatory patch.
Also why does this new syscall come without a corresponding
sys_get_robust_list2() implementation? CRIU definitely depends on
get_robust_list(), so it will need get_robust_list2() as well...
> @@ -458,6 +528,7 @@ COMPAT_SYSCALL_DEFINE3(get_robust_list, int, pid,
> struct robust_list_head32 __user *head;
> unsigned long ret;
> struct task_struct *p;
> + int index;
>
> rcu_read_lock();
>
> @@ -474,9 +545,11 @@ COMPAT_SYSCALL_DEFINE3(get_robust_list, int, pid,
> if (!ptrace_may_access(p, PTRACE_MODE_READ_REALCREDS))
> goto err_unlock;
>
> - head = p->compat_robust_list;
> + index = p->compat_robust_list_index;
> rcu_read_unlock();
>
> + head = (struct robust_list_head32 __user *) get_robust_list2(index, p);
> +
See above....
This all wants to be properly split up into:
1) Preparatory cleanups
2) Implementation of the new mechanism without touching the existing
syscalls.
3) Conversion of the existing syscalls
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH v3 1/3] Wire up lsm_config_self_policy and lsm_config_system_policy syscalls
From: kernel test robot @ 2025-06-27 6:31 UTC (permalink / raw)
To: Maxime Bélair, linux-security-module
Cc: oe-kbuild-all, john.johansen, paul, jmorris, serge, mic, kees,
stephen.smalley.work, casey, takedakn, penguin-kernel, song,
rdunlap, linux-api, apparmor, linux-kernel, Maxime Bélair
In-Reply-To: <20250624143211.436045-2-maxime.belair@canonical.com>
Hi Maxime,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 9c32cda43eb78f78c73aee4aa344b777714e259b]
url: https://github.com/intel-lab-lkp/linux/commits/Maxime-B-lair/Wire-up-lsm_config_self_policy-and-lsm_config_system_policy-syscalls/20250624-225008
base: 9c32cda43eb78f78c73aee4aa344b777714e259b
patch link: https://lore.kernel.org/r/20250624143211.436045-2-maxime.belair%40canonical.com
patch subject: [PATCH v3 1/3] Wire up lsm_config_self_policy and lsm_config_system_policy syscalls
config: riscv-randconfig-001-20250627 (https://download.01.org/0day-ci/archive/20250627/202506271432.Hsg0FzKQ-lkp@intel.com/config)
compiler: riscv32-linux-gcc (GCC) 8.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250627/202506271432.Hsg0FzKQ-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506271432.Hsg0FzKQ-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from include/linux/compat.h:34,
from arch/riscv/include/asm/elf.h:12,
from include/linux/elf.h:6,
from include/linux/module.h:19,
from include/linux/bpf.h:20,
from include/linux/security.h:35,
from security/lsm_syscalls.c:13:
arch/riscv/include/asm/syscall_wrapper.h:35:14: warning: '__se_sys_lsm_set_self_attr' alias between functions of incompatible types 'long int(ulong, ulong, ulong, ulong, ulong, ulong, ulong)' {aka 'long int(long unsigned int, long unsigned int, long unsigned int, long unsigned int, long unsigned int, long unsigned int, long unsigned int)'} and 'long int(long int, long int, long int, long int)' [-Wattribute-alias]
static long __se_##prefix##name(ulong, ulong, ulong, ulong, ulong, ulong, \
^~~~~
arch/riscv/include/asm/syscall_wrapper.h:82:2: note: in expansion of macro '__SYSCALL_SE_DEFINEx'
__SYSCALL_SE_DEFINEx(x, sys, name, __VA_ARGS__) \
^~~~~~~~~~~~~~~~~~~~
include/linux/syscalls.h:235:2: note: in expansion of macro '__SYSCALL_DEFINEx'
__SYSCALL_DEFINEx(x, sname, __VA_ARGS__)
^~~~~~~~~~~~~~~~~
include/linux/syscalls.h:227:36: note: in expansion of macro 'SYSCALL_DEFINEx'
#define SYSCALL_DEFINE4(name, ...) SYSCALL_DEFINEx(4, _##name, __VA_ARGS__)
^~~~~~~~~~~~~~~
security/lsm_syscalls.c:55:1: note: in expansion of macro 'SYSCALL_DEFINE4'
SYSCALL_DEFINE4(lsm_set_self_attr, unsigned int, attr, struct lsm_ctx __user *,
^~~~~~~~~~~~~~~
arch/riscv/include/asm/syscall_wrapper.h:41:14: note: aliased declaration here
static long ___se_##prefix##name(__MAP(x,__SC_LONG,__VA_ARGS__))
^~~~~~
arch/riscv/include/asm/syscall_wrapper.h:82:2: note: in expansion of macro '__SYSCALL_SE_DEFINEx'
__SYSCALL_SE_DEFINEx(x, sys, name, __VA_ARGS__) \
^~~~~~~~~~~~~~~~~~~~
include/linux/syscalls.h:235:2: note: in expansion of macro '__SYSCALL_DEFINEx'
__SYSCALL_DEFINEx(x, sname, __VA_ARGS__)
^~~~~~~~~~~~~~~~~
include/linux/syscalls.h:227:36: note: in expansion of macro 'SYSCALL_DEFINEx'
#define SYSCALL_DEFINE4(name, ...) SYSCALL_DEFINEx(4, _##name, __VA_ARGS__)
^~~~~~~~~~~~~~~
security/lsm_syscalls.c:55:1: note: in expansion of macro 'SYSCALL_DEFINE4'
SYSCALL_DEFINE4(lsm_set_self_attr, unsigned int, attr, struct lsm_ctx __user *,
^~~~~~~~~~~~~~~
>> arch/riscv/include/asm/syscall_wrapper.h:35:14: warning: '__se_sys_lsm_config_system_policy' alias between functions of incompatible types 'long int(ulong, ulong, ulong, ulong, ulong, ulong, ulong)' {aka 'long int(long unsigned int, long unsigned int, long unsigned int, long unsigned int, long unsigned int, long unsigned int, long unsigned int)'} and 'long int(long int, long int, long int, long int, long int)' [-Wattribute-alias]
static long __se_##prefix##name(ulong, ulong, ulong, ulong, ulong, ulong, \
^~~~~
arch/riscv/include/asm/syscall_wrapper.h:82:2: note: in expansion of macro '__SYSCALL_SE_DEFINEx'
__SYSCALL_SE_DEFINEx(x, sys, name, __VA_ARGS__) \
^~~~~~~~~~~~~~~~~~~~
include/linux/syscalls.h:235:2: note: in expansion of macro '__SYSCALL_DEFINEx'
__SYSCALL_DEFINEx(x, sname, __VA_ARGS__)
^~~~~~~~~~~~~~~~~
include/linux/syscalls.h:228:36: note: in expansion of macro 'SYSCALL_DEFINEx'
#define SYSCALL_DEFINE5(name, ...) SYSCALL_DEFINEx(5, _##name, __VA_ARGS__)
^~~~~~~~~~~~~~~
security/lsm_syscalls.c:128:1: note: in expansion of macro 'SYSCALL_DEFINE5'
SYSCALL_DEFINE5(lsm_config_system_policy, u32, lsm_id, u32, op, void __user *,
^~~~~~~~~~~~~~~
arch/riscv/include/asm/syscall_wrapper.h:41:14: note: aliased declaration here
static long ___se_##prefix##name(__MAP(x,__SC_LONG,__VA_ARGS__))
^~~~~~
arch/riscv/include/asm/syscall_wrapper.h:82:2: note: in expansion of macro '__SYSCALL_SE_DEFINEx'
__SYSCALL_SE_DEFINEx(x, sys, name, __VA_ARGS__) \
^~~~~~~~~~~~~~~~~~~~
include/linux/syscalls.h:235:2: note: in expansion of macro '__SYSCALL_DEFINEx'
__SYSCALL_DEFINEx(x, sname, __VA_ARGS__)
^~~~~~~~~~~~~~~~~
include/linux/syscalls.h:228:36: note: in expansion of macro 'SYSCALL_DEFINEx'
#define SYSCALL_DEFINE5(name, ...) SYSCALL_DEFINEx(5, _##name, __VA_ARGS__)
^~~~~~~~~~~~~~~
security/lsm_syscalls.c:128:1: note: in expansion of macro 'SYSCALL_DEFINE5'
SYSCALL_DEFINE5(lsm_config_system_policy, u32, lsm_id, u32, op, void __user *,
^~~~~~~~~~~~~~~
>> arch/riscv/include/asm/syscall_wrapper.h:35:14: warning: '__se_sys_lsm_config_self_policy' alias between functions of incompatible types 'long int(ulong, ulong, ulong, ulong, ulong, ulong, ulong)' {aka 'long int(long unsigned int, long unsigned int, long unsigned int, long unsigned int, long unsigned int, long unsigned int, long unsigned int)'} and 'long int(long int, long int, long int, long int, long int)' [-Wattribute-alias]
static long __se_##prefix##name(ulong, ulong, ulong, ulong, ulong, ulong, \
^~~~~
arch/riscv/include/asm/syscall_wrapper.h:82:2: note: in expansion of macro '__SYSCALL_SE_DEFINEx'
__SYSCALL_SE_DEFINEx(x, sys, name, __VA_ARGS__) \
^~~~~~~~~~~~~~~~~~~~
include/linux/syscalls.h:235:2: note: in expansion of macro '__SYSCALL_DEFINEx'
__SYSCALL_DEFINEx(x, sname, __VA_ARGS__)
^~~~~~~~~~~~~~~~~
include/linux/syscalls.h:228:36: note: in expansion of macro 'SYSCALL_DEFINEx'
#define SYSCALL_DEFINE5(name, ...) SYSCALL_DEFINEx(5, _##name, __VA_ARGS__)
^~~~~~~~~~~~~~~
security/lsm_syscalls.c:122:1: note: in expansion of macro 'SYSCALL_DEFINE5'
SYSCALL_DEFINE5(lsm_config_self_policy, u32, lsm_id, u32, op, void __user *,
^~~~~~~~~~~~~~~
arch/riscv/include/asm/syscall_wrapper.h:41:14: note: aliased declaration here
static long ___se_##prefix##name(__MAP(x,__SC_LONG,__VA_ARGS__))
^~~~~~
arch/riscv/include/asm/syscall_wrapper.h:82:2: note: in expansion of macro '__SYSCALL_SE_DEFINEx'
__SYSCALL_SE_DEFINEx(x, sys, name, __VA_ARGS__) \
^~~~~~~~~~~~~~~~~~~~
include/linux/syscalls.h:235:2: note: in expansion of macro '__SYSCALL_DEFINEx'
__SYSCALL_DEFINEx(x, sname, __VA_ARGS__)
^~~~~~~~~~~~~~~~~
include/linux/syscalls.h:228:36: note: in expansion of macro 'SYSCALL_DEFINEx'
#define SYSCALL_DEFINE5(name, ...) SYSCALL_DEFINEx(5, _##name, __VA_ARGS__)
^~~~~~~~~~~~~~~
security/lsm_syscalls.c:122:1: note: in expansion of macro 'SYSCALL_DEFINE5'
SYSCALL_DEFINE5(lsm_config_self_policy, u32, lsm_id, u32, op, void __user *,
^~~~~~~~~~~~~~~
arch/riscv/include/asm/syscall_wrapper.h:35:14: warning: '__se_sys_lsm_list_modules' alias between functions of incompatible types 'long int(ulong, ulong, ulong, ulong, ulong, ulong, ulong)' {aka 'long int(long unsigned int, long unsigned int, long unsigned int, long unsigned int, long unsigned int, long unsigned int, long unsigned int)'} and 'long int(long int, long int, long int)' [-Wattribute-alias]
static long __se_##prefix##name(ulong, ulong, ulong, ulong, ulong, ulong, \
^~~~~
arch/riscv/include/asm/syscall_wrapper.h:82:2: note: in expansion of macro '__SYSCALL_SE_DEFINEx'
__SYSCALL_SE_DEFINEx(x, sys, name, __VA_ARGS__) \
^~~~~~~~~~~~~~~~~~~~
include/linux/syscalls.h:235:2: note: in expansion of macro '__SYSCALL_DEFINEx'
__SYSCALL_DEFINEx(x, sname, __VA_ARGS__)
^~~~~~~~~~~~~~~~~
include/linux/syscalls.h:226:36: note: in expansion of macro 'SYSCALL_DEFINEx'
#define SYSCALL_DEFINE3(name, ...) SYSCALL_DEFINEx(3, _##name, __VA_ARGS__)
^~~~~~~~~~~~~~~
security/lsm_syscalls.c:96:1: note: in expansion of macro 'SYSCALL_DEFINE3'
SYSCALL_DEFINE3(lsm_list_modules, u64 __user *, ids, u32 __user *, size,
^~~~~~~~~~~~~~~
arch/riscv/include/asm/syscall_wrapper.h:41:14: note: aliased declaration here
static long ___se_##prefix##name(__MAP(x,__SC_LONG,__VA_ARGS__))
^~~~~~
arch/riscv/include/asm/syscall_wrapper.h:82:2: note: in expansion of macro '__SYSCALL_SE_DEFINEx'
__SYSCALL_SE_DEFINEx(x, sys, name, __VA_ARGS__) \
^~~~~~~~~~~~~~~~~~~~
include/linux/syscalls.h:235:2: note: in expansion of macro '__SYSCALL_DEFINEx'
__SYSCALL_DEFINEx(x, sname, __VA_ARGS__)
^~~~~~~~~~~~~~~~~
include/linux/syscalls.h:226:36: note: in expansion of macro 'SYSCALL_DEFINEx'
#define SYSCALL_DEFINE3(name, ...) SYSCALL_DEFINEx(3, _##name, __VA_ARGS__)
^~~~~~~~~~~~~~~
security/lsm_syscalls.c:96:1: note: in expansion of macro 'SYSCALL_DEFINE3'
SYSCALL_DEFINE3(lsm_list_modules, u64 __user *, ids, u32 __user *, size,
^~~~~~~~~~~~~~~
arch/riscv/include/asm/syscall_wrapper.h:35:14: warning: '__se_sys_lsm_get_self_attr' alias between functions of incompatible types 'long int(ulong, ulong, ulong, ulong, ulong, ulong, ulong)' {aka 'long int(long unsigned int, long unsigned int, long unsigned int, long unsigned int, long unsigned int, long unsigned int, long unsigned int)'} and 'long int(long int, long int, long int, long int)' [-Wattribute-alias]
static long __se_##prefix##name(ulong, ulong, ulong, ulong, ulong, ulong, \
^~~~~
arch/riscv/include/asm/syscall_wrapper.h:82:2: note: in expansion of macro '__SYSCALL_SE_DEFINEx'
__SYSCALL_SE_DEFINEx(x, sys, name, __VA_ARGS__) \
^~~~~~~~~~~~~~~~~~~~
include/linux/syscalls.h:235:2: note: in expansion of macro '__SYSCALL_DEFINEx'
__SYSCALL_DEFINEx(x, sname, __VA_ARGS__)
^~~~~~~~~~~~~~~~~
include/linux/syscalls.h:227:36: note: in expansion of macro 'SYSCALL_DEFINEx'
#define SYSCALL_DEFINE4(name, ...) SYSCALL_DEFINEx(4, _##name, __VA_ARGS__)
^~~~~~~~~~~~~~~
security/lsm_syscalls.c:77:1: note: in expansion of macro 'SYSCALL_DEFINE4'
SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct lsm_ctx __user *,
^~~~~~~~~~~~~~~
arch/riscv/include/asm/syscall_wrapper.h:41:14: note: aliased declaration here
static long ___se_##prefix##name(__MAP(x,__SC_LONG,__VA_ARGS__))
^~~~~~
arch/riscv/include/asm/syscall_wrapper.h:82:2: note: in expansion of macro '__SYSCALL_SE_DEFINEx'
__SYSCALL_SE_DEFINEx(x, sys, name, __VA_ARGS__) \
^~~~~~~~~~~~~~~~~~~~
include/linux/syscalls.h:235:2: note: in expansion of macro '__SYSCALL_DEFINEx'
__SYSCALL_DEFINEx(x, sname, __VA_ARGS__)
^~~~~~~~~~~~~~~~~
include/linux/syscalls.h:227:36: note: in expansion of macro 'SYSCALL_DEFINEx'
#define SYSCALL_DEFINE4(name, ...) SYSCALL_DEFINEx(4, _##name, __VA_ARGS__)
^~~~~~~~~~~~~~~
security/lsm_syscalls.c:77:1: note: in expansion of macro 'SYSCALL_DEFINE4'
SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct lsm_ctx __user *,
^~~~~~~~~~~~~~~
vim +35 arch/riscv/include/asm/syscall_wrapper.h
a9ad73295cc1e3 Sami Tolvanen 2024-03-11 16
a9ad73295cc1e3 Sami Tolvanen 2024-03-11 17 #define __SYSCALL_SE_DEFINEx(x, prefix, name, ...) \
a9ad73295cc1e3 Sami Tolvanen 2024-03-11 18 static long __se_##prefix##name(__MAP(x,__SC_LONG,__VA_ARGS__)); \
a9ad73295cc1e3 Sami Tolvanen 2024-03-11 19 static long __se_##prefix##name(__MAP(x,__SC_LONG,__VA_ARGS__))
a9ad73295cc1e3 Sami Tolvanen 2024-03-11 20
08d0ce30e0e4fc Sami Tolvanen 2023-07-10 21 #define SC_RISCV_REGS_TO_ARGS(x, ...) \
08d0ce30e0e4fc Sami Tolvanen 2023-07-10 22 __MAP(x,__SC_ARGS \
08d0ce30e0e4fc Sami Tolvanen 2023-07-10 23 ,,regs->orig_a0,,regs->a1,,regs->a2 \
08d0ce30e0e4fc Sami Tolvanen 2023-07-10 24 ,,regs->a3,,regs->a4,,regs->a5,,regs->a6)
08d0ce30e0e4fc Sami Tolvanen 2023-07-10 25
a9ad73295cc1e3 Sami Tolvanen 2024-03-11 26 #else
a9ad73295cc1e3 Sami Tolvanen 2024-03-11 27 /*
a9ad73295cc1e3 Sami Tolvanen 2024-03-11 28 * Use type aliasing to ensure registers a0-a6 are correctly passed to the syscall
a9ad73295cc1e3 Sami Tolvanen 2024-03-11 29 * implementation when >word-size arguments are used.
a9ad73295cc1e3 Sami Tolvanen 2024-03-11 30 */
a9ad73295cc1e3 Sami Tolvanen 2024-03-11 31 #define __SYSCALL_SE_DEFINEx(x, prefix, name, ...) \
a9ad73295cc1e3 Sami Tolvanen 2024-03-11 32 __diag_push(); \
a9ad73295cc1e3 Sami Tolvanen 2024-03-11 33 __diag_ignore(GCC, 8, "-Wattribute-alias", \
a9ad73295cc1e3 Sami Tolvanen 2024-03-11 34 "Type aliasing is used to sanitize syscall arguments"); \
a9ad73295cc1e3 Sami Tolvanen 2024-03-11 @35 static long __se_##prefix##name(ulong, ulong, ulong, ulong, ulong, ulong, \
a9ad73295cc1e3 Sami Tolvanen 2024-03-11 36 ulong) \
a9ad73295cc1e3 Sami Tolvanen 2024-03-11 37 __attribute__((alias(__stringify(___se_##prefix##name)))); \
a9ad73295cc1e3 Sami Tolvanen 2024-03-11 38 __diag_pop(); \
653650c468be21 Sami Tolvanen 2024-03-26 39 static long noinline ___se_##prefix##name(__MAP(x,__SC_LONG,__VA_ARGS__)) \
653650c468be21 Sami Tolvanen 2024-03-26 40 __used; \
a9ad73295cc1e3 Sami Tolvanen 2024-03-11 41 static long ___se_##prefix##name(__MAP(x,__SC_LONG,__VA_ARGS__))
a9ad73295cc1e3 Sami Tolvanen 2024-03-11 42
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [RFC 00/19] Kernel API Specification Framework
From: Dmitry Vyukov @ 2025-06-27 6:23 UTC (permalink / raw)
To: Sasha Levin; +Cc: kees, elver, linux-api, linux-kernel, tools, workflows
In-Reply-To: <aF10Cu0GI092Sjr3@lappy>
On Thu, 26 Jun 2025 at 18:23, Sasha Levin <sashal@kernel.org> wrote:
>
> On Thu, Jun 26, 2025 at 10:37:33AM +0200, Dmitry Vyukov wrote:
> >On Thu, 26 Jun 2025 at 10:32, Dmitry Vyukov <dvyukov@google.com> wrote:
> >>
> >> On Wed, 25 Jun 2025 at 17:55, Sasha Levin <sashal@kernel.org> wrote:
> >> >
> >> > On Wed, Jun 25, 2025 at 10:52:46AM +0200, Dmitry Vyukov wrote:
> >> > >On Tue, 24 Jun 2025 at 22:04, Sasha Levin <sashal@kernel.org> wrote:
> >> > >
> >> > >> >6. What's the goal of validation of the input arguments?
> >> > >> >Kernel code must do this validation anyway, right.
> >> > >> >Any non-trivial validation is hard, e.g. even for open the validation function
> >> > >> >for file name would need to have access to flags and check file precense for
> >> > >> >some flags combinations. That may add significant amount of non-trivial code
> >> > >> >that duplicates main syscall logic, and that logic may also have bugs and
> >> > >> >memory leaks.
> >> > >>
> >> > >> Mostly to catch divergence from the spec: think of a scenario where
> >> > >> someone added a new param/flag/etc but forgot to update the spec - this
> >> > >> will help catch it.
> >> > >
> >> > >How exactly is this supposed to work?
> >> > >Even if we run with a unit test suite, a test suite may include some
> >> > >incorrect inputs to check for error conditions. The framework will
> >> > >report violations on these incorrect inputs. These are not bugs in the
> >> > >API specifications, nor in the test suite (read false positives).
> >> >
> >> > Right now it would be something along the lines of the test checking for
> >> > an expected failure message in dmesg, something along the lines of:
> >> >
> >> > https://github.com/linux-test-project/ltp/blob/0c99c7915f029d32de893b15b0a213ff3de210af/testcases/commands/sysctl/sysctl02.sh#L67
> >> >
> >> > I'm not opposed to coming up with a better story...
> >
> >If the goal of validation is just indirectly validating correctness of
> >the specification itself, then I would look for other ways of
> >validating correctness of the spec.
> >Either removing duplication between specification and actual code
> >(i.e. generating it from SYSCALL_DEFINE, or the other way around) ,
> >then spec is correct by construction. Or, cross-validating it with
> >info automatically extracted from the source (using
> >clang/dwarf/pahole).
> >This would be more scalable (O(1) work, rather than thousands more
> >manually written tests).
> >
> >> Oh, you mean special tests for this framework (rather than existing tests).
> >> I don't think this is going to work in practice. Besides writing all
> >> these specifications, we will also need to write dozens of tests per
> >> each specification (e.g. for each fd arg one needs at least 3 tests:
> >> -1, valid fd, inclid fd; an enum may need 5 various inputs of
> >> something; let alone netlink specifications).
>
> I didn't mean just for the framework: being able to specify the APIs in
> machine readable format will enable us to automatically generate
> exhaustive tests for each such API.
>
> I've been playing with the kapi tool (see last patch) which already
> supports different formatters. Right now it outputs human readable
> output, but I have proof-of-concept code that outputs testcases for
> specced APIs.
>
> The dream here is to be able to automatically generate
> hundreds/thousands of tests for each API in an automated fashion, and
> verify the results with:
>
> 1. Simply checking expected return value.
>
> 2. Checking that the actual action happened (i.e. we called close(fd),
> verify that `fd` is really closed).
>
> 3. Check for side effects (i.e. close(fd) isn't supposed to allocate
> memory - verify that it didn't allocate memory).
>
> 4. Code coverage: our tests are supposed to cover 100% of the code in
> that APIs call chain, do we have code that didn't run (missing/incorrect
> specs).
This is all good. I was asking the argument verification part of the
framework. Is it required for any of this? How?
^ permalink raw reply
* Re: [PATCH v5 3/7] futex: Use explicit sizes for compat_exit_robust_list
From: Thomas Gleixner @ 2025-06-26 22:56 UTC (permalink / raw)
To: André Almeida, Ingo Molnar, Peter Zijlstra, Darren Hart,
Davidlohr Bueso, Shuah Khan, Arnd Bergmann,
Sebastian Andrzej Siewior, Waiman Long
Cc: linux-kernel, linux-kselftest, linux-api, kernel-dev,
André Almeida
In-Reply-To: <20250626-tonyk-robust_futex-v5-3-179194dbde8f@igalia.com>
On Thu, Jun 26 2025 at 14:11, André Almeida wrote:
$subject lacks a () function notation ....
> There are two functions for handling robust lists during the task
during a tasks exit
> exit: exit_robust_list() and compat_exit_robust_list(). The first one
> handles either 64bit or 32bit lists, depending if it's a 64bit or 32bit
> kernel. The compat_exit_robust_list() only exists in 64bit kernels that
s/The//
> supports 32bit syscalls, and handles 32bit lists.
32-bit 64-bit all over the place
> For the new syscall set_robust_list2(), 64bit kernels need to be able to
> handle 32bit lists despite having or not support for 32bit syscalls, so
> make compat_exit_robust_list() exist regardless of compat_ config.
What new syscall and what are the requirements here? You really want to
add some rationale and background here.
> Also, use explicitly sizing, otherwise in a 32bit kernel both
> exit_robust_list() and compat_exit_robust_list() would be the exactly
> same function, with none of them dealing with 64bit robust lists.
Explicit sizing of what? The changelog should give information which
allows me to verify the implementation and not some blurb which makes me
to oracle the meaning of the changelog out of the actual implementation.
What is the actual gist of this patch? The subject says:
Use explicit sizes for compat_exit_robust_list
Now you say 'Also,' which means aside of the above actual statement to
make compat_exit_robust_list() unconditional this is now a side effect
or what?
The subject line is misleading because the real purpose of this patch is
to make compat_exit_robust_list() unconditionally available independent
of bitness.
Now the obvious question is why this patch isn't split into two pieces:
1) The patch matching the above subject line and does the
struct/argument rename
2) A subsequent patch which makes the function unconditionally
available
That's not done because obfuscating changes makes everyones life easier,
right?
> +++ b/include/linux/compat.h
> @@ -385,16 +385,6 @@ struct compat_ifconf {
> compat_caddr_t ifcbuf;
> };
>
> -struct compat_robust_list {
> - compat_uptr_t next;
> -};
> -
> -struct compat_robust_list_head {
> - struct compat_robust_list list;
> - compat_long_t futex_offset;
> - compat_uptr_t list_op_pending;
> -};
> -
> #ifdef CONFIG_COMPAT_OLD_SIGACTION
> struct compat_old_sigaction {
> compat_uptr_t sa_handler;
> @@ -672,7 +662,7 @@ asmlinkage long compat_sys_waitid(int, compat_pid_t,
> struct compat_siginfo __user *, int,
> struct compat_rusage __user *);
> asmlinkage long
> -compat_sys_set_robust_list(struct compat_robust_list_head __user *head,
> +compat_sys_set_robust_list(struct robust_list_head32 __user *head,
> compat_size_t len);
How does this even survive a full kernel build without a forward
declaration of struct robust_list_head32?
Not everything which includes compat.h includes futex.h first. There is
a reason why the structs were define here. Sure you can move them, but
not without a forward declaration.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH v5 2/7] selftests/futex: Create test for robust list
From: Thomas Gleixner @ 2025-06-26 22:36 UTC (permalink / raw)
To: André Almeida, Ingo Molnar, Peter Zijlstra, Darren Hart,
Davidlohr Bueso, Shuah Khan, Arnd Bergmann,
Sebastian Andrzej Siewior, Waiman Long
Cc: linux-kernel, linux-kselftest, linux-api, kernel-dev,
André Almeida
In-Reply-To: <20250626-tonyk-robust_futex-v5-2-179194dbde8f@igalia.com>
On Thu, Jun 26 2025 at 14:11, André Almeida wrote:
> +
> +int set_robust_list(struct robust_list_head *head, size_t len)
This function and the get() counterpart are global because they can?
> +{
> + return syscall(SYS_set_robust_list, head, len);
> +}
> +/*
> + * Basic lock struct, contains just the futex word and the robust list element
> + * Real implementations have also a *prev to easily walk in the list
> + */
> +struct lock_struct {
> + _Atomic(unsigned int) futex;
> + struct robust_list list;
tabular arrangement please.
> + pthread_barrier_wait(&barrier);
> +
> + /*
> + * There's a race here: the parent thread needs to be inside
> + * futex_wait() before the child thread dies, otherwise it will miss the
> + * wakeup from handle_futex_death() that this child will emit. We wait a
> + * little bit just to make sure that this happens.
> + */
> + sleep(1);
One second is quite a little bit. :)
> + /*
> + * futex_wait() should return 0 and the futex word should be marked with
> + * FUTEX_OWNER_DIED
> + */
> + ASSERT_EQ(ret, 0);
> + if (ret != 0)
> + printf("futex wait returned %d", errno);
What's the purpose of the extra printf() after the assert here? This
code is not even reached when ret != 0, no?
> + ASSERT_TRUE(*futex | FUTEX_OWNER_DIED);
That's always true no matter what the content of the futex variable is, no?
> +/*
> + * The only valid value for len is sizeof(*head)
> + */
> +static void test_set_robust_list_invalid_size(void)
> +{
> + struct robust_list_head head;
> + size_t head_size = sizeof(struct robust_list_head);
Groan. You already define the robust_list_head variable ahead of
head_size and violate the reverse fir tree ordering, so why don't you
use the obvious and actually robust 'sizeof(head)'?
> +/*
> + * Test get_robust_list with pid = 0, getting the list of the running thread
> + */
> +static void test_get_robust_list_self(void)
> +{
> + struct robust_list_head head, head2, *get_head;
> + size_t head_size = sizeof(struct robust_list_head), len_ptr;
Ditto.
> +static int child_list(void *arg)
> +{
> + struct robust_list_head *head = (struct robust_list_head *) arg;
void pointers really don't require type casts
> + int ret;
> +
> + ret = set_robust_list(head, sizeof(struct robust_list_head));
sizeof(*head)
> + if (ret)
> + ksft_test_result_fail("set_robust_list error\n");
> +
> + pthread_barrier_wait(&barrier);
> + pthread_barrier_wait(&barrier2);
Lacks a comment what this waits for
> + return 0;
> +}
> +
> +/*
> + * Test get_robust_list from another thread. We use two barriers here to ensure
> + * that:
> + * 1) the child thread set the list before we try to get it from the
> + * parent
> + * 2) the child thread still alive when we try to get the list from it
> + */
> +static void test_get_robust_list_child(void)
> +{
> + pid_t tid;
> + int ret;
> + struct robust_list_head head, *get_head;
> + size_t len_ptr;
Reverse fir tree ordering please.
> + ret = pthread_barrier_init(&barrier, NULL, 2);
> + ret = pthread_barrier_init(&barrier2, NULL, 2);
> + ASSERT_EQ(ret, 0);
> +
> + tid = create_child(&child_list, &head);
> + ASSERT_NE(tid, -1);
> +
> + pthread_barrier_wait(&barrier);
> +
> + ret = get_robust_list(tid, &get_head, &len_ptr);
> + ASSERT_EQ(ret, 0);
> + ASSERT_EQ(&head, get_head);
> +
> + pthread_barrier_wait(&barrier2);
> +
> + wait(NULL);
> + pthread_barrier_destroy(&barrier);
> + pthread_barrier_destroy(&barrier2);
> +
> + ksft_test_result_pass("%s\n", __func__);
> +}
> +
> +static int child_fn_lock_with_error(void *arg)
> +{
> + struct lock_struct *lock = (struct lock_struct *) arg;
See above
> + struct robust_list_head head;
> + int ret;
> +
> + ret = set_list(&head);
> + if (ret)
> + ksft_test_result_fail("set_robust_list error\n");
So you fail the test and continue to produce more fails or what? Why
does this not use one of these ASSERT thingies or return?
> + ret = mutex_lock(lock, &head, true);
> + if (ret)
> + ksft_test_result_fail("mutex_lock error\n");
> +
> + pthread_barrier_wait(&barrier);
> +
> + sleep(1);
> +
> + return 0;
> +}
> +
> +/*
> + * Same as robustness test, but inject an error where the mutex_lock() exits
> + * earlier, just after setting list_op_pending and taking the lock, to test the
> + * list_op_pending mechanism
> + */
> +static void test_set_list_op_pending(void)
> +{
> + struct lock_struct lock = { .futex = 0 };
> + struct robust_list_head head;
> + _Atomic(unsigned int) *futex = &lock.futex;
> + int ret;
See above
> + ASSERT_EQ(ret, 0);
> + if (ret != 0)
> + printf("futex wait returned %d", errno);
The random insertion of completely pointless printf()'s is stunning.
> + ASSERT_TRUE(*futex | FUTEX_OWNER_DIED);
Yet another always true assert which is happily optimized out by the
compiler.
> + wait(NULL);
> + pthread_barrier_destroy(&barrier);
> +
> + ksft_test_result_pass("%s\n", __func__);
> +}
> +static int child_wait_lock(void *arg)
> +{
> + struct lock_struct *lock = (struct lock_struct *) arg;
> + struct robust_list_head head;
> + int ret;
> +
> + pthread_barrier_wait(&barrier2);
> + ret = mutex_lock(lock, &head, false);
> +
> + if (ret)
> + ksft_test_result_fail("mutex_lock error\n");
> +
> + if (!(lock->futex | FUTEX_OWNER_DIED))
> + ksft_test_result_fail("futex not marked with FUTEX_OWNER_DIED\n");
Now I kinda understand this insanity. The child emits a fail and
exits. Then the parent ...
> + for (i = 0; i < CHILD_NR; i++)
> + create_child(&child_wait_lock, &locks[i]);
> +
> + /* Wait for all children to return */
> + while (wait(NULL) > 0);
> +
> + pthread_barrier_destroy(&barrier);
> + pthread_barrier_destroy(&barrier2);
> +
> + ksft_test_result_pass("%s\n", __func__);
... happily claims that the test passed.
Seriously?
Thread functions have a return value for a reason and wait(2) has a
wstatus argument for the very same reason.
> +static int child_circular_list(void *arg)
> +{
> + static struct robust_list_head head;
> + struct lock_struct a, b, c;
> + int ret;
> +
> + ret = set_list(&head);
> + if (ret)
> + ksft_test_result_fail("set_list error\n");
Yet another instance of the same ....
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH v5 1/7] selftests/futex: Add ASSERT_ macros
From: Thomas Gleixner @ 2025-06-26 22:09 UTC (permalink / raw)
To: André Almeida, Ingo Molnar, Peter Zijlstra, Darren Hart,
Davidlohr Bueso, Shuah Khan, Arnd Bergmann,
Sebastian Andrzej Siewior, Waiman Long
Cc: linux-kernel, linux-kselftest, linux-api, kernel-dev,
André Almeida
In-Reply-To: <87ecv6p364.ffs@tglx>
On Fri, Jun 27 2025 at 00:07, Thomas Gleixner wrote:
> On Thu, Jun 26 2025 at 14:11, André Almeida wrote:
>
>> Create ASSERT_{EQ, NE, TRUE, FALSE} macros to make test creation easier.
>
> What's so futex special about this that it can't use the same muck in
>
> tools/testing/selftests/kselftest_harness.h
>
> or at least share the implementation in some way?
BPF has it's own set as well. Sigh...
^ permalink raw reply
* Re: [PATCH v5 1/7] selftests/futex: Add ASSERT_ macros
From: Thomas Gleixner @ 2025-06-26 22:07 UTC (permalink / raw)
To: André Almeida, Ingo Molnar, Peter Zijlstra, Darren Hart,
Davidlohr Bueso, Shuah Khan, Arnd Bergmann,
Sebastian Andrzej Siewior, Waiman Long
Cc: linux-kernel, linux-kselftest, linux-api, kernel-dev,
André Almeida
In-Reply-To: <20250626-tonyk-robust_futex-v5-1-179194dbde8f@igalia.com>
On Thu, Jun 26 2025 at 14:11, André Almeida wrote:
> Create ASSERT_{EQ, NE, TRUE, FALSE} macros to make test creation easier.
What's so futex special about this that it can't use the same muck in
tools/testing/selftests/kselftest_harness.h
or at least share the implementation in some way?
Thanks,
tglx
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox