On Fri, Mar 29, 2019 at 4:54 PM Christian Brauner <christian@xxxxxxxxxx> wrote: > With the addition of pidfd_open() it is possible for users to reference a > specific thread by doing: > > int pidfd = pidfd_open(<tid>, 0); > > This means we can extend pidfd_send_signal() to signal a specific thread. > As promised in the commit for pidfd_send_signal() [1] the extension is > based on a flag argument, i.e. the scope of the signal delivery is based on > the flag argument, not on the type of file descriptor. > To this end the flag PIDFD_SIGNAL_TID is added. With this change we now > cover most of the functionality of all the other signal sending functions > combined: [...] > diff --git a/include/uapi/linux/wait.h b/include/uapi/linux/wait.h > index d6c7c0701997..b72f0ef84fe5 100644 > --- a/include/uapi/linux/wait.h > +++ b/include/uapi/linux/wait.h [...] > +/* Flags to pass to pidfd_send_signal */ > +#define PIDFD_SIGNAL_TID 1 /* Send signal to specific thread */ nit: s/1/1U/; the flags argument is an `unsigned int` > #endif /* _UAPI_LINUX_WAIT_H */ > diff --git a/kernel/signal.c b/kernel/signal.c > index eb97d0cc6ef7..9f93da85b2b9 100644 > --- a/kernel/signal.c > +++ b/kernel/signal.c [...] > +static int pidfd_send_signal_specific(struct pid *pid, int sig, > + struct kernel_siginfo *info) > +{ > + struct task_struct *p; > + int error = -ESRCH; > + > + rcu_read_lock(); > + p = pid_task(pid, PIDTYPE_PID); > + if (p) > + error = __do_send_specific(p, sig, info); > + rcu_read_unlock(); > + > + return error; > +} > + > /** > - * sys_pidfd_send_signal - send a signal to a process through a task file > - * descriptor > + * sys_pidfd_send_signal - send a signal to a process through a pidfd > + > * @pidfd: the file descriptor of the process > * @sig: signal to be sent > * @info: the signal info > * @flags: future flags to be passed nit: comment is outdated, it isn't "future flags" anymore [...] > + * rt_tgsigqueueinfo(<tgid>, <tid>, <sig>, <uinfo>) > + * - pidfd_send_signal(<pidfd>, <sig>, <info>, PIDFD_SIGNAL_TID); > + * which is equivalent to > + * rt_tgsigqueueinfo(<tgid>, <tid>, <sig>, <uinfo>) > + * > * In order to extend the syscall to threads and process groups the @flags > * argument should be used. In essence, the @flags argument will determine > * what is signaled and not the file descriptor itself. Put in other words, nit: again, outdated comment about @flags [...] > @@ -3626,43 +3695,16 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig, > prepare_kill_siginfo(sig, &kinfo); > } > > - ret = kill_pid_info(sig, &kinfo, pid); > + if (flags & PIDFD_SIGNAL_TID) > + ret = pidfd_send_signal_specific(pid, sig, &kinfo); > + else > + ret = kill_pid_info(sig, &kinfo, pid); nit: maybe give pidfd_send_signal_specific() and kill_pid_info() the same signatures, since they perform similar operations with the same argument types? Something that was already kinda weird in the existing code, but is getting worse with TIDs is the handling of SI_USER with siginfo. Copying context lines from above here: 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); } So for signals to PIDs, the rule is that if you send siginfo with SI_USER to yourself, the siginfo is preserved; otherwise the kernel silently clobbers it. That's already kind of weird - silent behavior difference depending on a security check. But now, for signals to threads, I think the result is going to be that signalling the thread group leader preserves information, and signalling any other thread clobbers it? If so, that seems bad. do_rt_sigqueueinfo() seems to have the same issue, from a glance - but there, at least the error case is just a -EPERM, not a silent behavior difference. Would it make sense to refuse sending siginfo with SI_USER to non-current? If you actually want to send a normal SI_USER signal, you can use info==NULL, right? That should create wrongness parity with do_rt_sigqueueinfo(). To improve things further, I guess you'd have to move the comparison against current into pidfd_send_signal_specific(), or move the task lookup out of it, or something like that? > err: > fdput(f); > return ret; > } [...]