pidfd: change pidfd_send_signal() to respect PIDFD_THREAD

  Turn kill_pid_info() into kill_pid_info_type(), this allows to pass any
pid_type to group_send_sig_info(), despite its name it should work fine
even if type = PIDTYPE_PID.

Change pidfd_send_signal() to use PIDTYPE_PID or PIDTYPE_TGID depending
on PIDFD_THREAD.

While at it kill another TODO comment in pidfd_show_fdinfo(). As Christian
expains fdinfo reports f_flags, userspace can already detect PIDFD_THREAD.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/fork.c   |  2 --
 kernel/signal.c | 18 ++++++++++++------
 2 files changed, 12 insertions(+), 8 deletions(-)
  

On Wed, Feb 07, 2024 at 12:45:49PM +0100, Oleg Nesterov wrote:
> Turn kill_pid_info() into kill_pid_info_type(), this allows to pass any
> pid_type to group_send_sig_info(), despite its name it should work fine
> even if type = PIDTYPE_PID.
> 
> Change pidfd_send_signal() to use PIDTYPE_PID or PIDTYPE_TGID depending
> on PIDFD_THREAD.
> 
> While at it kill another TODO comment in pidfd_show_fdinfo(). As Christian
> expains fdinfo reports f_flags, userspace can already detect PIDFD_THREAD.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/fork.c   |  2 --
>  kernel/signal.c | 18 ++++++++++++------
>  2 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/fork.c b/kernel/fork.c
> index cd61ca87d0e6..47b565598063 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -2051,8 +2051,6 @@ static void pidfd_show_fdinfo(struct seq_file *m, struct file *f)
>  
>  	seq_put_decimal_ll(m, "Pid:\t", nr);
>  
> -	/* TODO: report PIDFD_THREAD */
> -
>  #ifdef CONFIG_PID_NS
>  	seq_put_decimal_ll(m, "\nNSpid:\t", nr);
>  	if (nr > 0) {
> diff --git a/kernel/signal.c b/kernel/signal.c
> index c3fac06937e2..e3edcd784e45 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -47,6 +47,7 @@
>  #include <linux/cgroup.h>
>  #include <linux/audit.h>
>  #include <linux/sysctl.h>
> +#include <uapi/linux/pidfd.h>
>  
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/signal.h>
> @@ -1478,7 +1479,8 @@ int __kill_pgrp_info(int sig, struct kernel_siginfo *info, struct pid *pgrp)
>  	return ret;
>  }
>  
> -int kill_pid_info(int sig, struct kernel_siginfo *info, struct pid *pid)
> +static int kill_pid_info_type(int sig, struct kernel_siginfo *info,
> +				struct pid *pid, enum pid_type type)
>  {
>  	int error = -ESRCH;
>  	struct task_struct *p;
> @@ -1487,11 +1489,10 @@ int kill_pid_info(int sig, struct kernel_siginfo *info, struct pid *pid)
>  		rcu_read_lock();
>  		p = pid_task(pid, PIDTYPE_PID);
>  		if (p)
> -			error = group_send_sig_info(sig, info, p, PIDTYPE_TGID);
> +			error = group_send_sig_info(sig, info, p, type);
>  		rcu_read_unlock();
>  		if (likely(!p || error != -ESRCH))
>  			return error;
> -
>  		/*
>  		 * The task was unhashed in between, try again.  If it
>  		 * is dead, pid_task() will return NULL, if we race with
> @@ -1500,6 +1501,11 @@ int kill_pid_info(int sig, struct kernel_siginfo *info, struct pid *pid)
>  	}
>  }
>  
> +int kill_pid_info(int sig, struct kernel_siginfo *info, struct pid *pid)
> +{
> +	return kill_pid_info_type(sig, info, pid, PIDTYPE_TGID);
> +}
> +
>  static int kill_proc_info(int sig, struct kernel_siginfo *info, pid_t pid)
>  {
>  	int error;
> @@ -3890,6 +3896,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
>  	struct fd f;
>  	struct pid *pid;
>  	kernel_siginfo_t kinfo;
> +	enum pid_type type;
>  
>  	/* Enforce flags be set to 0 until we add an extension. */
>  	if (flags)
> @@ -3928,9 +3935,8 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
>  		prepare_kill_siginfo(sig, &kinfo);
>  	}
>  
> -	/* TODO: respect PIDFD_THREAD */
> -	ret = kill_pid_info(sig, &kinfo, pid);
> -
> +	type = (f.file->f_flags & PIDFD_THREAD) ? PIDTYPE_PID : PIDTYPE_TGID;
> +	ret = kill_pid_info_type(sig, &kinfo, pid, type);

If the user doesn't provide siginfo then the kernel fills in the info in
prepare_kill_siginfo() a few lines above. That sets info->si_code to
SI_USER even for the PIDFD_THREAD case. Whenever the info is filled in
by the kernel it's not exactly userspace impersonating anything plus we
know that what we're sending to is a pidfd by the type of the pidfd. So
it feels like we should fill in SI_TKILL here as well?

I would also suggest we update the obsolete comment on top of
pidfd_send_signal() along the lines of:

diff --git a/kernel/signal.c b/kernel/signal.c
index e3edcd784e45..40df0c17abd7 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3878,14 +3878,10 @@ static struct pid *pidfd_to_pid(const struct file *file)
  * @info:   signal info
  * @flags:  future flags
  *
- * The syscall currently only signals via PIDTYPE_PID which covers
- * kill(<positive-pid>, <signal>. It does not signal threads or process
- * groups.
- * 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,
- * grouping is a property of the flags argument not a property of the file
- * descriptor.
+ * If the @pidfd refers to a thread-group leader the signal is thread-group
+ * directed. If @pidfd referes to a thread then the signal is thread directed.
+ * In the future extension to @flags may be used to override the default scope
+ * of @pidfd.
  *
  * Return: 0 on success, negative errno on failure
  */
  

On 02/08, Christian Brauner wrote:
>
> On Wed, Feb 07, 2024 at 12:45:49PM +0100, Oleg Nesterov wrote:
> > +	type = (f.file->f_flags & PIDFD_THREAD) ? PIDTYPE_PID : PIDTYPE_TGID;
> > +	ret = kill_pid_info_type(sig, &kinfo, pid, type);
>
> If the user doesn't provide siginfo then the kernel fills in the info in
> prepare_kill_siginfo() a few lines above. That sets info->si_code to
> SI_USER even for the PIDFD_THREAD case. Whenever the info is filled in
> by the kernel it's not exactly userspace impersonating anything plus we
> know that what we're sending to is a pidfd by the type of the pidfd. So
> it feels like we should fill in SI_TKILL here as well?

Hmm. Agreed, will do, thanks.

But then I think this needs another preparational 1/2 patch.
prepare_kill_siginfo() should have a new arg so that do_tkill() could
use it too.

(offtopic, but may be the "Only allow sending arbitrary signals to yourself"
 check in pidfd_send_signal() needs another helper, do_rt_sigqueueinfo()
 does the same check).

> I would also suggest we update the obsolete comment on top of
> pidfd_send_signal() along the lines of:

Ah, indeed, thanks.

Oleg.
  

On 02/08, Christian Brauner wrote:
>
> I would also suggest we update the obsolete comment on top of
> pidfd_send_signal() along the lines of:

Yes, but...

> + * If the @pidfd refers to a thread-group leader the signal is thread-group
> + * directed. If @pidfd referes to a thread then the signal is thread directed.

No, this depends on PIDFD_THREAD only.

If it is set then the signal is always "thread directed" even if @pidfd refers
to a thread-group leader.

Otherwise the target task must be a group leader and the signal will be
"thread-group directed".

Right?

Oleg.
  

On Thu, Feb 08, 2024 at 02:53:45PM +0100, Oleg Nesterov wrote:
> On 02/08, Christian Brauner wrote:
> >
> > On Wed, Feb 07, 2024 at 12:45:49PM +0100, Oleg Nesterov wrote:
> > > +	type = (f.file->f_flags & PIDFD_THREAD) ? PIDTYPE_PID : PIDTYPE_TGID;
> > > +	ret = kill_pid_info_type(sig, &kinfo, pid, type);
> >
> > If the user doesn't provide siginfo then the kernel fills in the info in
> > prepare_kill_siginfo() a few lines above. That sets info->si_code to
> > SI_USER even for the PIDFD_THREAD case. Whenever the info is filled in
> > by the kernel it's not exactly userspace impersonating anything plus we
> > know that what we're sending to is a pidfd by the type of the pidfd. So
> > it feels like we should fill in SI_TKILL here as well?
> 
> Hmm. Agreed, will do, thanks.
> 
> But then I think this needs another preparational 1/2 patch.
> prepare_kill_siginfo() should have a new arg so that do_tkill() could
> use it too.

Agreed.

> 
> (offtopic, but may be the "Only allow sending arbitrary signals to yourself"
>  check in pidfd_send_signal() needs another helper, do_rt_sigqueueinfo()
>  does the same check).

Agreed.
  

On Thu, Feb 08, 2024 at 03:06:10PM +0100, Oleg Nesterov wrote:
> On 02/08, Christian Brauner wrote:
> >
> > I would also suggest we update the obsolete comment on top of
> > pidfd_send_signal() along the lines of:
> 
> Yes, but...
> 
> > + * If the @pidfd refers to a thread-group leader the signal is thread-group
> > + * directed. If @pidfd referes to a thread then the signal is thread directed.
> 
> No, this depends on PIDFD_THREAD only.

Sorry, yes, that is what I meant to say but obviously wrote unclearly. I
mean that if pidfd->f_flags & PIDFD_THREAD then it's thread-directed.
That's what I meant by type of pidfd. Not type as in PIDTYPE_PID in
struct pid.

> 
> If it is set then the signal is always "thread directed" even if @pidfd refers
> to a thread-group leader.
> 
> Otherwise the target task must be a group leader and the signal will be
> "thread-group directed".
> 
> Right?

Yes, please feel free to update the comment!
  

On 02/08, Oleg Nesterov wrote:
>
> On 02/08, Christian Brauner wrote:
> >
> > On Wed, Feb 07, 2024 at 12:45:49PM +0100, Oleg Nesterov wrote:
> > > +	type = (f.file->f_flags & PIDFD_THREAD) ? PIDTYPE_PID : PIDTYPE_TGID;
> > > +	ret = kill_pid_info_type(sig, &kinfo, pid, type);
> >
> > If the user doesn't provide siginfo then the kernel fills in the info in
> > prepare_kill_siginfo() a few lines above. That sets info->si_code to
> > SI_USER even for the PIDFD_THREAD case. Whenever the info is filled in
> > by the kernel it's not exactly userspace impersonating anything plus we
> > know that what we're sending to is a pidfd by the type of the pidfd. So
> > it feels like we should fill in SI_TKILL here as well?
>
> Hmm. Agreed, will do, thanks.

Cough... lets forget this patch for the moment.

Is prepare_kill_siginfo() correct when we send a signal to the child
pid namespace? si_pid = task_tgid_vnr(current) doesn't look right in
this case but perhaps I am totally confused.

And why do we need it at all? Can't sys_kill() and pidfd_send_signal()
just use SEND_SIG_NOINFO?

OK, I am sure I missed something. Will read this code tomorrow.

Oleg.
  

> Is prepare_kill_siginfo() correct when we send a signal to the child
> pid namespace? si_pid = task_tgid_vnr(current) doesn't look right in
> this case but perhaps I am totally confused.
> 
> And why do we need it at all? Can't sys_kill() and pidfd_send_signal()
> just use SEND_SIG_NOINFO?

Yeah, good point. I don't remember as it's been quite a while ago. My
guess is that it just tried to mirror kill() itself without being aware
of SEND_SIG_NOINFO. If you don't find anything wrong with this then
switch it to SEND_SIG_NOINFO in a preparatory patch we can backport,
please.
  

Oleg Nesterov <oleg@redhat.com> writes:

> Turn kill_pid_info() into kill_pid_info_type(), this allows to pass any
> pid_type to group_send_sig_info(), despite its name it should work fine
> even if type = PIDTYPE_PID.
>
> Change pidfd_send_signal() to use PIDTYPE_PID or PIDTYPE_TGID depending
> on PIDFD_THREAD.
>
> While at it kill another TODO comment in pidfd_show_fdinfo(). As Christian
> expains fdinfo reports f_flags, userspace can already detect PIDFD_THREAD.
>

I have a question here.

Why is this based on group_send_sig_info instead of send_sig_info?

AKA why does this look like sys_kill instead of sys_tgkill?

In particular I am asking are the intended semantics that the signal is
sent to a single thread in a thread group and placed in the per thread
queue, or is the signal sent to the entire thread group and placed
in the thread group signal queue?

Does the type parameter handle that decision for us now?  If so
perhaps we should cleanup the helpers so that this easier to
see when reading the code.

Because honestly right now using group_send_sig_info when
the intended target of the signal is not the entire thread
group is very confusing when reading your change.

Eric

> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/fork.c   |  2 --
>  kernel/signal.c | 18 ++++++++++++------
>  2 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index cd61ca87d0e6..47b565598063 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -2051,8 +2051,6 @@ static void pidfd_show_fdinfo(struct seq_file *m, struct file *f)
>  
>  	seq_put_decimal_ll(m, "Pid:\t", nr);
>  
> -	/* TODO: report PIDFD_THREAD */
> -
>  #ifdef CONFIG_PID_NS
>  	seq_put_decimal_ll(m, "\nNSpid:\t", nr);
>  	if (nr > 0) {
> diff --git a/kernel/signal.c b/kernel/signal.c
> index c3fac06937e2..e3edcd784e45 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -47,6 +47,7 @@
>  #include <linux/cgroup.h>
>  #include <linux/audit.h>
>  #include <linux/sysctl.h>
> +#include <uapi/linux/pidfd.h>
>  
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/signal.h>
> @@ -1478,7 +1479,8 @@ int __kill_pgrp_info(int sig, struct kernel_siginfo *info, struct pid *pgrp)
>  	return ret;
>  }
>  
> -int kill_pid_info(int sig, struct kernel_siginfo *info, struct pid *pid)
> +static int kill_pid_info_type(int sig, struct kernel_siginfo *info,
> +				struct pid *pid, enum pid_type type)
>  {
>  	int error = -ESRCH;
>  	struct task_struct *p;
> @@ -1487,11 +1489,10 @@ int kill_pid_info(int sig, struct kernel_siginfo *info, struct pid *pid)
>  		rcu_read_lock();
>  		p = pid_task(pid, PIDTYPE_PID);
>  		if (p)
> -			error = group_send_sig_info(sig, info, p, PIDTYPE_TGID);
> +			error = group_send_sig_info(sig, info, p, type);
>  		rcu_read_unlock();
>  		if (likely(!p || error != -ESRCH))
>  			return error;
> -
>  		/*
>  		 * The task was unhashed in between, try again.  If it
>  		 * is dead, pid_task() will return NULL, if we race with
> @@ -1500,6 +1501,11 @@ int kill_pid_info(int sig, struct kernel_siginfo *info, struct pid *pid)
>  	}
>  }
>  
> +int kill_pid_info(int sig, struct kernel_siginfo *info, struct pid *pid)
> +{
> +	return kill_pid_info_type(sig, info, pid, PIDTYPE_TGID);
> +}
> +
>  static int kill_proc_info(int sig, struct kernel_siginfo *info, pid_t pid)
>  {
>  	int error;
> @@ -3890,6 +3896,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
>  	struct fd f;
>  	struct pid *pid;
>  	kernel_siginfo_t kinfo;
> +	enum pid_type type;
>  
>  	/* Enforce flags be set to 0 until we add an extension. */
>  	if (flags)
> @@ -3928,9 +3935,8 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
>  		prepare_kill_siginfo(sig, &kinfo);
>  	}
>  
> -	/* TODO: respect PIDFD_THREAD */
> -	ret = kill_pid_info(sig, &kinfo, pid);
> -
> +	type = (f.file->f_flags & PIDFD_THREAD) ? PIDTYPE_PID : PIDTYPE_TGID;
> +	ret = kill_pid_info_type(sig, &kinfo, pid, type);
>  err:
>  	fdput(f);
>  	return ret;
  

On 02/08, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@redhat.com> writes:
>
> > Turn kill_pid_info() into kill_pid_info_type(), this allows to pass any
> > pid_type to group_send_sig_info(), despite its name it should work fine
> > even if type = PIDTYPE_PID.
> >
> > Change pidfd_send_signal() to use PIDTYPE_PID or PIDTYPE_TGID depending
> > on PIDFD_THREAD.
> >
> > While at it kill another TODO comment in pidfd_show_fdinfo(). As Christian
> > expains fdinfo reports f_flags, userspace can already detect PIDFD_THREAD.
> >
>
> I have a question here.
>
> Why is this based on group_send_sig_info instead of send_sig_info?

Well. send_sig_info() accepts "struct task_struct *", not "struct pid *",
it doesn't do check_kill_permission(), and it doesn't handle the possible
race with mt-exec.

> In particular I am asking are the intended semantics that the signal is
> sent to a single thread in a thread group and placed in the per thread
> queue, or is the signal sent to the entire thread group and placed
> in the thread group signal queue?

This depends on PIDFD_THREAD. If it is set then the signal goes to
the per thread queue.

> Because honestly right now using group_send_sig_info when
> the intended target of the signal is not the entire thread
> group is very confusing when reading your change.

Agreed, so perhaps it makes sense to rename it later. See

	despite its name it should work fine even if type = PIDTYPE_PID.

in the changelog above.

Oleg.
  

On 02/08, Christian Brauner wrote:
>
> > Is prepare_kill_siginfo() correct when we send a signal to the child
> > pid namespace? si_pid = task_tgid_vnr(current) doesn't look right in
> > this case but perhaps I am totally confused.
> >
> > And why do we need it at all? Can't sys_kill() and pidfd_send_signal()
> > just use SEND_SIG_NOINFO?
>
> Yeah, good point. I don't remember as it's been quite a while ago. My
> guess is that it just tried to mirror kill() itself without being aware
> of SEND_SIG_NOINFO. If you don't find anything wrong with this then
> switch it to SEND_SIG_NOINFO in a preparatory patch we can backport,
> please.

Yes, but I still feel I must have missed something. Will read this code
tomorrow.

And another note for the record before I forget this. We can probably
improve and rename access_pidfd_pidns(). Currently it is only used by
pidfd_send_signal() but pidns_install() looks like another user.

Oleg.
  

On Thu, Feb 08, 2024 at 04:57:31PM +0100, Oleg Nesterov wrote:
> On 02/08, Eric W. Biederman wrote:
> >
> > Oleg Nesterov <oleg@redhat.com> writes:
> >
> > > Turn kill_pid_info() into kill_pid_info_type(), this allows to pass any
> > > pid_type to group_send_sig_info(), despite its name it should work fine
> > > even if type = PIDTYPE_PID.
> > >
> > > Change pidfd_send_signal() to use PIDTYPE_PID or PIDTYPE_TGID depending
> > > on PIDFD_THREAD.
> > >
> > > While at it kill another TODO comment in pidfd_show_fdinfo(). As Christian
> > > expains fdinfo reports f_flags, userspace can already detect PIDFD_THREAD.
> > >
> >
> > I have a question here.
> >
> > Why is this based on group_send_sig_info instead of send_sig_info?
> 
> Well. send_sig_info() accepts "struct task_struct *", not "struct pid *",
> it doesn't do check_kill_permission(), and it doesn't handle the possible
> race with mt-exec.
> 
> > In particular I am asking are the intended semantics that the signal is
> > sent to a single thread in a thread group and placed in the per thread
> > queue, or is the signal sent to the entire thread group and placed
> > in the thread group signal queue?
> 
> This depends on PIDFD_THREAD. If it is set then the signal goes to
> the per thread queue.
> 
> > Because honestly right now using group_send_sig_info when
> > the intended target of the signal is not the entire thread
> > group is very confusing when reading your change.
> 
> Agreed, so perhaps it makes sense to rename it later. See

Agreed. The function seems misnamed and incorrectly documented. It just
seems that it's never been used with PIDTYPE_PID but it's perfectly
capable of doing that. So maybe just put a patch on top renaming it to
send_sig_info_type() and remove the old comment. But I can live without
renaming it for now as well.
  

On 02/08, Oleg Nesterov wrote:
>
> Is prepare_kill_siginfo() correct when we send a signal to the child
> pid namespace? si_pid = task_tgid_vnr(current) doesn't look right

Yes, but iiuc send_signal_locked() should fixup si_pid/si_uid, so it
is not buggy.

> And why do we need it at all? Can't sys_kill() and pidfd_send_signal()
> just use SEND_SIG_NOINFO?

Probably yes. And even do_tkill() can use SEND_SIG_NOINFO if we change
__send_signal_locked() to check the type before ".si_code = SI_USER".
but then TP_STORE_SIGINFO() needs some changes...

I'll try to do this later, I do not want to mix this change with the
PIDFD_THREAD changes.

Oleg.
  

On 02/09, Christian Brauner wrote:
>
> On Thu, Feb 08, 2024 at 04:57:31PM +0100, Oleg Nesterov wrote:
> > On 02/08, Eric W. Biederman wrote:
> > >
> > > Because honestly right now using group_send_sig_info when
> > > the intended target of the signal is not the entire thread
> > > group is very confusing when reading your change.
> >
> > Agreed, so perhaps it makes sense to rename it later. See
>
> Agreed. The function seems misnamed and incorrectly documented. It just
> seems that it's never been used with PIDTYPE_PID but it's perfectly
> capable of doing that. So maybe just put a patch on top renaming it to
> send_sig_info_type() and remove the old comment. But I can live without
> renaming it for now as well.

OK, I'll update the comment, please see below.

It should probably say more about the case when type > PIDTYPE_TGID,
but this is "offtopic" for this patch.

Oleg.

--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -1437,7 +1437,8 @@ void lockdep_assert_task_sighand_held(struct task_struct *task)
 #endif
 
 /*
- * send signal info to all the members of a group
+ * send signal info to all the members of a thread group or to the
+ * individual thread if type == PIDTYPE_PID.
  */
 int group_send_sig_info(int sig, struct kernel_siginfo *info,
 			struct task_struct *p, enum pid_type type)
  

On Fri, Feb 09, 2024 at 11:28:17AM +0100, Oleg Nesterov wrote:
> On 02/08, Oleg Nesterov wrote:
> >
> > Is prepare_kill_siginfo() correct when we send a signal to the child
> > pid namespace? si_pid = task_tgid_vnr(current) doesn't look right
> 
> Yes, but iiuc send_signal_locked() should fixup si_pid/si_uid, so it
> is not buggy.

It must've been. Yesterday I realized that otherwise kill(2) would have
been broken for a long time. I think this was originally fixed in commit
6588c1e3ff01 ("signals: SI_USER: Masquerade si_pid when crossing pid ns
boundary").