* Re: [PATCH v2] seccomp: passthrough uretprobe systemcall without filtering
From: Oleg Nesterov @ 2025-01-28 15:54 UTC (permalink / raw)
To: Eyal Birger
Cc: kees, luto, wad, mhiramat, andrii, jolsa, alexei.starovoitov,
olsajiri, cyphar, songliubraving, yhs, john.fastabend, peterz,
tglx, bp, daniel, ast, andrii.nakryiko, rostedt, rafi,
shmulik.ladkani, bpf, linux-api, linux-trace-kernel, x86,
linux-kernel, stable
In-Reply-To: <20250128154424.GB24845@redhat.com>
On 01/28, Oleg Nesterov wrote:
>
> can't review, I know nothing about seccomp_cache, but
>
> On 01/28, Eyal Birger wrote:
> >
> > +static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
> > + struct seccomp_data *sd)
> > +{
> > +#ifdef __NR_uretprobe
> > + if (sd->nr == __NR_uretprobe
> > +#ifdef SECCOMP_ARCH_COMPAT
> > + && sd->arch != SECCOMP_ARCH_COMPAT
> > +#endif
>
> it seems you can check
>
> && sd->arch == SECCOMP_ARCH_NATIVE
>
> and avoid #ifdef SECCOMP_ARCH_COMPAT
Although perhaps you added this ifdef to avoid the unnecessary
sd->arch check if !CONFIG_COMPAT ...
Oleg.
^ permalink raw reply
* Re: [PATCH v3 5/6] ptrace: introduce PTRACE_SET_SYSCALL_INFO request
From: Dmitry V. Levin @ 2025-01-28 15:58 UTC (permalink / raw)
To: Christophe Leroy
Cc: Oleg Nesterov, Alexey Gladkov, Charlie Jenkins,
Eugene Syromyatnikov, Mike Frysinger, Renzo Davoli,
Davide Berardi, strace-devel, linux-kernel, linux-api
In-Reply-To: <3ee13371-0c7c-4264-b561-eceb4a7d7976@csgroup.eu>
On Tue, Jan 28, 2025 at 04:21:13PM +0100, Christophe Leroy wrote:
> Le 28/01/2025 à 10:16, Dmitry V. Levin a écrit :
> > PTRACE_SET_SYSCALL_INFO is a generic ptrace API that complements
> > PTRACE_GET_SYSCALL_INFO by letting the ptracer modify details of
> > system calls the tracee is blocked in.
> >
> > This API allows ptracers to obtain and modify system call details
> > in a straightforward and architecture-agnostic way.
> >
> > Current implementation supports changing only those bits of system call
> > information that are used by strace, namely, syscall number, syscall
> > arguments, and syscall return value.
>
> How do you handle changes related to syscalls that call
> force_successful_syscall_return() ?
I don't see why these syscalls would need any special treatment.
If a tracer wants to set an error status for a syscall that cannot return
an error, it's up to the tracer to face the consequences.
Tracers can do it now via PTRACE_SETREGS* anyway.
--
ldv
^ permalink raw reply
* Re: [PATCH v2] seccomp: passthrough uretprobe systemcall without filtering
From: Kees Cook @ 2025-01-29 1:41 UTC (permalink / raw)
To: Eyal Birger
Cc: luto, wad, oleg, mhiramat, andrii, jolsa, alexei.starovoitov,
olsajiri, cyphar, songliubraving, yhs, john.fastabend, peterz,
tglx, bp, daniel, ast, andrii.nakryiko, rostedt, rafi,
shmulik.ladkani, bpf, linux-api, linux-trace-kernel, x86,
linux-kernel, stable
In-Reply-To: <20250128145806.1849977-1-eyal.birger@gmail.com>
On Tue, Jan 28, 2025 at 06:58:06AM -0800, Eyal Birger wrote:
> Note: uretprobe isn't supported in i386 and __NR_ia32_rt_tgsigqueueinfo
> uses the same number as __NR_uretprobe so the syscall isn't forced in the
> compat bitmap.
So a 64-bit tracer cannot use uretprobe on a 32-bit process? Also is
uretprobe strictly an x86_64 feature?
> [...]
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 385d48293a5f..23b594a68bc0 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -734,13 +734,13 @@ seccomp_prepare_user_filter(const char __user *user_filter)
>
> #ifdef SECCOMP_ARCH_NATIVE
> /**
> - * seccomp_is_const_allow - check if filter is constant allow with given data
> + * seccomp_is_filter_const_allow - check if filter is constant allow with given data
> * @fprog: The BPF programs
> * @sd: The seccomp data to check against, only syscall number and arch
> * number are considered constant.
> */
> -static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
> - struct seccomp_data *sd)
> +static bool seccomp_is_filter_const_allow(struct sock_fprog_kern *fprog,
> + struct seccomp_data *sd)
> {
> unsigned int reg_value = 0;
> unsigned int pc;
> @@ -812,6 +812,21 @@ static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
> return false;
> }
>
> +static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
> + struct seccomp_data *sd)
> +{
> +#ifdef __NR_uretprobe
> + if (sd->nr == __NR_uretprobe
> +#ifdef SECCOMP_ARCH_COMPAT
> + && sd->arch != SECCOMP_ARCH_COMPAT
> +#endif
I don't like this because it's not future-proof enough. __NR_uretprobe
may collide with other syscalls at some point. And if __NR_uretprobe_32
is ever implemented, the seccomp logic will be missing. I think this
will work now and in the future:
#ifdef __NR_uretprobe
# ifdef SECCOMP_ARCH_COMPAT
if (sd->arch == SECCOMP_ARCH_COMPAT) {
# ifdef __NR_uretprobe_32
if (sd->nr == __NR_uretprobe_32)
return true;
# endif
} else
# endif
if (sd->nr == __NR_uretprobe)
return true;
#endif
Instead of doing a function rename dance, I think you can just stick
the above into seccomp_is_const_allow() after the WARN().
Also please add a KUnit tests to cover this in
tools/testing/selftests/seccomp/seccomp_bpf.c
With at least these cases combinations below. Check each of:
- not using uretprobe passes
- using uretprobe passes (and validates that uretprobe did work)
in each of the following conditions:
- default-allow filter
- default-block filter
- filter explicitly blocking __NR_uretprobe and nothing else
- filter explicitly allowing __NR_uretprobe (and only other
required syscalls)
Hm, is uretprobe expected to work on mips? Because if so, you'll need to
do something similar to the mode1 checking in the !SECCOMP_ARCH_NATIVE
version of seccomp_cache_check_allow().
(You can see why I really dislike having policy baked into seccomp!)
> + )
> + return true;
> +#endif
> +
> + return seccomp_is_filter_const_allow(fprog, sd);
> +}
> +
> static void seccomp_cache_prepare_bitmap(struct seccomp_filter *sfilter,
> void *bitmap, const void *bitmap_prev,
> size_t bitmap_size, int arch)
> @@ -1023,6 +1038,9 @@ static inline void seccomp_log(unsigned long syscall, long signr, u32 action,
> */
> static const int mode1_syscalls[] = {
> __NR_seccomp_read, __NR_seccomp_write, __NR_seccomp_exit, __NR_seccomp_sigreturn,
> +#ifdef __NR_uretprobe
> + __NR_uretprobe,
> +#endif
It'd be nice to update mode1_syscalls_32 with __NR_uretprobe_32 even
though it doesn't exist. (Is it _never_ planned to be implemented?) But
then, maybe the chances of a compat mode1 seccomp process running under
uretprobe is vanishingly small.
> -1, /* negative terminated */
> };
>
> --
> 2.43.0
>
-Kees
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v2] fs: introduce getfsxattrat and setfsxattrat syscalls
From: Andrey Albershteyn @ 2025-01-29 13:01 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-m68k, tglx, jcmvbkbc, linux-security-module, arnd,
linux-fsdevel, chris, npiggin, linuxppc-dev, mpe, luto, jack,
monstr, linux-arch, mingo, linux-alpha, christophe.leroy,
linux-sh, linux-parisc, naveen, bp, hpa, sparclinux, linux-kernel,
x86, maddy, dave.hansen, viro, linux-s390, linux-api, linux-xfs
In-Reply-To: <20250124-wasser-kopfsache-3dc12cb7f7ab@brauner>
On 2025-01-24 10:33:54, Christian Brauner wrote:
> On Wed, Jan 22, 2025 at 03:18:34PM +0100, Andrey Albershteyn wrote:
> > From: Andrey Albershteyn <aalbersh@redhat.com>
> >
> > Introduce getfsxattrat and setfsxattrat syscalls to manipulate inode
> > extended attributes/flags. The syscalls take parent directory FD and
> > path to the child together with struct fsxattr.
> >
> > This is an alternative to FS_IOC_FSSETXATTR ioctl with a difference
> > that file don't need to be open. By having this we can manipulated
>
> By that you mean that you can use absolute or relative paths instead of
> file descriptors?
yes
>
> > inode extended attributes not only on normal files but also on
> > special ones. This is not possible with FS_IOC_FSSETXATTR ioctl as
> > opening special files returns VFS special inode instead of
> > underlying filesystem one.
>
> I'm not following this argument currently. In what sense does opening
> special files return a VFS special inode and how does that prevent
> FS_IOC_FSSEETXATTR from working? The inode in
>
> 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;
>
> d_inode(dentry)
>
>
> and your:
>
> error = user_path_at(dfd, filename, lookup_flags, &filepath);
> if (error)
> goto out;
>
> d_inode(filepath.dentry)
>
> is the same.
This is probably not a good description. With these special files,
like pipe, we can not just open them to call FS_IOC_FSSETXATTR, as
with regular ones. As you asked above, with filepath it can be done
without opening files in userspace. I don't see how we can open()
special files and then call ioctl() on them and with paths we can do
it pretty easily.
>
> >
> > This patch adds two new syscalls which allows userspace to set
> > extended inode attributes on special files by using parent directory
> > to open FS inode.
> >
> > Also, as vfs_fileattr_set() is now will be called on special files
> > too, let's forbid any other attributes except projid and nextents
> > (symlink can have an extent).
> >
> > CC: linux-api@vger.kernel.org
> > CC: linux-fsdevel@vger.kernel.org
> > CC: linux-xfs@vger.kernel.org
> > Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com>
> > ---
> > 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/
> >
> > 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. Moreover, in the case
> > when special files are created in the directory with already
> > existing project quota, these inode inherit extended attributes.
> > This than leaves them with these attributes without the possibility
> > to clear them out. This, in turn, prevents userspace from
> > re-creating quota project on these existing files.
> > ---
> > 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/inode.c | 99 +++++++++++++++++++++++++++++
> > fs/ioctl.c | 16 ++++-
> > include/linux/fileattr.h | 1 +
> > include/linux/syscalls.h | 4 ++
> > include/uapi/asm-generic/unistd.h | 8 ++-
> > 21 files changed, 157 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/alpha/kernel/syscalls/syscall.tbl b/arch/alpha/kernel/syscalls/syscall.tbl
> > index c59d53d6d3f3490f976ca179ddfe02e69265ae4d..4b9e687494c16b60c6fd6ca1dc4d6564706a7e25 100644
> > --- a/arch/alpha/kernel/syscalls/syscall.tbl
> > +++ b/arch/alpha/kernel/syscalls/syscall.tbl
> > @@ -506,3 +506,5 @@
> > 574 common getxattrat sys_getxattrat
> > 575 common listxattrat sys_listxattrat
> > 576 common removexattrat sys_removexattrat
> > +577 common getfsxattrat sys_getfsxattrat
> > +578 common setfsxattrat sys_setfsxattrat
> > diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl
> > index 49eeb2ad8dbd8e074c6240417693f23fb328afa8..66466257f3c2debb3e2299f0b608c6740c98cab2 100644
> > --- a/arch/arm/tools/syscall.tbl
> > +++ b/arch/arm/tools/syscall.tbl
> > @@ -481,3 +481,5 @@
> > 464 common getxattrat sys_getxattrat
> > 465 common listxattrat sys_listxattrat
> > 466 common removexattrat sys_removexattrat
> > +467 common getfsxattrat sys_getfsxattrat
> > +468 common setfsxattrat sys_setfsxattrat
> > diff --git a/arch/arm64/tools/syscall_32.tbl b/arch/arm64/tools/syscall_32.tbl
> > index 69a829912a05eb8a3e21ed701d1030e31c0148bc..9c516118b154811d8d11d5696f32817430320dbf 100644
> > --- a/arch/arm64/tools/syscall_32.tbl
> > +++ b/arch/arm64/tools/syscall_32.tbl
> > @@ -478,3 +478,5 @@
> > 464 common getxattrat sys_getxattrat
> > 465 common listxattrat sys_listxattrat
> > 466 common removexattrat sys_removexattrat
> > +467 common getfsxattrat sys_getfsxattrat
> > +468 common setfsxattrat sys_setfsxattrat
> > diff --git a/arch/m68k/kernel/syscalls/syscall.tbl b/arch/m68k/kernel/syscalls/syscall.tbl
> > index f5ed71f1910d09769c845c2d062d99ee0449437c..159476387f394a92ee5e29db89b118c630372db2 100644
> > --- a/arch/m68k/kernel/syscalls/syscall.tbl
> > +++ b/arch/m68k/kernel/syscalls/syscall.tbl
> > @@ -466,3 +466,5 @@
> > 464 common getxattrat sys_getxattrat
> > 465 common listxattrat sys_listxattrat
> > 466 common removexattrat sys_removexattrat
> > +467 common getfsxattrat sys_getfsxattrat
> > +468 common setfsxattrat sys_setfsxattrat
> > diff --git a/arch/microblaze/kernel/syscalls/syscall.tbl b/arch/microblaze/kernel/syscalls/syscall.tbl
> > index 680f568b77f2cbefc3eacb2517f276041f229b1e..a6d59ee740b58cacf823702003cf9bad17c0d3b7 100644
> > --- a/arch/microblaze/kernel/syscalls/syscall.tbl
> > +++ b/arch/microblaze/kernel/syscalls/syscall.tbl
> > @@ -472,3 +472,5 @@
> > 464 common getxattrat sys_getxattrat
> > 465 common listxattrat sys_listxattrat
> > 466 common removexattrat sys_removexattrat
> > +467 common getfsxattrat sys_getfsxattrat
> > +468 common setfsxattrat sys_setfsxattrat
> > diff --git a/arch/mips/kernel/syscalls/syscall_n32.tbl b/arch/mips/kernel/syscalls/syscall_n32.tbl
> > index 0b9b7e25b69ad592642f8533bee9ccfe95ce9626..cfe38fcebe1a0279e11751378d3e71c5ec6b6569 100644
> > --- a/arch/mips/kernel/syscalls/syscall_n32.tbl
> > +++ b/arch/mips/kernel/syscalls/syscall_n32.tbl
> > @@ -405,3 +405,5 @@
> > 464 n32 getxattrat sys_getxattrat
> > 465 n32 listxattrat sys_listxattrat
> > 466 n32 removexattrat sys_removexattrat
> > +467 n32 getfsxattrat sys_getfsxattrat
> > +468 n32 setfsxattrat sys_setfsxattrat
> > diff --git a/arch/mips/kernel/syscalls/syscall_n64.tbl b/arch/mips/kernel/syscalls/syscall_n64.tbl
> > index c844cd5cda620b2809a397cdd6f4315ab6a1bfe2..29a0c5974d1aa2f01e33edc0252d75fb97abe230 100644
> > --- a/arch/mips/kernel/syscalls/syscall_n64.tbl
> > +++ b/arch/mips/kernel/syscalls/syscall_n64.tbl
> > @@ -381,3 +381,5 @@
> > 464 n64 getxattrat sys_getxattrat
> > 465 n64 listxattrat sys_listxattrat
> > 466 n64 removexattrat sys_removexattrat
> > +467 n64 getfsxattrat sys_getfsxattrat
> > +468 n64 setfsxattrat sys_setfsxattrat
> > diff --git a/arch/mips/kernel/syscalls/syscall_o32.tbl b/arch/mips/kernel/syscalls/syscall_o32.tbl
> > index 349b8aad1159f404103bd2057a1e64e9bf309f18..6c00436807c57c492ba957fcd59af1202231cf80 100644
> > --- a/arch/mips/kernel/syscalls/syscall_o32.tbl
> > +++ b/arch/mips/kernel/syscalls/syscall_o32.tbl
> > @@ -454,3 +454,5 @@
> > 464 o32 getxattrat sys_getxattrat
> > 465 o32 listxattrat sys_listxattrat
> > 466 o32 removexattrat sys_removexattrat
> > +467 o32 getfsxattrat sys_getfsxattrat
> > +468 o32 setfsxattrat sys_setfsxattrat
> > diff --git a/arch/parisc/kernel/syscalls/syscall.tbl b/arch/parisc/kernel/syscalls/syscall.tbl
> > index d9fc94c869657fcfbd7aca1d5f5abc9fae2fb9d8..b3578fac43d6b65167787fcc97d2d09f5a9828e7 100644
> > --- a/arch/parisc/kernel/syscalls/syscall.tbl
> > +++ b/arch/parisc/kernel/syscalls/syscall.tbl
> > @@ -465,3 +465,5 @@
> > 464 common getxattrat sys_getxattrat
> > 465 common listxattrat sys_listxattrat
> > 466 common removexattrat sys_removexattrat
> > +467 common getfsxattrat sys_getfsxattrat
> > +468 common setfsxattrat sys_setfsxattrat
> > diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
> > index d8b4ab78bef076bd50d49b87dea5060fd8c1686a..808045d82c9465c3bfa96b15947546efe5851e9a 100644
> > --- a/arch/powerpc/kernel/syscalls/syscall.tbl
> > +++ b/arch/powerpc/kernel/syscalls/syscall.tbl
> > @@ -557,3 +557,5 @@
> > 464 common getxattrat sys_getxattrat
> > 465 common listxattrat sys_listxattrat
> > 466 common removexattrat sys_removexattrat
> > +467 common getfsxattrat sys_getfsxattrat
> > +468 common setfsxattrat sys_setfsxattrat
> > diff --git a/arch/s390/kernel/syscalls/syscall.tbl b/arch/s390/kernel/syscalls/syscall.tbl
> > index e9115b4d8b635b846e5c9ad6ce229605323723a5..78dfc2c184d4815baf8a9e61c546c9936d58a47c 100644
> > --- a/arch/s390/kernel/syscalls/syscall.tbl
> > +++ b/arch/s390/kernel/syscalls/syscall.tbl
> > @@ -469,3 +469,5 @@
> > 464 common getxattrat sys_getxattrat sys_getxattrat
> > 465 common listxattrat sys_listxattrat sys_listxattrat
> > 466 common removexattrat sys_removexattrat sys_removexattrat
> > +467 common getfsxattrat sys_getfsxattrat sys_getfsxattrat
> > +468 common setfsxattrat sys_setfsxattrat sys_setfsxattrat
> > diff --git a/arch/sh/kernel/syscalls/syscall.tbl b/arch/sh/kernel/syscalls/syscall.tbl
> > index c8cad33bf250ea110de37bd1407f5a43ec5e38f2..d5a5c8339f0ed25ea07c4aba90351d352033c8a0 100644
> > --- a/arch/sh/kernel/syscalls/syscall.tbl
> > +++ b/arch/sh/kernel/syscalls/syscall.tbl
> > @@ -470,3 +470,5 @@
> > 464 common getxattrat sys_getxattrat
> > 465 common listxattrat sys_listxattrat
> > 466 common removexattrat sys_removexattrat
> > +467 common getfsxattrat sys_getfsxattrat
> > +468 common setfsxattrat sys_setfsxattrat
> > diff --git a/arch/sparc/kernel/syscalls/syscall.tbl b/arch/sparc/kernel/syscalls/syscall.tbl
> > index 727f99d333b304b3db0711953a3d91ece18a28eb..817dcd8603bcbffc47f3f59aa3b74b16486453d0 100644
> > --- a/arch/sparc/kernel/syscalls/syscall.tbl
> > +++ b/arch/sparc/kernel/syscalls/syscall.tbl
> > @@ -512,3 +512,5 @@
> > 464 common getxattrat sys_getxattrat
> > 465 common listxattrat sys_listxattrat
> > 466 common removexattrat sys_removexattrat
> > +467 common getfsxattrat sys_getfsxattrat
> > +468 common setfsxattrat sys_setfsxattrat
> > diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
> > index 4d0fb2fba7e208ae9455459afe11e277321d9f74..b4842c027c5d00c0236b2ba89387c5e2267447bd 100644
> > --- a/arch/x86/entry/syscalls/syscall_32.tbl
> > +++ b/arch/x86/entry/syscalls/syscall_32.tbl
> > @@ -472,3 +472,5 @@
> > 464 i386 getxattrat sys_getxattrat
> > 465 i386 listxattrat sys_listxattrat
> > 466 i386 removexattrat sys_removexattrat
> > +467 i386 getfsxattrat sys_getfsxattrat
> > +468 i386 setfsxattrat sys_setfsxattrat
> > diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
> > index 5eb708bff1c791debd6cfc5322583b2ae53f6437..b6f0a7236aaee624cf9b484239a1068085a8ffe1 100644
> > --- a/arch/x86/entry/syscalls/syscall_64.tbl
> > +++ b/arch/x86/entry/syscalls/syscall_64.tbl
> > @@ -390,6 +390,8 @@
> > 464 common getxattrat sys_getxattrat
> > 465 common listxattrat sys_listxattrat
> > 466 common removexattrat sys_removexattrat
> > +467 common getfsxattrat sys_getfsxattrat
> > +468 common setfsxattrat sys_setfsxattrat
> >
> > #
> > # 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 37effc1b134eea061f2c350c1d68b4436b65a4dd..425d56be337d1de22f205ac503df61ff86224fee 100644
> > --- a/arch/xtensa/kernel/syscalls/syscall.tbl
> > +++ b/arch/xtensa/kernel/syscalls/syscall.tbl
> > @@ -437,3 +437,5 @@
> > 464 common getxattrat sys_getxattrat
> > 465 common listxattrat sys_listxattrat
> > 466 common removexattrat sys_removexattrat
> > +467 common getfsxattrat sys_getfsxattrat
> > +468 common setfsxattrat sys_setfsxattrat
> > diff --git a/fs/inode.c b/fs/inode.c
> > index 6b4c77268fc0ecace4ac78a9ca777fbffc277f4a..cdecb793b2ab5ab01e2333da4382919b94c7f65f 100644
> > --- a/fs/inode.c
> > +++ b/fs/inode.c
> > @@ -23,6 +23,9 @@
> > #include <linux/rw_hint.h>
> > #include <linux/seq_file.h>
> > #include <linux/debugfs.h>
> > +#include <linux/syscalls.h>
> > +#include <linux/fileattr.h>
> > +#include <linux/namei.h>
> > #include <trace/events/writeback.h>
> > #define CREATE_TRACE_POINTS
> > #include <trace/events/timestamp.h>
> > @@ -2953,3 +2956,99 @@ umode_t mode_strip_sgid(struct mnt_idmap *idmap,
> > return mode & ~S_ISGID;
> > }
> > EXPORT_SYMBOL(mode_strip_sgid);
> > +
> > +SYSCALL_DEFINE4(getfsxattrat, int, dfd, const char __user *, filename,
> > + struct fsxattr __user *, fsx, unsigned int, at_flags)
> > +{
> > + struct fd dir;
> > + struct fileattr fa;
> > + struct path filepath;
> > + int error;
> > + unsigned int lookup_flags = 0;
> > +
> > + if ((at_flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
> > + return -EINVAL;
> > +
> > + if (at_flags & AT_SYMLINK_FOLLOW)
> > + lookup_flags |= LOOKUP_FOLLOW;
> > +
> > + if (at_flags & AT_EMPTY_PATH)
> > + lookup_flags |= LOOKUP_EMPTY;
> > +
> > + dir = fdget(dfd);
> > + if (!fd_file(dir))
> > + return -EBADF;
>
> Please rely on scope-based cleanup:
>
> CLASS(fd, f)(dfd);
> if (fd_empty(f))
> return -EBADF;
aha, thanks, didn't know about this
>
> > +
> > + if (!S_ISDIR(file_inode(fd_file(dir))->i_mode)) {
>
> This isn't needed as user_path_at() will return ENOTDIR.
> Your path as is wuld also preclude AT_EMPTY_PATH with directory file
> descriptors and afaict there's various filesystems that seem to support
> this on directory inodes.
I see, thanks, I will remove that
>
> > + error = -EBADF;
> > + goto out;
> > + }
> > +
> > + error = user_path_at(dfd, filename, lookup_flags, &filepath);
> > + if (error)
> > + goto out;
>
> I'm confused. You're using fdget() above but then you don't use the
> resulting file and call user_path_at() instead? Don't bother with
> fdget() at all and just call into user_path_at() directly.
sure, doesn't needed anymore with your suggestions above
>
> > +
> > + error = vfs_fileattr_get(filepath.dentry, &fa);
> > + if (error)
> > + goto out_path;
> > +
> > + if (copy_fsxattr_to_user(&fa, fsx))
> > + error = -EFAULT;
> > +
> > +out_path:
> > + path_put(&filepath);
> > +out:
> > + fdput(dir);
> > + return error;
> > +}
> > +
> > +SYSCALL_DEFINE4(setfsxattrat, int, dfd, const char __user *, filename,
> > + struct fsxattr __user *, fsx, unsigned int, at_flags)
> > +{
> > + struct fd dir;
> > + struct fileattr fa;
> > + struct path filepath;
> > + int error;
> > + unsigned int lookup_flags = 0;
> > +
> > + if ((at_flags & ~(AT_SYMLINK_FOLLOW | AT_EMPTY_PATH)) != 0)
> > + return -EINVAL;
> > +
> > + if (at_flags & AT_SYMLINK_FOLLOW)
> > + lookup_flags |= LOOKUP_FOLLOW;
> > +
> > + if (at_flags & AT_EMPTY_PATH)
> > + lookup_flags |= LOOKUP_EMPTY;
> > +
> > + dir = fdget(dfd);
> > + if (!fd_file(dir))
> > + return -EBADF;
> > +
> > + if (!S_ISDIR(file_inode(fd_file(dir))->i_mode)) {
> > + error = -EBADF;
> > + goto out;
> > + }
> > +
> > + if (copy_fsxattr_from_user(&fa, fsx)) {
> > + error = -EFAULT;
> > + goto out;
> > + }
> > +
> > + error = user_path_at(dfd, filename, lookup_flags, &filepath);
> > + if (error)
> > + goto out;
>
> Same problem as above. fdget() stuff isn't needed if you're calling
> user_path_at() anyway.
>
> > +
> > + error = mnt_want_write(filepath.mnt);
> > + if (error)
> > + goto out_path;
> > +
> > + error = vfs_fileattr_set(file_mnt_idmap(fd_file(dir)), filepath.dentry,
> > + &fa);
> > + mnt_drop_write(filepath.mnt);
>
> Just use the pattern:
>
> error = user_path_at(dfd, filename, lookup_flags, &filepath);
> if (error)
> return error; /* once you've removed the fdget() direct return works fine */
>
>
> error = mnt_want_write(filepath.mnt);
> if (!error) {
> error = vfs_fileattr_set(file_mnt_idmap(fd_file(dir)), filepath.dentry, &fa);
> mnt_drop_write(filepath.mnt);
> }
>
> return error;
sure, I will changed it as suggested
--
- Andrey
>
> > +
> > +out_path:
> > + path_put(&filepath);
> > +out:
> > + fdput(dir);
> > + return error;
> > +}
> > diff --git a/fs/ioctl.c b/fs/ioctl.c
> > index 638a36be31c14afc66a7fd6eb237d9545e8ad997..dc160c2ef145e4931d625f1f93c2a8ae7f87abf3 100644
> > --- a/fs/ioctl.c
> > +++ b/fs/ioctl.c
> > @@ -558,8 +558,7 @@ int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa)
> > }
> > EXPORT_SYMBOL(copy_fsxattr_to_user);
> >
> > -static int copy_fsxattr_from_user(struct fileattr *fa,
> > - struct fsxattr __user *ufa)
> > +int copy_fsxattr_from_user(struct fileattr *fa, struct fsxattr __user *ufa)
> > {
> > struct fsxattr xfa;
> >
> > @@ -646,6 +645,19 @@ static int fileattr_set_prepare(struct inode *inode,
> > if (fa->fsx_cowextsize == 0)
> > fa->fsx_xflags &= ~FS_XFLAG_COWEXTSIZE;
> >
> > + /*
> > + * The only use case for special files is to set project ID, forbid any
> > + * other attributes
> > + */
> > + if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode))) {
> > + if (fa->fsx_xflags & ~FS_XFLAG_PROJINHERIT)
> > + return -EINVAL;
> > + if (!S_ISLNK(inode->i_mode) && fa->fsx_nextents)
> > + return -EINVAL;
> > + if (fa->fsx_extsize || fa->fsx_cowextsize)
> > + return -EINVAL;
> > + }
> > +
> > return 0;
> > }
> >
> > diff --git a/include/linux/fileattr.h b/include/linux/fileattr.h
> > index 47c05a9851d0600964b644c9c7218faacfd865f8..8598e94b530b8b280a2697eaf918dd60f573d6ee 100644
> > --- a/include/linux/fileattr.h
> > +++ b/include/linux/fileattr.h
> > @@ -34,6 +34,7 @@ struct fileattr {
> > };
> >
> > int copy_fsxattr_to_user(const struct fileattr *fa, struct fsxattr __user *ufa);
> > +int copy_fsxattr_from_user(struct fileattr *fa, struct fsxattr __user *ufa);
> >
> > void fileattr_fill_xflags(struct fileattr *fa, u32 xflags);
> > void fileattr_fill_flags(struct fileattr *fa, u32 flags);
> > diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> > index c6333204d45130eb022f6db460eea34a1f6e91db..3134d463d9af64c6e78adb37bff4b91f77b5305f 100644
> > --- a/include/linux/syscalls.h
> > +++ b/include/linux/syscalls.h
> > @@ -371,6 +371,10 @@ 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_getfsxattrat(int dfd, const char __user *filename,
> > + struct fsxattr *fsx, unsigned int at_flags);
> > +asmlinkage long sys_setfsxattrat(int dfd, const char __user *filename,
> > + struct fsxattr *fsx, 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 88dc393c2bca38c0fa1b3fae579f7cfe4931223c..50be2e1007bc2779120d05c6e9512a689f86779c 100644
> > --- a/include/uapi/asm-generic/unistd.h
> > +++ b/include/uapi/asm-generic/unistd.h
> > @@ -850,8 +850,14 @@ __SYSCALL(__NR_listxattrat, sys_listxattrat)
> > #define __NR_removexattrat 466
> > __SYSCALL(__NR_removexattrat, sys_removexattrat)
> >
> > +/* fs/inode.c */
> > +#define __NR_getfsxattrat 467
> > +__SYSCALL(__NR_getfsxattrat, sys_getfsxattrat)
> > +#define __NR_setfsxattrat 468
> > +__SYSCALL(__NR_setfsxattrat, sys_setfsxattrat)
> > +
> > #undef __NR_syscalls
> > -#define __NR_syscalls 467
> > +#define __NR_syscalls 469
> >
> > /*
> > * 32 bit systems traditionally used different
> >
> > ---
> > base-commit: 4c538044ee2d11299cc57ac1e92d343e1e83b847
> > change-id: 20250114-xattrat-syscall-6a1136d2db59
> >
> > Best regards,
> > --
> > Andrey Albershteyn <aalbersh@kernel.org>
> >
>
^ permalink raw reply
* Re: [PATCH v2] seccomp: passthrough uretprobe systemcall without filtering
From: Eyal Birger @ 2025-01-29 17:27 UTC (permalink / raw)
To: Kees Cook
Cc: luto, wad, oleg, mhiramat, andrii, jolsa, alexei.starovoitov,
olsajiri, cyphar, songliubraving, yhs, john.fastabend, peterz,
tglx, bp, daniel, ast, andrii.nakryiko, rostedt, rafi,
shmulik.ladkani, bpf, linux-api, linux-trace-kernel, x86,
linux-kernel, stable
In-Reply-To: <202501281634.7F398CEA87@keescook>
Hi,
Thanks for the review!
On Tue, Jan 28, 2025 at 5:41 PM Kees Cook <kees@kernel.org> wrote:
>
> On Tue, Jan 28, 2025 at 06:58:06AM -0800, Eyal Birger wrote:
> > Note: uretprobe isn't supported in i386 and __NR_ia32_rt_tgsigqueueinfo
> > uses the same number as __NR_uretprobe so the syscall isn't forced in the
> > compat bitmap.
>
> So a 64-bit tracer cannot use uretprobe on a 32-bit process? Also is
> uretprobe strictly an x86_64 feature?
>
My understanding is that they'd be able to do so, but use the int3 trap
instead of the uretprobe syscall.
> > [...]
> > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > index 385d48293a5f..23b594a68bc0 100644
> > --- a/kernel/seccomp.c
> > +++ b/kernel/seccomp.c
> > @@ -734,13 +734,13 @@ seccomp_prepare_user_filter(const char __user *user_filter)
> >
> > #ifdef SECCOMP_ARCH_NATIVE
> > /**
> > - * seccomp_is_const_allow - check if filter is constant allow with given data
> > + * seccomp_is_filter_const_allow - check if filter is constant allow with given data
> > * @fprog: The BPF programs
> > * @sd: The seccomp data to check against, only syscall number and arch
> > * number are considered constant.
> > */
> > -static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
> > - struct seccomp_data *sd)
> > +static bool seccomp_is_filter_const_allow(struct sock_fprog_kern *fprog,
> > + struct seccomp_data *sd)
> > {
> > unsigned int reg_value = 0;
> > unsigned int pc;
> > @@ -812,6 +812,21 @@ static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
> > return false;
> > }
> >
> > +static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
> > + struct seccomp_data *sd)
> > +{
> > +#ifdef __NR_uretprobe
> > + if (sd->nr == __NR_uretprobe
> > +#ifdef SECCOMP_ARCH_COMPAT
> > + && sd->arch != SECCOMP_ARCH_COMPAT
> > +#endif
>
> I don't like this because it's not future-proof enough. __NR_uretprobe
> may collide with other syscalls at some point.
I'm not sure I got this point.
> And if __NR_uretprobe_32
> is ever implemented, the seccomp logic will be missing. I think this
> will work now and in the future:
>
> #ifdef __NR_uretprobe
> # ifdef SECCOMP_ARCH_COMPAT
> if (sd->arch == SECCOMP_ARCH_COMPAT) {
> # ifdef __NR_uretprobe_32
> if (sd->nr == __NR_uretprobe_32)
> return true;
> # endif
> } else
> # endif
> if (sd->nr == __NR_uretprobe)
> return true;
> #endif
I don't know if implementing uretprobe syscall for compat binaries is
planned or makes sense - I'd appreciate Jiri's and others opinion on that.
That said, I don't mind adding this code for the sake of future proofing.
>
> Instead of doing a function rename dance, I think you can just stick
> the above into seccomp_is_const_allow() after the WARN().
My motivation for the renaming dance was that you mentioned we might add
new syscalls to this as well, so I wanted to avoid cluttering the existing
function which seems to be well defined.
>
> Also please add a KUnit tests to cover this in
> tools/testing/selftests/seccomp/seccomp_bpf.c
I think this would mean that this test suite would need to run as
privileged. Is that Ok? or maybe it'd be better to have a new suite?
> With at least these cases combinations below. Check each of:
>
> - not using uretprobe passes
> - using uretprobe passes (and validates that uretprobe did work)
>
> in each of the following conditions:
>
> - default-allow filter
> - default-block filter
> - filter explicitly blocking __NR_uretprobe and nothing else
> - filter explicitly allowing __NR_uretprobe (and only other
> required syscalls)
Ok.
>
> Hm, is uretprobe expected to work on mips? Because if so, you'll need to
> do something similar to the mode1 checking in the !SECCOMP_ARCH_NATIVE
> version of seccomp_cache_check_allow().
I don't know if uretprobe syscall is expected to run on mips. Personally
I'd avoid adding this dead code.
>
> (You can see why I really dislike having policy baked into seccomp!)
I definitely understand :)
>
> > + )
> > + return true;
> > +#endif
> > +
> > + return seccomp_is_filter_const_allow(fprog, sd);
> > +}
> > +
> > static void seccomp_cache_prepare_bitmap(struct seccomp_filter *sfilter,
> > void *bitmap, const void *bitmap_prev,
> > size_t bitmap_size, int arch)
> > @@ -1023,6 +1038,9 @@ static inline void seccomp_log(unsigned long syscall, long signr, u32 action,
> > */
> > static const int mode1_syscalls[] = {
> > __NR_seccomp_read, __NR_seccomp_write, __NR_seccomp_exit, __NR_seccomp_sigreturn,
> > +#ifdef __NR_uretprobe
> > + __NR_uretprobe,
> > +#endif
>
> It'd be nice to update mode1_syscalls_32 with __NR_uretprobe_32 even
> though it doesn't exist. (Is it _never_ planned to be implemented?) But
> then, maybe the chances of a compat mode1 seccomp process running under
> uretprobe is vanishingly small.
It seems to me very unlikely. BTW, when I tested the "strict" mode change
my program was killed by seccomp. The reason wasn't the uretprobe syscall
(which I added to the list), it was actually the exit_group syscall which
libc uses instead of the exit syscall.
Thanks again,
Eyal.
^ permalink raw reply
* Re: [PATCH v3 0/6] ptrace: introduce PTRACE_SET_SYSCALL_INFO API
From: Oleg Nesterov @ 2025-01-29 18:51 UTC (permalink / raw)
To: Dmitry V. Levin
Cc: Alexey Gladkov, Charlie Jenkins, Eugene Syromyatnikov,
Mike Frysinger, Renzo Davoli, Davide Berardi, strace-devel,
Vineet Gupta, Russell King, Will Deacon, Guo Ren, Brian Cain,
Huacai Chen, WANG Xuerui, Geert Uytterhoeven, Michal Simek,
Thomas Bogendoerfer, Dinh Nguyen, Jonas Bonn, Stefan Kristiansson,
Stafford Horne, James E.J. Bottomley, Helge Deller,
Michael Ellerman, Nicholas Piggin, Christophe Leroy, Naveen N Rao,
Madhavan Srinivasan, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Heiko Carstens, Vasily Gorbik, Alexander Gordeev,
Christian Borntraeger, Sven Schnelle, Yoshinori Sato, Rich Felker,
John Paul Adrian Glaubitz, David S. Miller, Andreas Larsson,
Richard Weinberger, Anton Ivanov, Johannes Berg, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin,
Chris Zankel, Max Filippov, Arnd Bergmann, Shuah Khan,
linux-snps-arc, linux-kernel, linux-arm-kernel, linux-csky,
linux-hexagon, loongarch, linux-m68k, linux-mips, linux-openrisc,
linux-parisc, linuxppc-dev, linux-riscv, linux-s390, linux-sh,
sparclinux, linux-um, linux-arch, linux-kselftest, linux-api
In-Reply-To: <20250128091445.GA8257@strace.io>
On 01/28, Dmitry V. Levin wrote:
>
> * ptrace: Add explicit padding to the end of struct ptrace_syscall_info,
> simplify obtaining of user ptrace_syscall_info,
> do not introduce PTRACE_SYSCALL_INFO_SIZE_VER0
> * ptrace: Change the return type of ptrace_set_syscall_info_* functions
> from "unsigned long" to "int"
> * ptrace: Add -ERANGE check to ptrace_set_syscall_info_exit,
> add comments to -ERANGE checks
> * ptrace: Update comments about supported syscall stops
> * selftests: Extend set_syscall_info test, fix for mips n32
Again, I can't review 1-3, I know nothing about the non-x86 architectures.
As for 4-6, feel free to add
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
^ permalink raw reply
* Re: [PATCH v2] seccomp: passthrough uretprobe systemcall without filtering
From: Andrii Nakryiko @ 2025-01-29 22:52 UTC (permalink / raw)
To: Eyal Birger
Cc: Kees Cook, luto, wad, oleg, mhiramat, andrii, jolsa,
alexei.starovoitov, olsajiri, cyphar, songliubraving, yhs,
john.fastabend, peterz, tglx, bp, daniel, ast, rostedt, rafi,
shmulik.ladkani, bpf, linux-api, linux-trace-kernel, x86,
linux-kernel, stable
In-Reply-To: <CAHsH6Gsv3DB0O5oiEDsf2+Go4O1+tnKm-Ab0QPyohKSaroSxxA@mail.gmail.com>
On Wed, Jan 29, 2025 at 9:27 AM Eyal Birger <eyal.birger@gmail.com> wrote:
>
> Hi,
>
> Thanks for the review!
>
> On Tue, Jan 28, 2025 at 5:41 PM Kees Cook <kees@kernel.org> wrote:
> >
> > On Tue, Jan 28, 2025 at 06:58:06AM -0800, Eyal Birger wrote:
> > > Note: uretprobe isn't supported in i386 and __NR_ia32_rt_tgsigqueueinfo
> > > uses the same number as __NR_uretprobe so the syscall isn't forced in the
> > > compat bitmap.
> >
> > So a 64-bit tracer cannot use uretprobe on a 32-bit process? Also is
> > uretprobe strictly an x86_64 feature?
> >
>
> My understanding is that they'd be able to do so, but use the int3 trap
> instead of the uretprobe syscall.
>
Syscall-based uretprobe implementation is strictly x86-64 and I don't
think we have any plans to expand it beyond x86-64. But uretprobes in
general do work across many bitnesses and architectures, they are just
implemented through a trap approach (int3 on x86), so none of that
should be relevant to seccomp. It's just that trapping on x86-64 is
that much slower that we had to do syscall to speed it up but quite a
lot.
> > > [...]
> > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > > index 385d48293a5f..23b594a68bc0 100644
> > > --- a/kernel/seccomp.c
> > > +++ b/kernel/seccomp.c
> > > @@ -734,13 +734,13 @@ seccomp_prepare_user_filter(const char __user *user_filter)
> > >
[...]
^ permalink raw reply
* Re: [PATCH v2] seccomp: passthrough uretprobe systemcall without filtering
From: Jiri Olsa @ 2025-01-30 8:24 UTC (permalink / raw)
To: Eyal Birger
Cc: Kees Cook, luto, wad, oleg, mhiramat, andrii, alexei.starovoitov,
olsajiri, cyphar, songliubraving, yhs, john.fastabend, peterz,
tglx, bp, daniel, ast, andrii.nakryiko, rostedt, rafi,
shmulik.ladkani, bpf, linux-api, linux-trace-kernel, x86,
linux-kernel, stable
In-Reply-To: <CAHsH6Gsv3DB0O5oiEDsf2+Go4O1+tnKm-Ab0QPyohKSaroSxxA@mail.gmail.com>
On Wed, Jan 29, 2025 at 09:27:49AM -0800, Eyal Birger wrote:
> Hi,
>
> Thanks for the review!
>
> On Tue, Jan 28, 2025 at 5:41 PM Kees Cook <kees@kernel.org> wrote:
> >
> > On Tue, Jan 28, 2025 at 06:58:06AM -0800, Eyal Birger wrote:
> > > Note: uretprobe isn't supported in i386 and __NR_ia32_rt_tgsigqueueinfo
> > > uses the same number as __NR_uretprobe so the syscall isn't forced in the
> > > compat bitmap.
> >
> > So a 64-bit tracer cannot use uretprobe on a 32-bit process? Also is
> > uretprobe strictly an x86_64 feature?
> >
>
> My understanding is that they'd be able to do so, but use the int3 trap
> instead of the uretprobe syscall.
>
> > > [...]
> > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > > index 385d48293a5f..23b594a68bc0 100644
> > > --- a/kernel/seccomp.c
> > > +++ b/kernel/seccomp.c
> > > @@ -734,13 +734,13 @@ seccomp_prepare_user_filter(const char __user *user_filter)
> > >
> > > #ifdef SECCOMP_ARCH_NATIVE
> > > /**
> > > - * seccomp_is_const_allow - check if filter is constant allow with given data
> > > + * seccomp_is_filter_const_allow - check if filter is constant allow with given data
> > > * @fprog: The BPF programs
> > > * @sd: The seccomp data to check against, only syscall number and arch
> > > * number are considered constant.
> > > */
> > > -static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
> > > - struct seccomp_data *sd)
> > > +static bool seccomp_is_filter_const_allow(struct sock_fprog_kern *fprog,
> > > + struct seccomp_data *sd)
> > > {
> > > unsigned int reg_value = 0;
> > > unsigned int pc;
> > > @@ -812,6 +812,21 @@ static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
> > > return false;
> > > }
> > >
> > > +static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
> > > + struct seccomp_data *sd)
> > > +{
> > > +#ifdef __NR_uretprobe
> > > + if (sd->nr == __NR_uretprobe
> > > +#ifdef SECCOMP_ARCH_COMPAT
> > > + && sd->arch != SECCOMP_ARCH_COMPAT
> > > +#endif
> >
> > I don't like this because it's not future-proof enough. __NR_uretprobe
> > may collide with other syscalls at some point.
>
> I'm not sure I got this point.
>
> > And if __NR_uretprobe_32
> > is ever implemented, the seccomp logic will be missing. I think this
> > will work now and in the future:
> >
> > #ifdef __NR_uretprobe
> > # ifdef SECCOMP_ARCH_COMPAT
> > if (sd->arch == SECCOMP_ARCH_COMPAT) {
> > # ifdef __NR_uretprobe_32
> > if (sd->nr == __NR_uretprobe_32)
> > return true;
> > # endif
> > } else
> > # endif
> > if (sd->nr == __NR_uretprobe)
> > return true;
> > #endif
>
> I don't know if implementing uretprobe syscall for compat binaries is
> planned or makes sense - I'd appreciate Jiri's and others opinion on that.
> That said, I don't mind adding this code for the sake of future proofing.
as Andrii wrote in the other email ATM it's just strictly x86_64,
but let's future proof it
AFAIK there was an attempt to do similar on arm but it did not show
any speed up
>
> >
> > Instead of doing a function rename dance, I think you can just stick
> > the above into seccomp_is_const_allow() after the WARN().
>
> My motivation for the renaming dance was that you mentioned we might add
> new syscalls to this as well, so I wanted to avoid cluttering the existing
> function which seems to be well defined.
>
> >
> > Also please add a KUnit tests to cover this in
> > tools/testing/selftests/seccomp/seccomp_bpf.c
>
> I think this would mean that this test suite would need to run as
> privileged. Is that Ok? or maybe it'd be better to have a new suite?
>
> > With at least these cases combinations below. Check each of:
> >
> > - not using uretprobe passes
> > - using uretprobe passes (and validates that uretprobe did work)
> >
> > in each of the following conditions:
> >
> > - default-allow filter
> > - default-block filter
> > - filter explicitly blocking __NR_uretprobe and nothing else
> > - filter explicitly allowing __NR_uretprobe (and only other
> > required syscalls)
>
> Ok.
please let me know if I can help in any way with tests
>
> >
> > Hm, is uretprobe expected to work on mips? Because if so, you'll need to
> > do something similar to the mode1 checking in the !SECCOMP_ARCH_NATIVE
> > version of seccomp_cache_check_allow().
>
> I don't know if uretprobe syscall is expected to run on mips. Personally
> I'd avoid adding this dead code.
>
> >
> > (You can see why I really dislike having policy baked into seccomp!)
>
> I definitely understand :)
>
> >
> > > + )
> > > + return true;
> > > +#endif
> > > +
> > > + return seccomp_is_filter_const_allow(fprog, sd);
> > > +}
> > > +
> > > static void seccomp_cache_prepare_bitmap(struct seccomp_filter *sfilter,
> > > void *bitmap, const void *bitmap_prev,
> > > size_t bitmap_size, int arch)
> > > @@ -1023,6 +1038,9 @@ static inline void seccomp_log(unsigned long syscall, long signr, u32 action,
> > > */
> > > static const int mode1_syscalls[] = {
> > > __NR_seccomp_read, __NR_seccomp_write, __NR_seccomp_exit, __NR_seccomp_sigreturn,
> > > +#ifdef __NR_uretprobe
> > > + __NR_uretprobe,
> > > +#endif
> >
> > It'd be nice to update mode1_syscalls_32 with __NR_uretprobe_32 even
> > though it doesn't exist. (Is it _never_ planned to be implemented?) But
> > then, maybe the chances of a compat mode1 seccomp process running under
> > uretprobe is vanishingly small.
no plans for __NR_uretprobe_32 at this point
>
> It seems to me very unlikely. BTW, when I tested the "strict" mode change
> my program was killed by seccomp. The reason wasn't the uretprobe syscall
> (which I added to the list), it was actually the exit_group syscall which
> libc uses instead of the exit syscall.
>
> Thanks again,
> Eyal.
thanks,
jirka
^ permalink raw reply
* Re: [PATCH v2] seccomp: passthrough uretprobe systemcall without filtering
From: Eyal Birger @ 2025-01-30 15:05 UTC (permalink / raw)
To: Jiri Olsa, Kees Cook
Cc: luto, wad, oleg, mhiramat, andrii, alexei.starovoitov, cyphar,
songliubraving, yhs, john.fastabend, peterz, tglx, bp, daniel,
ast, andrii.nakryiko, rostedt, rafi, shmulik.ladkani, bpf,
linux-api, linux-trace-kernel, x86, linux-kernel, stable
In-Reply-To: <Z5s3S5X8FYJDAHfR@krava>
On Thu, Jan 30, 2025 at 12:24 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Wed, Jan 29, 2025 at 09:27:49AM -0800, Eyal Birger wrote:
> > Hi,
> >
> > Thanks for the review!
> >
> > On Tue, Jan 28, 2025 at 5:41 PM Kees Cook <kees@kernel.org> wrote:
> > >
> > > On Tue, Jan 28, 2025 at 06:58:06AM -0800, Eyal Birger wrote:
> > > > Note: uretprobe isn't supported in i386 and __NR_ia32_rt_tgsigqueueinfo
> > > > uses the same number as __NR_uretprobe so the syscall isn't forced in the
> > > > compat bitmap.
> > >
> > > So a 64-bit tracer cannot use uretprobe on a 32-bit process? Also is
> > > uretprobe strictly an x86_64 feature?
> > >
> >
> > My understanding is that they'd be able to do so, but use the int3 trap
> > instead of the uretprobe syscall.
> >
> > > > [...]
> > > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > > > index 385d48293a5f..23b594a68bc0 100644
> > > > --- a/kernel/seccomp.c
> > > > +++ b/kernel/seccomp.c
> > > > @@ -734,13 +734,13 @@ seccomp_prepare_user_filter(const char __user *user_filter)
> > > >
> > > > #ifdef SECCOMP_ARCH_NATIVE
> > > > /**
> > > > - * seccomp_is_const_allow - check if filter is constant allow with given data
> > > > + * seccomp_is_filter_const_allow - check if filter is constant allow with given data
> > > > * @fprog: The BPF programs
> > > > * @sd: The seccomp data to check against, only syscall number and arch
> > > > * number are considered constant.
> > > > */
> > > > -static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
> > > > - struct seccomp_data *sd)
> > > > +static bool seccomp_is_filter_const_allow(struct sock_fprog_kern *fprog,
> > > > + struct seccomp_data *sd)
> > > > {
> > > > unsigned int reg_value = 0;
> > > > unsigned int pc;
> > > > @@ -812,6 +812,21 @@ static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
> > > > return false;
> > > > }
> > > >
> > > > +static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
> > > > + struct seccomp_data *sd)
> > > > +{
> > > > +#ifdef __NR_uretprobe
> > > > + if (sd->nr == __NR_uretprobe
> > > > +#ifdef SECCOMP_ARCH_COMPAT
> > > > + && sd->arch != SECCOMP_ARCH_COMPAT
> > > > +#endif
> > >
> > > I don't like this because it's not future-proof enough. __NR_uretprobe
> > > may collide with other syscalls at some point.
> >
> > I'm not sure I got this point.
> >
> > > And if __NR_uretprobe_32
> > > is ever implemented, the seccomp logic will be missing. I think this
> > > will work now and in the future:
> > >
> > > #ifdef __NR_uretprobe
> > > # ifdef SECCOMP_ARCH_COMPAT
> > > if (sd->arch == SECCOMP_ARCH_COMPAT) {
> > > # ifdef __NR_uretprobe_32
> > > if (sd->nr == __NR_uretprobe_32)
> > > return true;
> > > # endif
> > > } else
> > > # endif
> > > if (sd->nr == __NR_uretprobe)
> > > return true;
> > > #endif
> >
> > I don't know if implementing uretprobe syscall for compat binaries is
> > planned or makes sense - I'd appreciate Jiri's and others opinion on that.
> > That said, I don't mind adding this code for the sake of future proofing.
>
> as Andrii wrote in the other email ATM it's just strictly x86_64,
> but let's future proof it
Thank you. So I'm ok with using the suggestion above, but more on this below.
>
> AFAIK there was an attempt to do similar on arm but it did not show
> any speed up
>
> >
> > >
> > > Instead of doing a function rename dance, I think you can just stick
> > > the above into seccomp_is_const_allow() after the WARN().
> >
> > My motivation for the renaming dance was that you mentioned we might add
> > new syscalls to this as well, so I wanted to avoid cluttering the existing
> > function which seems to be well defined.
> >
> > >
> > > Also please add a KUnit tests to cover this in
> > > tools/testing/selftests/seccomp/seccomp_bpf.c
> >
> > I think this would mean that this test suite would need to run as
> > privileged. Is that Ok? or maybe it'd be better to have a new suite?
> >
> > > With at least these cases combinations below. Check each of:
> > >
> > > - not using uretprobe passes
> > > - using uretprobe passes (and validates that uretprobe did work)
> > >
> > > in each of the following conditions:
> > >
> > > - default-allow filter
> > > - default-block filter
> > > - filter explicitly blocking __NR_uretprobe and nothing else
> > > - filter explicitly allowing __NR_uretprobe (and only other
> > > required syscalls)
> >
> > Ok.
>
> please let me know if I can help in any way with tests
Thanks! Is there a way to partition this work? I'd appreciate the help
if we can find some way of doing so.
>
> >
> > >
> > > Hm, is uretprobe expected to work on mips? Because if so, you'll need to
> > > do something similar to the mode1 checking in the !SECCOMP_ARCH_NATIVE
> > > version of seccomp_cache_check_allow().
> >
> > I don't know if uretprobe syscall is expected to run on mips. Personally
> > I'd avoid adding this dead code.
Jiri, what is your take on this one?
> >
> > >
> > > (You can see why I really dislike having policy baked into seccomp!)
> >
> > I definitely understand :)
> >
> > >
> > > > + )
> > > > + return true;
> > > > +#endif
> > > > +
> > > > + return seccomp_is_filter_const_allow(fprog, sd);
> > > > +}
> > > > +
> > > > static void seccomp_cache_prepare_bitmap(struct seccomp_filter *sfilter,
> > > > void *bitmap, const void *bitmap_prev,
> > > > size_t bitmap_size, int arch)
> > > > @@ -1023,6 +1038,9 @@ static inline void seccomp_log(unsigned long syscall, long signr, u32 action,
> > > > */
> > > > static const int mode1_syscalls[] = {
> > > > __NR_seccomp_read, __NR_seccomp_write, __NR_seccomp_exit, __NR_seccomp_sigreturn,
> > > > +#ifdef __NR_uretprobe
> > > > + __NR_uretprobe,
> > > > +#endif
> > >
> > > It'd be nice to update mode1_syscalls_32 with __NR_uretprobe_32 even
> > > though it doesn't exist. (Is it _never_ planned to be implemented?) But
> > > then, maybe the chances of a compat mode1 seccomp process running under
> > > uretprobe is vanishingly small.
>
> no plans for __NR_uretprobe_32 at this point
So if we go with the suggestion above, we'll support the theoretical
__NR_uretprobe_32 for filtered seccomp, but not for strict seccomp, and
that's ok because strict seccomp is less common?
Personally I'd prefer to limit the scope of this fix to the problem we
are aware of, and not possible problems should someone decide to reimplement
uretprobes on different archs in a different way. Especially as this fix needs
to be backmerged to stable kernels.
So my personal preference would be to avoid __NR_uretprobe_32 in this patch
and deal with it if it ever gets implemented.
Thoughts and advice appreciated,
Eyal.
^ permalink raw reply
* Re: [PATCH v2] seccomp: passthrough uretprobe systemcall without filtering
From: Oleg Nesterov @ 2025-01-30 15:57 UTC (permalink / raw)
To: Eyal Birger
Cc: Jiri Olsa, Kees Cook, luto, wad, mhiramat, andrii,
alexei.starovoitov, cyphar, songliubraving, yhs, john.fastabend,
peterz, tglx, bp, daniel, ast, andrii.nakryiko, rostedt, rafi,
shmulik.ladkani, bpf, linux-api, linux-trace-kernel, x86,
linux-kernel, stable
In-Reply-To: <CAHsH6GvsGbZ4a=-oSpD1j8jx11T=Y4SysAtkzAu+H4_Gh7v3Qg@mail.gmail.com>
On 01/30, Eyal Birger wrote:
>
> Personally I'd prefer to limit the scope of this fix to the problem we
> are aware of, and not possible problems should someone decide to reimplement
> uretprobes on different archs in a different way. Especially as this fix needs
> to be backmerged to stable kernels.
+1
Oleg.
^ permalink raw reply
* Re: [PATCH v2] seccomp: passthrough uretprobe systemcall without filtering
From: Kees Cook @ 2025-01-30 15:57 UTC (permalink / raw)
To: Eyal Birger
Cc: Jiri Olsa, luto, wad, oleg, mhiramat, andrii, alexei.starovoitov,
cyphar, songliubraving, yhs, john.fastabend, peterz, tglx, bp,
daniel, ast, andrii.nakryiko, rostedt, rafi, shmulik.ladkani, bpf,
linux-api, linux-trace-kernel, x86, linux-kernel, stable
In-Reply-To: <CAHsH6GvsGbZ4a=-oSpD1j8jx11T=Y4SysAtkzAu+H4_Gh7v3Qg@mail.gmail.com>
On Thu, Jan 30, 2025 at 07:05:42AM -0800, Eyal Birger wrote:
> So if we go with the suggestion above, we'll support the theoretical
> __NR_uretprobe_32 for filtered seccomp, but not for strict seccomp, and
> that's ok because strict seccomp is less common?
It's so uncommon I regularly consider removing it entirely. :)
> Personally I'd prefer to limit the scope of this fix to the problem we
> are aware of, and not possible problems should someone decide to reimplement
> uretprobes on different archs in a different way. Especially as this fix needs
> to be backmerged to stable kernels.
> So my personal preference would be to avoid __NR_uretprobe_32 in this patch
> and deal with it if it ever gets implemented.
That's fine, but I want the exception to be designed to fail closed
instead of failing open. I think my proposed future-proof check does
this.
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v2] seccomp: passthrough uretprobe systemcall without filtering
From: Eyal Birger @ 2025-01-30 16:29 UTC (permalink / raw)
To: Kees Cook
Cc: Jiri Olsa, luto, wad, oleg, mhiramat, andrii, alexei.starovoitov,
cyphar, songliubraving, yhs, john.fastabend, peterz, tglx, bp,
daniel, ast, andrii.nakryiko, rostedt, rafi, shmulik.ladkani, bpf,
linux-api, linux-trace-kernel, x86, linux-kernel, stable
In-Reply-To: <202501300756.E473D10@keescook>
On Thu, Jan 30, 2025 at 7:57 AM Kees Cook <kees@kernel.org> wrote:
>
> On Thu, Jan 30, 2025 at 07:05:42AM -0800, Eyal Birger wrote:
> > So if we go with the suggestion above, we'll support the theoretical
> > __NR_uretprobe_32 for filtered seccomp, but not for strict seccomp, and
> > that's ok because strict seccomp is less common?
>
> It's so uncommon I regularly consider removing it entirely. :)
>
> > Personally I'd prefer to limit the scope of this fix to the problem we
> > are aware of, and not possible problems should someone decide to reimplement
> > uretprobes on different archs in a different way. Especially as this fix needs
> > to be backmerged to stable kernels.
> > So my personal preference would be to avoid __NR_uretprobe_32 in this patch
> > and deal with it if it ever gets implemented.
>
> That's fine, but I want the exception to be designed to fail closed
> instead of failing open. I think my proposed future-proof check does
> this.
I think it does. I think the code in the patch does too, since it
avoids the special handling for compat, so defaults to the existing
behavior which blocks the syscall.
Eyal.
^ permalink raw reply
* [PATCH v7 1/6] pidfd: add PIDFD_SELF* sentinels to refer to own thread/process
From: Lorenzo Stoakes @ 2025-01-30 20:40 UTC (permalink / raw)
To: Christian Brauner
Cc: Shuah Khan, Liam R . Howlett, Suren Baghdasaryan, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel, Oliver Sang, John Hubbard, Tejun Heo,
Johannes Weiner, Michal Koutny, Andrew Morton, Shakeel Butt
In-Reply-To: <cover.1738268370.git.lorenzo.stoakes@oracle.com>
It is useful to be able to utilise the pidfd mechanism to reference the
current thread or process (from a userland point of view - thread group
leader from the kernel's point of view).
Therefore introduce PIDFD_SELF_THREAD to refer to the current thread, and
PIDFD_SELF_THREAD_GROUP to refer to the current thread group leader.
For convenience and to avoid confusion from userland's perspective we alias
these:
* PIDFD_SELF is an alias for PIDFD_SELF_THREAD - This is nearly always what
the user will want to use, as they would find it surprising if for
instance fd's were unshared()'d and they wanted to invoke pidfd_getfd()
and that failed.
* PIDFD_SELF_PROCESS is an alias for PIDFD_SELF_THREAD_GROUP - Most users
have no concept of thread groups or what a thread group leader is, and
from userland's perspective and nomenclature this is what userland
considers to be a process.
We adjust pidfd_get_task() and the pidfd_send_signal() system call with
specific handling for this, implementing this functionality for
process_madvise(), process_mrelease() (albeit, using it here wouldn't
really make sense) and pidfd_send_signal().
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
include/uapi/linux/pidfd.h | 24 +++++++++
kernel/pid.c | 24 +++++++--
kernel/signal.c | 106 ++++++++++++++++++++++---------------
3 files changed, 107 insertions(+), 47 deletions(-)
diff --git a/include/uapi/linux/pidfd.h b/include/uapi/linux/pidfd.h
index 4540f6301b8c..e0abd0b18841 100644
--- a/include/uapi/linux/pidfd.h
+++ b/include/uapi/linux/pidfd.h
@@ -23,6 +23,30 @@
#define PIDFD_INFO_SIZE_VER0 64 /* sizeof first published struct */
+/*
+ * The concept of process and threads in userland and the kernel is a confusing
+ * one - within the kernel every thread is a 'task' with its own individual PID,
+ * however from userland's point of view threads are grouped by a single PID,
+ * which is that of the 'thread group leader', typically the first thread
+ * spawned.
+ *
+ * To cut the Gideon knot, for internal kernel usage, we refer to
+ * PIDFD_SELF_THREAD to refer to the current thread (or task from a kernel
+ * perspective), and PIDFD_SELF_THREAD_GROUP to refer to the current thread
+ * group leader...
+ */
+#define PIDFD_SELF_THREAD -10000 /* Current thread. */
+#define PIDFD_SELF_THREAD_GROUP -20000 /* Current thread group leader. */
+
+/*
+ * ...and for userland we make life simpler - PIDFD_SELF refers to the current
+ * thread, PIDFD_SELF_PROCESS refers to the process thread group leader.
+ *
+ * For nearly all practical uses, a user will want to use PIDFD_SELF.
+ */
+#define PIDFD_SELF PIDFD_SELF_THREAD
+#define PIDFD_SELF_PROCESS PIDFD_SELF_THREAD_GROUP
+
struct pidfd_info {
/*
* This mask is similar to the request_mask in statx(2).
diff --git a/kernel/pid.c b/kernel/pid.c
index 3a10a7b6fcf8..1d2fc59d64fc 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -564,15 +564,29 @@ struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags)
*/
struct task_struct *pidfd_get_task(int pidfd, unsigned int *flags)
{
- unsigned int f_flags;
+ unsigned int f_flags = 0;
struct pid *pid;
struct task_struct *task;
+ enum pid_type type;
- pid = pidfd_get_pid(pidfd, &f_flags);
- if (IS_ERR(pid))
- return ERR_CAST(pid);
+ switch (pidfd) {
+ case PIDFD_SELF_THREAD:
+ type = PIDTYPE_PID;
+ pid = get_task_pid(current, type);
+ break;
+ case PIDFD_SELF_THREAD_GROUP:
+ type = PIDTYPE_TGID;
+ pid = get_task_pid(current, type);
+ break;
+ default:
+ pid = pidfd_get_pid(pidfd, &f_flags);
+ if (IS_ERR(pid))
+ return ERR_CAST(pid);
+ type = PIDTYPE_TGID;
+ break;
+ }
- task = get_pid_task(pid, PIDTYPE_TGID);
+ task = get_pid_task(pid, type);
put_pid(pid);
if (!task)
return ERR_PTR(-ESRCH);
diff --git a/kernel/signal.c b/kernel/signal.c
index a2afd54303f0..1e8f792f88de 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -4009,6 +4009,46 @@ static struct pid *pidfd_to_pid(const struct file *file)
(PIDFD_SIGNAL_THREAD | PIDFD_SIGNAL_THREAD_GROUP | \
PIDFD_SIGNAL_PROCESS_GROUP)
+static int do_pidfd_send_signal(struct pid *pid, int sig, enum pid_type type,
+ siginfo_t __user *info, unsigned int flags)
+{
+ kernel_siginfo_t kinfo;
+
+ switch (flags) {
+ case PIDFD_SIGNAL_THREAD:
+ type = PIDTYPE_PID;
+ break;
+ case PIDFD_SIGNAL_THREAD_GROUP:
+ type = PIDTYPE_TGID;
+ break;
+ case PIDFD_SIGNAL_PROCESS_GROUP:
+ type = PIDTYPE_PGID;
+ break;
+ }
+
+ if (info) {
+ int ret = copy_siginfo_from_user_any(&kinfo, info);
+
+ if (unlikely(ret))
+ return ret;
+
+ if (unlikely(sig != kinfo.si_signo))
+ return -EINVAL;
+
+ /* Only allow sending arbitrary signals to yourself. */
+ if ((task_pid(current) != pid || type > PIDTYPE_TGID) &&
+ (kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL))
+ return -EPERM;
+ } else {
+ prepare_kill_siginfo(sig, &kinfo, type);
+ }
+
+ if (type == PIDTYPE_PGID)
+ return kill_pgrp_info(sig, &kinfo, pid);
+
+ return kill_pid_info_type(sig, &kinfo, pid, type);
+}
+
/**
* sys_pidfd_send_signal - Signal a process through a pidfd
* @pidfd: file descriptor of the process
@@ -4028,7 +4068,6 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
{
int ret;
struct pid *pid;
- kernel_siginfo_t kinfo;
enum pid_type type;
/* Enforce flags be set to 0 until we add an extension. */
@@ -4040,54 +4079,37 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
return -EINVAL;
CLASS(fd, f)(pidfd);
- if (fd_empty(f))
- return -EBADF;
- /* Is this a pidfd? */
- pid = pidfd_to_pid(fd_file(f));
- if (IS_ERR(pid))
- return PTR_ERR(pid);
+ switch (pidfd) {
+ case PIDFD_SELF_THREAD:
+ pid = get_task_pid(current, PIDTYPE_PID);
+ type = PIDTYPE_PID;
+ break;
+ case PIDFD_SELF_THREAD_GROUP:
+ pid = get_task_pid(current, PIDTYPE_TGID);
+ type = PIDTYPE_TGID;
+ break;
+ default:
+ if (fd_empty(f))
+ return -EBADF;
- if (!access_pidfd_pidns(pid))
- return -EINVAL;
+ /* Is this a pidfd? */
+ pid = pidfd_to_pid(fd_file(f));
+ if (IS_ERR(pid))
+ return PTR_ERR(pid);
- switch (flags) {
- case 0:
+ if (!access_pidfd_pidns(pid))
+ return -EINVAL;
/* Infer scope from the type of pidfd. */
if (fd_file(f)->f_flags & PIDFD_THREAD)
type = PIDTYPE_PID;
else
type = PIDTYPE_TGID;
break;
- case PIDFD_SIGNAL_THREAD:
- type = PIDTYPE_PID;
- break;
- case PIDFD_SIGNAL_THREAD_GROUP:
- type = PIDTYPE_TGID;
- break;
- case PIDFD_SIGNAL_PROCESS_GROUP:
- type = PIDTYPE_PGID;
- break;
- }
-
- if (info) {
- ret = copy_siginfo_from_user_any(&kinfo, info);
- if (unlikely(ret))
- return ret;
-
- if (unlikely(sig != kinfo.si_signo))
- return -EINVAL;
-
- /* Only allow sending arbitrary signals to yourself. */
- if ((task_pid(current) != pid || type > PIDTYPE_TGID) &&
- (kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL))
- return -EPERM;
- } else {
- prepare_kill_siginfo(sig, &kinfo, type);
}
- if (type == PIDTYPE_PGID)
- return kill_pgrp_info(sig, &kinfo, pid);
- else
- return kill_pid_info_type(sig, &kinfo, pid, type);
+ ret = do_pidfd_send_signal(pid, sig, type, info, flags);
+ if (fd_empty(f))
+ put_pid(pid);
+ return ret;
}
--
2.48.1
^ permalink raw reply related
* [PATCH v7 2/6] selftests/pidfd: add missing system header imcludes to pidfd tests
From: Lorenzo Stoakes @ 2025-01-30 20:40 UTC (permalink / raw)
To: Christian Brauner
Cc: Shuah Khan, Liam R . Howlett, Suren Baghdasaryan, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel, Oliver Sang, John Hubbard, Tejun Heo,
Johannes Weiner, Michal Koutny, Andrew Morton, Shakeel Butt
In-Reply-To: <cover.1738268370.git.lorenzo.stoakes@oracle.com>
The pidfd_fdinfo_test.c and pidfd_setns_test.c tests appear to be missing
fundamental system header imports required to execute correctly. Add these.
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
tools/testing/selftests/pidfd/pidfd_fdinfo_test.c | 1 +
tools/testing/selftests/pidfd/pidfd_setns_test.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/tools/testing/selftests/pidfd/pidfd_fdinfo_test.c b/tools/testing/selftests/pidfd/pidfd_fdinfo_test.c
index f062a986e382..f718aac75068 100644
--- a/tools/testing/selftests/pidfd/pidfd_fdinfo_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_fdinfo_test.c
@@ -13,6 +13,7 @@
#include <syscall.h>
#include <sys/wait.h>
#include <sys/mman.h>
+#include <sys/mount.h>
#include "pidfd.h"
#include "../kselftest.h"
diff --git a/tools/testing/selftests/pidfd/pidfd_setns_test.c b/tools/testing/selftests/pidfd/pidfd_setns_test.c
index 222f8131283b..a55f6641e0b6 100644
--- a/tools/testing/selftests/pidfd/pidfd_setns_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_setns_test.c
@@ -14,6 +14,7 @@
#include <sys/prctl.h>
#include <sys/wait.h>
#include <unistd.h>
+#include <sys/ioctl.h>
#include <sys/socket.h>
#include <sys/stat.h>
#include <linux/ioctl.h>
--
2.48.1
^ permalink raw reply related
* [PATCH v7 3/6] tools: testing: separate out wait_for_pid() into helper header
From: Lorenzo Stoakes @ 2025-01-30 20:40 UTC (permalink / raw)
To: Christian Brauner
Cc: Shuah Khan, Liam R . Howlett, Suren Baghdasaryan, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel, Oliver Sang, John Hubbard, Tejun Heo,
Johannes Weiner, Michal Koutny, Andrew Morton, Shakeel Butt
In-Reply-To: <cover.1738268370.git.lorenzo.stoakes@oracle.com>
It seems tests other than the pidfd tests use the wait_for_pid() function
declared in pidfd.h.
Since we will shortly be modifying pidfd.h in a way that might clash with
other tests, separate this out and update tests accordingly.
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
tools/testing/selftests/cgroup/test_kill.c | 2 +-
.../pid_namespace/regression_enomem.c | 2 +-
tools/testing/selftests/pidfd/pidfd.h | 26 +------------
tools/testing/selftests/pidfd/pidfd_helpers.h | 39 +++++++++++++++++++
4 files changed, 42 insertions(+), 27 deletions(-)
create mode 100644 tools/testing/selftests/pidfd/pidfd_helpers.h
diff --git a/tools/testing/selftests/cgroup/test_kill.c b/tools/testing/selftests/cgroup/test_kill.c
index 0e5bb6c7307a..2367f645fe89 100644
--- a/tools/testing/selftests/cgroup/test_kill.c
+++ b/tools/testing/selftests/cgroup/test_kill.c
@@ -10,7 +10,7 @@
#include <unistd.h>
#include "../kselftest.h"
-#include "../pidfd/pidfd.h"
+#include "../pidfd/pidfd_helpers.h"
#include "cgroup_util.h"
/*
diff --git a/tools/testing/selftests/pid_namespace/regression_enomem.c b/tools/testing/selftests/pid_namespace/regression_enomem.c
index 7d84097ad45c..f3e6989c8069 100644
--- a/tools/testing/selftests/pid_namespace/regression_enomem.c
+++ b/tools/testing/selftests/pid_namespace/regression_enomem.c
@@ -12,7 +12,7 @@
#include <sys/wait.h>
#include "../kselftest_harness.h"
-#include "../pidfd/pidfd.h"
+#include "../pidfd/pidfd_helpers.h"
/*
* Regression test for:
diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h
index 0b96ac4b8ce5..d02cfc5ef77b 100644
--- a/tools/testing/selftests/pidfd/pidfd.h
+++ b/tools/testing/selftests/pidfd/pidfd.h
@@ -17,6 +17,7 @@
#include "../kselftest.h"
#include "../clone3/clone3_selftests.h"
+#include "pidfd_helpers.h"
#ifndef P_PIDFD
#define P_PIDFD 3
@@ -73,31 +74,6 @@ static inline int sys_waitid(int which, pid_t pid, siginfo_t *info, int options)
return syscall(__NR_waitid, which, pid, info, options, NULL);
}
-static inline int wait_for_pid(pid_t pid)
-{
- int status, ret;
-
-again:
- ret = waitpid(pid, &status, 0);
- if (ret == -1) {
- if (errno == EINTR)
- goto again;
-
- ksft_print_msg("waitpid returned -1, errno=%d\n", errno);
- return -1;
- }
-
- if (!WIFEXITED(status)) {
- ksft_print_msg(
- "waitpid !WIFEXITED, WIFSIGNALED=%d, WTERMSIG=%d\n",
- WIFSIGNALED(status), WTERMSIG(status));
- return -1;
- }
-
- ret = WEXITSTATUS(status);
- return ret;
-}
-
static inline int sys_pidfd_open(pid_t pid, unsigned int flags)
{
return syscall(__NR_pidfd_open, pid, flags);
diff --git a/tools/testing/selftests/pidfd/pidfd_helpers.h b/tools/testing/selftests/pidfd/pidfd_helpers.h
new file mode 100644
index 000000000000..5637bfe888de
--- /dev/null
+++ b/tools/testing/selftests/pidfd/pidfd_helpers.h
@@ -0,0 +1,39 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __PIDFD_HELPERS_H
+#define __PIDFD_HELPERS_H
+
+#define _GNU_SOURCE
+#include <errno.h>
+#include <stdlib.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <unistd.h>
+#include "../kselftest.h"
+
+static inline int wait_for_pid(pid_t pid)
+{
+ int status, ret;
+
+again:
+ ret = waitpid(pid, &status, 0);
+ if (ret == -1) {
+ if (errno == EINTR)
+ goto again;
+
+ ksft_print_msg("waitpid returned -1, errno=%d\n", errno);
+ return -1;
+ }
+
+ if (!WIFEXITED(status)) {
+ ksft_print_msg(
+ "waitpid !WIFEXITED, WIFSIGNALED=%d, WTERMSIG=%d\n",
+ WIFSIGNALED(status), WTERMSIG(status));
+ return -1;
+ }
+
+ ret = WEXITSTATUS(status);
+ return ret;
+}
+
+#endif /* __PIDFD_HELPERS_H */
--
2.48.1
^ permalink raw reply related
* [PATCH v7 0/6] introduce PIDFD_SELF* sentinels
From: Lorenzo Stoakes @ 2025-01-30 20:40 UTC (permalink / raw)
To: Christian Brauner
Cc: Shuah Khan, Liam R . Howlett, Suren Baghdasaryan, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel, Oliver Sang, John Hubbard, Tejun Heo,
Johannes Weiner, Michal Koutny, Andrew Morton, Shakeel Butt
If you wish to utilise a pidfd interface to refer to the current process or
thread it is rather cumbersome, requiring something like:
int pidfd = pidfd_open(getpid(), 0 or PIDFD_THREAD);
...
close(pidfd);
Or the equivalent call opening /proc/self. It is more convenient to use a
sentinel value to indicate to an interface that accepts a pidfd that we
simply wish to refer to the current process thread.
This series introduces sentinels for this purposes which can be passed as
the pidfd in this instance rather than having to establish a dummy fd for
this purpose.
It is useful to refer to both the current thread from the userland's
perspective for which we use PIDFD_SELF, and the current process from the
userland's perspective, for which we use PIDFD_SELF_PROCESS.
There is unfortunately some confusion between the kernel and userland as to
what constitutes a process - a thread from the userland perspective is a
process in userland, and a userland process is a thread group (more
specifically the thread group leader from the kernel perspective). We
therefore alias things thusly:
* PIDFD_SELF_THREAD aliased by PIDFD_SELF - use PIDTYPE_PID.
* PIDFD_SELF_THREAD_GROUP alised by PIDFD_SELF_PROCESS - use PIDTYPE_TGID.
In all of the kernel code we refer to PIDFD_SELF_THREAD and
PIDFD_SELF_THREAD_GROUP. However we expect users to use PIDFD_SELF and
PIDFD_SELF_PROCESS.
This matters for cases where, for instance, a user unshare()'s FDs or does
thread-specific signal handling and where the user would be hugely confused
if the FDs referenced or signal processed referred to the thread group
leader rather than the individual thread.
For now we only adjust pidfd_get_task() and the pidfd_send_signal() system
call with specific handling for this, implementing this functionality for
process_madvise(), process_mrelease() (albeit, using it here wouldn't
really make sense) and pidfd_send_signal().
We defer making further changes, as this would require a significant rework
of the pidfd mechanism.
The motivating case here is to support PIDFD_SELF in process_madvise(), so
this suffices for immediate uses. Moving forward, this can be further
expanded to other uses.
v7:
* Reworked implementation according to Christian's requirements. We now
only support process_madvise() and pidfd_send_signal() system calls with
PIDFD_SELF as specified.
* Updated tests to account for broken pidfd_open_test.c implementation.
* Fixed missing includes in pidfd self tests.
* Removed tests relating to functionality no longer supported.
* Update guard pages test to use PIDFD_SELF.
v6:
* Avoid static inline in UAPI header as suggested by Pedro.
* Place PIDFD_SELF values out of range of errors and any other sentinel as
suggested by Pedro.
https://lore.kernel.org/linux-mm/cover.1729926229.git.lorenzo.stoakes@oracle.com/
v5:
* Fixup self test dependencies on pidfd/pidfd.h.
https://lore.kernel.org/linux-mm/cover.1729848252.git.lorenzo.stoakes@oracle.com/
v4:
* Avoid returning an fd in the __pidfd_get_pid() function as pointed out by
Christian, instead simply always pin the pid and maintain fd scope in the
helper alone.
* Add wrapper header file in tools/include/linux to allow for import of
UAPI pidfd.h header without encountering the collision between system
fcntl.h and linux/fcntl.h as discussed with Shuah and John.
* Fixup tests to import the UAPI pidfd.h header working around conflicts
between system fcntl.h and linux/fcntl.h which the UAPI pidfd.h imports,
as reported by Shuah.
* Use an int for pidfd_is_self_sentinel() to avoid any dependency on
stdbool.h in userland.
https://lore.kernel.org/linux-mm/cover.1729198898.git.lorenzo.stoakes@oracle.com/
v3:
* Do not fput() an invalid fd as reported by kernel test bot.
* Fix unintended churn from moving variable declaration.
https://lore.kernel.org/linux-mm/cover.1729073310.git.lorenzo.stoakes@oracle.com/
v2:
* Fix tests as reported by Shuah.
* Correct RFC version lore link.
https://lore.kernel.org/linux-mm/cover.1728643714.git.lorenzo.stoakes@oracle.com/
Non-RFC v1:
* Removed RFC tag - there seems to be general consensus that this change is
a good idea, but perhaps some debate to be had on implementation. It
seems sensible then to move forward with the RFC flag removed.
* Introduced PIDFD_SELF_THREAD, PIDFD_SELF_THREAD_GROUP and their aliases
PIDFD_SELF and PIDFD_SELF_PROCESS respectively.
* Updated testing accordingly.
https://lore.kernel.org/linux-mm/cover.1728578231.git.lorenzo.stoakes@oracle.com/
RFC version:
https://lore.kernel.org/linux-mm/cover.1727644404.git.lorenzo.stoakes@oracle.com/
Lorenzo Stoakes (6):
pidfd: add PIDFD_SELF* sentinels to refer to own thread/process
selftests/pidfd: add missing system header imcludes to pidfd tests
tools: testing: separate out wait_for_pid() into helper header
selftests: pidfd: add pidfd.h UAPI wrapper
selftests: pidfd: add tests for PIDFD_SELF_*
selftests/mm: use PIDFD_SELF in guard pages test
include/uapi/linux/pidfd.h | 24 ++++
kernel/pid.c | 24 +++-
kernel/signal.c | 106 +++++++++++-------
tools/include/linux/pidfd.h | 14 +++
tools/testing/selftests/cgroup/test_kill.c | 2 +-
tools/testing/selftests/mm/Makefile | 4 +
tools/testing/selftests/mm/guard-pages.c | 15 +--
.../pid_namespace/regression_enomem.c | 2 +-
tools/testing/selftests/pidfd/Makefile | 3 +-
tools/testing/selftests/pidfd/pidfd.h | 28 +----
.../selftests/pidfd/pidfd_fdinfo_test.c | 1 +
tools/testing/selftests/pidfd/pidfd_helpers.h | 39 +++++++
.../testing/selftests/pidfd/pidfd_open_test.c | 6 +-
.../selftests/pidfd/pidfd_setns_test.c | 1 +
tools/testing/selftests/pidfd/pidfd_test.c | 76 +++++++++++--
15 files changed, 242 insertions(+), 103 deletions(-)
create mode 100644 tools/include/linux/pidfd.h
create mode 100644 tools/testing/selftests/pidfd/pidfd_helpers.h
--
2.48.1
^ permalink raw reply
* [PATCH v7 4/6] selftests: pidfd: add pidfd.h UAPI wrapper
From: Lorenzo Stoakes @ 2025-01-30 20:40 UTC (permalink / raw)
To: Christian Brauner
Cc: Shuah Khan, Liam R . Howlett, Suren Baghdasaryan, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel, Oliver Sang, John Hubbard, Tejun Heo,
Johannes Weiner, Michal Koutny, Andrew Morton, Shakeel Butt
In-Reply-To: <cover.1738268370.git.lorenzo.stoakes@oracle.com>
Conflicts can arise between system fcntl.h and linux/fcntl.h, imported by
the linux/pidfd.h UAPI header.
Work around this by adding a wrapper for linux/pidfd.h to
tools/include/ which sets the linux/fcntl.h header guard ahead of
importing the pidfd.h header file.
Adjust the pidfd selftests Makefile to reference this include directory and
put it at a higher precidence than any make header installed headers to
ensure the wrapper is preferred.
This way we can directly import the UAPI header file without issue, use the
latest system header file without having to duplicate anything.
Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
tools/include/linux/pidfd.h | 14 ++++++++++++++
tools/testing/selftests/pidfd/Makefile | 3 +--
2 files changed, 15 insertions(+), 2 deletions(-)
create mode 100644 tools/include/linux/pidfd.h
diff --git a/tools/include/linux/pidfd.h b/tools/include/linux/pidfd.h
new file mode 100644
index 000000000000..113c8023072d
--- /dev/null
+++ b/tools/include/linux/pidfd.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef _TOOLS_LINUX_PIDFD_H
+#define _TOOLS_LINUX_PIDFD_H
+
+/*
+ * Some systems have issues with the linux/fcntl.h import in linux/pidfd.h, so
+ * work around this by setting the header guard.
+ */
+#define _LINUX_FCNTL_H
+#include "../../../include/uapi/linux/pidfd.h"
+#undef _LINUX_FCNTL_H
+
+#endif /* _TOOLS_LINUX_PIDFD_H */
diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile
index 301343a11b62..5363d5ab27a4 100644
--- a/tools/testing/selftests/pidfd/Makefile
+++ b/tools/testing/selftests/pidfd/Makefile
@@ -1,9 +1,8 @@
# SPDX-License-Identifier: GPL-2.0-only
-CFLAGS += -g $(KHDR_INCLUDES) -pthread -Wall
+CFLAGS += -g -isystem $(top_srcdir)/tools/include $(KHDR_INCLUDES) -pthread -Wall
TEST_GEN_PROGS := pidfd_test pidfd_fdinfo_test pidfd_open_test \
pidfd_poll_test pidfd_wait pidfd_getfd_test pidfd_setns_test \
pidfd_file_handle_test pidfd_bind_mount
include ../lib.mk
-
--
2.48.1
^ permalink raw reply related
* [PATCH v7 5/6] selftests: pidfd: add tests for PIDFD_SELF_*
From: Lorenzo Stoakes @ 2025-01-30 20:40 UTC (permalink / raw)
To: Christian Brauner
Cc: Shuah Khan, Liam R . Howlett, Suren Baghdasaryan, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel, Oliver Sang, John Hubbard, Tejun Heo,
Johannes Weiner, Michal Koutny, Andrew Morton, Shakeel Butt
In-Reply-To: <cover.1738268370.git.lorenzo.stoakes@oracle.com>
Add tests to assert that PIDFD_SELF* correctly refers to the current
thread and process.
We explicitly test pidfd_send_signal(), however We defer testing of
mm-specific functionality which uses pidfd, namely process_madvise() and
process_mrelease() to mm testing (though note the latter can not be
sensibly tested as it would require the testing process to be dying).
We also correct the pidfd_open_test.c fields which refer to .request_mask
whereas the UAPI header refers to .mask, which otherwise break the import
of the UAPI header.
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
tools/testing/selftests/pidfd/pidfd.h | 2 +
.../testing/selftests/pidfd/pidfd_open_test.c | 6 +-
tools/testing/selftests/pidfd/pidfd_test.c | 76 ++++++++++++++++---
3 files changed, 69 insertions(+), 15 deletions(-)
diff --git a/tools/testing/selftests/pidfd/pidfd.h b/tools/testing/selftests/pidfd/pidfd.h
index d02cfc5ef77b..cc684d872253 100644
--- a/tools/testing/selftests/pidfd/pidfd.h
+++ b/tools/testing/selftests/pidfd/pidfd.h
@@ -15,6 +15,8 @@
#include <sys/types.h>
#include <sys/wait.h>
+#include <linux/pidfd.h>
+
#include "../kselftest.h"
#include "../clone3/clone3_selftests.h"
#include "pidfd_helpers.h"
diff --git a/tools/testing/selftests/pidfd/pidfd_open_test.c b/tools/testing/selftests/pidfd/pidfd_open_test.c
index ce413a221bac..9a40ccb1ff6d 100644
--- a/tools/testing/selftests/pidfd/pidfd_open_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_open_test.c
@@ -31,7 +31,7 @@
#define PIDFD_INFO_CGROUPID (1UL << 0)
struct pidfd_info {
- __u64 request_mask;
+ __u64 mask;
__u64 cgroupid;
__u32 pid;
__u32 tgid;
@@ -148,7 +148,7 @@ static pid_t get_pid_from_fdinfo_file(int pidfd, const char *key, size_t keylen)
int main(int argc, char **argv)
{
struct pidfd_info info = {
- .request_mask = PIDFD_INFO_CGROUPID,
+ .mask = PIDFD_INFO_CGROUPID,
};
int pidfd = -1, ret = 1;
pid_t pid;
@@ -227,7 +227,7 @@ int main(int argc, char **argv)
getegid(), info.sgid);
goto on_error;
}
- if ((info.request_mask & PIDFD_INFO_CGROUPID) && info.cgroupid == 0) {
+ if ((info.mask & PIDFD_INFO_CGROUPID) && info.cgroupid == 0) {
ksft_print_msg("cgroupid should not be 0 when PIDFD_INFO_CGROUPID is set\n");
goto on_error;
}
diff --git a/tools/testing/selftests/pidfd/pidfd_test.c b/tools/testing/selftests/pidfd/pidfd_test.c
index e9728e86b4f2..fcd85cad9f18 100644
--- a/tools/testing/selftests/pidfd/pidfd_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_test.c
@@ -42,12 +42,41 @@ static pid_t pidfd_clone(int flags, int *pidfd, int (*fn)(void *))
#endif
}
-static int signal_received;
+static pthread_t signal_received;
static void set_signal_received_on_sigusr1(int sig)
{
if (sig == SIGUSR1)
- signal_received = 1;
+ signal_received = pthread_self();
+}
+
+static int send_signal(int pidfd)
+{
+ int ret = 0;
+
+ if (sys_pidfd_send_signal(pidfd, SIGUSR1, NULL, 0) < 0) {
+ ret = -EINVAL;
+ goto exit;
+ }
+
+ if (signal_received != pthread_self()) {
+ ret = -EINVAL;
+ goto exit;
+ }
+
+exit:
+ signal_received = 0;
+ return ret;
+}
+
+static void *send_signal_worker(void *arg)
+{
+ int pidfd = (int)(intptr_t)arg;
+ int ret;
+
+ /* We forward any errors for the caller to handle. */
+ ret = send_signal(pidfd);
+ return (void *)(intptr_t)ret;
}
/*
@@ -56,8 +85,11 @@ static void set_signal_received_on_sigusr1(int sig)
*/
static int test_pidfd_send_signal_simple_success(void)
{
- int pidfd, ret;
+ int pidfd;
const char *test_name = "pidfd_send_signal send SIGUSR1";
+ pthread_t thread;
+ void *thread_res;
+ int err;
if (!have_pidfd_send_signal) {
ksft_test_result_skip(
@@ -66,25 +98,45 @@ static int test_pidfd_send_signal_simple_success(void)
return 0;
}
+ signal(SIGUSR1, set_signal_received_on_sigusr1);
+
+ /* Try sending a signal to ourselves via /proc/self. */
pidfd = open("/proc/self", O_DIRECTORY | O_CLOEXEC);
if (pidfd < 0)
ksft_exit_fail_msg(
"%s test: Failed to open process file descriptor\n",
test_name);
+ err = send_signal(pidfd);
+ if (err)
+ ksft_exit_fail_msg(
+ "%s test: Error %d on sending pidfd signal\n",
+ test_name, err);
+ close(pidfd);
- signal(SIGUSR1, set_signal_received_on_sigusr1);
+ /* Now try the same thing only using PIDFD_SELF_THREAD_GROUP. */
+ err = send_signal(PIDFD_SELF_THREAD_GROUP);
+ if (err)
+ ksft_exit_fail_msg(
+ "%s test: Error %d on PIDFD_SELF_THREAD_GROUP signal\n",
+ test_name, err);
- ret = sys_pidfd_send_signal(pidfd, SIGUSR1, NULL, 0);
- close(pidfd);
- if (ret < 0)
- ksft_exit_fail_msg("%s test: Failed to send signal\n",
+ /*
+ * Now try the same thing in a thread and assert thread ID is equal to
+ * worker thread ID.
+ */
+ if (pthread_create(&thread, NULL, send_signal_worker,
+ (void *)(intptr_t)PIDFD_SELF_THREAD))
+ ksft_exit_fail_msg("%s test: Failed to create thread\n",
test_name);
-
- if (signal_received != 1)
- ksft_exit_fail_msg("%s test: Failed to receive signal\n",
+ if (pthread_join(thread, &thread_res))
+ ksft_exit_fail_msg("%s test: Failed to join thread\n",
test_name);
+ err = (int)(intptr_t)thread_res;
+ if (err)
+ ksft_exit_fail_msg(
+ "%s test: Error %d on PIDFD_SELF_THREAD signal\n",
+ test_name, err);
- signal_received = 0;
ksft_test_result_pass("%s test: Sent signal\n", test_name);
return 0;
}
--
2.48.1
^ permalink raw reply related
* [PATCH v7 6/6] selftests/mm: use PIDFD_SELF in guard pages test
From: Lorenzo Stoakes @ 2025-01-30 20:40 UTC (permalink / raw)
To: Christian Brauner
Cc: Shuah Khan, Liam R . Howlett, Suren Baghdasaryan, Vlastimil Babka,
pedro.falcato, linux-kselftest, linux-mm, linux-fsdevel,
linux-api, linux-kernel, Oliver Sang, John Hubbard, Tejun Heo,
Johannes Weiner, Michal Koutny, Andrew Morton, Shakeel Butt
In-Reply-To: <cover.1738268370.git.lorenzo.stoakes@oracle.com>
Now we have PIDFD_SELF available for process_madvise(), make use of it in
the guard pages test.
This is both more convenient and asserts that PIDFD_SELF works as expected.
Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
tools/testing/selftests/mm/Makefile | 4 ++++
tools/testing/selftests/mm/guard-pages.c | 15 +++------------
2 files changed, 7 insertions(+), 12 deletions(-)
diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile
index 63ce39d024bb..ecc4252fa3fe 100644
--- a/tools/testing/selftests/mm/Makefile
+++ b/tools/testing/selftests/mm/Makefile
@@ -225,6 +225,10 @@ $(OUTPUT)/ksm_tests: LDLIBS += -lnuma
$(OUTPUT)/migration: LDLIBS += -lnuma
+# We need uapi/pdifd.h but need to work around broken linux/fcntl.h, so
+# explicitly import
+$(OUTPUT)/guard-pages: CFLAGS += -I $(top_srcdir)/tools/include
+
local_config.mk local_config.h: check_config.sh
/bin/sh ./check_config.sh $(CC)
diff --git a/tools/testing/selftests/mm/guard-pages.c b/tools/testing/selftests/mm/guard-pages.c
index ece37212a8a2..549653724661 100644
--- a/tools/testing/selftests/mm/guard-pages.c
+++ b/tools/testing/selftests/mm/guard-pages.c
@@ -18,6 +18,7 @@
#include <sys/syscall.h>
#include <sys/uio.h>
#include <unistd.h>
+#include <linux/pidfd.h>
/*
* Ignore the checkpatch warning, as per the C99 standard, section 7.14.1.1:
@@ -50,11 +51,6 @@ static void handle_fatal(int c)
siglongjmp(signal_jmp_buf, c);
}
-static int pidfd_open(pid_t pid, unsigned int flags)
-{
- return syscall(SYS_pidfd_open, pid, flags);
-}
-
static ssize_t sys_process_madvise(int pidfd, const struct iovec *iovec,
size_t n, int advice, unsigned int flags)
{
@@ -370,14 +366,10 @@ TEST_F(guard_pages, multi_vma)
TEST_F(guard_pages, process_madvise)
{
const unsigned long page_size = self->page_size;
- pid_t pid = getpid();
- int pidfd = pidfd_open(pid, 0);
char *ptr_region, *ptr1, *ptr2, *ptr3;
ssize_t count;
struct iovec vec[6];
- ASSERT_NE(pidfd, -1);
-
/* Reserve region to map over. */
ptr_region = mmap(NULL, 100 * page_size, PROT_NONE,
MAP_ANON | MAP_PRIVATE, -1, 0);
@@ -425,7 +417,7 @@ TEST_F(guard_pages, process_madvise)
ASSERT_EQ(munmap(&ptr_region[99 * page_size], page_size), 0);
/* Now guard in one step. */
- count = sys_process_madvise(pidfd, vec, 6, MADV_GUARD_INSTALL, 0);
+ count = sys_process_madvise(PIDFD_SELF, vec, 6, MADV_GUARD_INSTALL, 0);
/* OK we don't have permission to do this, skip. */
if (count == -1 && errno == EPERM)
@@ -446,7 +438,7 @@ TEST_F(guard_pages, process_madvise)
ASSERT_FALSE(try_read_write_buf(&ptr3[19 * page_size]));
/* Now do the same with unguard... */
- count = sys_process_madvise(pidfd, vec, 6, MADV_GUARD_REMOVE, 0);
+ count = sys_process_madvise(PIDFD_SELF, vec, 6, MADV_GUARD_REMOVE, 0);
/* ...and everything should now succeed. */
@@ -463,7 +455,6 @@ TEST_F(guard_pages, process_madvise)
ASSERT_EQ(munmap(ptr1, 10 * page_size), 0);
ASSERT_EQ(munmap(ptr2, 5 * page_size), 0);
ASSERT_EQ(munmap(ptr3, 20 * page_size), 0);
- close(pidfd);
}
/* Assert that unmapping ranges does not leave guard markers behind. */
--
2.48.1
^ permalink raw reply related
* Re: [PATCH v2] seccomp: passthrough uretprobe systemcall without filtering
From: Jiri Olsa @ 2025-01-30 21:53 UTC (permalink / raw)
To: Eyal Birger
Cc: Jiri Olsa, Kees Cook, luto, wad, oleg, mhiramat, andrii,
alexei.starovoitov, cyphar, songliubraving, yhs, john.fastabend,
peterz, tglx, bp, daniel, ast, andrii.nakryiko, rostedt, rafi,
shmulik.ladkani, bpf, linux-api, linux-trace-kernel, x86,
linux-kernel, stable
In-Reply-To: <CAHsH6GvsGbZ4a=-oSpD1j8jx11T=Y4SysAtkzAu+H4_Gh7v3Qg@mail.gmail.com>
On Thu, Jan 30, 2025 at 07:05:42AM -0800, Eyal Birger wrote:
> On Thu, Jan 30, 2025 at 12:24 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Wed, Jan 29, 2025 at 09:27:49AM -0800, Eyal Birger wrote:
> > > Hi,
> > >
> > > Thanks for the review!
> > >
> > > On Tue, Jan 28, 2025 at 5:41 PM Kees Cook <kees@kernel.org> wrote:
> > > >
> > > > On Tue, Jan 28, 2025 at 06:58:06AM -0800, Eyal Birger wrote:
> > > > > Note: uretprobe isn't supported in i386 and __NR_ia32_rt_tgsigqueueinfo
> > > > > uses the same number as __NR_uretprobe so the syscall isn't forced in the
> > > > > compat bitmap.
> > > >
> > > > So a 64-bit tracer cannot use uretprobe on a 32-bit process? Also is
> > > > uretprobe strictly an x86_64 feature?
> > > >
> > >
> > > My understanding is that they'd be able to do so, but use the int3 trap
> > > instead of the uretprobe syscall.
> > >
> > > > > [...]
> > > > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > > > > index 385d48293a5f..23b594a68bc0 100644
> > > > > --- a/kernel/seccomp.c
> > > > > +++ b/kernel/seccomp.c
> > > > > @@ -734,13 +734,13 @@ seccomp_prepare_user_filter(const char __user *user_filter)
> > > > >
> > > > > #ifdef SECCOMP_ARCH_NATIVE
> > > > > /**
> > > > > - * seccomp_is_const_allow - check if filter is constant allow with given data
> > > > > + * seccomp_is_filter_const_allow - check if filter is constant allow with given data
> > > > > * @fprog: The BPF programs
> > > > > * @sd: The seccomp data to check against, only syscall number and arch
> > > > > * number are considered constant.
> > > > > */
> > > > > -static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
> > > > > - struct seccomp_data *sd)
> > > > > +static bool seccomp_is_filter_const_allow(struct sock_fprog_kern *fprog,
> > > > > + struct seccomp_data *sd)
> > > > > {
> > > > > unsigned int reg_value = 0;
> > > > > unsigned int pc;
> > > > > @@ -812,6 +812,21 @@ static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
> > > > > return false;
> > > > > }
> > > > >
> > > > > +static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
> > > > > + struct seccomp_data *sd)
> > > > > +{
> > > > > +#ifdef __NR_uretprobe
> > > > > + if (sd->nr == __NR_uretprobe
> > > > > +#ifdef SECCOMP_ARCH_COMPAT
> > > > > + && sd->arch != SECCOMP_ARCH_COMPAT
> > > > > +#endif
> > > >
> > > > I don't like this because it's not future-proof enough. __NR_uretprobe
> > > > may collide with other syscalls at some point.
> > >
> > > I'm not sure I got this point.
> > >
> > > > And if __NR_uretprobe_32
> > > > is ever implemented, the seccomp logic will be missing. I think this
> > > > will work now and in the future:
> > > >
> > > > #ifdef __NR_uretprobe
> > > > # ifdef SECCOMP_ARCH_COMPAT
> > > > if (sd->arch == SECCOMP_ARCH_COMPAT) {
> > > > # ifdef __NR_uretprobe_32
> > > > if (sd->nr == __NR_uretprobe_32)
> > > > return true;
> > > > # endif
> > > > } else
> > > > # endif
> > > > if (sd->nr == __NR_uretprobe)
> > > > return true;
> > > > #endif
> > >
> > > I don't know if implementing uretprobe syscall for compat binaries is
> > > planned or makes sense - I'd appreciate Jiri's and others opinion on that.
> > > That said, I don't mind adding this code for the sake of future proofing.
> >
> > as Andrii wrote in the other email ATM it's just strictly x86_64,
> > but let's future proof it
>
> Thank you. So I'm ok with using the suggestion above, but more on this below.
>
> >
> > AFAIK there was an attempt to do similar on arm but it did not show
> > any speed up
> >
> > >
> > > >
> > > > Instead of doing a function rename dance, I think you can just stick
> > > > the above into seccomp_is_const_allow() after the WARN().
> > >
> > > My motivation for the renaming dance was that you mentioned we might add
> > > new syscalls to this as well, so I wanted to avoid cluttering the existing
> > > function which seems to be well defined.
> > >
> > > >
> > > > Also please add a KUnit tests to cover this in
> > > > tools/testing/selftests/seccomp/seccomp_bpf.c
> > >
> > > I think this would mean that this test suite would need to run as
> > > privileged. Is that Ok? or maybe it'd be better to have a new suite?
> > >
> > > > With at least these cases combinations below. Check each of:
> > > >
> > > > - not using uretprobe passes
> > > > - using uretprobe passes (and validates that uretprobe did work)
> > > >
> > > > in each of the following conditions:
> > > >
> > > > - default-allow filter
> > > > - default-block filter
> > > > - filter explicitly blocking __NR_uretprobe and nothing else
> > > > - filter explicitly allowing __NR_uretprobe (and only other
> > > > required syscalls)
> > >
> > > Ok.
> >
> > please let me know if I can help in any way with tests
>
> Thanks! Is there a way to partition this work? I'd appreciate the help
> if we can find some way of doing so.
sure, I'll check the seccomp selftests and let you know
>
> >
> > >
> > > >
> > > > Hm, is uretprobe expected to work on mips? Because if so, you'll need to
> > > > do something similar to the mode1 checking in the !SECCOMP_ARCH_NATIVE
> > > > version of seccomp_cache_check_allow().
> > >
> > > I don't know if uretprobe syscall is expected to run on mips. Personally
> > > I'd avoid adding this dead code.
>
> Jiri, what is your take on this one?
uretprobe syscall is not expected to work on mips, atm it's strictly x86_64
jirka
^ permalink raw reply
* Re: [PATCH v7 0/6] introduce PIDFD_SELF* sentinels
From: Andrew Morton @ 2025-01-30 22:37 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Christian Brauner, Shuah Khan, Liam R . Howlett,
Suren Baghdasaryan, Vlastimil Babka, pedro.falcato,
linux-kselftest, linux-mm, linux-fsdevel, linux-api, linux-kernel,
Oliver Sang, John Hubbard, Tejun Heo, Johannes Weiner,
Michal Koutny, Shakeel Butt
In-Reply-To: <cover.1738268370.git.lorenzo.stoakes@oracle.com>
On Thu, 30 Jan 2025 20:40:25 +0000 Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:
> If you wish to utilise a pidfd interface to refer to the current process or
> thread it is rather cumbersome, requiring something like:
>
> int pidfd = pidfd_open(getpid(), 0 or PIDFD_THREAD);
>
> ...
>
> close(pidfd);
>
> Or the equivalent call opening /proc/self. It is more convenient to use a
> sentinel value to indicate to an interface that accepts a pidfd that we
> simply wish to refer to the current process thread.
>
The above code sequence doesn't seem at all onerous. I'm not
understanding why it's worth altering the kernel to permit this little
shortcut?
^ permalink raw reply
* Re: [PATCH v7 0/6] introduce PIDFD_SELF* sentinels
From: Lorenzo Stoakes @ 2025-01-30 22:53 UTC (permalink / raw)
To: Andrew Morton
Cc: Christian Brauner, Shuah Khan, Liam R . Howlett,
Suren Baghdasaryan, Vlastimil Babka, pedro.falcato,
linux-kselftest, linux-mm, linux-fsdevel, linux-api, linux-kernel,
Oliver Sang, John Hubbard, Tejun Heo, Johannes Weiner,
Michal Koutny, Shakeel Butt
In-Reply-To: <20250130143754.1b8bb87bfb15175dd434529b@linux-foundation.org>
On Thu, Jan 30, 2025 at 02:37:54PM -0800, Andrew Morton wrote:
> On Thu, 30 Jan 2025 20:40:25 +0000 Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:
>
> > If you wish to utilise a pidfd interface to refer to the current process or
> > thread it is rather cumbersome, requiring something like:
> >
> > int pidfd = pidfd_open(getpid(), 0 or PIDFD_THREAD);
> >
> > ...
> >
> > close(pidfd);
> >
> > Or the equivalent call opening /proc/self. It is more convenient to use a
> > sentinel value to indicate to an interface that accepts a pidfd that we
> > simply wish to refer to the current process thread.
> >
>
> The above code sequence doesn't seem at all onerous. I'm not
> understanding why it's worth altering the kernel to permit this little
> shortcut?
In practice it adds quite a bit of overhead for something that whatever
mechanism is using the pidfd can avoid.
It was specifically intended for a real case of utilising
process_madvise(), using the newly extended ability to batch _any_
madvise() operations for the current process, like:
if (process_madvise(PIDFD_SELF, iovec, 10, MADV_GUARD_INSTALL, 0)) {
... error handling ...
}
vs.
pid_t pid = getpid();
int pidfd = pidfd_open(pid, PIDFD_THREAD);
if (pidfd < 0) {
... error handling ...
}
if (process_madvise(PIDFD_SELF, iovec, 10, MADV_GUARD_INSTALL, 0)) {
... cleanup pidfd ...
... error handling ...
}
...
... cleanup pidfd ...
So in practice, it's actually a lot more ceremony and noise. Suren has been
working with this code in practice and found this to be useful.
The suggestion to embed it as PIDFD_SELF rather than to pass it as a
process_madvise() flag was made on the original series where I extended its
functionality.
So in practice I think it's onerous enough to justify this, plus it allows
for a more fluent use of pidfd's in other cases where one is referring to
the same process/thread, to the extent that I've seen people commenting on
supporting it while sending series relating to pidfd.
Also Christian and others appear to support this idea.
^ permalink raw reply
* Re: [PATCH v7 0/6] introduce PIDFD_SELF* sentinels
From: Pedro Falcato @ 2025-01-30 23:10 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Andrew Morton, Christian Brauner, Shuah Khan, Liam R . Howlett,
Suren Baghdasaryan, Vlastimil Babka, linux-kselftest, linux-mm,
linux-fsdevel, linux-api, linux-kernel, Oliver Sang, John Hubbard,
Tejun Heo, Johannes Weiner, Michal Koutny, Shakeel Butt
In-Reply-To: <b396487f-b906-410d-9ff4-6956d99e2771@lucifer.local>
On Thu, Jan 30, 2025 at 10:53 PM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> On Thu, Jan 30, 2025 at 02:37:54PM -0800, Andrew Morton wrote:
> > On Thu, 30 Jan 2025 20:40:25 +0000 Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:
> >
> > > If you wish to utilise a pidfd interface to refer to the current process or
> > > thread it is rather cumbersome, requiring something like:
> > >
> > > int pidfd = pidfd_open(getpid(), 0 or PIDFD_THREAD);
> > >
> > > ...
> > >
> > > close(pidfd);
> > >
> > > Or the equivalent call opening /proc/self. It is more convenient to use a
> > > sentinel value to indicate to an interface that accepts a pidfd that we
> > > simply wish to refer to the current process thread.
> > >
> >
> > The above code sequence doesn't seem at all onerous. I'm not
> > understanding why it's worth altering the kernel to permit this little
> > shortcut?
>
> In practice it adds quite a bit of overhead for something that whatever
> mechanism is using the pidfd can avoid.
>
> It was specifically intended for a real case of utilising
> process_madvise(), using the newly extended ability to batch _any_
> madvise() operations for the current process, like:
>
> if (process_madvise(PIDFD_SELF, iovec, 10, MADV_GUARD_INSTALL, 0)) {
> ... error handling ...
> }
>
> vs.
>
> pid_t pid = getpid();
> int pidfd = pidfd_open(pid, PIDFD_THREAD);
>
> if (pidfd < 0) {
> ... error handling ...
> }
>
> if (process_madvise(PIDFD_SELF, iovec, 10, MADV_GUARD_INSTALL, 0)) {
> ... cleanup pidfd ...
> ... error handling ...
> }
>
> ...
>
> ... cleanup pidfd ...
>
> So in practice, it's actually a lot more ceremony and noise. Suren has been
> working with this code in practice and found this to be useful.
It's also nice to add that people on the libc/allocator side should
also appreciate skipping pidfd_open's reliability concerns (mostly,
that RLIMIT_NOFILE Should Not(tm) ever affect thread spawning or a
malloc[1]). Besides the big syscall reduction and nice speedup, that
is.
[1] whether this is the already case is an exercise left to the
reader, but at the very least we should not add onto existing problems
--
Pedro
^ permalink raw reply
* Re: [PATCH v7 0/6] introduce PIDFD_SELF* sentinels
From: Andrew Morton @ 2025-01-30 23:32 UTC (permalink / raw)
To: Pedro Falcato
Cc: Lorenzo Stoakes, Christian Brauner, Shuah Khan, Liam R . Howlett,
Suren Baghdasaryan, Vlastimil Babka, linux-kselftest, linux-mm,
linux-fsdevel, linux-api, linux-kernel, Oliver Sang, John Hubbard,
Tejun Heo, Johannes Weiner, Michal Koutny, Shakeel Butt
In-Reply-To: <CAKbZUD3w4_4MjrME-0mgRL01eFggb7et2BLa6012tzQX78KK9w@mail.gmail.com>
On Thu, 30 Jan 2025 23:10:53 +0000 Pedro Falcato <pedro.falcato@gmail.com> wrote:
> On Thu, Jan 30, 2025 at 10:53 PM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> >
> > > The above code sequence doesn't seem at all onerous. I'm not
> > > understanding why it's worth altering the kernel to permit this little
> > > shortcut?
> >
> > In practice it adds quite a bit of overhead for something that whatever
> > mechanism is using the pidfd can avoid.
> >
> > It was specifically intended for a real case of utilising
> > process_madvise(), using the newly extended ability to batch _any_
> > madvise() operations for the current process, like:
> >
> > if (process_madvise(PIDFD_SELF, iovec, 10, MADV_GUARD_INSTALL, 0)) {
> > ... error handling ...
> > }
> >
> > vs.
> >
> > pid_t pid = getpid();
> > int pidfd = pidfd_open(pid, PIDFD_THREAD);
> >
> > if (pidfd < 0) {
> > ... error handling ...
> > }
> >
> > if (process_madvise(PIDFD_SELF, iovec, 10, MADV_GUARD_INSTALL, 0)) {
> > ... cleanup pidfd ...
> > ... error handling ...
> > }
> >
> > ...
> >
> > ... cleanup pidfd ...
> >
> > So in practice, it's actually a lot more ceremony and noise. Suren has been
> > working with this code in practice and found this to be useful.
>
> It's also nice to add that people on the libc/allocator side should
> also appreciate skipping pidfd_open's reliability concerns (mostly,
> that RLIMIT_NOFILE Should Not(tm) ever affect thread spawning or a
> malloc[1]). Besides the big syscall reduction and nice speedup, that
> is.
>
> [1] whether this is the already case is an exercise left to the
> reader, but at the very least we should not add onto existing problems
Thanks.
Could we please get all the above spelled out much more thoroughly in
the [0/n] description (aka Patch Series Sales Brochure)?
^ permalink raw reply
* Re: [PATCH v7 0/6] introduce PIDFD_SELF* sentinels
From: Lorenzo Stoakes @ 2025-01-31 10:21 UTC (permalink / raw)
To: Andrew Morton
Cc: Pedro Falcato, Christian Brauner, Shuah Khan, Liam R . Howlett,
Suren Baghdasaryan, Vlastimil Babka, linux-kselftest, linux-mm,
linux-fsdevel, linux-api, linux-kernel, Oliver Sang, John Hubbard,
Tejun Heo, Johannes Weiner, Michal Koutny, Shakeel Butt
In-Reply-To: <20250130153236.198664b9a19ccfcdb24f888b@linux-foundation.org>
On Thu, Jan 30, 2025 at 03:32:36PM -0800, Andrew Morton wrote:
> On Thu, 30 Jan 2025 23:10:53 +0000 Pedro Falcato <pedro.falcato@gmail.com> wrote:
>
> > On Thu, Jan 30, 2025 at 10:53 PM Lorenzo Stoakes
> > <lorenzo.stoakes@oracle.com> wrote:
> > >
> > > > The above code sequence doesn't seem at all onerous. I'm not
> > > > understanding why it's worth altering the kernel to permit this little
> > > > shortcut?
> > >
> > > In practice it adds quite a bit of overhead for something that whatever
> > > mechanism is using the pidfd can avoid.
> > >
> > > It was specifically intended for a real case of utilising
> > > process_madvise(), using the newly extended ability to batch _any_
> > > madvise() operations for the current process, like:
> > >
> > > if (process_madvise(PIDFD_SELF, iovec, 10, MADV_GUARD_INSTALL, 0)) {
> > > ... error handling ...
> > > }
> > >
> > > vs.
> > >
> > > pid_t pid = getpid();
> > > int pidfd = pidfd_open(pid, PIDFD_THREAD);
> > >
> > > if (pidfd < 0) {
> > > ... error handling ...
> > > }
> > >
> > > if (process_madvise(PIDFD_SELF, iovec, 10, MADV_GUARD_INSTALL, 0)) {
> > > ... cleanup pidfd ...
> > > ... error handling ...
> > > }
> > >
> > > ...
> > >
> > > ... cleanup pidfd ...
> > >
> > > So in practice, it's actually a lot more ceremony and noise. Suren has been
> > > working with this code in practice and found this to be useful.
> >
> > It's also nice to add that people on the libc/allocator side should
> > also appreciate skipping pidfd_open's reliability concerns (mostly,
> > that RLIMIT_NOFILE Should Not(tm) ever affect thread spawning or a
> > malloc[1]). Besides the big syscall reduction and nice speedup, that
> > is.
> >
> > [1] whether this is the already case is an exercise left to the
> > reader, but at the very least we should not add onto existing problems
>
> Thanks.
>
> Could we please get all the above spelled out much more thoroughly in
> the [0/n] description (aka Patch Series Sales Brochure)?
Ack, will expand if there's a respin, or Christian - perhaps you could fold
the above explanation into the cover letter?
Intent is for Christian to take this in his tree (if he so wishes) to be
clear!
Cheers
^ 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