* Re: [PATCH v2 0/1] Add prctl to kill descendants on exit
From: Jürg Billeter @ 2018-12-06 15:54 UTC (permalink / raw)
To: Andrew Morton, Eric Biederman
Cc: Oleg Nesterov, Thomas Gleixner, Kees Cook, Andy Lutomirski,
linux-api, linux-kernel
In-Reply-To: <20181130080004.23635-1-j@bitron.ch>
On Fri, 2018-11-30 at 08:00 +0000, Jürg Billeter wrote:
> This patch adds a new prctl to kill all descendant processes on exit.
> See commit message for details of the prctl.
>
> This is a replacement of PR_SET_PDEATHSIG_PROC I proposed last year [1].
> In the following discussion, Oleg suggested this approach.
>
> The motivation for this is to provide a lightweight mechanism to prevent
> stray processes. There is also a related Bugzilla entry [2].
Andrew, Eric, does this look good to you as well?
Jürg
^ permalink raw reply
* Re: [PATCH v4] signal: add taskfd_send_signal() syscall
From: Eric W. Biederman @ 2018-12-06 15:01 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-kernel, linux-api, luto, arnd, serge, jannh, akpm, oleg,
cyphar, viro, linux-fsdevel, dancol, timmurray, linux-man,
keescook, fweimer, tglx, x86
In-Reply-To: <20181206121858.12215-1-christian@brauner.io>
Christian Brauner <christian@brauner.io> writes:
> The kill() syscall operates on process identifiers (pid). After a process
> has exited its pid can be reused by another process. If a caller sends a
> signal to a reused pid it will end up signaling the wrong process. This
> issue has often surfaced and there has been a push [1] to address this
> problem.
>
> This patch uses file descriptors (fd) from proc/<pid> as stable handles on
> struct pid. Even if a pid is recycled the handle will not change. The fd
> can be used to send signals to the process it refers to.
> Thus, the new syscall taskfd_send_signal() is introduced to solve this
> problem. Instead of pids it operates on process fds (taskfd).
I am not yet thrilled with the taskfd naming.
Is there any plan to support sesssions and process groups?
I am concerned about using kill_pid_info. It does this:
rcu_read_lock();
p = pid_task(pid, PIDTYPE_PID);
if (p)
error = group_send_sig_info(sig, info, p, PIDTYPE_TGID);
rcu_read_unlock();
That pid_task(PIDTYPE_PID) is fine for existing callers that need bug
compatibility. For new interfaces I would strongly prefer pid_task(PIDTYPE_TGID).
Eric
>
> /* prototype and argument /*
> long taskfd_send_signal(int taskfd, int sig, siginfo_t *info, unsigned int flags);
>
> In addition to the taskfd and signal argument it takes an additional
> siginfo_t and flags argument. If the siginfo_t argument is NULL then
> taskfd_send_signal() behaves like kill(). If it is not NULL
> taskfd_send_signal() behaves like rt_sigqueueinfo().
> The flags argument is added to allow for future extensions of this syscall.
> It currently needs to be passed as 0. Failing to do so will cause EINVAL.
>
> /* taskfd_send_signal() replaces multiple pid-based syscalls */
> The taskfd_send_signal() syscall currently takes on the job of the
> following syscalls that operate on pids:
> - kill(2)
> - rt_sigqueueinfo(2)
> The syscall is defined in such a way that it can also operate on thread fds
> instead of process fds. In a future patchset I will extend it to operate on
> taskfds from /proc/<pid>/task/<tid> at which point it will additionally
> take on the job of:
> - tgkill(2)
> - rt_tgsigqueueinfo(2)
> Right now this is intentionally left out to keep this patchset as simple as
> possible (cf. [4]). If a taskfd of /proc/<pid>/task/<tid> is passed
> EOPNOTSUPP will be returned to give userspace a way to detect when I add
> support for such taskfds (cf. [10]).
>
> /* naming */
> The original prefix of the syscall was "procfd_". However, it has been
> pointed out that since this syscall will eventually operate on both
> processes and threads the name should reflect this (cf. [12]). The best
> possible candidate even from a userspace perspective seems to be "task".
> Although "task" is used internally we are alreday deviating from POSIX by
> using file descriptors to processes in the first place so it seems fine to
> use the "taskfd_" prefix.
>
> The name taskfd_send_signal() was also chosen to reflect the fact that it
> takes on the job of multiple syscalls. It is intentional that the name is
> not reminiscent of neither kill(2) nor rt_sigqueueinfo(2). Not the fomer
> because it might imply that taskfd_send_signal() is only a replacement for
> kill(2) and not the latter because it is a hazzle to remember the correct
> spelling (especially for non-native speakers) and because it is not
> descriptive enough of what the syscall actually does. The name
> "taskfd_send_signal" makes it very clear that its job is to send signals.
>
> /* O_PATH file descriptors */
> taskfds opened as O_PATH fds cannot be used to send signals to a process
> (cf. [2]). Signaling processes through taskfds is the equivalent of writing
> to a file. Thus, this is not an operation that operates "purely at the
> file descriptor level" as required by the open(2) manpage.
>
> /* zombies */
> Zombies can be signaled just as any other process. No special error will be
> reported since a zombie state is an unreliable state (cf. [3]).
>
> /* cross-namespace signals */
> The patch currently enforces that the signaler and signalee either are in
> the same pid namespace or that the signaler's pid namespace is an ancestor
> of the signalee's pid namespace. This is done for the sake of simplicity
> and because it is unclear to what values certain members of struct
> siginfo_t would need to be set to (cf. [5], [6]).
>
> /* compat syscalls */
> It became clear that we would like to avoid adding compat syscalls (cf.
> [7]). The compat syscall handling is now done in kernel/signal.c itself by
> adding __copy_siginfo_from_user_generic() which lets us avoid compat
> syscalls (cf. [8]). It should be noted that the addition of
> __copy_siginfo_from_user_any() is caused by a bug in the original
> implementation of rt_sigqueueinfo(2) (cf. 12).
> With upcoming rework for syscall handling things might improve
> significantly (cf. [11]) and __copy_siginfo_from_user_any() will not gain
> any additional callers.
>
> /* testing */
> This patch was tested on x64 and x86.
>
> /* userspace usage */
> An asciinema recording for the basic functionality can be found under [9].
> With this patch a process can be killed via:
>
> #define _GNU_SOURCE
> #include <errno.h>
> #include <fcntl.h>
> #include <signal.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <sys/stat.h>
> #include <sys/syscall.h>
> #include <sys/types.h>
> #include <unistd.h>
>
> static inline int do_taskfd_send_signal(int taskfd, int sig, siginfo_t *info,
> unsigned int flags)
> {
> #ifdef __NR_taskfd_send_signal
> return syscall(__NR_taskfd_send_signal, taskfd, sig, info, flags);
> #else
> return -ENOSYS;
> #endif
> }
>
> int main(int argc, char *argv[])
> {
> int fd, ret, saved_errno, sig;
>
> if (argc < 3)
> exit(EXIT_FAILURE);
>
> fd = open(argv[1], O_DIRECTORY | O_CLOEXEC);
> if (fd < 0) {
> printf("%s - Failed to open \"%s\"\n", strerror(errno), argv[1]);
> exit(EXIT_FAILURE);
> }
>
> sig = atoi(argv[2]);
>
> printf("Sending signal %d to process %s\n", sig, argv[1]);
> ret = do_taskfd_send_signal(fd, sig, NULL, 0);
>
> saved_errno = errno;
> close(fd);
> errno = saved_errno;
>
> if (ret < 0) {
> printf("%s - Failed to send signal %d to process %s\n",
> strerror(errno), sig, argv[1]);
> exit(EXIT_FAILURE);
> }
>
> exit(EXIT_SUCCESS);
> }
>
> [1]: https://lore.kernel.org/lkml/20181029221037.87724-1-dancol@google.com/
> [2]: https://lore.kernel.org/lkml/874lbtjvtd.fsf@oldenburg2.str.redhat.com/
> [3]: https://lore.kernel.org/lkml/20181204132604.aspfupwjgjx6fhva@brauner.io/
> [4]: https://lore.kernel.org/lkml/20181203180224.fkvw4kajtbvru2ku@brauner.io/
> [5]: https://lore.kernel.org/lkml/20181121213946.GA10795@mail.hallyn.com/
> [6]: https://lore.kernel.org/lkml/20181120103111.etlqp7zop34v6nv4@brauner.io/
> [7]: https://lore.kernel.org/lkml/36323361-90BD-41AF-AB5B-EE0D7BA02C21@amacapital.net/
> [8]: https://lore.kernel.org/lkml/87tvjxp8pc.fsf@xmission.com/
> [9]: https://asciinema.org/a/IQjuCHew6bnq1cr78yuMv16cy
> [10]: https://lore.kernel.org/lkml/20181203180224.fkvw4kajtbvru2ku@brauner.io/
> [11]: https://lore.kernel.org/lkml/F53D6D38-3521-4C20-9034-5AF447DF62FF@amacapital.net/
> [12]: https://lore.kernel.org/lkml/87zhtjn8ck.fsf@xmission.com/
>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: Serge Hallyn <serge@hallyn.com>
> Cc: Jann Horn <jannh@google.com>
> Cc: Andy Lutomirsky <luto@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Aleksa Sarai <cyphar@cyphar.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Florian Weimer <fweimer@redhat.com>
> Signed-off-by: Christian Brauner <christian@brauner.io>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> ---
> Changelog:
> v4:
> - updated asciinema to use "taskfd_" prefix
> - s/procfd_send_signal/taskfd_send_signal/g
> - s/proc_is_tgid_procfd/tgid_taskfd_to_pid/b
> - s/proc_is_tid_procfd/tid_taskfd_to_pid/b
> - s/__copy_siginfo_from_user_generic/__copy_siginfo_from_user_any/g
> - make it clear that __copy_siginfo_from_user_any() is a workaround caused
> by a bug in the original implementation of rt_sigqueueinfo()
> - when spoofing signals turn them into regular kill signals if si_code is
> set to SI_USER
> - make proc_is_t{g}id_procfd() return struct pid to allow proc_pid() to
> stay private to fs/proc/
> v3:
> - add __copy_siginfo_from_user_generic() to avoid adding compat syscalls
> - s/procfd_signal/procfd_send_signal/g
> - change type of flags argument from int to unsigned int
> - add comment about what happens to zombies
> - add proc_is_tid_procfd()
> - return EOPNOTSUPP when /proc/<pid>/task/<tid> fd is passed so userspace
> has a way of knowing that tidfds are not supported currently.
> v2:
> - define __NR_procfd_signal in unistd.h
> - wire up compat syscall
> - s/proc_is_procfd/proc_is_tgid_procfd/g
> - provide stubs when CONFIG_PROC_FS=n
> - move proc_pid() to linux/proc_fs.h header
> - use proc_pid() to grab struct pid from /proc/<pid> fd
> v1:
> - patch introduced
> ---
> arch/x86/entry/syscalls/syscall_32.tbl | 1 +
> arch/x86/entry/syscalls/syscall_64.tbl | 1 +
> fs/proc/base.c | 20 +++-
> include/linux/proc_fs.h | 12 +++
> include/linux/syscalls.h | 3 +
> include/uapi/asm-generic/unistd.h | 4 +-
> kernel/signal.c | 132 +++++++++++++++++++++++--
> 7 files changed, 164 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
> index 3cf7b533b3d1..7efb63fd0617 100644
> --- a/arch/x86/entry/syscalls/syscall_32.tbl
> +++ b/arch/x86/entry/syscalls/syscall_32.tbl
> @@ -398,3 +398,4 @@
> 384 i386 arch_prctl sys_arch_prctl __ia32_compat_sys_arch_prctl
> 385 i386 io_pgetevents sys_io_pgetevents __ia32_compat_sys_io_pgetevents
> 386 i386 rseq sys_rseq __ia32_sys_rseq
> +387 i386 taskfd_send_signal sys_taskfd_send_signal __ia32_sys_taskfd_send_signal
> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
> index f0b1709a5ffb..be894f4a84e9 100644
> --- a/arch/x86/entry/syscalls/syscall_64.tbl
> +++ b/arch/x86/entry/syscalls/syscall_64.tbl
> @@ -343,6 +343,7 @@
> 332 common statx __x64_sys_statx
> 333 common io_pgetevents __x64_sys_io_pgetevents
> 334 common rseq __x64_sys_rseq
> +335 common taskfd_send_signal __x64_sys_taskfd_send_signal
>
> #
> # x32-specific system call numbers start at 512 to avoid cache impact
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index ce3465479447..b8b88bfee455 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -716,8 +716,6 @@ static int proc_pid_permission(struct inode *inode, int mask)
> return generic_permission(inode, mask);
> }
>
> -
> -
> static const struct inode_operations proc_def_inode_operations = {
> .setattr = proc_setattr,
> };
> @@ -3038,6 +3036,15 @@ static const struct file_operations proc_tgid_base_operations = {
> .llseek = generic_file_llseek,
> };
>
> +struct pid *tgid_taskfd_to_pid(const struct file *file)
> +{
> + if (!d_is_dir(file->f_path.dentry) ||
> + (file->f_op != &proc_tgid_base_operations))
> + return ERR_PTR(-EBADF);
> +
> + return proc_pid(file_inode(file));
> +}
> +
> static struct dentry *proc_tgid_base_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
> {
> return proc_pident_lookup(dir, dentry,
> @@ -3422,6 +3429,15 @@ static const struct file_operations proc_tid_base_operations = {
> .llseek = generic_file_llseek,
> };
>
> +struct pid *tid_taskfd_to_pid(const struct file *file)
> +{
> + if (!d_is_dir(file->f_path.dentry) ||
> + (file->f_op != &proc_tid_base_operations))
> + return ERR_PTR(-EBADF);
> +
> + return proc_pid(file_inode(file));
> +}
> +
> static const struct inode_operations proc_tid_base_inode_operations = {
> .lookup = proc_tid_base_lookup,
> .getattr = pid_getattr,
> diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
> index d0e1f1522a78..96817415c420 100644
> --- a/include/linux/proc_fs.h
> +++ b/include/linux/proc_fs.h
> @@ -73,6 +73,8 @@ struct proc_dir_entry *proc_create_net_single_write(const char *name, umode_t mo
> int (*show)(struct seq_file *, void *),
> proc_write_t write,
> void *data);
> +extern struct pid *tgid_taskfd_to_pid(const struct file *file);
> +extern struct pid *tid_taskfd_to_pid(const struct file *file);
>
> #else /* CONFIG_PROC_FS */
>
> @@ -114,6 +116,16 @@ static inline int remove_proc_subtree(const char *name, struct proc_dir_entry *p
> #define proc_create_net(name, mode, parent, state_size, ops) ({NULL;})
> #define proc_create_net_single(name, mode, parent, show, data) ({NULL;})
>
> +static inline struct pid *tgid_taskfd_to_pid(const struct file *file)
> +{
> + return ERR_PTR(-EBADF);
> +}
> +
> +static inline struct pid *tid_taskfd_to_pid(const struct file *file)
> +{
> + return ERR_PTR(-EBADF);
> +}
> +
> #endif /* CONFIG_PROC_FS */
>
> struct net;
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index 2ac3d13a915b..5ffe194ef29b 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -907,6 +907,9 @@ asmlinkage long sys_statx(int dfd, const char __user *path, unsigned flags,
> unsigned mask, struct statx __user *buffer);
> asmlinkage long sys_rseq(struct rseq __user *rseq, uint32_t rseq_len,
> int flags, uint32_t sig);
> +asmlinkage long sys_taskfd_send_signal(int taskfd, int sig,
> + siginfo_t __user *info,
> + unsigned int flags);
>
> /*
> * Architecture-specific system calls
> diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
> index 538546edbfbd..9343dca63fd9 100644
> --- a/include/uapi/asm-generic/unistd.h
> +++ b/include/uapi/asm-generic/unistd.h
> @@ -738,9 +738,11 @@ __SYSCALL(__NR_statx, sys_statx)
> __SC_COMP(__NR_io_pgetevents, sys_io_pgetevents, compat_sys_io_pgetevents)
> #define __NR_rseq 293
> __SYSCALL(__NR_rseq, sys_rseq)
> +#define __NR_taskfd_send_signal 294
> +__SYSCALL(__NR_taskfd_send_signal, sys_taskfd_send_signal)
>
> #undef __NR_syscalls
> -#define __NR_syscalls 294
> +#define __NR_syscalls 295
>
> /*
> * 32 bit systems traditionally used different
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 9a32bc2088c9..a00a4bcb7605 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -19,7 +19,9 @@
> #include <linux/sched/task.h>
> #include <linux/sched/task_stack.h>
> #include <linux/sched/cputime.h>
> +#include <linux/file.h>
> #include <linux/fs.h>
> +#include <linux/proc_fs.h>
> #include <linux/tty.h>
> #include <linux/binfmts.h>
> #include <linux/coredump.h>
> @@ -3286,6 +3288,16 @@ COMPAT_SYSCALL_DEFINE4(rt_sigtimedwait, compat_sigset_t __user *, uthese,
> }
> #endif
>
> +static inline void prepare_kill_siginfo(int sig, struct kernel_siginfo *info)
> +{
> + clear_siginfo(info);
> + info->si_signo = sig;
> + info->si_errno = 0;
> + info->si_code = SI_USER;
> + info->si_pid = task_tgid_vnr(current);
> + info->si_uid = from_kuid_munged(current_user_ns(), current_uid());
> +}
> +
> /**
> * sys_kill - send a signal to a process
> * @pid: the PID of the process
> @@ -3295,16 +3307,124 @@ SYSCALL_DEFINE2(kill, pid_t, pid, int, sig)
> {
> struct kernel_siginfo info;
>
> - clear_siginfo(&info);
> - info.si_signo = sig;
> - info.si_errno = 0;
> - info.si_code = SI_USER;
> - info.si_pid = task_tgid_vnr(current);
> - info.si_uid = from_kuid_munged(current_user_ns(), current_uid());
> + prepare_kill_siginfo(sig, &info);
>
> return kill_something_info(sig, &info, pid);
> }
>
> +/*
> + * Verify that the signaler and signalee either are in the same pid namespace
> + * or that the signaler's pid namespace is an ancestor of the signalee's pid
> + * namespace.
> + */
> +static bool may_signal_taskfd(struct pid *pid)
> +{
> + struct pid_namespace *active = task_active_pid_ns(current);
> + struct pid_namespace *p = ns_of_pid(pid);
> +
> + for (;;) {
> + if (!p)
> + return false;
> + if (p == active)
> + break;
> + p = p->parent;
> + }
> +
> + return true;
> +}
> +
> +static int copy_siginfo_from_user_any(kernel_siginfo_t *kinfo, siginfo_t *info)
> +{
> +#ifdef CONFIG_COMPAT
> + /*
> + * Avoid hooking up compat syscalls and instead handle necessary
> + * conversions here. Note, this is a stop-gap measure and should not be
> + * considered a generic solution.
> + */
> + if (in_compat_syscall())
> + return copy_siginfo_from_user32(
> + kinfo, (struct compat_siginfo __user *)info);
> +#endif
> + return copy_siginfo_from_user(kinfo, info);
> +}
> +
> +/**
> + * sys_taskfd_send_signal - send a signal to a process through a task file
> + * descriptor
> + * @taskfd: the file descriptor of the process
> + * @sig: signal to be sent
> + * @info: the signal info
> + * @flags: future flags to be passed
> + *
> + * Return: 0 on success, negative errno on failure
> + */
> +SYSCALL_DEFINE4(taskfd_send_signal, int, taskfd, int, sig,
> + siginfo_t __user *, info, unsigned int, flags)
> +{
> + int ret;
> + struct fd f;
> + struct pid *pid;
> + kernel_siginfo_t kinfo;
> +
> + /* Enforce flags be set to 0 until we add an extension. */
> + if (flags)
> + return -EINVAL;
> +
> + f = fdget_raw(taskfd);
> + if (!f.file)
> + return -EBADF;
> +
> + pid = tid_taskfd_to_pid(f.file);
> + if (!IS_ERR(pid)) {
> + /*
> + * Give userspace a way to detect /proc/<pid>/task/<tid>
> + * support when we add it.
> + */
> + ret = -EOPNOTSUPP;
> + goto err;
> + }
> +
> + /* Is this a procfd? */
> + pid = tgid_taskfd_to_pid(f.file);
> + if (IS_ERR(pid)) {
> + ret = PTR_ERR(pid);
> + goto err;
> + }
> +
> + ret = -EINVAL;
> + if (!may_signal_taskfd(pid))
> + goto err;
> +
> + if (info) {
> + ret = copy_siginfo_from_user_any(&kinfo, info);
> + if (unlikely(ret))
> + goto err;
> +
> + ret = -EINVAL;
> + if (unlikely(sig != kinfo.si_signo))
> + goto err;
> +
> + if ((task_pid(current) != pid) &&
> + (kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL)) {
> + /* Only allow sending arbitrary signals to yourself. */
> + ret = -EPERM;
> + if (kinfo.si_code != SI_USER)
> + goto err;
> +
> + /* Turn this into a regular kill signal. */
> + prepare_kill_siginfo(sig, &kinfo);
> + }
> + } else {
> + prepare_kill_siginfo(sig, &kinfo);
> + }
> +
> + ret = kill_pid_info(sig, &kinfo, pid);
> +
> +err:
> + fdput(f);
> + return ret;
> +}
> +
> static int
> do_send_specific(pid_t tgid, pid_t pid, int sig, struct kernel_siginfo *info)
> {
^ permalink raw reply
* Re: [PATCH v4] signal: add taskfd_send_signal() syscall
From: Eric W. Biederman @ 2018-12-06 14:46 UTC (permalink / raw)
To: Florian Weimer
Cc: Jürg Billeter, Christian Brauner, linux-kernel, linux-api,
luto, arnd, serge, jannh, akpm, oleg, cyphar, viro, linux-fsdevel,
dancol, timmurray, linux-man, keescook, tglx, x86
In-Reply-To: <877egm6a7v.fsf@oldenburg2.str.redhat.com>
Florian Weimer <fweimer@redhat.com> writes:
> * Eric W. Biederman:
>
>> Floriam are you seeing a problem with this behavior or the way Christian
>> was describing it?
>
> My hope is that you could use taskfd_send_signal one day to send a
> signal to a process which you *known* (based on how you've written your
> application) should be running and not in a zombie state, and get back
> an error if it has exited.
>
> If you get this error, only then you wait on the process, using the file
> descriptor you have, and run some recovery code.
>
> Wouldn't that be a reasonable approach once we've got task descriptors?
Getting an error back if the target was a zombie does seem reasonable,
as in principle it is an easy thing to notice, and post zombie once the
process has been reaped we definitely get an error back.
I also agree that it sounds like an extension, as changing the default
would violate the princile of least surprise.
Eric
^ permalink raw reply
* [PATCH man-pages] Add rseq manpage
From: Mathieu Desnoyers @ 2018-12-06 14:42 UTC (permalink / raw)
To: Michael Kerrisk
Cc: linux-kernel, linux-api, Peter Zijlstra, Paul E . McKenney,
Boqun Feng, Andy Lutomirski, Dave Watson, Paul Turner,
Andrew Morton, Russell King, Thomas Gleixner, Ingo Molnar,
H . Peter Anvin, Andi Kleen, Chris Lameter, Ben Maurer,
Steven Rostedt, Josh Triplett, Linus Torvalds, Catalin Marinas,
Will Deacon
[ Michael, rseq(2) was merged into 4.18. Can you have a look at this
patch which adds rseq documentation to the man-pages project ? ]
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
CC: Peter Zijlstra <peterz@infradead.org>
CC: Paul Turner <pjt@google.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Andy Lutomirski <luto@amacapital.net>
CC: Andi Kleen <andi@firstfloor.org>
CC: Dave Watson <davejwatson@fb.com>
CC: Chris Lameter <cl@linux.com>
CC: Ingo Molnar <mingo@redhat.com>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Ben Maurer <bmaurer@fb.com>
CC: Steven Rostedt <rostedt@goodmis.org>
CC: Josh Triplett <josh@joshtriplett.org>
CC: Linus Torvalds <torvalds@linux-foundation.org>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Russell King <linux@arm.linux.org.uk>
CC: Catalin Marinas <catalin.marinas@arm.com>
CC: Will Deacon <will.deacon@arm.com>
CC: Michael Kerrisk <mtk.manpages@gmail.com>
CC: Boqun Feng <boqun.feng@gmail.com>
CC: linux-api@vger.kernel.org
---
man2/rseq.2 | 299 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 299 insertions(+)
create mode 100644 man2/rseq.2
diff --git a/man2/rseq.2 b/man2/rseq.2
new file mode 100644
index 000000000..005c1cee4
--- /dev/null
+++ b/man2/rseq.2
@@ -0,0 +1,299 @@
+.\" Copyright 2015-2018 Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
+.\"
+.\" %%%LICENSE_START(VERBATIM)
+.\" Permission is granted to make and distribute verbatim copies of this
+.\" manual provided the copyright notice and this permission notice are
+.\" preserved on all copies.
+.\"
+.\" Permission is granted to copy and distribute modified versions of this
+.\" manual under the conditions for verbatim copying, provided that the
+.\" entire resulting derived work is distributed under the terms of a
+.\" permission notice identical to this one.
+.\"
+.\" Since the Linux kernel and libraries are constantly changing, this
+.\" manual page may be incorrect or out-of-date. The author(s) assume no
+.\" responsibility for errors or omissions, or for damages resulting from
+.\" the use of the information contained herein. The author(s) may not
+.\" have taken the same level of care in the production of this manual,
+.\" which is licensed free of charge, as they might when working
+.\" professionally.
+.\"
+.\" Formatted or processed versions of this manual, if unaccompanied by
+.\" the source, must acknowledge the copyright and authors of this work.
+.\" %%%LICENSE_END
+.\"
+.TH RSEQ 2 2018-09-19 "Linux" "Linux Programmer's Manual"
+.SH NAME
+rseq \- Restartable sequences and cpu number cache
+.SH SYNOPSIS
+.nf
+.B #include <linux/rseq.h>
+.sp
+.BI "int rseq(struct rseq * " rseq ", uint32_t " rseq_len ", int " flags ", uint32_t " sig ");
+.sp
+.SH DESCRIPTION
+The
+.BR rseq ()
+ABI accelerates user-space operations on per-cpu data by defining a
+shared data structure ABI between each user-space thread and the kernel.
+
+It allows user-space to perform update operations on per-cpu data
+without requiring heavy-weight atomic operations.
+
+The term CPU used in this documentation refers to a hardware execution
+context.
+
+Restartable sequences are atomic with respect to preemption (making it
+atomic with respect to other threads running on the same CPU), as well
+as signal delivery (user-space execution contexts nested over the same
+thread). They either complete atomically with respect to preemption on
+the current CPU and signal delivery, or they are aborted.
+
+It is suited for update operations on per-cpu data.
+
+It can be used on data structures shared between threads within a
+process, and on data structures shared between threads across different
+processes.
+
+.PP
+Some examples of operations that can be accelerated or improved
+by this ABI:
+.IP \[bu] 2
+Memory allocator per-cpu free-lists,
+.IP \[bu] 2
+Querying the current CPU number,
+.IP \[bu] 2
+Incrementing per-CPU counters,
+.IP \[bu] 2
+Modifying data protected by per-CPU spinlocks,
+.IP \[bu] 2
+Inserting/removing elements in per-CPU linked-lists,
+.IP \[bu] 2
+Writing/reading per-CPU ring buffers content.
+.IP \[bu] 2
+Accurately reading performance monitoring unit counters
+with respect to thread migration.
+
+.PP
+Restartable sequences must not perform system calls. Doing so may result
+in termination of the process by a segmentation fault.
+
+.PP
+The
+.I rseq
+argument is a pointer to the thread-local rseq structure to be shared
+between kernel and user-space.
+
+.PP
+The layout of
+.B struct rseq
+is as follows:
+.TP
+.B Structure alignment
+This structure is aligned on 32-byte boundary.
+.TP
+.B Structure size
+This structure is extensible. Its size is passed as parameter to the
+rseq system call.
+.TP
+.B Fields
+
+.TP
+.in +4n
+.I cpu_id_start
+Optimistic cache of the CPU number on which the current thread is
+running. Its value is guaranteed to always be a possible CPU number,
+even when rseq is not initialized. The value it contains should always
+be confirmed by reading the cpu_id field.
+
+This field is an optimistic cache in the sense that it is always
+guaranteed to hold a valid CPU number in the range [ 0 ..
+nr_possible_cpus - 1 ]. It can therefore be loaded by user-space and
+used as an offset in per-cpu data structures without having to
+check whether its value is within the valid bounds compared to the
+number of possible CPUs in the system.
+
+For user-space applications executed on a kernel without rseq support,
+the cpu_id_start field stays initialized at 0, which is indeed a valid
+CPU number. It is therefore valid to use it as an offset in per-cpu data
+structures, and only validate whether it's actually the current CPU
+number by comparing it with the cpu_id field within the rseq critical
+section. If the kernel does not provide rseq support, that cpu_id field
+stays initialized at -1, so the comparison always fails, as intended.
+
+It is then up to user-space to use a fall-back mechanism, considering
+that rseq is not available.
+
+.in
+.TP
+.in +4n
+.I cpu_id
+Cache of the CPU number on which the current thread is running.
+-1 if uninitialized.
+.in
+.TP
+.in +4n
+.I rseq_cs
+The rseq_cs field is a pointer to a struct rseq_cs. Is is NULL when no
+rseq assembly block critical section is active for the current thread.
+Setting it to point to a critical section descriptor (struct rseq_cs)
+marks the beginning of the critical section.
+.in
+.TP
+.in +4n
+.I flags
+Flags indicating the restart behavior for the current thread. This is
+mainly used for debugging purposes. Can be either:
+.IP \[bu]
+RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT
+.IP \[bu]
+RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL
+.IP \[bu]
+RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE
+.in
+
+.PP
+The layout of
+.B struct rseq_cs
+version 0 is as follows:
+.TP
+.B Structure alignment
+This structure is aligned on 32-byte boundary.
+.TP
+.B Structure size
+This structure has a fixed size of 32 bytes.
+.TP
+.B Fields
+
+.TP
+.in +4n
+.I version
+Version of this structure.
+.in
+.TP
+.in +4n
+.I flags
+Flags indicating the restart behavior of this structure. Can be
+a combination of:
+.IP \[bu]
+RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT
+.IP \[bu]
+RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL
+.IP \[bu]
+RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE
+.TP
+.in +4n
+.I start_ip
+Instruction pointer address of the first instruction of the sequence of
+consecutive assembly instructions.
+.in
+.TP
+.in +4n
+.I post_commit_offset
+Offset (from start_ip address) of the address after the last instruction
+of the sequence of consecutive assembly instructions.
+.in
+.TP
+.in +4n
+.I abort_ip
+Instruction pointer address where to move the execution flow in case of
+abort of the sequence of consecutive assembly instructions.
+.in
+
+.PP
+The
+.I rseq_len
+argument is the size of the
+.I struct rseq
+to register.
+
+.PP
+The
+.I flags
+argument is 0 for registration, and
+.IR RSEQ_FLAG_UNREGISTER
+for unregistration.
+
+.PP
+The
+.I sig
+argument is the 32-bit signature to be expected before the abort
+handler code.
+
+.PP
+A single library per process should keep the rseq structure in a
+thread-local storage variable.
+The
+.I cpu_id
+field should be initialized to -1, and the
+.I cpu_id_start
+field should be initialized to a possible CPU value (typically 0).
+
+.PP
+Each thread is responsible for registering and unregistering its rseq
+structure. No more than one rseq structure address can be registered
+per thread at a given time.
+
+.PP
+Memory of a registered rseq object must not be freed before the thread
+exits. Reclaim of rseq object's memory must only be done after either an
+explicit rseq unregistration is performed or after the thread exits. Keep
+in mind that the implementation of the Thread-Local Storage (C language
+__thread) lifetime does not guarantee existence of the TLS area up until
+the thread exits.
+
+.PP
+In a typical usage scenario, the thread registering the rseq
+structure will be performing loads and stores from/to that structure. It
+is however also allowed to read that structure from other threads.
+The rseq field updates performed by the kernel provide relaxed atomicity
+semantics, which guarantee that other threads performing relaxed atomic
+reads of the cpu number cache will always observe a consistent value.
+
+.SH RETURN VALUE
+A return value of 0 indicates success. On error, \-1 is returned, and
+.I errno
+is set appropriately.
+
+.SH ERRORS
+.TP
+.B EINVAL
+Either
+.I flags
+contains an invalid value, or
+.I rseq
+contains an address which is not appropriately aligned, or
+.I rseq_len
+contains a size that does not match the size received on registration.
+.TP
+.B ENOSYS
+The
+.BR rseq ()
+system call is not implemented by this kernel.
+.TP
+.B EFAULT
+.I rseq
+is an invalid address.
+.TP
+.B EBUSY
+Restartable sequence is already registered for this thread.
+.TP
+.B EPERM
+The
+.I sig
+argument on unregistration does not match the signature received
+on registration.
+
+.SH VERSIONS
+The
+.BR rseq ()
+system call was added in Linux 4.18.
+
+.SH CONFORMING TO
+.BR rseq ()
+is Linux-specific.
+
+.in
+.SH SEE ALSO
+.BR sched_getcpu (3) ,
+.BR membarrier (2)
--
2.11.0
^ permalink raw reply related
* Re: [PATCH v4] signal: add taskfd_send_signal() syscall
From: Aleksa Sarai @ 2018-12-06 14:27 UTC (permalink / raw)
To: Florian Weimer
Cc: Eric W. Biederman, Jürg Billeter, Christian Brauner,
linux-kernel, linux-api, luto, arnd, serge, jannh, akpm, oleg,
viro, linux-fsdevel, dancol, timmurray, linux-man, keescook, tglx,
x86
In-Reply-To: <877egm6a7v.fsf@oldenburg2.str.redhat.com>
[-- Attachment #1: Type: text/plain, Size: 1854 bytes --]
On 2018-12-06, Florian Weimer <fweimer@redhat.com> wrote:
> > Floriam are you seeing a problem with this behavior or the way Christian
> > was describing it?
>
> My hope is that you could use taskfd_send_signal one day to send a
> signal to a process which you *known* (based on how you've written your
> application) should be running and not in a zombie state, and get back
> an error if it has exited.
You can detect if a process is a zombie via the procfd by observing "stat"
(the state will be "Z"). Personally I'm with Christian that we should
maintain compatibility with the rest of the signal APIs -- sending a
signal to a zombie is a defined (though no-op) concept.
I don't understand why sending a signal should fail in this case -- a
zombie is not the same as a non-existent process. If we need to have a
way of checking whether something is a zombie (other than through
"stat") we can add another method (or flag if it has to be atomic) in
the future. And given the complexity of doing it, I'm even less of a
fan of doing it in the initial patchset.
> If you get this error, only then you wait on the process, using the file
> descriptor you have, and run some recovery code.
>
> Wouldn't that be a reasonable approach once we've got task descriptors?
I think taskfd_wait() is something we'll need eventually, but I don't
think that making taskfd_send_signal() do something that is contrary to
existing kill(2) interfaces (making it so that transitioning to it won't
be seamless),
What would the error be? ESRCH would be _very_ wrong, given that it
would confuse the two states (zombie/dead-for-real) and would lead to
weird cases where fstatat(taskfd) succeeds but taskfd_send_signal(2)
fails.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v4] signal: add taskfd_send_signal() syscall
From: Florian Weimer @ 2018-12-06 13:44 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Jürg Billeter, Christian Brauner, linux-kernel, linux-api,
luto, arnd, serge, jannh, akpm, oleg, cyphar, viro, linux-fsdevel,
dancol, timmurray, linux-man, keescook, tglx, x86
In-Reply-To: <87efaun587.fsf@xmission.com>
* Eric W. Biederman:
> Floriam are you seeing a problem with this behavior or the way Christian
> was describing it?
My hope is that you could use taskfd_send_signal one day to send a
signal to a process which you *known* (based on how you've written your
application) should be running and not in a zombie state, and get back
an error if it has exited.
If you get this error, only then you wait on the process, using the file
descriptor you have, and run some recovery code.
Wouldn't that be a reasonable approach once we've got task descriptors?
Thanks,
Florian
^ permalink raw reply
* Re: [PATCH v4] signal: add taskfd_send_signal() syscall
From: Eric W. Biederman @ 2018-12-06 13:40 UTC (permalink / raw)
To: Florian Weimer
Cc: Jürg Billeter, Christian Brauner, linux-kernel, linux-api,
luto, arnd, serge, jannh, akpm, oleg, cyphar, viro, linux-fsdevel,
dancol, timmurray, linux-man, keescook, tglx, x86
In-Reply-To: <87pnue6bp2.fsf@oldenburg2.str.redhat.com>
Florian Weimer <fweimer@redhat.com> writes:
> * Jürg Billeter:
>
>> On Thu, 2018-12-06 at 13:30 +0100, Florian Weimer wrote:
>>> * Christian Brauner:
>>>
>>> > /* zombies */
>>> > Zombies can be signaled just as any other process. No special error will be
>>> > reported since a zombie state is an unreliable state (cf. [3]).
>>>
>>> I still disagree with this analysis. If I know that the target process
>>> is still alive, and it is not, this is a persistent error condition
>>> which can be reliably reported. Given that someone might send SIGKILL
>>> to the process behind my back, detecting this error condition could be
>>> useful.
>>
>> As I understand it, kill() behaves the same way. I think it's good that
>> this new syscall keeps the behavior as close as possible to kill().
>
> No, kill does not behave in this way because the PID can be reused.
> The error condition is not stable there.
I am not quite certain what is being discussed here.
Posix says:
[ESRCH]
No process or process group can be found corresponding to that specified by pid.
The linux man page says:
ESRCH The process or process group does not exist. Note that an
existing process might be a zombie, a process that has terminated
execution, but has not yet been wait(2)ed for.
What happens with this new system call is exactly the linux behavior.
Success is returned until the specified process or thread group has
been waited for.
The only difference from the behavior of kill is that because the
association between the file descriptor and the pid is not affected by
pid reuse once ESRCH is returned ESRCH will always be returned.
Floriam are you seeing a problem with this behavior or the way Christian
was describing it?
Eric
^ permalink raw reply
* Re: [PATCH v4] signal: add taskfd_send_signal() syscall
From: Florian Weimer @ 2018-12-06 13:20 UTC (permalink / raw)
To: Jürg Billeter
Cc: Christian Brauner, linux-kernel, linux-api, luto, arnd, ebiederm,
serge, jannh, akpm, oleg, cyphar, viro, linux-fsdevel, dancol,
timmurray, linux-man, keescook, tglx, x86
In-Reply-To: <89a6b3a24412d385a816d7d981c60cb1e1bbc0ca.camel@bitron.ch>
* Jürg Billeter:
> On Thu, 2018-12-06 at 14:12 +0100, Florian Weimer wrote:
>> * Jürg Billeter:
>>
>> > On Thu, 2018-12-06 at 13:30 +0100, Florian Weimer wrote:
>> > > * Christian Brauner:
>> > >
>> > > > /* zombies */
>> > > > Zombies can be signaled just as any other process. No special error will be
>> > > > reported since a zombie state is an unreliable state (cf. [3]).
>> > >
>> > > I still disagree with this analysis. If I know that the target process
>> > > is still alive, and it is not, this is a persistent error condition
>> > > which can be reliably reported. Given that someone might send SIGKILL
>> > > to the process behind my back, detecting this error condition could be
>> > > useful.
>> >
>> > As I understand it, kill() behaves the same way. I think it's good that
>> > this new syscall keeps the behavior as close as possible to kill().
>>
>> No, kill does not behave in this way because the PID can be reused.
>> The error condition is not stable there.
>
> The PID can't be reused as long as it's a zombie. It can only be reused
> when it has been wait()ed for. Or am I misunderstanding something?
Hmm, that's a fair point. So the original interface is just broken.
Florian
^ permalink raw reply
* Re: [PATCH v4] signal: add taskfd_send_signal() syscall
From: Jürg Billeter @ 2018-12-06 13:18 UTC (permalink / raw)
To: Florian Weimer
Cc: Christian Brauner, linux-kernel, linux-api, luto, arnd, ebiederm,
serge, jannh, akpm, oleg, cyphar, viro, linux-fsdevel, dancol,
timmurray, linux-man, keescook, tglx, x86
In-Reply-To: <87pnue6bp2.fsf@oldenburg2.str.redhat.com>
On Thu, 2018-12-06 at 14:12 +0100, Florian Weimer wrote:
> * Jürg Billeter:
>
> > On Thu, 2018-12-06 at 13:30 +0100, Florian Weimer wrote:
> > > * Christian Brauner:
> > >
> > > > /* zombies */
> > > > Zombies can be signaled just as any other process. No special error will be
> > > > reported since a zombie state is an unreliable state (cf. [3]).
> > >
> > > I still disagree with this analysis. If I know that the target process
> > > is still alive, and it is not, this is a persistent error condition
> > > which can be reliably reported. Given that someone might send SIGKILL
> > > to the process behind my back, detecting this error condition could be
> > > useful.
> >
> > As I understand it, kill() behaves the same way. I think it's good that
> > this new syscall keeps the behavior as close as possible to kill().
>
> No, kill does not behave in this way because the PID can be reused.
> The error condition is not stable there.
The PID can't be reused as long as it's a zombie. It can only be reused
when it has been wait()ed for. Or am I misunderstanding something?
Jürg
^ permalink raw reply
* Re: [PATCH v4] signal: add taskfd_send_signal() syscall
From: Florian Weimer @ 2018-12-06 13:17 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-kernel, linux-api, luto, arnd, ebiederm, serge, jannh, akpm,
oleg, cyphar, viro, linux-fsdevel, dancol, timmurray, linux-man,
keescook, tglx, x86
In-Reply-To: <20181206125354.ef3zlg3o75w32ymx@brauner.io>
* Christian Brauner:
> On Thu, Dec 06, 2018 at 01:30:19PM +0100, Florian Weimer wrote:
>> * Christian Brauner:
>>
>> > /* zombies */
>> > Zombies can be signaled just as any other process. No special error will be
>> > reported since a zombie state is an unreliable state (cf. [3]).
>>
>> I still disagree with this analysis. If I know that the target process
>> is still alive, and it is not, this is a persistent error condition
>> which can be reliably reported. Given that someone might send SIGKILL
>> to the process behind my back, detecting this error condition could be
>> useful.
>
> Apart from my objection that this is not actually a reliable state
> because of timing issues between e.g. calling wait and a process
> exiting
The point is that if you are in an error state, the error state does not
go away, *especially* if you do not expect the process to terminate and
have not arranged for something calling waitpid on the PID.
> I have two more concerns and one helpful suggestion.
> First, this is hooking pretty deep into kernel internals. So far
> EXIT_ZOMBIE is only exposed in kernel/exit.c and I don't see enough
> value to drag all of this into kernel/signal.c
> Second, all other signal syscalls don't do report errors when signaling
> to zombies as well.
They cannot do this reliably because the error state is not persistent:
the PID can be reused. So for the legacy interface, a difference in
error signaling would just have encouraged a bad programming model.
> It would be odd if this one suddenly did.
I don't think so. My point is that the FD-based mechanism finally
allows to cope with this in a reasonable way.
> Third, if this really becomes such a big issue for userspace in the
> future that we want to do that work then we can add a flag like
> TASKFD_DETECT_ZOMBIE (or some such name) that will allow userspace to
> get an error back when signaling a zombie.
I can live with that.
Thanks,
Florian
^ permalink raw reply
* Re: [PATCH v4] signal: add taskfd_send_signal() syscall
From: Florian Weimer @ 2018-12-06 13:12 UTC (permalink / raw)
To: Jürg Billeter
Cc: Christian Brauner, linux-kernel, linux-api, luto, arnd, ebiederm,
serge, jannh, akpm, oleg, cyphar, viro, linux-fsdevel, dancol,
timmurray, linux-man, keescook, tglx, x86
In-Reply-To: <b0e946bcac12358841c9a99017da819b00794d9c.camel@bitron.ch>
* Jürg Billeter:
> On Thu, 2018-12-06 at 13:30 +0100, Florian Weimer wrote:
>> * Christian Brauner:
>>
>> > /* zombies */
>> > Zombies can be signaled just as any other process. No special error will be
>> > reported since a zombie state is an unreliable state (cf. [3]).
>>
>> I still disagree with this analysis. If I know that the target process
>> is still alive, and it is not, this is a persistent error condition
>> which can be reliably reported. Given that someone might send SIGKILL
>> to the process behind my back, detecting this error condition could be
>> useful.
>
> As I understand it, kill() behaves the same way. I think it's good that
> this new syscall keeps the behavior as close as possible to kill().
No, kill does not behave in this way because the PID can be reused.
The error condition is not stable there.
Thanks,
Florian
^ permalink raw reply
* Re: [PATCH v4] signal: add taskfd_send_signal() syscall
From: Christian Brauner @ 2018-12-06 12:53 UTC (permalink / raw)
To: Florian Weimer
Cc: linux-kernel, linux-api, luto, arnd, ebiederm, serge, jannh, akpm,
oleg, cyphar, viro, linux-fsdevel, dancol, timmurray, linux-man,
keescook, tglx, x86
In-Reply-To: <87h8fq7s84.fsf@oldenburg2.str.redhat.com>
On Thu, Dec 06, 2018 at 01:30:19PM +0100, Florian Weimer wrote:
> * Christian Brauner:
>
> > /* zombies */
> > Zombies can be signaled just as any other process. No special error will be
> > reported since a zombie state is an unreliable state (cf. [3]).
>
> I still disagree with this analysis. If I know that the target process
> is still alive, and it is not, this is a persistent error condition
> which can be reliably reported. Given that someone might send SIGKILL
> to the process behind my back, detecting this error condition could be
> useful.
Apart from my objection that this is not actually a reliable state
because of timing issues between e.g. calling wait and a process exiting
I have two more concerns and one helpful suggestion.
First, this is hooking pretty deep into kernel internals. So far
EXIT_ZOMBIE is only exposed in kernel/exit.c and I don't see enough
value to drag all of this into kernel/signal.c
Second, all other signal syscalls don't do report errors when signaling
to zombies as well. It would be odd if this one suddenly did.
Third, if this really becomes such a big issue for userspace in the
future that we want to do that work then we can add a flag like
TASKFD_DETECT_ZOMBIE (or some such name) that will allow userspace to
get an error back when signaling a zombie.
As far as I'm concerned, this is out of scope for an initial
implementation. We are going to use fds for tasks that's enough
excitement for one patchset!
>
> Rest looks good to me (with the usual caveats).
I take it that's your way of saying Acked-by? :)
Christian
^ permalink raw reply
* Re: [PATCH v4] signal: add taskfd_send_signal() syscall
From: Jürg Billeter @ 2018-12-06 12:45 UTC (permalink / raw)
To: Florian Weimer, Christian Brauner
Cc: linux-kernel, linux-api, luto, arnd, ebiederm, serge, jannh, akpm,
oleg, cyphar, viro, linux-fsdevel, dancol, timmurray, linux-man,
keescook, tglx, x86
In-Reply-To: <87h8fq7s84.fsf@oldenburg2.str.redhat.com>
On Thu, 2018-12-06 at 13:30 +0100, Florian Weimer wrote:
> * Christian Brauner:
>
> > /* zombies */
> > Zombies can be signaled just as any other process. No special error will be
> > reported since a zombie state is an unreliable state (cf. [3]).
>
> I still disagree with this analysis. If I know that the target process
> is still alive, and it is not, this is a persistent error condition
> which can be reliably reported. Given that someone might send SIGKILL
> to the process behind my back, detecting this error condition could be
> useful.
As I understand it, kill() behaves the same way. I think it's good that
this new syscall keeps the behavior as close as possible to kill().
E.g., this would allow emulating kill() (or a higher level API
equivalent) on top of taskfds without subtle differences in behavior.
As the new syscall supports flags, we could consider introducing a flag
that changes the behavior in the zombie case. However, I think that
should be a separate discussion (after merge of the syscall) and the
default behavior makes sense as is.
Jürg
^ permalink raw reply
* Re: [PATCH v4] signal: add taskfd_send_signal() syscall
From: Florian Weimer @ 2018-12-06 12:30 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-kernel, linux-api, luto, arnd, ebiederm, serge, jannh, akpm,
oleg, cyphar, viro, linux-fsdevel, dancol, timmurray, linux-man,
keescook, tglx, x86
In-Reply-To: <20181206121858.12215-1-christian@brauner.io>
* Christian Brauner:
> /* zombies */
> Zombies can be signaled just as any other process. No special error will be
> reported since a zombie state is an unreliable state (cf. [3]).
I still disagree with this analysis. If I know that the target process
is still alive, and it is not, this is a persistent error condition
which can be reliably reported. Given that someone might send SIGKILL
to the process behind my back, detecting this error condition could be
useful.
Rest looks good to me (with the usual caveats).
Thanks,
Florian
^ permalink raw reply
* [PATCH v4] signal: add taskfd_send_signal() syscall
From: Christian Brauner @ 2018-12-06 12:18 UTC (permalink / raw)
To: linux-kernel, linux-api, luto, arnd, ebiederm
Cc: serge, jannh, akpm, oleg, cyphar, viro, linux-fsdevel, dancol,
timmurray, linux-man, keescook, fweimer, tglx, x86,
Christian Brauner
The kill() syscall operates on process identifiers (pid). After a process
has exited its pid can be reused by another process. If a caller sends a
signal to a reused pid it will end up signaling the wrong process. This
issue has often surfaced and there has been a push [1] to address this
problem.
This patch uses file descriptors (fd) from proc/<pid> as stable handles on
struct pid. Even if a pid is recycled the handle will not change. The fd
can be used to send signals to the process it refers to.
Thus, the new syscall taskfd_send_signal() is introduced to solve this
problem. Instead of pids it operates on process fds (taskfd).
/* prototype and argument /*
long taskfd_send_signal(int taskfd, int sig, siginfo_t *info, unsigned int flags);
In addition to the taskfd and signal argument it takes an additional
siginfo_t and flags argument. If the siginfo_t argument is NULL then
taskfd_send_signal() behaves like kill(). If it is not NULL
taskfd_send_signal() behaves like rt_sigqueueinfo().
The flags argument is added to allow for future extensions of this syscall.
It currently needs to be passed as 0. Failing to do so will cause EINVAL.
/* taskfd_send_signal() replaces multiple pid-based syscalls */
The taskfd_send_signal() syscall currently takes on the job of the
following syscalls that operate on pids:
- kill(2)
- rt_sigqueueinfo(2)
The syscall is defined in such a way that it can also operate on thread fds
instead of process fds. In a future patchset I will extend it to operate on
taskfds from /proc/<pid>/task/<tid> at which point it will additionally
take on the job of:
- tgkill(2)
- rt_tgsigqueueinfo(2)
Right now this is intentionally left out to keep this patchset as simple as
possible (cf. [4]). If a taskfd of /proc/<pid>/task/<tid> is passed
EOPNOTSUPP will be returned to give userspace a way to detect when I add
support for such taskfds (cf. [10]).
/* naming */
The original prefix of the syscall was "procfd_". However, it has been
pointed out that since this syscall will eventually operate on both
processes and threads the name should reflect this (cf. [12]). The best
possible candidate even from a userspace perspective seems to be "task".
Although "task" is used internally we are alreday deviating from POSIX by
using file descriptors to processes in the first place so it seems fine to
use the "taskfd_" prefix.
The name taskfd_send_signal() was also chosen to reflect the fact that it
takes on the job of multiple syscalls. It is intentional that the name is
not reminiscent of neither kill(2) nor rt_sigqueueinfo(2). Not the fomer
because it might imply that taskfd_send_signal() is only a replacement for
kill(2) and not the latter because it is a hazzle to remember the correct
spelling (especially for non-native speakers) and because it is not
descriptive enough of what the syscall actually does. The name
"taskfd_send_signal" makes it very clear that its job is to send signals.
/* O_PATH file descriptors */
taskfds opened as O_PATH fds cannot be used to send signals to a process
(cf. [2]). Signaling processes through taskfds is the equivalent of writing
to a file. Thus, this is not an operation that operates "purely at the
file descriptor level" as required by the open(2) manpage.
/* zombies */
Zombies can be signaled just as any other process. No special error will be
reported since a zombie state is an unreliable state (cf. [3]).
/* cross-namespace signals */
The patch currently enforces that the signaler and signalee either are in
the same pid namespace or that the signaler's pid namespace is an ancestor
of the signalee's pid namespace. This is done for the sake of simplicity
and because it is unclear to what values certain members of struct
siginfo_t would need to be set to (cf. [5], [6]).
/* compat syscalls */
It became clear that we would like to avoid adding compat syscalls (cf.
[7]). The compat syscall handling is now done in kernel/signal.c itself by
adding __copy_siginfo_from_user_generic() which lets us avoid compat
syscalls (cf. [8]). It should be noted that the addition of
__copy_siginfo_from_user_any() is caused by a bug in the original
implementation of rt_sigqueueinfo(2) (cf. 12).
With upcoming rework for syscall handling things might improve
significantly (cf. [11]) and __copy_siginfo_from_user_any() will not gain
any additional callers.
/* testing */
This patch was tested on x64 and x86.
/* userspace usage */
An asciinema recording for the basic functionality can be found under [9].
With this patch a process can be killed via:
#define _GNU_SOURCE
#include <errno.h>
#include <fcntl.h>
#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/stat.h>
#include <sys/syscall.h>
#include <sys/types.h>
#include <unistd.h>
static inline int do_taskfd_send_signal(int taskfd, int sig, siginfo_t *info,
unsigned int flags)
{
#ifdef __NR_taskfd_send_signal
return syscall(__NR_taskfd_send_signal, taskfd, sig, info, flags);
#else
return -ENOSYS;
#endif
}
int main(int argc, char *argv[])
{
int fd, ret, saved_errno, sig;
if (argc < 3)
exit(EXIT_FAILURE);
fd = open(argv[1], O_DIRECTORY | O_CLOEXEC);
if (fd < 0) {
printf("%s - Failed to open \"%s\"\n", strerror(errno), argv[1]);
exit(EXIT_FAILURE);
}
sig = atoi(argv[2]);
printf("Sending signal %d to process %s\n", sig, argv[1]);
ret = do_taskfd_send_signal(fd, sig, NULL, 0);
saved_errno = errno;
close(fd);
errno = saved_errno;
if (ret < 0) {
printf("%s - Failed to send signal %d to process %s\n",
strerror(errno), sig, argv[1]);
exit(EXIT_FAILURE);
}
exit(EXIT_SUCCESS);
}
[1]: https://lore.kernel.org/lkml/20181029221037.87724-1-dancol@google.com/
[2]: https://lore.kernel.org/lkml/874lbtjvtd.fsf@oldenburg2.str.redhat.com/
[3]: https://lore.kernel.org/lkml/20181204132604.aspfupwjgjx6fhva@brauner.io/
[4]: https://lore.kernel.org/lkml/20181203180224.fkvw4kajtbvru2ku@brauner.io/
[5]: https://lore.kernel.org/lkml/20181121213946.GA10795@mail.hallyn.com/
[6]: https://lore.kernel.org/lkml/20181120103111.etlqp7zop34v6nv4@brauner.io/
[7]: https://lore.kernel.org/lkml/36323361-90BD-41AF-AB5B-EE0D7BA02C21@amacapital.net/
[8]: https://lore.kernel.org/lkml/87tvjxp8pc.fsf@xmission.com/
[9]: https://asciinema.org/a/IQjuCHew6bnq1cr78yuMv16cy
[10]: https://lore.kernel.org/lkml/20181203180224.fkvw4kajtbvru2ku@brauner.io/
[11]: https://lore.kernel.org/lkml/F53D6D38-3521-4C20-9034-5AF447DF62FF@amacapital.net/
[12]: https://lore.kernel.org/lkml/87zhtjn8ck.fsf@xmission.com/
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Serge Hallyn <serge@hallyn.com>
Cc: Jann Horn <jannh@google.com>
Cc: Andy Lutomirsky <luto@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Aleksa Sarai <cyphar@cyphar.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Florian Weimer <fweimer@redhat.com>
Signed-off-by: Christian Brauner <christian@brauner.io>
Reviewed-by: Kees Cook <keescook@chromium.org>
---
Changelog:
v4:
- updated asciinema to use "taskfd_" prefix
- s/procfd_send_signal/taskfd_send_signal/g
- s/proc_is_tgid_procfd/tgid_taskfd_to_pid/b
- s/proc_is_tid_procfd/tid_taskfd_to_pid/b
- s/__copy_siginfo_from_user_generic/__copy_siginfo_from_user_any/g
- make it clear that __copy_siginfo_from_user_any() is a workaround caused
by a bug in the original implementation of rt_sigqueueinfo()
- when spoofing signals turn them into regular kill signals if si_code is
set to SI_USER
- make proc_is_t{g}id_procfd() return struct pid to allow proc_pid() to
stay private to fs/proc/
v3:
- add __copy_siginfo_from_user_generic() to avoid adding compat syscalls
- s/procfd_signal/procfd_send_signal/g
- change type of flags argument from int to unsigned int
- add comment about what happens to zombies
- add proc_is_tid_procfd()
- return EOPNOTSUPP when /proc/<pid>/task/<tid> fd is passed so userspace
has a way of knowing that tidfds are not supported currently.
v2:
- define __NR_procfd_signal in unistd.h
- wire up compat syscall
- s/proc_is_procfd/proc_is_tgid_procfd/g
- provide stubs when CONFIG_PROC_FS=n
- move proc_pid() to linux/proc_fs.h header
- use proc_pid() to grab struct pid from /proc/<pid> fd
v1:
- patch introduced
---
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
fs/proc/base.c | 20 +++-
include/linux/proc_fs.h | 12 +++
include/linux/syscalls.h | 3 +
include/uapi/asm-generic/unistd.h | 4 +-
kernel/signal.c | 132 +++++++++++++++++++++++--
7 files changed, 164 insertions(+), 9 deletions(-)
diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
index 3cf7b533b3d1..7efb63fd0617 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -398,3 +398,4 @@
384 i386 arch_prctl sys_arch_prctl __ia32_compat_sys_arch_prctl
385 i386 io_pgetevents sys_io_pgetevents __ia32_compat_sys_io_pgetevents
386 i386 rseq sys_rseq __ia32_sys_rseq
+387 i386 taskfd_send_signal sys_taskfd_send_signal __ia32_sys_taskfd_send_signal
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index f0b1709a5ffb..be894f4a84e9 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -343,6 +343,7 @@
332 common statx __x64_sys_statx
333 common io_pgetevents __x64_sys_io_pgetevents
334 common rseq __x64_sys_rseq
+335 common taskfd_send_signal __x64_sys_taskfd_send_signal
#
# x32-specific system call numbers start at 512 to avoid cache impact
diff --git a/fs/proc/base.c b/fs/proc/base.c
index ce3465479447..b8b88bfee455 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -716,8 +716,6 @@ static int proc_pid_permission(struct inode *inode, int mask)
return generic_permission(inode, mask);
}
-
-
static const struct inode_operations proc_def_inode_operations = {
.setattr = proc_setattr,
};
@@ -3038,6 +3036,15 @@ static const struct file_operations proc_tgid_base_operations = {
.llseek = generic_file_llseek,
};
+struct pid *tgid_taskfd_to_pid(const struct file *file)
+{
+ if (!d_is_dir(file->f_path.dentry) ||
+ (file->f_op != &proc_tgid_base_operations))
+ return ERR_PTR(-EBADF);
+
+ return proc_pid(file_inode(file));
+}
+
static struct dentry *proc_tgid_base_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
{
return proc_pident_lookup(dir, dentry,
@@ -3422,6 +3429,15 @@ static const struct file_operations proc_tid_base_operations = {
.llseek = generic_file_llseek,
};
+struct pid *tid_taskfd_to_pid(const struct file *file)
+{
+ if (!d_is_dir(file->f_path.dentry) ||
+ (file->f_op != &proc_tid_base_operations))
+ return ERR_PTR(-EBADF);
+
+ return proc_pid(file_inode(file));
+}
+
static const struct inode_operations proc_tid_base_inode_operations = {
.lookup = proc_tid_base_lookup,
.getattr = pid_getattr,
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index d0e1f1522a78..96817415c420 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -73,6 +73,8 @@ struct proc_dir_entry *proc_create_net_single_write(const char *name, umode_t mo
int (*show)(struct seq_file *, void *),
proc_write_t write,
void *data);
+extern struct pid *tgid_taskfd_to_pid(const struct file *file);
+extern struct pid *tid_taskfd_to_pid(const struct file *file);
#else /* CONFIG_PROC_FS */
@@ -114,6 +116,16 @@ static inline int remove_proc_subtree(const char *name, struct proc_dir_entry *p
#define proc_create_net(name, mode, parent, state_size, ops) ({NULL;})
#define proc_create_net_single(name, mode, parent, show, data) ({NULL;})
+static inline struct pid *tgid_taskfd_to_pid(const struct file *file)
+{
+ return ERR_PTR(-EBADF);
+}
+
+static inline struct pid *tid_taskfd_to_pid(const struct file *file)
+{
+ return ERR_PTR(-EBADF);
+}
+
#endif /* CONFIG_PROC_FS */
struct net;
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 2ac3d13a915b..5ffe194ef29b 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -907,6 +907,9 @@ asmlinkage long sys_statx(int dfd, const char __user *path, unsigned flags,
unsigned mask, struct statx __user *buffer);
asmlinkage long sys_rseq(struct rseq __user *rseq, uint32_t rseq_len,
int flags, uint32_t sig);
+asmlinkage long sys_taskfd_send_signal(int taskfd, int sig,
+ siginfo_t __user *info,
+ unsigned int flags);
/*
* Architecture-specific system calls
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index 538546edbfbd..9343dca63fd9 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -738,9 +738,11 @@ __SYSCALL(__NR_statx, sys_statx)
__SC_COMP(__NR_io_pgetevents, sys_io_pgetevents, compat_sys_io_pgetevents)
#define __NR_rseq 293
__SYSCALL(__NR_rseq, sys_rseq)
+#define __NR_taskfd_send_signal 294
+__SYSCALL(__NR_taskfd_send_signal, sys_taskfd_send_signal)
#undef __NR_syscalls
-#define __NR_syscalls 294
+#define __NR_syscalls 295
/*
* 32 bit systems traditionally used different
diff --git a/kernel/signal.c b/kernel/signal.c
index 9a32bc2088c9..a00a4bcb7605 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -19,7 +19,9 @@
#include <linux/sched/task.h>
#include <linux/sched/task_stack.h>
#include <linux/sched/cputime.h>
+#include <linux/file.h>
#include <linux/fs.h>
+#include <linux/proc_fs.h>
#include <linux/tty.h>
#include <linux/binfmts.h>
#include <linux/coredump.h>
@@ -3286,6 +3288,16 @@ COMPAT_SYSCALL_DEFINE4(rt_sigtimedwait, compat_sigset_t __user *, uthese,
}
#endif
+static inline void prepare_kill_siginfo(int sig, struct kernel_siginfo *info)
+{
+ clear_siginfo(info);
+ info->si_signo = sig;
+ info->si_errno = 0;
+ info->si_code = SI_USER;
+ info->si_pid = task_tgid_vnr(current);
+ info->si_uid = from_kuid_munged(current_user_ns(), current_uid());
+}
+
/**
* sys_kill - send a signal to a process
* @pid: the PID of the process
@@ -3295,16 +3307,124 @@ SYSCALL_DEFINE2(kill, pid_t, pid, int, sig)
{
struct kernel_siginfo info;
- clear_siginfo(&info);
- info.si_signo = sig;
- info.si_errno = 0;
- info.si_code = SI_USER;
- info.si_pid = task_tgid_vnr(current);
- info.si_uid = from_kuid_munged(current_user_ns(), current_uid());
+ prepare_kill_siginfo(sig, &info);
return kill_something_info(sig, &info, pid);
}
+/*
+ * Verify that the signaler and signalee either are in the same pid namespace
+ * or that the signaler's pid namespace is an ancestor of the signalee's pid
+ * namespace.
+ */
+static bool may_signal_taskfd(struct pid *pid)
+{
+ struct pid_namespace *active = task_active_pid_ns(current);
+ struct pid_namespace *p = ns_of_pid(pid);
+
+ for (;;) {
+ if (!p)
+ return false;
+ if (p == active)
+ break;
+ p = p->parent;
+ }
+
+ return true;
+}
+
+static int copy_siginfo_from_user_any(kernel_siginfo_t *kinfo, siginfo_t *info)
+{
+#ifdef CONFIG_COMPAT
+ /*
+ * Avoid hooking up compat syscalls and instead handle necessary
+ * conversions here. Note, this is a stop-gap measure and should not be
+ * considered a generic solution.
+ */
+ if (in_compat_syscall())
+ return copy_siginfo_from_user32(
+ kinfo, (struct compat_siginfo __user *)info);
+#endif
+ return copy_siginfo_from_user(kinfo, info);
+}
+
+/**
+ * sys_taskfd_send_signal - send a signal to a process through a task file
+ * descriptor
+ * @taskfd: the file descriptor of the process
+ * @sig: signal to be sent
+ * @info: the signal info
+ * @flags: future flags to be passed
+ *
+ * Return: 0 on success, negative errno on failure
+ */
+SYSCALL_DEFINE4(taskfd_send_signal, int, taskfd, int, sig,
+ siginfo_t __user *, info, unsigned int, flags)
+{
+ int ret;
+ struct fd f;
+ struct pid *pid;
+ kernel_siginfo_t kinfo;
+
+ /* Enforce flags be set to 0 until we add an extension. */
+ if (flags)
+ return -EINVAL;
+
+ f = fdget_raw(taskfd);
+ if (!f.file)
+ return -EBADF;
+
+ pid = tid_taskfd_to_pid(f.file);
+ if (!IS_ERR(pid)) {
+ /*
+ * Give userspace a way to detect /proc/<pid>/task/<tid>
+ * support when we add it.
+ */
+ ret = -EOPNOTSUPP;
+ goto err;
+ }
+
+ /* Is this a procfd? */
+ pid = tgid_taskfd_to_pid(f.file);
+ if (IS_ERR(pid)) {
+ ret = PTR_ERR(pid);
+ goto err;
+ }
+
+ ret = -EINVAL;
+ if (!may_signal_taskfd(pid))
+ goto err;
+
+ if (info) {
+ ret = copy_siginfo_from_user_any(&kinfo, info);
+ if (unlikely(ret))
+ goto err;
+
+ ret = -EINVAL;
+ if (unlikely(sig != kinfo.si_signo))
+ goto err;
+
+ if ((task_pid(current) != pid) &&
+ (kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL)) {
+ /* Only allow sending arbitrary signals to yourself. */
+ ret = -EPERM;
+ if (kinfo.si_code != SI_USER)
+ goto err;
+
+ /* Turn this into a regular kill signal. */
+ prepare_kill_siginfo(sig, &kinfo);
+ }
+ } else {
+ prepare_kill_siginfo(sig, &kinfo);
+ }
+
+ ret = kill_pid_info(sig, &kinfo, pid);
+
+err:
+ fdput(f);
+ return ret;
+}
+
static int
do_send_specific(pid_t tgid, pid_t pid, int sig, struct kernel_siginfo *info)
{
--
2.19.1
^ permalink raw reply related
* Re: [PATCH v3] signal: add procfd_send_signal() syscall
From: Kees Cook @ 2018-12-06 3:27 UTC (permalink / raw)
To: Christian Brauner
Cc: Eric W. Biederman, LKML, Linux API, Andy Lutomirski,
Arnd Bergmann, Serge E. Hallyn, Jann Horn, Andrew Morton,
Oleg Nesterov, Aleksa Sarai, Al Viro,
linux-fsdevel@vger.kernel.org, Daniel Colascione, Tim Murray,
linux-man, Florian Weimer, Thomas Gleixner, X86 ML
In-Reply-To: <20181206030810.jo5julsc4v5zy34z@brauner.io>
On Wed, Dec 5, 2018 at 7:08 PM Christian Brauner <christian@brauner.io> wrote:
> As a sidenote I'm switching the name from procfd_send_signal() to
> taskfd_send_signal(). It seems to me the best way to handle Eric's
> request to reflect that we can eventually both signal tgids and tids.
Cool; sounds fine to me.
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v3] signal: add procfd_send_signal() syscall
From: Christian Brauner @ 2018-12-06 3:08 UTC (permalink / raw)
To: Kees Cook
Cc: Eric W. Biederman, LKML, Linux API, Andy Lutomirski,
Arnd Bergmann, Serge E. Hallyn, Jann Horn, Andrew Morton,
Oleg Nesterov, Aleksa Sarai, Al Viro,
linux-fsdevel@vger.kernel.org, Daniel Colascione, Tim Murray,
linux-man, Florian Weimer, Thomas Gleixner, X86 ML
In-Reply-To: <CAGXu5jJgYyhUBgzfkkcHGtu6AcC4MBpfwcEoj3M=Y+8q0FZ_dA@mail.gmail.com>
On Wed, Dec 05, 2018 at 03:24:08PM -0800, Kees Cook wrote:
> On Wed, Dec 5, 2018 at 12:53 PM Christian Brauner <christian@brauner.io> wrote:
> > On Wed, Dec 05, 2018 at 12:20:43PM -0600, Eric W. Biederman wrote:
> > > Christian Brauner <christian@brauner.io> writes:
> > > > [1]: https://lkml.org/lkml/2018/11/18/130
> > > > [2]: https://lore.kernel.org/lkml/874lbtjvtd.fsf@oldenburg2.str.redhat.com/
> > > > [3]: https://lore.kernel.org/lkml/20181204132604.aspfupwjgjx6fhva@brauner.io/
> > > > [4]: https://lore.kernel.org/lkml/20181203180224.fkvw4kajtbvru2ku@brauner.io/
> > > > [5]: https://lore.kernel.org/lkml/20181121213946.GA10795@mail.hallyn.com/
> > > > [6]: https://lore.kernel.org/lkml/20181120103111.etlqp7zop34v6nv4@brauner.io/
> > > > [7]: https://lore.kernel.org/lkml/36323361-90BD-41AF-AB5B-EE0D7BA02C21@amacapital.net/
> > > > [8]: https://lore.kernel.org/lkml/87tvjxp8pc.fsf@xmission.com/
> > > > [9]: https://asciinema.org/a/X1J8eGhe3vCfBE2b9TXtTaSJ7
> > > > [10]: https://lore.kernel.org/lkml/20181203180224.fkvw4kajtbvru2ku@brauner.io/
> > > > [11]: https://lore.kernel.org/lkml/F53D6D38-3521-4C20-9034-5AF447DF62FF@amacapital.net/
>
> I nominate this for 2018's most-well-documented syscall commit log award. ;)
Hahaha. If I win can I get my price in beer(s)? :)
>
> > > > + /*
> > > > + * Give userspace a way to detect whether /proc/<pid>/task/<tid> fds
> > > > + * are supported.
> > > > + */
> > > > + ret = -EOPNOTSUPP;
> > > > + if (proc_is_tid_procfd(f.file))
> > > > + goto err;
> > >
> > > -EBADF is the proper error code.
> >
> > This is done so that userspace has a way of figuring out that tid fds
> > are not yet supported. This has been discussed with Florian (see commit
> > message).
>
> Right, we should keep this -EOPNOTSUPP.
>
> > > > + /* Is this a procfd? */
> > > > + ret = -EINVAL;
> > > > + if (!proc_is_tgid_procfd(f.file))
> > > > + goto err;
> > >
> > > -EBADF is the proper error code.
>
> Yeah, EINVAL tends to be used for bad flags... this is more about an
> improper fd.
>
> > >
> > > > + /* Without CONFIG_PROC_FS proc_pid() returns NULL. */
> > > > + pid = proc_pid(file_inode(f.file));
> > > > + if (!pid)
> > > > + goto err;
> > >
> > > Perhaps you want to fold the proc_pid into the proc_is_tgid_procfd
> > > call. That way proc_pid can stay private to proc.
> >
> > Hm, I guess we can do that for now. My intention was to have reuseable
> > helpers but I guess it would be fine for now.
> >
> > >
> > > > + if (!may_signal_procfd(pid))
> > > > + goto err;
> > > > +
>
> Does the ns parent checking in may_signal_procfd need any locking or
> RCU? I know pid and current namespaces are "pinned", but I don't know
> how parent ns works here. I'm assuming the parents are stuck until all
> children go away?
Yeah, since they are hierarchical killing an ancestor means killing the
children. Also, in case you're interested, there's precedent for that:
kernel/pid_namespace.c:static struct ns_common *pidns_get_parent(struct ns_common *ns)
I'm not using this function because a) I would have to special case the
initial test-case and b) it takes a get() on the pid ns which would
force us to use another put which is unnecessary.
>
> > > > + ret = kill_pid_info(sig, &kinfo, pid);
>
> Just double-checking for myself: this does not bypass
> security_task_kill(), so no problem there AFAIK.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
Thanks! :)
As a sidenote I'm switching the name from procfd_send_signal() to
taskfd_send_signal(). It seems to me the best way to handle Eric's
request to reflect that we can eventually both signal tgids and tids.
>
> --
> Kees Cook
^ permalink raw reply
* Re: [PATCH v3] signal: add procfd_send_signal() syscall
From: Kees Cook @ 2018-12-05 23:24 UTC (permalink / raw)
To: Christian Brauner
Cc: Eric W. Biederman, LKML, Linux API, Andy Lutomirski,
Arnd Bergmann, Serge E. Hallyn, Jann Horn, Andrew Morton,
Oleg Nesterov, Aleksa Sarai, Al Viro,
linux-fsdevel@vger.kernel.org, Daniel Colascione, Tim Murray,
linux-man, Florian Weimer, Thomas Gleixner, X86 ML
In-Reply-To: <20181205205242.hxgba5opiapinj56@brauner.io>
On Wed, Dec 5, 2018 at 12:53 PM Christian Brauner <christian@brauner.io> wrote:
> On Wed, Dec 05, 2018 at 12:20:43PM -0600, Eric W. Biederman wrote:
> > Christian Brauner <christian@brauner.io> writes:
> > > [1]: https://lkml.org/lkml/2018/11/18/130
> > > [2]: https://lore.kernel.org/lkml/874lbtjvtd.fsf@oldenburg2.str.redhat.com/
> > > [3]: https://lore.kernel.org/lkml/20181204132604.aspfupwjgjx6fhva@brauner.io/
> > > [4]: https://lore.kernel.org/lkml/20181203180224.fkvw4kajtbvru2ku@brauner.io/
> > > [5]: https://lore.kernel.org/lkml/20181121213946.GA10795@mail.hallyn.com/
> > > [6]: https://lore.kernel.org/lkml/20181120103111.etlqp7zop34v6nv4@brauner.io/
> > > [7]: https://lore.kernel.org/lkml/36323361-90BD-41AF-AB5B-EE0D7BA02C21@amacapital.net/
> > > [8]: https://lore.kernel.org/lkml/87tvjxp8pc.fsf@xmission.com/
> > > [9]: https://asciinema.org/a/X1J8eGhe3vCfBE2b9TXtTaSJ7
> > > [10]: https://lore.kernel.org/lkml/20181203180224.fkvw4kajtbvru2ku@brauner.io/
> > > [11]: https://lore.kernel.org/lkml/F53D6D38-3521-4C20-9034-5AF447DF62FF@amacapital.net/
I nominate this for 2018's most-well-documented syscall commit log award. ;)
> > > + /*
> > > + * Give userspace a way to detect whether /proc/<pid>/task/<tid> fds
> > > + * are supported.
> > > + */
> > > + ret = -EOPNOTSUPP;
> > > + if (proc_is_tid_procfd(f.file))
> > > + goto err;
> >
> > -EBADF is the proper error code.
>
> This is done so that userspace has a way of figuring out that tid fds
> are not yet supported. This has been discussed with Florian (see commit
> message).
Right, we should keep this -EOPNOTSUPP.
> > > + /* Is this a procfd? */
> > > + ret = -EINVAL;
> > > + if (!proc_is_tgid_procfd(f.file))
> > > + goto err;
> >
> > -EBADF is the proper error code.
Yeah, EINVAL tends to be used for bad flags... this is more about an
improper fd.
> >
> > > + /* Without CONFIG_PROC_FS proc_pid() returns NULL. */
> > > + pid = proc_pid(file_inode(f.file));
> > > + if (!pid)
> > > + goto err;
> >
> > Perhaps you want to fold the proc_pid into the proc_is_tgid_procfd
> > call. That way proc_pid can stay private to proc.
>
> Hm, I guess we can do that for now. My intention was to have reuseable
> helpers but I guess it would be fine for now.
>
> >
> > > + if (!may_signal_procfd(pid))
> > > + goto err;
> > > +
Does the ns parent checking in may_signal_procfd need any locking or
RCU? I know pid and current namespaces are "pinned", but I don't know
how parent ns works here. I'm assuming the parents are stuck until all
children go away?
> > > + ret = kill_pid_info(sig, &kinfo, pid);
Just double-checking for myself: this does not bypass
security_task_kill(), so no problem there AFAIK.
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v3] signal: add procfd_send_signal() syscall
From: Christian Brauner @ 2018-12-05 20:52 UTC (permalink / raw)
To: Eric W. Biederman
Cc: linux-kernel, linux-api, luto, arnd, serge, jannh, akpm, oleg,
cyphar, viro, linux-fsdevel, dancol, timmurray, linux-man,
keescook, fweimer, tglx, x86
In-Reply-To: <87zhtjn8ck.fsf@xmission.com>
On Wed, Dec 05, 2018 at 12:20:43PM -0600, Eric W. Biederman wrote:
> Christian Brauner <christian@brauner.io> writes:
>
> > The kill() syscall operates on process identifiers (pid). After a process
> > has exited its pid can be reused by another process. If a caller sends a
> > signal to a reused pid it will end up signaling the wrong process. This
> > issue has often surfaced and there has been a push [1] to address this
> > problem.
> >
> > This patch uses file descriptors (fd) from proc/<pid> as stable handles on
> > struct pid. Even if a pid is recycled the handle will not change. The fd
> > can be used to send signals to the process it refers to.
> > Thus, the new syscall procfd_send_signal() is introduced to solve this
> > problem. Instead of pids it operates on process fds (procfd).
> >
> > /* prototype and argument /*
> > long procfd_send_signal(int fd, int sig, siginfo_t *info, unsigned int flags);
> >
> > In addition to the procfd and signal argument it takes an additional
> > siginfo_t and flags argument. If the siginfo_t argument is NULL then
> > procfd_send_signal() behaves like kill(). If it is not NULL
> > procfd_send_signal() behaves like rt_sigqueueinfo().
> > The flags argument is added to allow for future extensions of this syscall.
> > It currently needs to be passed as 0. Failing to do so will cause EINVAL.
> >
> > /* procfd_send_signal() replaces multiple pid-based syscalls */
> > The procfd_send_signal() syscall currently takes on the job of the
> > following syscalls that operate on pids:
> > - kill(2)
> > - rt_sigqueueinfo(2)
> > The syscall is defined in such a way that it can also operate on thread fds
> > instead of process fds. In a future patchset I will extend it to operate on
> > procfds from /proc/<pid>/task/<tid> at which point it will additionally
> > take on the job of:
> > - tgkill(2)
> > - rt_tgsigqueueinfo(2)
> > Right now this is intentionally left out to keep this patchset as simple as
> > possible (cf. [4]). If a procfd of /proc/<pid>/task/<tid> is passed
> > EOPNOTSUPP will be returned to give userspace a way to detect when I add
> > support for such procfds {cf. [10]).
> >
> > /* naming */
> > The name procfd_send_signal() was chosen to reflect the fact that it takes
> > on the job of multiple syscalls. It is intentional that the name is not
> > reminiscent of neither kill(2) nor rt_sigqueueinfo(2). Not the fomer
> > because it might imply that procfd_send_signal() is only a replacement for
> > kill(2) and not the latter because it is a hazzle to remember the correct
> > spelling (especially for non-native speakers) and because it is not
> > descriptive enough of what the syscall actually does. The name
> > "procfd_send_signal" makes it very clear that its job is to send signals.
> >
> > /* O_PATH file descriptors */
> > procfds opened as O_PATH fds cannot be used to send signals to a process
> > (cf. [2]). Signaling processes through procfds is the equivalent of writing
> > to a file. Thus, this is not an operation that operates "purely at the
> > file descriptor level" as required by the open(2) manpage.
> >
> > /* zombies */
> > Zombies can be signaled just as any other process. No special error will be
> > reported since a zombie state is an unreliable state (cf. [3]).
> >
> > /* cross-namespace signals */
> > The patch currently enforces that the signaler and signalee either are in
> > the same pid namespace or that the signaler's pid namespace is an ancestor
> > of the signalee's pid namespace. This is done for the sake of simplicity
> > and because it is unclear to what values certain members of struct
> > siginfo_t would need to be set to (cf. [5], [6]).
> >
> > /* compat syscalls */
> > It became clear that we would like to avoid adding compat syscalls (cf.
> > [7]). The compat syscall handling is now done in kernel/signal.c itself by
> > adding __copy_siginfo_from_user_generic() which lets us avoid compat
> > syscalls (cf. [8]).
> > With upcoming rework for syscall handling things might improve even further
> > (cf. [11]).
> > This patch was tested on x64 and x86.
> >
> > /* userspace usage */
> > An asciinema recording for the basic functionality can be found under [9].
> > With this patch a process can be killed via:
> >
> > #define _GNU_SOURCE
> > #include <errno.h>
> > #include <fcntl.h>
> > #include <signal.h>
> > #include <stdio.h>
> > #include <stdlib.h>
> > #include <string.h>
> > #include <sys/stat.h>
> > #include <sys/syscall.h>
> > #include <sys/types.h>
> > #include <unistd.h>
> >
> > static inline int do_procfd_send_signal(int procfd, int sig, siginfo_t *info,
> > unsigned int flags)
> > {
> > #ifdef __NR_procfd_send_signal
> > return syscall(__NR_procfd_send_signal, procfd, sig, info, flags);
> > #else
> > return -ENOSYS;
> > #endif
> > }
> >
> > int main(int argc, char *argv[])
> > {
> > int fd, ret, saved_errno, sig;
> >
> > if (argc < 3)
> > exit(EXIT_FAILURE);
> >
> > fd = open(argv[1], O_DIRECTORY | O_CLOEXEC);
> > if (fd < 0) {
> > printf("%s - Failed to open \"%s\"\n", strerror(errno), argv[1]);
> > exit(EXIT_FAILURE);
> > }
> >
> > sig = atoi(argv[2]);
> >
> > printf("Sending signal %d to process %s\n", sig, argv[1]);
> > ret = do_procfd_send_signal(fd, sig, NULL, 0);
> >
> > saved_errno = errno;
> > close(fd);
> > errno = saved_errno;
> >
> > if (ret < 0) {
> > printf("%s - Failed to send signal %d to process %s\n",
> > strerror(errno), sig, argv[1]);
> > exit(EXIT_FAILURE);
> > }
> >
> > exit(EXIT_SUCCESS);
> > }
> >
> > [1]: https://lkml.org/lkml/2018/11/18/130
> > [2]: https://lore.kernel.org/lkml/874lbtjvtd.fsf@oldenburg2.str.redhat.com/
> > [3]: https://lore.kernel.org/lkml/20181204132604.aspfupwjgjx6fhva@brauner.io/
> > [4]: https://lore.kernel.org/lkml/20181203180224.fkvw4kajtbvru2ku@brauner.io/
> > [5]: https://lore.kernel.org/lkml/20181121213946.GA10795@mail.hallyn.com/
> > [6]: https://lore.kernel.org/lkml/20181120103111.etlqp7zop34v6nv4@brauner.io/
> > [7]: https://lore.kernel.org/lkml/36323361-90BD-41AF-AB5B-EE0D7BA02C21@amacapital.net/
> > [8]: https://lore.kernel.org/lkml/87tvjxp8pc.fsf@xmission.com/
> > [9]: https://asciinema.org/a/X1J8eGhe3vCfBE2b9TXtTaSJ7
> > [10]: https://lore.kernel.org/lkml/20181203180224.fkvw4kajtbvru2ku@brauner.io/
> > [11]: https://lore.kernel.org/lkml/F53D6D38-3521-4C20-9034-5AF447DF62FF@amacapital.net/
> >
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Serge Hallyn <serge@hallyn.com>
> > Cc: Jann Horn <jannh@google.com>
> > Cc: Andy Lutomirsky <luto@kernel.org>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Oleg Nesterov <oleg@redhat.com>
> > Cc: Aleksa Sarai <cyphar@cyphar.com>
> > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > Cc: Florian Weimer <fweimer@redhat.com>
> > Signed-off-by: Christian Brauner <christian@brauner.io>
> > ---
> > Changelog:
> > v3:
> > - add __copy_siginfo_from_user_generic() to avoid adding compat syscalls
> > - s/procfd_signal/procfd_send_signal/g
> > - change type of flags argument from int to unsigned int
> > - add comment about what happens to zombies
> > - add proc_is_tid_procfd()
> > - return EOPNOTSUPP when /proc/<pid>/task/<tid> fd is passed so userspace
> > has a way of knowing that tidfds are not supported currently.
> > v2:
> > - define __NR_procfd_signal in unistd.h
> > - wire up compat syscall
> > - s/proc_is_procfd/proc_is_tgid_procfd/g
> > - provide stubs when CONFIG_PROC_FS=n
> > - move proc_pid() to linux/proc_fs.h header
> > - use proc_pid() to grab struct pid from /proc/<pid> fd
> > v1:
> > - patch introduced
> > ---
> > arch/x86/entry/syscalls/syscall_32.tbl | 1 +
> > arch/x86/entry/syscalls/syscall_64.tbl | 1 +
> > fs/proc/base.c | 17 +++-
> > fs/proc/internal.h | 5 -
> > include/linux/proc_fs.h | 18 ++++
> > include/linux/syscalls.h | 3 +
> > include/uapi/asm-generic/unistd.h | 4 +-
> > kernel/signal.c | 127 +++++++++++++++++++++++--
> > 8 files changed, 163 insertions(+), 13 deletions(-)
> >
> > diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
> > index 3cf7b533b3d1..9ab0477d6dc3 100644
> > --- a/arch/x86/entry/syscalls/syscall_32.tbl
> > +++ b/arch/x86/entry/syscalls/syscall_32.tbl
> > @@ -398,3 +398,4 @@
> > 384 i386 arch_prctl sys_arch_prctl __ia32_compat_sys_arch_prctl
> > 385 i386 io_pgetevents sys_io_pgetevents __ia32_compat_sys_io_pgetevents
> > 386 i386 rseq sys_rseq __ia32_sys_rseq
> > +387 i386 procfd_send_signal sys_procfd_send_signal __ia32_sys_procfd_send_signal
> > diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
> > index f0b1709a5ffb..6d22b1130ed0 100644
> > --- a/arch/x86/entry/syscalls/syscall_64.tbl
> > +++ b/arch/x86/entry/syscalls/syscall_64.tbl
> > @@ -343,6 +343,7 @@
> > 332 common statx __x64_sys_statx
> > 333 common io_pgetevents __x64_sys_io_pgetevents
> > 334 common rseq __x64_sys_rseq
> > +335 common procfd_send_signal __x64_sys_procfd_send_signal
> >
> > #
> > # x32-specific system call numbers start at 512 to avoid cache impact
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index ce3465479447..906647d51f6d 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -716,7 +716,10 @@ static int proc_pid_permission(struct inode *inode, int mask)
> > return generic_permission(inode, mask);
> > }
> >
> > -
> > +struct pid *proc_pid(const struct inode *inode)
> > +{
> > + return PROC_I(inode)->pid;
> > +}
> >
> > static const struct inode_operations proc_def_inode_operations = {
> > .setattr = proc_setattr,
> > @@ -3038,6 +3041,12 @@ static const struct file_operations proc_tgid_base_operations = {
> > .llseek = generic_file_llseek,
> > };
> >
> > +bool proc_is_tgid_procfd(const struct file *file)
> > +{
> > + return d_is_dir(file->f_path.dentry) &&
> > + (file->f_op == &proc_tgid_base_operations);
> > +}
> > +
> > static struct dentry *proc_tgid_base_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
> > {
> > return proc_pident_lookup(dir, dentry,
> > @@ -3422,6 +3431,12 @@ static const struct file_operations proc_tid_base_operations = {
> > .llseek = generic_file_llseek,
> > };
> >
> > +bool proc_is_tid_procfd(const struct file *file)
> > +{
> > + return d_is_dir(file->f_path.dentry) &&
> > + (file->f_op == &proc_tid_base_operations);
> > +}
> > +
> > static const struct inode_operations proc_tid_base_inode_operations = {
> > .lookup = proc_tid_base_lookup,
> > .getattr = pid_getattr,
> > diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> > index 5185d7f6a51e..eb69afba83f3 100644
> > --- a/fs/proc/internal.h
> > +++ b/fs/proc/internal.h
> > @@ -113,11 +113,6 @@ static inline void *__PDE_DATA(const struct inode *inode)
> > return PDE(inode)->data;
> > }
> >
> > -static inline struct pid *proc_pid(const struct inode *inode)
> > -{
> > - return PROC_I(inode)->pid;
> > -}
> > -
> > static inline struct task_struct *get_proc_task(const struct inode *inode)
> > {
> > return get_pid_task(proc_pid(inode), PIDTYPE_PID);
> > diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
> > index d0e1f1522a78..ebc0e0ca5256 100644
> > --- a/include/linux/proc_fs.h
> > +++ b/include/linux/proc_fs.h
> > @@ -73,6 +73,9 @@ struct proc_dir_entry *proc_create_net_single_write(const char *name, umode_t mo
> > int (*show)(struct seq_file *, void *),
> > proc_write_t write,
> > void *data);
> > +extern bool proc_is_tgid_procfd(const struct file *file);
> > +extern bool proc_is_tid_procfd(const struct file *file);
> > +extern struct pid *proc_pid(const struct inode *inode);
> >
> > #else /* CONFIG_PROC_FS */
> >
> > @@ -114,6 +117,21 @@ static inline int remove_proc_subtree(const char *name, struct proc_dir_entry *p
> > #define proc_create_net(name, mode, parent, state_size, ops) ({NULL;})
> > #define proc_create_net_single(name, mode, parent, show, data) ({NULL;})
> >
> > +static inline bool proc_is_tgid_procfd(const struct file *file)
> > +{
> > + return false;
> > +}
> > +
> > +static inline bool proc_is_tid_procfd(const struct file *file)
> > +{
> > + return false;
> > +}
> > +
> > +static inline struct pid *proc_pid(const struct inode *inode)
> > +{
> > + return NULL;
> > +}
> > +
> > #endif /* CONFIG_PROC_FS */
> >
> > struct net;
> > diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> > index 2ac3d13a915b..b3a02c6780d0 100644
> > --- a/include/linux/syscalls.h
> > +++ b/include/linux/syscalls.h
> > @@ -907,6 +907,9 @@ asmlinkage long sys_statx(int dfd, const char __user *path, unsigned flags,
> > unsigned mask, struct statx __user *buffer);
> > asmlinkage long sys_rseq(struct rseq __user *rseq, uint32_t rseq_len,
> > int flags, uint32_t sig);
> > +asmlinkage long sys_procfd_send_signal(int procfd, int sig,
> > + siginfo_t __user *info,
> > + unsigned int flags);
> >
> > /*
> > * Architecture-specific system calls
> > diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
> > index 538546edbfbd..b409ee26f8e9 100644
> > --- a/include/uapi/asm-generic/unistd.h
> > +++ b/include/uapi/asm-generic/unistd.h
> > @@ -738,9 +738,11 @@ __SYSCALL(__NR_statx, sys_statx)
> > __SC_COMP(__NR_io_pgetevents, sys_io_pgetevents, compat_sys_io_pgetevents)
> > #define __NR_rseq 293
> > __SYSCALL(__NR_rseq, sys_rseq)
> > +#define __NR_procfd_send_signal 294
> > +__SYSCALL(__NR_procfd_send_signal, sys_procfd_send_signal)
> >
> > #undef __NR_syscalls
> > -#define __NR_syscalls 294
> > +#define __NR_syscalls 295
> >
> > /*
> > * 32 bit systems traditionally used different
> > diff --git a/kernel/signal.c b/kernel/signal.c
> > index 9a32bc2088c9..79a7e0396b0f 100644
> > --- a/kernel/signal.c
> > +++ b/kernel/signal.c
> > @@ -19,7 +19,9 @@
> > #include <linux/sched/task.h>
> > #include <linux/sched/task_stack.h>
> > #include <linux/sched/cputime.h>
> > +#include <linux/file.h>
> > #include <linux/fs.h>
> > +#include <linux/proc_fs.h>
> > #include <linux/tty.h>
> > #include <linux/binfmts.h>
> > #include <linux/coredump.h>
> > @@ -3286,6 +3288,16 @@ COMPAT_SYSCALL_DEFINE4(rt_sigtimedwait, compat_sigset_t __user *, uthese,
> > }
> > #endif
> >
> > +static inline void prepare_kill_siginfo(int sig, struct kernel_siginfo *info)
> > +{
> > + clear_siginfo(info);
> > + info->si_signo = sig;
> > + info->si_errno = 0;
> > + info->si_code = SI_USER;
> > + info->si_pid = task_tgid_vnr(current);
> > + info->si_uid = from_kuid_munged(current_user_ns(), current_uid());
> > +}
> > +
> > /**
> > * sys_kill - send a signal to a process
> > * @pid: the PID of the process
> > @@ -3295,16 +3307,119 @@ SYSCALL_DEFINE2(kill, pid_t, pid, int, sig)
> > {
> > struct kernel_siginfo info;
> >
> > - clear_siginfo(&info);
> > - info.si_signo = sig;
> > - info.si_errno = 0;
> > - info.si_code = SI_USER;
> > - info.si_pid = task_tgid_vnr(current);
> > - info.si_uid = from_kuid_munged(current_user_ns(), current_uid());
> > + prepare_kill_siginfo(sig, &info);
> >
> > return kill_something_info(sig, &info, pid);
> > }
> >
> > +/*
> > + * Verify that the signaler and signalee either are in the same pid namespace
> > + * or that the signaler's pid namespace is an ancestor of the signalee's pid
> > + * namespace.
> > + */
> > +static bool may_signal_procfd(struct pid *pid)
> > +{
> > + struct pid_namespace *active = task_active_pid_ns(current);
> > + struct pid_namespace *p = ns_of_pid(pid);
> > +
> > + for (;;) {
> > + if (!p)
> > + return false;
> > + if (p == active)
> > + break;
> > + p = p->parent;
> > + }
> > +
> > + return true;
> > +}
> > +
> > +static int __copy_siginfo_from_user_generic(int signo, kernel_siginfo_t *kinfo,
> > + siginfo_t *info)
> Ugh. _generic is a poor fit here. In the kernel _generic tends to be
> for the default helper function for a set of operations.
>
> A suffix like _any or anything that implies you can be a compat or not
> a compat syscall would be better.
I can rename it.
>
> > +{
> > +#ifdef CONFIG_COMPAT
> > + /*
> > + * Avoid hooking up compat syscalls and instead handle necessary
> > + * conversions here.
> > + */
> > + if (in_compat_syscall())
> > + return __copy_siginfo_from_user32(
> > + signo, kinfo, (struct compat_siginfo __user *)info);
> > +#endif
> > + return __copy_siginfo_from_user(signo, kinfo, info);
> > +}
>
> Let me be very clear. __copy_siginfo_from_user exists because of a bug
> in the original implementation of rt_sigqueueinfo. It should not gain
> any new callers. It only exists because there is userspace that is
> confirmed to care.
I think that this is very clear from previous discussions we all had. I
can make it clearer in the commit message itself though.
>
> If you must take a separate signal parameter please verify
> "signo == si_signo" and fail if they do not match.
We can do that. But please also note, that the double underscore
variants like __copy_from_siginfo() all set si_signo = signo:
static int __copy_siginfo_from_user(int signo, kernel_siginfo_t *to,
const siginfo_t __user *from)
{
if (copy_from_user(to, from, sizeof(struct kernel_siginfo)))
return -EFAULT;
to->si_signo = signo;
return post_copy_siginfo_from_user(to, from);
}
>
> > +/**
> > + * sys_procfd_send_signal - send a signal to a process through a process file
> > + * descriptor
> > + * @procfd: the file descriptor of the process
>
> There is a subtle naming and semantic issue here that needs to be
> addressed.
> - Are you only going to support processes?
> If so procfd is fine, otherwise another name needs to be chosen.
As said in the commit message and discussed on prior threads it will
eventually work with /proc/<pid>/task/<tid> fds as well. I'll try to
come up with a better name then.
>
> - Do you want to embed what will get signaled into the file descriptor?
There is no need to do this right now since there are no other codepaths
that currently want that information. Once we have them it should be
trivial to stash this in private_data or other.
>
> - Are you about pids? In which case a pid type needs to be added to the
> parameters so you can send to sessions, pgrps, processes, and threads.
No, we are not. The syscall shouldn't get an additional pids parameter.
Imho, that would make it less focused and clean, and would increase the
complexity in the code. The point is that the syscall is reliable once a
procfd has been passed to it. If we bring pids to the game it all
becomes fuzzy again. So I'm very opposed to this.
To my knowledge we have established in prior threads that the syscall
can cover all syscalls that userspace is interested in:
kill(2)
rt_sigqueueinfo(2)
tgkill(2)
rt_tgsigqueueinfo(2)
(See the links to relevant threads in the commit message.)
>
> This is important to get right so that people can know how to use this
> system call.
I agree. All very sensible questions.
>
>
> > + * @sig: signal to be sent
> > + * @info: the signal info
> > + * @flags: future flags to be passed
> > + *
> > + * Return: 0 on success, negative errno on failure
> > + */
> > +SYSCALL_DEFINE4(procfd_send_signal, int, procfd, int, sig,
> > + siginfo_t __user *, info, unsigned int, flags)
> > +{
> > + int ret;
> > + struct fd f;
> > + struct pid *pid;
> > + kernel_siginfo_t kinfo;
> > +
> > + /* Enforce flags be set to 0 until we add an extension. */
> > + if (flags)
> > + return -EINVAL;
> > +
> > + f = fdget_raw(procfd);
> > + if (!f.file)
> > + return -EBADF;
> > +
> > + /*
> > + * Give userspace a way to detect whether /proc/<pid>/task/<tid> fds
> > + * are supported.
> > + */
> > + ret = -EOPNOTSUPP;
> > + if (proc_is_tid_procfd(f.file))
> > + goto err;
>
> -EBADF is the proper error code.
This is done so that userspace has a way of figuring out that tid fds
are not yet supported. This has been discussed with Florian (see commit
message).
>
> > + /* Is this a procfd? */
> > + ret = -EINVAL;
> > + if (!proc_is_tgid_procfd(f.file))
> > + goto err;
>
> -EBADF is the proper error code.
>
> > + /* Without CONFIG_PROC_FS proc_pid() returns NULL. */
> > + pid = proc_pid(file_inode(f.file));
> > + if (!pid)
> > + goto err;
>
> Perhaps you want to fold the proc_pid into the proc_is_tgid_procfd
> call. That way proc_pid can stay private to proc.
Hm, I guess we can do that for now. My intention was to have reuseable
helpers but I guess it would be fine for now.
>
> > + if (!may_signal_procfd(pid))
> > + goto err;
> > +
> > + if (info) {
> > + ret = __copy_siginfo_from_user_generic(sig, &kinfo, info);
> > + if (unlikely(ret))
> > + goto err;
> > +
> > + /*
> > + * Not even root can pretend to send signals from the kernel.
> > + * Nor can they impersonate a kill()/tgkill(), which adds
> > + * source info.
> > + */
> > + ret = -EPERM;
> > + if ((kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL) &&
> > + (task_pid(current) != pid))
> > + goto err;
> > + } else {
> > + prepare_kill_siginfo(sig, &kinfo);
> > + }
>
> I think I would just make the section above say:
>
> ret = copy_siginfo_from_user_any(&kinfo, info);
> if (ret)
> goto err;
>
> /* Only allow sending arbitrary signals to yourself */
> if ((task_pid(current) != pid) && ((kinfo.si_code >= 0) || (kinfo.si_code == SI_TKILL)) {
> if (kinfo.si_code == SI_USER) {
> /* Make this a regular kill signal */
> prepare_kill_siginfo(kinfo.si_signo, &kinfo);
> } else {
> ret = -EPERM;
> goto err;
> }
> }
I'm not sure that's an improvement. We definitely can't move
__copy_siginfo_from_user_any() out of the if (info) branch since we want
to allow passing in siginfo_t as NULL. Allowing it to be NULL makes the
syscall easy to use and spares userspace declaring dummy variables just
to call that function.
I'm also not a fan that we increase the if-nesting with this. But I try
and see if I can work this in nicely.
> > +
> > + ret = kill_pid_info(sig, &kinfo, pid);
> > +
> > +err:
> > + fdput(f);
> > + return ret;
> > +}
> > +
> > static int
> > do_send_specific(pid_t tgid, pid_t pid, int sig, struct kernel_siginfo *info)
> > {
^ permalink raw reply
* Re: pkeys: Reserve PKEY_DISABLE_READ
From: Ram Pai @ 2018-12-05 20:36 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Florian Weimer, Dave Hansen, Linux API, linuxppc-dev, Linux-MM
In-Reply-To: <CALCETrXeSQ8T9nvK7WpgPpkraLfg70FoDWvPZeLS3KiDaqXwtw@mail.gmail.com>
On Wed, Dec 05, 2018 at 08:21:02AM -0800, Andy Lutomirski wrote:
> > On Dec 2, 2018, at 8:02 PM, Ram Pai <linuxram@us.ibm.com> wrote:
> >
> >> On Thu, Nov 29, 2018 at 12:37:15PM +0100, Florian Weimer wrote:
> >> * Dave Hansen:
> >>
> >>>> On 11/27/18 3:57 AM, Florian Weimer wrote:
> >>>> I would have expected something that translates PKEY_DISABLE_WRITE |
> >>>> PKEY_DISABLE_READ into PKEY_DISABLE_ACCESS, and also accepts
> >>>> PKEY_DISABLE_ACCESS | PKEY_DISABLE_READ, for consistency with POWER.
> >>>>
> >>>> (My understanding is that PKEY_DISABLE_ACCESS does not disable all
> >>>> access, but produces execute-only memory.)
> >>>
> >>> Correct, it disables all data access, but not execution.
> >>
> >> So I would expect something like this (completely untested, I did not
> >> even compile this):
> >
> >
> > Ok. I re-read through the entire email thread to understand the problem and
> > the proposed solution. Let me summarize it below. Lets see if we are on the same
> > plate.
> >
> > So the problem is as follows:
> >
> > Currently the kernel supports 'disable-write' and 'disable-access'.
> >
> > On x86, cpu supports 'disable-write' and 'disable-access'. This
> > matches with what the kernel supports. All good.
> >
> > However on power, cpu supports 'disable-read' too. Since userspace can
> > program the cpu directly, userspace has the ability to set
> > 'disable-read' too. This can lead to inconsistency between the kernel
> > and the userspace.
> >
> > We want the kernel to match userspace on all architectures.
> >
> > Proposed Solution:
> >
> > Enhance the kernel to understand 'disable-read', and facilitate architectures
> > that understand 'disable-read' to allow it.
> >
> > Also explicitly define the semantics of disable-access as
> > 'disable-read and disable-write'
> >
> > Did I get this right? Assuming I did, the implementation has to do
> > the following --
> >
> > On power, sys_pkey_alloc() should succeed if the init_val
> > is PKEY_DISABLE_READ, PKEY_DISABLE_WRITE, PKEY_DISABLE_ACCESS
> > or any combination of the three.
> >
> > On x86, sys_pkey_alloc() should succeed if the init_val is
> > PKEY_DISABLE_WRITE or PKEY_DISABLE_ACCESS or PKEY_DISABLE_READ
> > or any combination of the three, except PKEY_DISABLE_READ
> > specified all by itself.
> >
> > On all other arches, none of the flags are supported.
>
> I don’t really love having a situation where you can use different
> flag combinations to refer to the same mode.
true. But it is a side-effect of x86 cpu implicitly defining
'disable-access' as a combination of 'disable-read' and 'disable_write'.
In other words, if you disable-access on a pte on x86, you are
automatically disabling read and disabling write on that page.
The software/kernel just happens to explicitly capture that implicit
behavior.
>
> Also, we should document the effect these flags have on execute permission.
Actually none of the above flags, interact with execute permission. They
operate independently; both on x86 and on POWER. But yes, this
statement needs to be documented somewhere.
RP
^ permalink raw reply
* Re: pkeys: Reserve PKEY_DISABLE_READ
From: Ram Pai @ 2018-12-05 20:23 UTC (permalink / raw)
To: Florian Weimer; +Cc: Dave Hansen, linux-api, linuxppc-dev, linux-mm
In-Reply-To: <87zhtki0vo.fsf@oldenburg2.str.redhat.com>
On Wed, Dec 05, 2018 at 02:00:59PM +0100, Florian Weimer wrote:
> * Ram Pai:
>
> > Ok. here is a patch, compiled but not tested. See if this meets the
> > specifications.
> >
> > -----------------------------------------------------------------------------------
> >
> > commit 3dc06e73f3795921265d5d1d935e428deab01616
> > Author: Ram Pai <linuxram@us.ibm.com>
> > Date: Tue Dec 4 00:04:11 2018 -0500
> >
> > pkeys: add support of PKEY_DISABLE_READ
>
> Thanks. In the x86 code, the translation of PKEY_DISABLE_READ |
> PKEY_DISABLE_WRITE to PKEY_DISABLE_ACCESS appears to be missing. I
> believe the existing code produces PKEY_DISABLE_WRITE, which is wrong.
ah. yes. good point.
>
> Rest looks okay to me (again not tested).
thanks,
RP
^ permalink raw reply
* Re: [PATCH v3] signal: add procfd_send_signal() syscall
From: Eric W. Biederman @ 2018-12-05 18:20 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-kernel, linux-api, luto, arnd, serge, jannh, akpm, oleg,
cyphar, viro, linux-fsdevel, dancol, timmurray, linux-man,
keescook, fweimer, tglx, x86
In-Reply-To: <20181205092203.14105-1-christian@brauner.io>
Christian Brauner <christian@brauner.io> writes:
> The kill() syscall operates on process identifiers (pid). After a process
> has exited its pid can be reused by another process. If a caller sends a
> signal to a reused pid it will end up signaling the wrong process. This
> issue has often surfaced and there has been a push [1] to address this
> problem.
>
> This patch uses file descriptors (fd) from proc/<pid> as stable handles on
> struct pid. Even if a pid is recycled the handle will not change. The fd
> can be used to send signals to the process it refers to.
> Thus, the new syscall procfd_send_signal() is introduced to solve this
> problem. Instead of pids it operates on process fds (procfd).
>
> /* prototype and argument /*
> long procfd_send_signal(int fd, int sig, siginfo_t *info, unsigned int flags);
>
> In addition to the procfd and signal argument it takes an additional
> siginfo_t and flags argument. If the siginfo_t argument is NULL then
> procfd_send_signal() behaves like kill(). If it is not NULL
> procfd_send_signal() behaves like rt_sigqueueinfo().
> The flags argument is added to allow for future extensions of this syscall.
> It currently needs to be passed as 0. Failing to do so will cause EINVAL.
>
> /* procfd_send_signal() replaces multiple pid-based syscalls */
> The procfd_send_signal() syscall currently takes on the job of the
> following syscalls that operate on pids:
> - kill(2)
> - rt_sigqueueinfo(2)
> The syscall is defined in such a way that it can also operate on thread fds
> instead of process fds. In a future patchset I will extend it to operate on
> procfds from /proc/<pid>/task/<tid> at which point it will additionally
> take on the job of:
> - tgkill(2)
> - rt_tgsigqueueinfo(2)
> Right now this is intentionally left out to keep this patchset as simple as
> possible (cf. [4]). If a procfd of /proc/<pid>/task/<tid> is passed
> EOPNOTSUPP will be returned to give userspace a way to detect when I add
> support for such procfds {cf. [10]).
>
> /* naming */
> The name procfd_send_signal() was chosen to reflect the fact that it takes
> on the job of multiple syscalls. It is intentional that the name is not
> reminiscent of neither kill(2) nor rt_sigqueueinfo(2). Not the fomer
> because it might imply that procfd_send_signal() is only a replacement for
> kill(2) and not the latter because it is a hazzle to remember the correct
> spelling (especially for non-native speakers) and because it is not
> descriptive enough of what the syscall actually does. The name
> "procfd_send_signal" makes it very clear that its job is to send signals.
>
> /* O_PATH file descriptors */
> procfds opened as O_PATH fds cannot be used to send signals to a process
> (cf. [2]). Signaling processes through procfds is the equivalent of writing
> to a file. Thus, this is not an operation that operates "purely at the
> file descriptor level" as required by the open(2) manpage.
>
> /* zombies */
> Zombies can be signaled just as any other process. No special error will be
> reported since a zombie state is an unreliable state (cf. [3]).
>
> /* cross-namespace signals */
> The patch currently enforces that the signaler and signalee either are in
> the same pid namespace or that the signaler's pid namespace is an ancestor
> of the signalee's pid namespace. This is done for the sake of simplicity
> and because it is unclear to what values certain members of struct
> siginfo_t would need to be set to (cf. [5], [6]).
>
> /* compat syscalls */
> It became clear that we would like to avoid adding compat syscalls (cf.
> [7]). The compat syscall handling is now done in kernel/signal.c itself by
> adding __copy_siginfo_from_user_generic() which lets us avoid compat
> syscalls (cf. [8]).
> With upcoming rework for syscall handling things might improve even further
> (cf. [11]).
> This patch was tested on x64 and x86.
>
> /* userspace usage */
> An asciinema recording for the basic functionality can be found under [9].
> With this patch a process can be killed via:
>
> #define _GNU_SOURCE
> #include <errno.h>
> #include <fcntl.h>
> #include <signal.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <sys/stat.h>
> #include <sys/syscall.h>
> #include <sys/types.h>
> #include <unistd.h>
>
> static inline int do_procfd_send_signal(int procfd, int sig, siginfo_t *info,
> unsigned int flags)
> {
> #ifdef __NR_procfd_send_signal
> return syscall(__NR_procfd_send_signal, procfd, sig, info, flags);
> #else
> return -ENOSYS;
> #endif
> }
>
> int main(int argc, char *argv[])
> {
> int fd, ret, saved_errno, sig;
>
> if (argc < 3)
> exit(EXIT_FAILURE);
>
> fd = open(argv[1], O_DIRECTORY | O_CLOEXEC);
> if (fd < 0) {
> printf("%s - Failed to open \"%s\"\n", strerror(errno), argv[1]);
> exit(EXIT_FAILURE);
> }
>
> sig = atoi(argv[2]);
>
> printf("Sending signal %d to process %s\n", sig, argv[1]);
> ret = do_procfd_send_signal(fd, sig, NULL, 0);
>
> saved_errno = errno;
> close(fd);
> errno = saved_errno;
>
> if (ret < 0) {
> printf("%s - Failed to send signal %d to process %s\n",
> strerror(errno), sig, argv[1]);
> exit(EXIT_FAILURE);
> }
>
> exit(EXIT_SUCCESS);
> }
>
> [1]: https://lkml.org/lkml/2018/11/18/130
> [2]: https://lore.kernel.org/lkml/874lbtjvtd.fsf@oldenburg2.str.redhat.com/
> [3]: https://lore.kernel.org/lkml/20181204132604.aspfupwjgjx6fhva@brauner.io/
> [4]: https://lore.kernel.org/lkml/20181203180224.fkvw4kajtbvru2ku@brauner.io/
> [5]: https://lore.kernel.org/lkml/20181121213946.GA10795@mail.hallyn.com/
> [6]: https://lore.kernel.org/lkml/20181120103111.etlqp7zop34v6nv4@brauner.io/
> [7]: https://lore.kernel.org/lkml/36323361-90BD-41AF-AB5B-EE0D7BA02C21@amacapital.net/
> [8]: https://lore.kernel.org/lkml/87tvjxp8pc.fsf@xmission.com/
> [9]: https://asciinema.org/a/X1J8eGhe3vCfBE2b9TXtTaSJ7
> [10]: https://lore.kernel.org/lkml/20181203180224.fkvw4kajtbvru2ku@brauner.io/
> [11]: https://lore.kernel.org/lkml/F53D6D38-3521-4C20-9034-5AF447DF62FF@amacapital.net/
>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Serge Hallyn <serge@hallyn.com>
> Cc: Jann Horn <jannh@google.com>
> Cc: Andy Lutomirsky <luto@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Aleksa Sarai <cyphar@cyphar.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Florian Weimer <fweimer@redhat.com>
> Signed-off-by: Christian Brauner <christian@brauner.io>
> ---
> Changelog:
> v3:
> - add __copy_siginfo_from_user_generic() to avoid adding compat syscalls
> - s/procfd_signal/procfd_send_signal/g
> - change type of flags argument from int to unsigned int
> - add comment about what happens to zombies
> - add proc_is_tid_procfd()
> - return EOPNOTSUPP when /proc/<pid>/task/<tid> fd is passed so userspace
> has a way of knowing that tidfds are not supported currently.
> v2:
> - define __NR_procfd_signal in unistd.h
> - wire up compat syscall
> - s/proc_is_procfd/proc_is_tgid_procfd/g
> - provide stubs when CONFIG_PROC_FS=n
> - move proc_pid() to linux/proc_fs.h header
> - use proc_pid() to grab struct pid from /proc/<pid> fd
> v1:
> - patch introduced
> ---
> arch/x86/entry/syscalls/syscall_32.tbl | 1 +
> arch/x86/entry/syscalls/syscall_64.tbl | 1 +
> fs/proc/base.c | 17 +++-
> fs/proc/internal.h | 5 -
> include/linux/proc_fs.h | 18 ++++
> include/linux/syscalls.h | 3 +
> include/uapi/asm-generic/unistd.h | 4 +-
> kernel/signal.c | 127 +++++++++++++++++++++++--
> 8 files changed, 163 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
> index 3cf7b533b3d1..9ab0477d6dc3 100644
> --- a/arch/x86/entry/syscalls/syscall_32.tbl
> +++ b/arch/x86/entry/syscalls/syscall_32.tbl
> @@ -398,3 +398,4 @@
> 384 i386 arch_prctl sys_arch_prctl __ia32_compat_sys_arch_prctl
> 385 i386 io_pgetevents sys_io_pgetevents __ia32_compat_sys_io_pgetevents
> 386 i386 rseq sys_rseq __ia32_sys_rseq
> +387 i386 procfd_send_signal sys_procfd_send_signal __ia32_sys_procfd_send_signal
> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
> index f0b1709a5ffb..6d22b1130ed0 100644
> --- a/arch/x86/entry/syscalls/syscall_64.tbl
> +++ b/arch/x86/entry/syscalls/syscall_64.tbl
> @@ -343,6 +343,7 @@
> 332 common statx __x64_sys_statx
> 333 common io_pgetevents __x64_sys_io_pgetevents
> 334 common rseq __x64_sys_rseq
> +335 common procfd_send_signal __x64_sys_procfd_send_signal
>
> #
> # x32-specific system call numbers start at 512 to avoid cache impact
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index ce3465479447..906647d51f6d 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -716,7 +716,10 @@ static int proc_pid_permission(struct inode *inode, int mask)
> return generic_permission(inode, mask);
> }
>
> -
> +struct pid *proc_pid(const struct inode *inode)
> +{
> + return PROC_I(inode)->pid;
> +}
>
> static const struct inode_operations proc_def_inode_operations = {
> .setattr = proc_setattr,
> @@ -3038,6 +3041,12 @@ static const struct file_operations proc_tgid_base_operations = {
> .llseek = generic_file_llseek,
> };
>
> +bool proc_is_tgid_procfd(const struct file *file)
> +{
> + return d_is_dir(file->f_path.dentry) &&
> + (file->f_op == &proc_tgid_base_operations);
> +}
> +
> static struct dentry *proc_tgid_base_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
> {
> return proc_pident_lookup(dir, dentry,
> @@ -3422,6 +3431,12 @@ static const struct file_operations proc_tid_base_operations = {
> .llseek = generic_file_llseek,
> };
>
> +bool proc_is_tid_procfd(const struct file *file)
> +{
> + return d_is_dir(file->f_path.dentry) &&
> + (file->f_op == &proc_tid_base_operations);
> +}
> +
> static const struct inode_operations proc_tid_base_inode_operations = {
> .lookup = proc_tid_base_lookup,
> .getattr = pid_getattr,
> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> index 5185d7f6a51e..eb69afba83f3 100644
> --- a/fs/proc/internal.h
> +++ b/fs/proc/internal.h
> @@ -113,11 +113,6 @@ static inline void *__PDE_DATA(const struct inode *inode)
> return PDE(inode)->data;
> }
>
> -static inline struct pid *proc_pid(const struct inode *inode)
> -{
> - return PROC_I(inode)->pid;
> -}
> -
> static inline struct task_struct *get_proc_task(const struct inode *inode)
> {
> return get_pid_task(proc_pid(inode), PIDTYPE_PID);
> diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
> index d0e1f1522a78..ebc0e0ca5256 100644
> --- a/include/linux/proc_fs.h
> +++ b/include/linux/proc_fs.h
> @@ -73,6 +73,9 @@ struct proc_dir_entry *proc_create_net_single_write(const char *name, umode_t mo
> int (*show)(struct seq_file *, void *),
> proc_write_t write,
> void *data);
> +extern bool proc_is_tgid_procfd(const struct file *file);
> +extern bool proc_is_tid_procfd(const struct file *file);
> +extern struct pid *proc_pid(const struct inode *inode);
>
> #else /* CONFIG_PROC_FS */
>
> @@ -114,6 +117,21 @@ static inline int remove_proc_subtree(const char *name, struct proc_dir_entry *p
> #define proc_create_net(name, mode, parent, state_size, ops) ({NULL;})
> #define proc_create_net_single(name, mode, parent, show, data) ({NULL;})
>
> +static inline bool proc_is_tgid_procfd(const struct file *file)
> +{
> + return false;
> +}
> +
> +static inline bool proc_is_tid_procfd(const struct file *file)
> +{
> + return false;
> +}
> +
> +static inline struct pid *proc_pid(const struct inode *inode)
> +{
> + return NULL;
> +}
> +
> #endif /* CONFIG_PROC_FS */
>
> struct net;
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index 2ac3d13a915b..b3a02c6780d0 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -907,6 +907,9 @@ asmlinkage long sys_statx(int dfd, const char __user *path, unsigned flags,
> unsigned mask, struct statx __user *buffer);
> asmlinkage long sys_rseq(struct rseq __user *rseq, uint32_t rseq_len,
> int flags, uint32_t sig);
> +asmlinkage long sys_procfd_send_signal(int procfd, int sig,
> + siginfo_t __user *info,
> + unsigned int flags);
>
> /*
> * Architecture-specific system calls
> diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
> index 538546edbfbd..b409ee26f8e9 100644
> --- a/include/uapi/asm-generic/unistd.h
> +++ b/include/uapi/asm-generic/unistd.h
> @@ -738,9 +738,11 @@ __SYSCALL(__NR_statx, sys_statx)
> __SC_COMP(__NR_io_pgetevents, sys_io_pgetevents, compat_sys_io_pgetevents)
> #define __NR_rseq 293
> __SYSCALL(__NR_rseq, sys_rseq)
> +#define __NR_procfd_send_signal 294
> +__SYSCALL(__NR_procfd_send_signal, sys_procfd_send_signal)
>
> #undef __NR_syscalls
> -#define __NR_syscalls 294
> +#define __NR_syscalls 295
>
> /*
> * 32 bit systems traditionally used different
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 9a32bc2088c9..79a7e0396b0f 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -19,7 +19,9 @@
> #include <linux/sched/task.h>
> #include <linux/sched/task_stack.h>
> #include <linux/sched/cputime.h>
> +#include <linux/file.h>
> #include <linux/fs.h>
> +#include <linux/proc_fs.h>
> #include <linux/tty.h>
> #include <linux/binfmts.h>
> #include <linux/coredump.h>
> @@ -3286,6 +3288,16 @@ COMPAT_SYSCALL_DEFINE4(rt_sigtimedwait, compat_sigset_t __user *, uthese,
> }
> #endif
>
> +static inline void prepare_kill_siginfo(int sig, struct kernel_siginfo *info)
> +{
> + clear_siginfo(info);
> + info->si_signo = sig;
> + info->si_errno = 0;
> + info->si_code = SI_USER;
> + info->si_pid = task_tgid_vnr(current);
> + info->si_uid = from_kuid_munged(current_user_ns(), current_uid());
> +}
> +
> /**
> * sys_kill - send a signal to a process
> * @pid: the PID of the process
> @@ -3295,16 +3307,119 @@ SYSCALL_DEFINE2(kill, pid_t, pid, int, sig)
> {
> struct kernel_siginfo info;
>
> - clear_siginfo(&info);
> - info.si_signo = sig;
> - info.si_errno = 0;
> - info.si_code = SI_USER;
> - info.si_pid = task_tgid_vnr(current);
> - info.si_uid = from_kuid_munged(current_user_ns(), current_uid());
> + prepare_kill_siginfo(sig, &info);
>
> return kill_something_info(sig, &info, pid);
> }
>
> +/*
> + * Verify that the signaler and signalee either are in the same pid namespace
> + * or that the signaler's pid namespace is an ancestor of the signalee's pid
> + * namespace.
> + */
> +static bool may_signal_procfd(struct pid *pid)
> +{
> + struct pid_namespace *active = task_active_pid_ns(current);
> + struct pid_namespace *p = ns_of_pid(pid);
> +
> + for (;;) {
> + if (!p)
> + return false;
> + if (p == active)
> + break;
> + p = p->parent;
> + }
> +
> + return true;
> +}
> +
> +static int __copy_siginfo_from_user_generic(int signo, kernel_siginfo_t *kinfo,
> + siginfo_t *info)
Ugh. _generic is a poor fit here. In the kernel _generic tends to be
for the default helper function for a set of operations.
A suffix like _any or anything that implies you can be a compat or not
a compat syscall would be better.
> +{
> +#ifdef CONFIG_COMPAT
> + /*
> + * Avoid hooking up compat syscalls and instead handle necessary
> + * conversions here.
> + */
> + if (in_compat_syscall())
> + return __copy_siginfo_from_user32(
> + signo, kinfo, (struct compat_siginfo __user *)info);
> +#endif
> + return __copy_siginfo_from_user(signo, kinfo, info);
> +}
Let me be very clear. __copy_siginfo_from_user exists because of a bug
in the original implementation of rt_sigqueueinfo. It should not gain
any new callers. It only exists because there is userspace that is
confirmed to care.
If you must take a separate signal parameter please verify
"signo == si_signo" and fail if they do not match.
> +/**
> + * sys_procfd_send_signal - send a signal to a process through a process file
> + * descriptor
> + * @procfd: the file descriptor of the process
There is a subtle naming and semantic issue here that needs to be
addressed.
- Are you only going to support processes?
If so procfd is fine, otherwise another name needs to be chosen.
- Do you want to embed what will get signaled into the file descriptor?
- Are you about pids? In which case a pid type needs to be added to the
parameters so you can send to sessions, pgrps, processes, and threads.
This is important to get right so that people can know how to use this
system call.
> + * @sig: signal to be sent
> + * @info: the signal info
> + * @flags: future flags to be passed
> + *
> + * Return: 0 on success, negative errno on failure
> + */
> +SYSCALL_DEFINE4(procfd_send_signal, int, procfd, int, sig,
> + siginfo_t __user *, info, unsigned int, flags)
> +{
> + int ret;
> + struct fd f;
> + struct pid *pid;
> + kernel_siginfo_t kinfo;
> +
> + /* Enforce flags be set to 0 until we add an extension. */
> + if (flags)
> + return -EINVAL;
> +
> + f = fdget_raw(procfd);
> + if (!f.file)
> + return -EBADF;
> +
> + /*
> + * Give userspace a way to detect whether /proc/<pid>/task/<tid> fds
> + * are supported.
> + */
> + ret = -EOPNOTSUPP;
> + if (proc_is_tid_procfd(f.file))
> + goto err;
-EBADF is the proper error code.
> + /* Is this a procfd? */
> + ret = -EINVAL;
> + if (!proc_is_tgid_procfd(f.file))
> + goto err;
-EBADF is the proper error code.
> + /* Without CONFIG_PROC_FS proc_pid() returns NULL. */
> + pid = proc_pid(file_inode(f.file));
> + if (!pid)
> + goto err;
Perhaps you want to fold the proc_pid into the proc_is_tgid_procfd
call. That way proc_pid can stay private to proc.
> + if (!may_signal_procfd(pid))
> + goto err;
> +
> + if (info) {
> + ret = __copy_siginfo_from_user_generic(sig, &kinfo, info);
> + if (unlikely(ret))
> + goto err;
> +
> + /*
> + * Not even root can pretend to send signals from the kernel.
> + * Nor can they impersonate a kill()/tgkill(), which adds
> + * source info.
> + */
> + ret = -EPERM;
> + if ((kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL) &&
> + (task_pid(current) != pid))
> + goto err;
> + } else {
> + prepare_kill_siginfo(sig, &kinfo);
> + }
I think I would just make the section above say:
ret = copy_siginfo_from_user_any(&kinfo, info);
if (ret)
goto err;
/* Only allow sending arbitrary signals to yourself */
if ((task_pid(current) != pid) &&
((kinfo.si_code >= 0) || (kinfo.si_code == SI_TKILL)) {
if (kinfo.si_code == SI_USER) {
/* Make this a regular kill signal */
prepare_kill_siginfo(kinfo.si_signo, &kinfo);
} else {
ret = -EPERM;
goto err;
}
}
> +
> + ret = kill_pid_info(sig, &kinfo, pid);
> +
> +err:
> + fdput(f);
> + return ret;
> +}
> +
> static int
> do_send_specific(pid_t tgid, pid_t pid, int sig, struct kernel_siginfo *info)
> {
^ permalink raw reply
* Re: [PATCH v3] signal: add procfd_send_signal() syscall
From: Eric W. Biederman @ 2018-12-05 18:10 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-kernel, linux-api, luto, arnd, serge, jannh, akpm, oleg,
cyphar, viro, linux-fsdevel, dancol, timmurray, linux-man,
keescook, fweimer, tglx, x86
In-Reply-To: <20181205092203.14105-1-christian@brauner.io>
Christian Brauner <christian@brauner.io> writes:
> The kill() syscall operates on process identifiers (pid). After a process
> has exited its pid can be reused by another process. If a caller sends a
> signal to a reused pid it will end up signaling the wrong process. This
> issue has often surfaced and there has been a push [1] to address this
> problem.
>
> This patch uses file descriptors (fd) from proc/<pid> as stable handles on
> struct pid. Even if a pid is recycled the handle will not change. The fd
> can be used to send signals to the process it refers to.
> Thus, the new syscall procfd_send_signal() is introduced to solve this
> problem. Instead of pids it operates on process fds (procfd).
>
> /* prototype and argument /*
> long procfd_send_signal(int fd, int sig, siginfo_t *info, unsigned int flags);
>
> In addition to the procfd and signal argument it takes an additional
> siginfo_t and flags argument. If the siginfo_t argument is NULL then
> procfd_send_signal() behaves like kill(). If it is not NULL
> procfd_send_signal() behaves like rt_sigqueueinfo().
> The flags argument is added to allow for future extensions of this syscall.
> It currently needs to be passed as 0. Failing to do so will cause EINVAL.
>
> /* procfd_send_signal() replaces multiple pid-based syscalls */
> The procfd_send_signal() syscall currently takes on the job of the
> following syscalls that operate on pids:
> - kill(2)
> - rt_sigqueueinfo(2)
> The syscall is defined in such a way that it can also operate on thread fds
> instead of process fds. In a future patchset I will extend it to operate on
> procfds from /proc/<pid>/task/<tid> at which point it will additionally
> take on the job of:
> - tgkill(2)
> - rt_tgsigqueueinfo(2)
> Right now this is intentionally left out to keep this patchset as simple as
> possible (cf. [4]). If a procfd of /proc/<pid>/task/<tid> is passed
> EOPNOTSUPP will be returned to give userspace a way to detect when I add
> support for such procfds {cf. [10]).
>
> /* naming */
> The name procfd_send_signal() was chosen to reflect the fact that it takes
> on the job of multiple syscalls. It is intentional that the name is not
> reminiscent of neither kill(2) nor rt_sigqueueinfo(2). Not the fomer
> because it might imply that procfd_send_signal() is only a replacement for
> kill(2) and not the latter because it is a hazzle to remember the correct
> spelling (especially for non-native speakers) and because it is not
> descriptive enough of what the syscall actually does. The name
> "procfd_send_signal" makes it very clear that its job is to send signals.
>
> /* O_PATH file descriptors */
> procfds opened as O_PATH fds cannot be used to send signals to a process
> (cf. [2]). Signaling processes through procfds is the equivalent of writing
> to a file. Thus, this is not an operation that operates "purely at the
> file descriptor level" as required by the open(2) manpage.
>
> /* zombies */
> Zombies can be signaled just as any other process. No special error will be
> reported since a zombie state is an unreliable state (cf. [3]).
>
> /* cross-namespace signals */
> The patch currently enforces that the signaler and signalee either are in
> the same pid namespace or that the signaler's pid namespace is an ancestor
> of the signalee's pid namespace. This is done for the sake of simplicity
> and because it is unclear to what values certain members of struct
> siginfo_t would need to be set to (cf. [5], [6]).
>
> /* compat syscalls */
> It became clear that we would like to avoid adding compat syscalls (cf.
> [7]). The compat syscall handling is now done in kernel/signal.c itself by
> adding __copy_siginfo_from_user_generic() which lets us avoid compat
> syscalls (cf. [8]).
> With upcoming rework for syscall handling things might improve even further
> (cf. [11]).
> This patch was tested on x64 and x86.
>
> /* userspace usage */
> An asciinema recording for the basic functionality can be found under [9].
> With this patch a process can be killed via:
>
> #define _GNU_SOURCE
> #include <errno.h>
> #include <fcntl.h>
> #include <signal.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <sys/stat.h>
> #include <sys/syscall.h>
> #include <sys/types.h>
> #include <unistd.h>
>
> static inline int do_procfd_send_signal(int procfd, int sig, siginfo_t *info,
> unsigned int flags)
> {
> #ifdef __NR_procfd_send_signal
> return syscall(__NR_procfd_send_signal, procfd, sig, info, flags);
> #else
> return -ENOSYS;
> #endif
> }
>
> int main(int argc, char *argv[])
> {
> int fd, ret, saved_errno, sig;
>
> if (argc < 3)
> exit(EXIT_FAILURE);
>
> fd = open(argv[1], O_DIRECTORY | O_CLOEXEC);
> if (fd < 0) {
> printf("%s - Failed to open \"%s\"\n", strerror(errno), argv[1]);
> exit(EXIT_FAILURE);
> }
>
> sig = atoi(argv[2]);
>
> printf("Sending signal %d to process %s\n", sig, argv[1]);
> ret = do_procfd_send_signal(fd, sig, NULL, 0);
>
> saved_errno = errno;
> close(fd);
> errno = saved_errno;
>
> if (ret < 0) {
> printf("%s - Failed to send signal %d to process %s\n",
> strerror(errno), sig, argv[1]);
> exit(EXIT_FAILURE);
> }
>
> exit(EXIT_SUCCESS);
> }
>
> [1]: https://lkml.org/lkml/2018/11/18/130
> [2]: https://lore.kernel.org/lkml/874lbtjvtd.fsf@oldenburg2.str.redhat.com/
> [3]: https://lore.kernel.org/lkml/20181204132604.aspfupwjgjx6fhva@brauner.io/
> [4]: https://lore.kernel.org/lkml/20181203180224.fkvw4kajtbvru2ku@brauner.io/
> [5]: https://lore.kernel.org/lkml/20181121213946.GA10795@mail.hallyn.com/
> [6]: https://lore.kernel.org/lkml/20181120103111.etlqp7zop34v6nv4@brauner.io/
> [7]: https://lore.kernel.org/lkml/36323361-90BD-41AF-AB5B-EE0D7BA02C21@amacapital.net/
> [8]: https://lore.kernel.org/lkml/87tvjxp8pc.fsf@xmission.com/
> [9]: https://asciinema.org/a/X1J8eGhe3vCfBE2b9TXtTaSJ7
> [10]: https://lore.kernel.org/lkml/20181203180224.fkvw4kajtbvru2ku@brauner.io/
> [11]: https://lore.kernel.org/lkml/F53D6D38-3521-4C20-9034-5AF447DF62FF@amacapital.net/
>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Serge Hallyn <serge@hallyn.com>
> Cc: Jann Horn <jannh@google.com>
> Cc: Andy Lutomirsky <luto@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Aleksa Sarai <cyphar@cyphar.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Florian Weimer <fweimer@redhat.com>
> Signed-off-by: Christian Brauner <christian@brauner.io>
> ---
> Changelog:
> v3:
> - add __copy_siginfo_from_user_generic() to avoid adding compat syscalls
> - s/procfd_signal/procfd_send_signal/g
> - change type of flags argument from int to unsigned int
> - add comment about what happens to zombies
> - add proc_is_tid_procfd()
> - return EOPNOTSUPP when /proc/<pid>/task/<tid> fd is passed so userspace
> has a way of knowing that tidfds are not supported currently.
> v2:
> - define __NR_procfd_signal in unistd.h
> - wire up compat syscall
> - s/proc_is_procfd/proc_is_tgid_procfd/g
> - provide stubs when CONFIG_PROC_FS=n
> - move proc_pid() to linux/proc_fs.h header
> - use proc_pid() to grab struct pid from /proc/<pid> fd
> v1:
> - patch introduced
> ---
> arch/x86/entry/syscalls/syscall_32.tbl | 1 +
> arch/x86/entry/syscalls/syscall_64.tbl | 1 +
> fs/proc/base.c | 17 +++-
> fs/proc/internal.h | 5 -
> include/linux/proc_fs.h | 18 ++++
> include/linux/syscalls.h | 3 +
> include/uapi/asm-generic/unistd.h | 4 +-
> kernel/signal.c | 127 +++++++++++++++++++++++--
> 8 files changed, 163 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
> index 3cf7b533b3d1..9ab0477d6dc3 100644
> --- a/arch/x86/entry/syscalls/syscall_32.tbl
> +++ b/arch/x86/entry/syscalls/syscall_32.tbl
> @@ -398,3 +398,4 @@
> 384 i386 arch_prctl sys_arch_prctl __ia32_compat_sys_arch_prctl
> 385 i386 io_pgetevents sys_io_pgetevents __ia32_compat_sys_io_pgetevents
> 386 i386 rseq sys_rseq __ia32_sys_rseq
> +387 i386 procfd_send_signal sys_procfd_send_signal __ia32_sys_procfd_send_signal
> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
> index f0b1709a5ffb..6d22b1130ed0 100644
> --- a/arch/x86/entry/syscalls/syscall_64.tbl
> +++ b/arch/x86/entry/syscalls/syscall_64.tbl
> @@ -343,6 +343,7 @@
> 332 common statx __x64_sys_statx
> 333 common io_pgetevents __x64_sys_io_pgetevents
> 334 common rseq __x64_sys_rseq
> +335 common procfd_send_signal __x64_sys_procfd_send_signal
>
> #
> # x32-specific system call numbers start at 512 to avoid cache impact
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index ce3465479447..906647d51f6d 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -716,7 +716,10 @@ static int proc_pid_permission(struct inode *inode, int mask)
> return generic_permission(inode, mask);
> }
>
> -
> +struct pid *proc_pid(const struct inode *inode)
> +{
> + return PROC_I(inode)->pid;
> +}
>
> static const struct inode_operations proc_def_inode_operations = {
> .setattr = proc_setattr,
> @@ -3038,6 +3041,12 @@ static const struct file_operations proc_tgid_base_operations = {
> .llseek = generic_file_llseek,
> };
>
> +bool proc_is_tgid_procfd(const struct file *file)
> +{
> + return d_is_dir(file->f_path.dentry) &&
> + (file->f_op == &proc_tgid_base_operations);
> +}
> +
> static struct dentry *proc_tgid_base_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags)
> {
> return proc_pident_lookup(dir, dentry,
> @@ -3422,6 +3431,12 @@ static const struct file_operations proc_tid_base_operations = {
> .llseek = generic_file_llseek,
> };
>
> +bool proc_is_tid_procfd(const struct file *file)
> +{
> + return d_is_dir(file->f_path.dentry) &&
> + (file->f_op == &proc_tid_base_operations);
> +}
> +
> static const struct inode_operations proc_tid_base_inode_operations = {
> .lookup = proc_tid_base_lookup,
> .getattr = pid_getattr,
> diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> index 5185d7f6a51e..eb69afba83f3 100644
> --- a/fs/proc/internal.h
> +++ b/fs/proc/internal.h
> @@ -113,11 +113,6 @@ static inline void *__PDE_DATA(const struct inode *inode)
> return PDE(inode)->data;
> }
>
> -static inline struct pid *proc_pid(const struct inode *inode)
> -{
> - return PROC_I(inode)->pid;
> -}
> -
> static inline struct task_struct *get_proc_task(const struct inode *inode)
> {
> return get_pid_task(proc_pid(inode), PIDTYPE_PID);
> diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
> index d0e1f1522a78..ebc0e0ca5256 100644
> --- a/include/linux/proc_fs.h
> +++ b/include/linux/proc_fs.h
> @@ -73,6 +73,9 @@ struct proc_dir_entry *proc_create_net_single_write(const char *name, umode_t mo
> int (*show)(struct seq_file *, void *),
> proc_write_t write,
> void *data);
> +extern bool proc_is_tgid_procfd(const struct file *file);
> +extern bool proc_is_tid_procfd(const struct file *file);
> +extern struct pid *proc_pid(const struct inode *inode);
>
> #else /* CONFIG_PROC_FS */
>
> @@ -114,6 +117,21 @@ static inline int remove_proc_subtree(const char *name, struct proc_dir_entry *p
> #define proc_create_net(name, mode, parent, state_size, ops) ({NULL;})
> #define proc_create_net_single(name, mode, parent, show, data) ({NULL;})
>
> +static inline bool proc_is_tgid_procfd(const struct file *file)
> +{
> + return false;
> +}
> +
> +static inline bool proc_is_tid_procfd(const struct file *file)
> +{
> + return false;
> +}
> +
> +static inline struct pid *proc_pid(const struct inode *inode)
> +{
> + return NULL;
> +}
> +
> #endif /* CONFIG_PROC_FS */
>
> struct net;
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index 2ac3d13a915b..b3a02c6780d0 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -907,6 +907,9 @@ asmlinkage long sys_statx(int dfd, const char __user *path, unsigned flags,
> unsigned mask, struct statx __user *buffer);
> asmlinkage long sys_rseq(struct rseq __user *rseq, uint32_t rseq_len,
> int flags, uint32_t sig);
> +asmlinkage long sys_procfd_send_signal(int procfd, int sig,
> + siginfo_t __user *info,
> + unsigned int flags);
>
> /*
> * Architecture-specific system calls
> diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
> index 538546edbfbd..b409ee26f8e9 100644
> --- a/include/uapi/asm-generic/unistd.h
> +++ b/include/uapi/asm-generic/unistd.h
> @@ -738,9 +738,11 @@ __SYSCALL(__NR_statx, sys_statx)
> __SC_COMP(__NR_io_pgetevents, sys_io_pgetevents, compat_sys_io_pgetevents)
> #define __NR_rseq 293
> __SYSCALL(__NR_rseq, sys_rseq)
> +#define __NR_procfd_send_signal 294
> +__SYSCALL(__NR_procfd_send_signal, sys_procfd_send_signal)
>
> #undef __NR_syscalls
> -#define __NR_syscalls 294
> +#define __NR_syscalls 295
>
> /*
> * 32 bit systems traditionally used different
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 9a32bc2088c9..79a7e0396b0f 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -19,7 +19,9 @@
> #include <linux/sched/task.h>
> #include <linux/sched/task_stack.h>
> #include <linux/sched/cputime.h>
> +#include <linux/file.h>
> #include <linux/fs.h>
> +#include <linux/proc_fs.h>
> #include <linux/tty.h>
> #include <linux/binfmts.h>
> #include <linux/coredump.h>
> @@ -3286,6 +3288,16 @@ COMPAT_SYSCALL_DEFINE4(rt_sigtimedwait, compat_sigset_t __user *, uthese,
> }
> #endif
>
> +static inline void prepare_kill_siginfo(int sig, struct kernel_siginfo *info)
> +{
> + clear_siginfo(info);
> + info->si_signo = sig;
> + info->si_errno = 0;
> + info->si_code = SI_USER;
> + info->si_pid = task_tgid_vnr(current);
> + info->si_uid = from_kuid_munged(current_user_ns(), current_uid());
> +}
> +
> /**
> * sys_kill - send a signal to a process
> * @pid: the PID of the process
> @@ -3295,16 +3307,119 @@ SYSCALL_DEFINE2(kill, pid_t, pid, int, sig)
> {
> struct kernel_siginfo info;
>
> - clear_siginfo(&info);
> - info.si_signo = sig;
> - info.si_errno = 0;
> - info.si_code = SI_USER;
> - info.si_pid = task_tgid_vnr(current);
> - info.si_uid = from_kuid_munged(current_user_ns(), current_uid());
> + prepare_kill_siginfo(sig, &info);
>
> return kill_something_info(sig, &info, pid);
> }
>
> +/*
> + * Verify that the signaler and signalee either are in the same pid namespace
> + * or that the signaler's pid namespace is an ancestor of the signalee's pid
> + * namespace.
> + */
> +static bool may_signal_procfd(struct pid *pid)
> +{
> + struct pid_namespace *active = task_active_pid_ns(current);
> + struct pid_namespace *p = ns_of_pid(pid);
> +
> + for (;;) {
> + if (!p)
> + return false;
> + if (p == active)
> + break;
> + p = p->parent;
> + }
> +
> + return true;
> +}
> +
> +static int __copy_siginfo_from_user_generic(int signo, kernel_siginfo_t *kinfo,
> + siginfo_t *info)
Ugh. _generic is a poor fit here. In the kernel _generic tends to be
for the default helper function for a set of operations.
A suffix like _any or anything that implemies you can be a compat or not
a compat syscall would be better.
> +{
> +#ifdef CONFIG_COMPAT
> + /*
> + * Avoid hooking up compat syscalls and instead handle necessary
> + * conversions here.
> + */
> + if (in_compat_syscall())
> + return __copy_siginfo_from_user32(
> + signo, kinfo, (struct compat_siginfo __user *)info);
> +#endif
> + return __copy_siginfo_from_user(signo, kinfo, info);
> +}
Let me be very clear. __copy_siginfo_from_user exists because of a bug
in the original implementation of rt_sigqueueinfo. It should not gain
any new callers. It only exists because there is userspace that is
confirmed to care.
If you must take a separate signal parameter please verify
"signo == si_signo" and fail if they do not match.
> +/**
> + * sys_procfd_send_signal - send a signal to a process through a process file
> + * descriptor
> + * @procfd: the file descriptor of the process
There is a subtle naming and semantic issue here that needs to be
addressed.
- Are you only going to support processes?
If so procfd is fine, otherwise another name needs to be chosen.
- Do you want to embed what will get signaled into the file descriptor?
- Are you about pids? In which case a pid type needs to be added to the
parameters so you can send to sessions, pgrps, processes, and threads.
This is important to get right so that people can know how to use this
system call.
> + * @sig: signal to be sent
> + * @info: the signal info
> + * @flags: future flags to be passed
> + *
> + * Return: 0 on success, negative errno on failure
> + */
> +SYSCALL_DEFINE4(procfd_send_signal, int, procfd, int, sig,
> + siginfo_t __user *, info, unsigned int, flags)
> +{
> + int ret;
> + struct fd f;
> + struct pid *pid;
> + kernel_siginfo_t kinfo;
> +
> + /* Enforce flags be set to 0 until we add an extension. */
> + if (flags)
> + return -EINVAL;
> +
> + f = fdget_raw(procfd);
> + if (!f.file)
> + return -EBADF;
> +
> + /*
> + * Give userspace a way to detect whether /proc/<pid>/task/<tid> fds
> + * are supported.
> + */
> + ret = -EOPNOTSUPP;
> + if (proc_is_tid_procfd(f.file))
> + goto err;
-EBADF is the proper error code.
> + /* Is this a procfd? */
> + ret = -EINVAL;
> + if (!proc_is_tgid_procfd(f.file))
> + goto err;
-EBADF is the proper error code.
> + /* Without CONFIG_PROC_FS proc_pid() returns NULL. */
> + pid = proc_pid(file_inode(f.file));
> + if (!pid)
> + goto err;
Perhaps you want to fold the proc_pid into the proc_is_tgid_procfd call.
> + if (!may_signal_procfd(pid))
> + goto err;
> +
> + if (info) {
> + ret = __copy_siginfo_from_user_generic(sig, &kinfo, info);
> + if (unlikely(ret))
> + goto err;
> +
> + /*
> + * Not even root can pretend to send signals from the kernel.
> + * Nor can they impersonate a kill()/tgkill(), which adds
> + * source info.
> + */
> + ret = -EPERM;
> + if ((kinfo.si_code >= 0 || kinfo.si_code == SI_TKILL) &&
> + (task_pid(current) != pid))
> + goto err;
> + } else {
> + prepare_kill_siginfo(sig, &kinfo);
> + }
I think I would just make the section above say:
ret = copy_siginfo_from_user_any(
> +
> + ret = kill_pid_info(sig, &kinfo, pid);
> +
> +err:
> + fdput(f);
> + return ret;
> +}
> +
> static int
> do_send_specific(pid_t tgid, pid_t pid, int sig, struct kernel_siginfo *info)
> {
^ permalink raw reply
* Re: [PATCH v3] signal: add procfd_send_signal() syscall
From: kbuild test robot @ 2018-12-05 17:29 UTC (permalink / raw)
Cc: kbuild-all, linux-kernel, linux-api, luto, arnd, ebiederm, serge,
jannh, akpm, oleg, cyphar, viro, linux-fsdevel, dancol, timmurray,
linux-man, keescook, fweimer, tglx, x86, Christian Brauner
In-Reply-To: <20181205092203.14105-1-christian@brauner.io>
[-- Attachment #1: Type: text/plain, Size: 1524 bytes --]
Hi Christian,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v4.20-rc5]
[cannot apply to next-20181205]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Christian-Brauner/signal-add-procfd_send_signal-syscall/20181205-174512
config: sh-titan_defconfig (attached as .config)
compiler: sh4-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=sh
All warnings (new ones prefixed by >>):
<stdin>:1317:2: warning: #warning syscall pkey_mprotect not implemented [-Wcpp]
<stdin>:1320:2: warning: #warning syscall pkey_alloc not implemented [-Wcpp]
<stdin>:1323:2: warning: #warning syscall pkey_free not implemented [-Wcpp]
<stdin>:1326:2: warning: #warning syscall statx not implemented [-Wcpp]
<stdin>:1332:2: warning: #warning syscall io_pgetevents not implemented [-Wcpp]
<stdin>:1335:2: warning: #warning syscall rseq not implemented [-Wcpp]
>> <stdin>:1338:2: warning: #warning syscall procfd_send_signal not implemented [-Wcpp]
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 16439 bytes --]
^ permalink raw reply
* Re: [RFC PATCH v4 1/5] glibc: Perform rseq(2) registration at nptl init and thread creation
From: Mathieu Desnoyers @ 2018-12-05 17:24 UTC (permalink / raw)
To: Rich Felker
Cc: Florian Weimer, carlos, Joseph Myers, Szabolcs Nagy, libc-alpha,
Thomas Gleixner, Ben Maurer, Peter Zijlstra, Paul E. McKenney,
Boqun Feng, Will Deacon, Dave Watson, Paul Turner, linux-kernel,
linux-api
In-Reply-To: <20181126170758.GP23599@brightrain.aerifal.cx>
----- On Nov 26, 2018, at 12:07 PM, Rich Felker dalias@libc.org wrote:
> On Mon, Nov 26, 2018 at 11:30:51AM -0500, Mathieu Desnoyers wrote:
>> ----- On Nov 26, 2018, at 10:51 AM, Mathieu Desnoyers
>> mathieu.desnoyers@efficios.com wrote:
>>
>> > ----- On Nov 26, 2018, at 3:28 AM, Florian Weimer fweimer@redhat.com wrote:
>> >
>> >> * Mathieu Desnoyers:
>> >>
>> >>> Using a "weak" symbol in early adopter libraries is important, so they
>> >>> can be loaded together into the same process without causing loader
>> >>> errors due to many definitions of the same strong symbol.
>> >>
>> >> This is not how ELF dynamic linking works. If the symbol name is the
>> >> same, one definition interposes the others.
>> >>
>> >> You need to ensure that the symbol has the same size everywhere, though.
>> >> There are some tricky interactions with symbol versions, too. (The
>> >> interposing libraries must not use symbol versioning.)
>> >
>> > I was under the impression that loading the same strong symbol into an
>> > application multiple times would cause some kind of warning if non-weak. I did
>> > some testing to figure out which case I remembered would cause this.
>> >
>> > When compiling with "-fno-common", dynamic and static linking work fine, but
>> > trying to add multiple instances of a given symbol into a single object fails
>> > with:
>> >
>> > /tmp/ccSakXZV.o:(.bss+0x0): multiple definition of `a'
>> > /tmp/ccQBJBOo.o:(.bss+0x0): first defined here
>> >
>> > Even if the symbol has the same size.
>> >
>> > So considering that we don't care about compiling into a single object here,
>> > and only care about static and dynamic linking of libraries, indeed the "weak"
>> > symbol is not useful.
>> >
>> > So let's make __rseq_abi and __rseq_refcount strong symbols then ?
>>
>> Actually, looking into ld(1) --warn-common, it looks like "weak" would be
>> cleaner
>> after all, especially for __rseq_abi which we needs to be initialized to a
>> specific
>> value, which is therefore not a common symbol.
>>
>> " --warn-common
>> Warn when a common symbol is combined with another common symbol or with a
>> symbol definition. Unix
>> linkers allow this somewhat sloppy practice, but linkers on some other operating
>> systems do not.
>> This option allows you to find potential problems from combining global symbols.
>> Unfortunately,
>> some C libraries use this practice, so you may get some warnings about symbols
>> in the libraries as
>> well as in your programs."
>>
>> Thoughts ?
>
> AFAIK this has nothing to do with dynamic linking.
Reading through the ELF specification, it seems to imply that "weak" only affects the link
editor behavior when combining relocatable objects, not the behavior of the dynamic linker.
Is that what you refer to when you say "weak" has nothing to do with dynamic linking ?
If that interpretation is correct, then indeed I should remove the "weak" from the __rseq_abi
and __rseq_refcount.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ 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