On 01/27, Oleg Nesterov wrote:
>
> I'll (hopefully) send v2 on top of
>
> 	pidfd: cleanup the usage of __pidfd_prepare's flags
> 	pidfd: don't do_notify_pidfd() if !thread_group_empty()
>
> on Monday

Sorry, I don't have time to finish v2 today, I need to update the comments
and write the changelog.

But the patch itself is ready, I am sending it for review.

Tycho, Christian, any comments?

Oleg.


From c31780f6c1136a72048d24701ac6d8401fc1afda Mon Sep 17 00:00:00 2001
From: Oleg Nesterov <oleg@redhat.com>
Date: Sat, 27 Jan 2024 16:59:18 +0100
Subject: [PATCH] pidfd: implement PIDFD_THREAD flag for pidfd_open()

---
 include/uapi/linux/pidfd.h |  3 ++-
 kernel/exit.c              |  7 +++++++
 kernel/fork.c              | 29 +++++++++++++++++++++++++++--
 kernel/pid.c               |  2 +-
 kernel/signal.c            |  4 +++-
 5 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/include/uapi/linux/pidfd.h b/include/uapi/linux/pidfd.h
index 5406fbc13074..2e6461459877 100644
--- a/include/uapi/linux/pidfd.h
+++ b/include/uapi/linux/pidfd.h
@@ -7,6 +7,7 @@
 #include <linux/fcntl.h>
 
 /* Flags for pidfd_open().  */
-#define PIDFD_NONBLOCK O_NONBLOCK
+#define PIDFD_NONBLOCK	O_NONBLOCK
+#define PIDFD_THREAD	O_EXCL
 
 #endif /* _UAPI_LINUX_PIDFD_H */
diff --git a/kernel/exit.c b/kernel/exit.c
index dfb963d2f862..74fe6bfb9577 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -739,6 +739,13 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
 		kill_orphaned_pgrp(tsk->group_leader, NULL);
 
 	tsk->exit_state = EXIT_ZOMBIE;
+	/*
+	 * sub-thread or delay_group_leader(), wake up the PIDFD_THREAD
+	 * waiters.
+	 */
+	if (!thread_group_empty(tsk))
+		do_notify_pidfd(tsk);
+
 	if (unlikely(tsk->ptrace)) {
 		int sig = thread_group_leader(tsk) &&
 				thread_group_empty(tsk) &&
diff --git a/kernel/fork.c b/kernel/fork.c
index 347641398f9d..977b58c0eac6 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -101,6 +101,7 @@
 #include <linux/user_events.h>
 #include <linux/iommu.h>
 #include <linux/rseq.h>
+#include <uapi/linux/pidfd.h>
 
 #include <asm/pgalloc.h>
 #include <linux/uaccess.h>
@@ -2050,6 +2051,8 @@ 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) {
@@ -2068,12 +2071,27 @@ static void pidfd_show_fdinfo(struct seq_file *m, struct file *f)
 }
 #endif
 
+static bool pidfd_task_exited(struct pid *pid, bool thread)
+{
+	struct task_struct *task;
+	bool exited;
+
+	rcu_read_lock();
+	task = pid_task(pid, PIDTYPE_PID);
+	exited = !task ||
+		(READ_ONCE(task->exit_state) && (thread || thread_group_empty(task)));
+	rcu_read_unlock();
+
+	return exited;
+}
+
 /*
  * Poll support for process exit notification.
  */
 static __poll_t pidfd_poll(struct file *file, struct poll_table_struct *pts)
 {
 	struct pid *pid = file->private_data;
+	bool thread = file->f_flags & PIDFD_THREAD;
 	__poll_t poll_flags = 0;
 
 	poll_wait(file, &pid->wait_pidfd, pts);
@@ -2083,7 +2101,7 @@ static __poll_t pidfd_poll(struct file *file, struct poll_table_struct *pts)
 	 * If the thread group leader exits before all other threads in the
 	 * group, then poll(2) should block, similar to the wait(2) family.
 	 */
-	if (thread_group_exited(pid))
+	if (pidfd_task_exited(pid, thread))
 		poll_flags = EPOLLIN | EPOLLRDNORM;
 
 	return poll_flags;
@@ -2141,6 +2159,11 @@ static int __pidfd_prepare(struct pid *pid, unsigned int flags, struct file **re
 		return PTR_ERR(pidfd_file);
 	}
 	get_pid(pid); /* held by pidfd_file now */
+	/*
+	 * anon_inode_getfile() ignores everything outside of the
+	 * O_ACCMODE | O_NONBLOCK mask, set PIDFD_THREAD manually.
+	 */
+	pidfd_file->f_flags |= (flags & PIDFD_THREAD);
 	*ret = pidfd_file;
 	return pidfd;
 }
@@ -2173,7 +2196,9 @@ static int __pidfd_prepare(struct pid *pid, unsigned int flags, struct file **re
  */
 int pidfd_prepare(struct pid *pid, unsigned int flags, struct file **ret)
 {
-	if (!pid || !pid_has_task(pid, PIDTYPE_TGID))
+	bool thread = flags & PIDFD_THREAD;
+
+	if (!pid || !pid_has_task(pid, thread ? PIDTYPE_PID : PIDTYPE_TGID));
 		return -EINVAL;
 
 	return __pidfd_prepare(pid, flags, ret);
diff --git a/kernel/pid.c b/kernel/pid.c
index c7a3e359f8f5..04bdd5ecf183 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -629,7 +629,7 @@ SYSCALL_DEFINE2(pidfd_open, pid_t, pid, unsigned int, flags)
 	int fd;
 	struct pid *p;
 
-	if (flags & ~PIDFD_NONBLOCK)
+	if (flags & ~(PIDFD_NONBLOCK | PIDFD_THREAD))
 		return -EINVAL;
 
 	if (pid <= 0)
diff --git a/kernel/signal.c b/kernel/signal.c
index 9561a3962ca6..919cd33a0405 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2051,7 +2051,8 @@ bool do_notify_parent(struct task_struct *tsk, int sig)
 	WARN_ON_ONCE(!tsk->ptrace &&
 	       (tsk->group_leader != tsk || !thread_group_empty(tsk)));
 	/*
-	 * tsk is a group leader and has no threads, wake up the pidfd waiters.
+	 * tsk is a group leader and has no threads, wake up the !PIDFD_THREAD
+	 * waiters.
 	 */
 	if (thread_group_empty(tsk))
 		do_notify_pidfd(tsk);
@@ -3926,6 +3927,7 @@ 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);
 
 err:
-- 
2.25.1.362.g51ebf55

On Mon, Jan 29, 2024 at 3:24 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> On 01/27, Oleg Nesterov wrote:
> >
> > I'll (hopefully) send v2 on top of
> >
> >       pidfd: cleanup the usage of __pidfd_prepare's flags
> >       pidfd: don't do_notify_pidfd() if !thread_group_empty()
> >
> > on Monday
>
> Sorry, I don't have time to finish v2 today, I need to update the comments
> and write the changelog.
>
> But the patch itself is ready, I am sending it for review.
>
> Tycho, Christian, any comments?

Right now, pidfd_send_signal() sends signals to processes, like so:

 * The syscall currently only signals via PIDTYPE_PID which covers
 * kill(<positive-pid>, <signal>. It does not signal threads or process
 * groups.

This patch adds PIDFD_THREAD which, potentially confusingly, doesn't
change this (AFAICS).  So at least that should be documented loudly
and clearly, IMO.  But I actually just bumped in to this limitation in
pidfd_send_signal(), like so:

https://github.com/systemd/systemd/issues/31093

Specifically, systemd can't properly emulate Ctrl-C using pidfd_send_signal().

I don't know whether implementing the other signal types belongs as
part of this patch, but they're at least thematically related.

--Andy

On 01/31, Andy Lutomirski wrote:
>
> Right now, pidfd_send_signal() sends signals to processes, like so:
>
>  * The syscall currently only signals via PIDTYPE_PID which covers
>  * kill(<positive-pid>, <signal>. It does not signal threads or process
>  * groups.
>
> This patch adds PIDFD_THREAD which, potentially confusingly, doesn't
> change this (AFAICS).

Yes,

> So at least that should be documented loudly
> and clearly, IMO.

Please note

	/* TODO: respect PIDFD_THREAD */

this patch adds into pidfd_send_signal().

See also this part of discussion

	> > +	/* TODO: respect PIDFD_THREAD */
	>
	> So I've been thinking about this at the end of last week. Do we need to
	> give userspace a way to send a thread-group wide signal even when a
	> PIDFD_THREAD pidfd is passed? Or should we just not worry about this
	> right now and wait until someone needs this?

	I don't know. I am fine either way, but I think this needs a separate
	patch and another discussion in any case. Anyway should be trivial,
	pidfd_send_signal() has the "flags" argument.

with Christian in https://lore.kernel.org/all/20240130112126.GA26108@redhat.com/

Or did I misunderstand you?

Oleg.

Forgot to mention...

And I agree that pidfd_send_signal(flags => PGID/SID) can make
some sense too.

But this a) doesn't depend on PIDFD_THREAD, and b) needs another
patch/discussion.

But again, I am not sure I understood you correctly.

On 01/31, Oleg Nesterov wrote:
>
> On 01/31, Andy Lutomirski wrote:
> >
> > Right now, pidfd_send_signal() sends signals to processes, like so:
> >
> >  * The syscall currently only signals via PIDTYPE_PID which covers
> >  * kill(<positive-pid>, <signal>. It does not signal threads or process
> >  * groups.
> >
> > This patch adds PIDFD_THREAD which, potentially confusingly, doesn't
> > change this (AFAICS).
>
> Yes,
>
> > So at least that should be documented loudly
> > and clearly, IMO.
>
> Please note
>
> 	/* TODO: respect PIDFD_THREAD */
>
> this patch adds into pidfd_send_signal().
>
> See also this part of discussion
>
> 	> > +	/* TODO: respect PIDFD_THREAD */
> 	>
> 	> So I've been thinking about this at the end of last week. Do we need to
> 	> give userspace a way to send a thread-group wide signal even when a
> 	> PIDFD_THREAD pidfd is passed? Or should we just not worry about this
> 	> right now and wait until someone needs this?
>
> 	I don't know. I am fine either way, but I think this needs a separate
> 	patch and another discussion in any case. Anyway should be trivial,
> 	pidfd_send_signal() has the "flags" argument.
>
> with Christian in https://lore.kernel.org/all/20240130112126.GA26108@redhat.com/
>
> Or did I misunderstand you?
>
> Oleg.

> On 01/31, Oleg Nesterov wrote:
> >
> > On 01/31, Andy Lutomirski wrote:
> > Please note
> >
> >       /* TODO: respect PIDFD_THREAD */
> >
> > this patch adds into pidfd_send_signal().
> >
> > See also this part of discussion
> >
> >       > > +   /* TODO: respect PIDFD_THREAD */
> >       >
> >       > So I've been thinking about this at the end of last week. Do we need to
> >       > give userspace a way to send a thread-group wide signal even when a
> >       > PIDFD_THREAD pidfd is passed? Or should we just not worry about this
> >       > right now and wait until someone needs this?
> >
> >       I don't know. I am fine either way, but I think this needs a separate
> >       patch and another discussion in any case. Anyway should be trivial,
> >       pidfd_send_signal() has the "flags" argument.
> >
> > with Christian in https://lore.kernel.org/all/20240130112126.GA26108@redhat.com/

I missed that.  Whoops.

On Wed, Jan 31, 2024 at 11:15 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> Forgot to mention...
>
> And I agree that pidfd_send_signal(flags => PGID/SID) can make
> some sense too.
>
> But this a) doesn't depend on PIDFD_THREAD, and b) needs another
> patch/discussion.
>
> But again, I am not sure I understood you correctly.
>

Hmm.

When one works with regular (non-fd) pids / pgids etc, one specifies
the signal domain at the time that one sends the signal.  I don't know
what pidfds should do.  It seems a bit inefficient for anything that
wants a pidfd and might send a signal in a different mode in the
future to have to hold on to multiple pidfds, so it probably should be
a pidfd_send_signal flag.

Which leaves the question of what the default should be.  Should
pidfd_send_signal with flags = 0 on a PIDFD_THREAD signal the process
or the thread?  I guess there are two reasonable solutions:

1. flags = 0 always means process.  And maybe there's a special flag
to send a signal that matches the pidfd type, or maybe not.

2. flags = 0 does what the pidfd seems to imply, and a new
PIDFD_SIGNAL_PID flag overrides it to signal the whole PID even if the
pidfd is PIDFD_THREAD.

Do any of you have actual use cases in mind where one choice is
clearly better than the other choice?

--Andy

On Wed, Jan 31, 2024 at 11:24:48AM -0800, Andy Lutomirski wrote:
> > On 01/31, Oleg Nesterov wrote:
> > >
> > > On 01/31, Andy Lutomirski wrote:
> > > Please note
> > >
> > >       /* TODO: respect PIDFD_THREAD */
> > >
> > > this patch adds into pidfd_send_signal().
> > >
> > > See also this part of discussion
> > >
> > >       > > +   /* TODO: respect PIDFD_THREAD */
> > >       >
> > >       > So I've been thinking about this at the end of last week. Do we need to
> > >       > give userspace a way to send a thread-group wide signal even when a
> > >       > PIDFD_THREAD pidfd is passed? Or should we just not worry about this
> > >       > right now and wait until someone needs this?
> > >
> > >       I don't know. I am fine either way, but I think this needs a separate
> > >       patch and another discussion in any case. Anyway should be trivial,
> > >       pidfd_send_signal() has the "flags" argument.
> > >
> > > with Christian in https://lore.kernel.org/all/20240130112126.GA26108@redhat.com/
> 
> I missed that.  Whoops.
> 
> On Wed, Jan 31, 2024 at 11:15 AM Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > Forgot to mention...
> >
> > And I agree that pidfd_send_signal(flags => PGID/SID) can make
> > some sense too.
> >
> > But this a) doesn't depend on PIDFD_THREAD, and b) needs another
> > patch/discussion.
> >
> > But again, I am not sure I understood you correctly.
> >
> 
> Hmm.
> 
> When one works with regular (non-fd) pids / pgids etc, one specifies
> the signal domain at the time that one sends the signal.  I don't know
> what pidfds should do.  It seems a bit inefficient for anything that
> wants a pidfd and might send a signal in a different mode in the
> future to have to hold on to multiple pidfds, so it probably should be
> a pidfd_send_signal flag.
> 
> Which leaves the question of what the default should be.  Should
> pidfd_send_signal with flags = 0 on a PIDFD_THREAD signal the process
> or the thread?  I guess there are two reasonable solutions:
> 
> 1. flags = 0 always means process.  And maybe there's a special flag
> to send a signal that matches the pidfd type, or maybe not.
> 
> 2. flags = 0 does what the pidfd seems to imply, and a new
> PIDFD_SIGNAL_PID flag overrides it to signal the whole PID even if the
> pidfd is PIDFD_THREAD.
> 
> Do any of you have actual use cases in mind where one choice is
> clearly better than the other choice?

So conceptually I think having the type of pidfd dictate the default
scope of the signal is the most elegant approach. And then very likely
we should just have:

PIDFD_SIGNAL_THREAD
PIDFD_SIGNAL_THREAD_GROUP
PIDFD_SIGNAL_PROCESS_GROUP

I think for userspace it doesn't really matter as long as we clearly
document what's going on.

Thoughts?

On Wed, Jan 31, 2024 at 11:46 AM Christian Brauner <brauner@kernel.org> wrote:
>
> On Wed, Jan 31, 2024 at 11:24:48AM -0800, Andy Lutomirski wrote:
> > > On 01/31, Oleg Nesterov wrote:
> > > >
> > > > On 01/31, Andy Lutomirski wrote:
> > > > Please note
> > > >
> > > >       /* TODO: respect PIDFD_THREAD */
> > > >
> > > > this patch adds into pidfd_send_signal().
> > > >
> > > > See also this part of discussion
> > > >
> > > >       > > +   /* TODO: respect PIDFD_THREAD */
> > > >       >
> > > >       > So I've been thinking about this at the end of last week. Do we need to
> > > >       > give userspace a way to send a thread-group wide signal even when a
> > > >       > PIDFD_THREAD pidfd is passed? Or should we just not worry about this
> > > >       > right now and wait until someone needs this?
> > > >
> > > >       I don't know. I am fine either way, but I think this needs a separate
> > > >       patch and another discussion in any case. Anyway should be trivial,
> > > >       pidfd_send_signal() has the "flags" argument.
> > > >
> > > > with Christian in https://lore.kernel.org/all/20240130112126.GA26108@redhat.com/
> >
> > I missed that.  Whoops.
> >
> > On Wed, Jan 31, 2024 at 11:15 AM Oleg Nesterov <oleg@redhat.com> wrote:
> > >
> > > Forgot to mention...
> > >
> > > And I agree that pidfd_send_signal(flags => PGID/SID) can make
> > > some sense too.
> > >
> > > But this a) doesn't depend on PIDFD_THREAD, and b) needs another
> > > patch/discussion.
> > >
> > > But again, I am not sure I understood you correctly.
> > >
> >
> > Hmm.
> >
> > When one works with regular (non-fd) pids / pgids etc, one specifies
> > the signal domain at the time that one sends the signal.  I don't know
> > what pidfds should do.  It seems a bit inefficient for anything that
> > wants a pidfd and might send a signal in a different mode in the
> > future to have to hold on to multiple pidfds, so it probably should be
> > a pidfd_send_signal flag.
> >
> > Which leaves the question of what the default should be.  Should
> > pidfd_send_signal with flags = 0 on a PIDFD_THREAD signal the process
> > or the thread?  I guess there are two reasonable solutions:
> >
> > 1. flags = 0 always means process.  And maybe there's a special flag
> > to send a signal that matches the pidfd type, or maybe not.
> >
> > 2. flags = 0 does what the pidfd seems to imply, and a new
> > PIDFD_SIGNAL_PID flag overrides it to signal the whole PID even if the
> > pidfd is PIDFD_THREAD.
> >
> > Do any of you have actual use cases in mind where one choice is
> > clearly better than the other choice?
>
> So conceptually I think having the type of pidfd dictate the default
> scope of the signal is the most elegant approach. And then very likely
> we should just have:
>
> PIDFD_SIGNAL_THREAD
> PIDFD_SIGNAL_THREAD_GROUP
> PIDFD_SIGNAL_PROCESS_GROUP
>
> I think for userspace it doesn't really matter as long as we clearly
> document what's going on.
>

This seems reasonable unless we're likely to end up with a pidfd mode
that doesn't actually make sense in a send_signal context.  But I'm
not immediately seeing any reason that that would happen.

--Andy

On Wed, Jan 31, 2024 at 11:50:23AM -0800, Andy Lutomirski wrote:
> On Wed, Jan 31, 2024 at 11:46 AM Christian Brauner <brauner@kernel.org> wrote:
> >
> > On Wed, Jan 31, 2024 at 11:24:48AM -0800, Andy Lutomirski wrote:
> > > > On 01/31, Oleg Nesterov wrote:
> > > > >
> > > > > On 01/31, Andy Lutomirski wrote:
> > > > > Please note
> > > > >
> > > > >       /* TODO: respect PIDFD_THREAD */
> > > > >
> > > > > this patch adds into pidfd_send_signal().
> > > > >
> > > > > See also this part of discussion
> > > > >
> > > > >       > > +   /* TODO: respect PIDFD_THREAD */
> > > > >       >
> > > > >       > So I've been thinking about this at the end of last week. Do we need to
> > > > >       > give userspace a way to send a thread-group wide signal even when a
> > > > >       > PIDFD_THREAD pidfd is passed? Or should we just not worry about this
> > > > >       > right now and wait until someone needs this?
> > > > >
> > > > >       I don't know. I am fine either way, but I think this needs a separate
> > > > >       patch and another discussion in any case. Anyway should be trivial,
> > > > >       pidfd_send_signal() has the "flags" argument.
> > > > >
> > > > > with Christian in https://lore.kernel.org/all/20240130112126.GA26108@redhat.com/
> > >
> > > I missed that.  Whoops.
> > >
> > > On Wed, Jan 31, 2024 at 11:15 AM Oleg Nesterov <oleg@redhat.com> wrote:
> > > >
> > > > Forgot to mention...
> > > >
> > > > And I agree that pidfd_send_signal(flags => PGID/SID) can make
> > > > some sense too.
> > > >
> > > > But this a) doesn't depend on PIDFD_THREAD, and b) needs another
> > > > patch/discussion.
> > > >
> > > > But again, I am not sure I understood you correctly.
> > > >
> > >
> > > Hmm.
> > >
> > > When one works with regular (non-fd) pids / pgids etc, one specifies
> > > the signal domain at the time that one sends the signal.  I don't know
> > > what pidfds should do.  It seems a bit inefficient for anything that
> > > wants a pidfd and might send a signal in a different mode in the
> > > future to have to hold on to multiple pidfds, so it probably should be
> > > a pidfd_send_signal flag.
> > >
> > > Which leaves the question of what the default should be.  Should
> > > pidfd_send_signal with flags = 0 on a PIDFD_THREAD signal the process
> > > or the thread?  I guess there are two reasonable solutions:
> > >
> > > 1. flags = 0 always means process.  And maybe there's a special flag
> > > to send a signal that matches the pidfd type, or maybe not.
> > >
> > > 2. flags = 0 does what the pidfd seems to imply, and a new
> > > PIDFD_SIGNAL_PID flag overrides it to signal the whole PID even if the
> > > pidfd is PIDFD_THREAD.
> > >
> > > Do any of you have actual use cases in mind where one choice is
> > > clearly better than the other choice?
> >
> > So conceptually I think having the type of pidfd dictate the default
> > scope of the signal is the most elegant approach. And then very likely
> > we should just have:
> >
> > PIDFD_SIGNAL_THREAD
> > PIDFD_SIGNAL_THREAD_GROUP
> > PIDFD_SIGNAL_PROCESS_GROUP
> >
> > I think for userspace it doesn't really matter as long as we clearly
> > document what's going on.
> >
> 
> This seems reasonable unless we're likely to end up with a pidfd mode
> that doesn't actually make sense in a send_signal context.  But I'm
> not immediately seeing any reason that that would happen.

Yeah, I think that's very unlikely and we could reject it obased on api
design considerations.

On Thu, Feb 01, 2024 at 02:30:46PM +0100, Christian Brauner wrote:
> On Wed, Jan 31, 2024 at 11:50:23AM -0800, Andy Lutomirski wrote:
> > On Wed, Jan 31, 2024 at 11:46 AM Christian Brauner <brauner@kernel.org> wrote:
> > >
> > > On Wed, Jan 31, 2024 at 11:24:48AM -0800, Andy Lutomirski wrote:
> > > > > On 01/31, Oleg Nesterov wrote:
> > > > > >
> > > > > > On 01/31, Andy Lutomirski wrote:
> > > > > > Please note
> > > > > >
> > > > > >       /* TODO: respect PIDFD_THREAD */
> > > > > >
> > > > > > this patch adds into pidfd_send_signal().
> > > > > >
> > > > > > See also this part of discussion
> > > > > >
> > > > > >       > > +   /* TODO: respect PIDFD_THREAD */
> > > > > >       >
> > > > > >       > So I've been thinking about this at the end of last week. Do we need to
> > > > > >       > give userspace a way to send a thread-group wide signal even when a
> > > > > >       > PIDFD_THREAD pidfd is passed? Or should we just not worry about this
> > > > > >       > right now and wait until someone needs this?
> > > > > >
> > > > > >       I don't know. I am fine either way, but I think this needs a separate
> > > > > >       patch and another discussion in any case. Anyway should be trivial,
> > > > > >       pidfd_send_signal() has the "flags" argument.
> > > > > >
> > > > > > with Christian in https://lore.kernel.org/all/20240130112126.GA26108@redhat.com/
> > > >
> > > > I missed that.  Whoops.
> > > >
> > > > On Wed, Jan 31, 2024 at 11:15 AM Oleg Nesterov <oleg@redhat.com> wrote:
> > > > >
> > > > > Forgot to mention...
> > > > >
> > > > > And I agree that pidfd_send_signal(flags => PGID/SID) can make
> > > > > some sense too.
> > > > >
> > > > > But this a) doesn't depend on PIDFD_THREAD, and b) needs another
> > > > > patch/discussion.
> > > > >
> > > > > But again, I am not sure I understood you correctly.
> > > > >
> > > >
> > > > Hmm.
> > > >
> > > > When one works with regular (non-fd) pids / pgids etc, one specifies
> > > > the signal domain at the time that one sends the signal.  I don't know
> > > > what pidfds should do.  It seems a bit inefficient for anything that
> > > > wants a pidfd and might send a signal in a different mode in the
> > > > future to have to hold on to multiple pidfds, so it probably should be
> > > > a pidfd_send_signal flag.
> > > >
> > > > Which leaves the question of what the default should be.  Should
> > > > pidfd_send_signal with flags = 0 on a PIDFD_THREAD signal the process
> > > > or the thread?  I guess there are two reasonable solutions:
> > > >
> > > > 1. flags = 0 always means process.  And maybe there's a special flag
> > > > to send a signal that matches the pidfd type, or maybe not.
> > > >
> > > > 2. flags = 0 does what the pidfd seems to imply, and a new
> > > > PIDFD_SIGNAL_PID flag overrides it to signal the whole PID even if the
> > > > pidfd is PIDFD_THREAD.
> > > >
> > > > Do any of you have actual use cases in mind where one choice is
> > > > clearly better than the other choice?
> > >
> > > So conceptually I think having the type of pidfd dictate the default
> > > scope of the signal is the most elegant approach. And then very likely
> > > we should just have:
> > >
> > > PIDFD_SIGNAL_THREAD
> > > PIDFD_SIGNAL_THREAD_GROUP
> > > PIDFD_SIGNAL_PROCESS_GROUP
> > >
> > > I think for userspace it doesn't really matter as long as we clearly
> > > document what's going on.
> > >
> > 
> > This seems reasonable unless we're likely to end up with a pidfd mode
> > that doesn't actually make sense in a send_signal context.  But I'm
> > not immediately seeing any reason that that would happen.
> 
> Yeah, I think that's very unlikely and we could reject it obased on api
> design considerations.

Ah, forgot to ask. Did you intend to send a patch for this?

On Thu, Feb 1, 2024 at 5:39 AM Christian Brauner <brauner@kernel.org> wrote:
>
> On Thu, Feb 01, 2024 at 02:30:46PM +0100, Christian Brauner wrote:
> > On Wed, Jan 31, 2024 at 11:50:23AM -0800, Andy Lutomirski wrote:
> > > On Wed, Jan 31, 2024 at 11:46 AM Christian Brauner <brauner@kernel.org> wrote:
> > > >
> > > > On Wed, Jan 31, 2024 at 11:24:48AM -0800, Andy Lutomirski wrote:
> > > > > > On 01/31, Oleg Nesterov wrote:
> > > > > > >
> > > > > > > On 01/31, Andy Lutomirski wrote:
> > > > > > > Please note
> > > > > > >
> > > > > > >       /* TODO: respect PIDFD_THREAD */
> > > > > > >
> > > > > > > this patch adds into pidfd_send_signal().
> > > > > > >
> > > > > > > See also this part of discussion
> > > > > > >
> > > > > > >       > > +   /* TODO: respect PIDFD_THREAD */
> > > > > > >       >
> > > > > > >       > So I've been thinking about this at the end of last week. Do we need to
> > > > > > >       > give userspace a way to send a thread-group wide signal even when a
> > > > > > >       > PIDFD_THREAD pidfd is passed? Or should we just not worry about this
> > > > > > >       > right now and wait until someone needs this?
> > > > > > >
> > > > > > >       I don't know. I am fine either way, but I think this needs a separate
> > > > > > >       patch and another discussion in any case. Anyway should be trivial,
> > > > > > >       pidfd_send_signal() has the "flags" argument.
> > > > > > >
> > > > > > > with Christian in https://lore.kernel.org/all/20240130112126.GA26108@redhat.com/
> > > > >
> > > > > I missed that.  Whoops.
> > > > >
> > > > > On Wed, Jan 31, 2024 at 11:15 AM Oleg Nesterov <oleg@redhat.com> wrote:
> > > > > >
> > > > > > Forgot to mention...
> > > > > >
> > > > > > And I agree that pidfd_send_signal(flags => PGID/SID) can make
> > > > > > some sense too.
> > > > > >
> > > > > > But this a) doesn't depend on PIDFD_THREAD, and b) needs another
> > > > > > patch/discussion.
> > > > > >
> > > > > > But again, I am not sure I understood you correctly.
> > > > > >
> > > > >
> > > > > Hmm.
> > > > >
> > > > > When one works with regular (non-fd) pids / pgids etc, one specifies
> > > > > the signal domain at the time that one sends the signal.  I don't know
> > > > > what pidfds should do.  It seems a bit inefficient for anything that
> > > > > wants a pidfd and might send a signal in a different mode in the
> > > > > future to have to hold on to multiple pidfds, so it probably should be
> > > > > a pidfd_send_signal flag.
> > > > >
> > > > > Which leaves the question of what the default should be.  Should
> > > > > pidfd_send_signal with flags = 0 on a PIDFD_THREAD signal the process
> > > > > or the thread?  I guess there are two reasonable solutions:
> > > > >
> > > > > 1. flags = 0 always means process.  And maybe there's a special flag
> > > > > to send a signal that matches the pidfd type, or maybe not.
> > > > >
> > > > > 2. flags = 0 does what the pidfd seems to imply, and a new
> > > > > PIDFD_SIGNAL_PID flag overrides it to signal the whole PID even if the
> > > > > pidfd is PIDFD_THREAD.
> > > > >
> > > > > Do any of you have actual use cases in mind where one choice is
> > > > > clearly better than the other choice?
> > > >
> > > > So conceptually I think having the type of pidfd dictate the default
> > > > scope of the signal is the most elegant approach. And then very likely
> > > > we should just have:
> > > >
> > > > PIDFD_SIGNAL_THREAD
> > > > PIDFD_SIGNAL_THREAD_GROUP
> > > > PIDFD_SIGNAL_PROCESS_GROUP
> > > >
> > > > I think for userspace it doesn't really matter as long as we clearly
> > > > document what's going on.
> > > >
> > >
> > > This seems reasonable unless we're likely to end up with a pidfd mode
> > > that doesn't actually make sense in a send_signal context.  But I'm
> > > not immediately seeing any reason that that would happen.
> >
> > Yeah, I think that's very unlikely and we could reject it obased on api
> > design considerations.
>
> Ah, forgot to ask. Did you intend to send a patch for this?

I can try to get to it tomorrow.  Currently trying to madly line up a
whole bunch of stuff in time for a maintenance window.

On Mon, Jan 29, 2024 at 12:23:15PM +0100, Oleg Nesterov wrote:
> On 01/27, Oleg Nesterov wrote:
> >
> > I'll (hopefully) send v2 on top of
> >
> > 	pidfd: cleanup the usage of __pidfd_prepare's flags
> > 	pidfd: don't do_notify_pidfd() if !thread_group_empty()
> >
> > on Monday
> 
> Sorry, I don't have time to finish v2 today, I need to update the comments
> and write the changelog.
> 
> But the patch itself is ready, I am sending it for review.
> 
> Tycho, Christian, any comments?
> 
> Oleg.
> 
> 
> From c31780f6c1136a72048d24701ac6d8401fc1afda Mon Sep 17 00:00:00 2001
> From: Oleg Nesterov <oleg@redhat.com>
> Date: Sat, 27 Jan 2024 16:59:18 +0100
> Subject: [PATCH] pidfd: implement PIDFD_THREAD flag for pidfd_open()
> 
> ---
>  include/uapi/linux/pidfd.h |  3 ++-
>  kernel/exit.c              |  7 +++++++
>  kernel/fork.c              | 29 +++++++++++++++++++++++++++--
>  kernel/pid.c               |  2 +-
>  kernel/signal.c            |  4 +++-
>  5 files changed, 40 insertions(+), 5 deletions(-)
> 
> diff --git a/include/uapi/linux/pidfd.h b/include/uapi/linux/pidfd.h
> index 5406fbc13074..2e6461459877 100644
> --- a/include/uapi/linux/pidfd.h
> +++ b/include/uapi/linux/pidfd.h
> @@ -7,6 +7,7 @@
>  #include <linux/fcntl.h>
>  
>  /* Flags for pidfd_open().  */
> -#define PIDFD_NONBLOCK O_NONBLOCK
> +#define PIDFD_NONBLOCK	O_NONBLOCK
> +#define PIDFD_THREAD	O_EXCL
>  
>  #endif /* _UAPI_LINUX_PIDFD_H */
> diff --git a/kernel/exit.c b/kernel/exit.c
> index dfb963d2f862..74fe6bfb9577 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -739,6 +739,13 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
>  		kill_orphaned_pgrp(tsk->group_leader, NULL);
>  
>  	tsk->exit_state = EXIT_ZOMBIE;
> +	/*
> +	 * sub-thread or delay_group_leader(), wake up the PIDFD_THREAD
> +	 * waiters.
> +	 */
> +	if (!thread_group_empty(tsk))
> +		do_notify_pidfd(tsk);
> +
>  	if (unlikely(tsk->ptrace)) {
>  		int sig = thread_group_leader(tsk) &&
>  				thread_group_empty(tsk) &&
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 347641398f9d..977b58c0eac6 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -101,6 +101,7 @@
>  #include <linux/user_events.h>
>  #include <linux/iommu.h>
>  #include <linux/rseq.h>
> +#include <uapi/linux/pidfd.h>
>  
>  #include <asm/pgalloc.h>
>  #include <linux/uaccess.h>
> @@ -2050,6 +2051,8 @@ static void pidfd_show_fdinfo(struct seq_file *m, struct file *f)
>  
>  	seq_put_decimal_ll(m, "Pid:\t", nr);
>  
> +	/* TODO: report PIDFD_THREAD */

Ah yes, very good point. We should give userspace an indicator whether
something is thread pidfd or not.

> +
>  #ifdef CONFIG_PID_NS
>  	seq_put_decimal_ll(m, "\nNSpid:\t", nr);
>  	if (nr > 0) {
> @@ -2068,12 +2071,27 @@ static void pidfd_show_fdinfo(struct seq_file *m, struct file *f)
>  }
>  #endif
>  
> +static bool pidfd_task_exited(struct pid *pid, bool thread)
> +{
> +	struct task_struct *task;
> +	bool exited;
> +
> +	rcu_read_lock();
> +	task = pid_task(pid, PIDTYPE_PID);
> +	exited = !task ||
> +		(READ_ONCE(task->exit_state) && (thread || thread_group_empty(task)));
> +	rcu_read_unlock();
> +
> +	return exited;
> +}
> +
>  /*
>   * Poll support for process exit notification.
>   */
>  static __poll_t pidfd_poll(struct file *file, struct poll_table_struct *pts)
>  {
>  	struct pid *pid = file->private_data;
> +	bool thread = file->f_flags & PIDFD_THREAD;
>  	__poll_t poll_flags = 0;
>  
>  	poll_wait(file, &pid->wait_pidfd, pts);
> @@ -2083,7 +2101,7 @@ static __poll_t pidfd_poll(struct file *file, struct poll_table_struct *pts)
>  	 * If the thread group leader exits before all other threads in the
>  	 * group, then poll(2) should block, similar to the wait(2) family.
>  	 */
> -	if (thread_group_exited(pid))
> +	if (pidfd_task_exited(pid, thread))
>  		poll_flags = EPOLLIN | EPOLLRDNORM;
>  
>  	return poll_flags;
> @@ -2141,6 +2159,11 @@ static int __pidfd_prepare(struct pid *pid, unsigned int flags, struct file **re
>  		return PTR_ERR(pidfd_file);
>  	}
>  	get_pid(pid); /* held by pidfd_file now */
> +	/*
> +	 * anon_inode_getfile() ignores everything outside of the
> +	 * O_ACCMODE | O_NONBLOCK mask, set PIDFD_THREAD manually.
> +	 */
> +	pidfd_file->f_flags |= (flags & PIDFD_THREAD);
>  	*ret = pidfd_file;
>  	return pidfd;
>  }
> @@ -2173,7 +2196,9 @@ static int __pidfd_prepare(struct pid *pid, unsigned int flags, struct file **re
>   */
>  int pidfd_prepare(struct pid *pid, unsigned int flags, struct file **ret)
>  {
> -	if (!pid || !pid_has_task(pid, PIDTYPE_TGID))
> +	bool thread = flags & PIDFD_THREAD;
> +
> +	if (!pid || !pid_has_task(pid, thread ? PIDTYPE_PID : PIDTYPE_TGID));
>  		return -EINVAL;
>  
>  	return __pidfd_prepare(pid, flags, ret);
> diff --git a/kernel/pid.c b/kernel/pid.c
> index c7a3e359f8f5..04bdd5ecf183 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -629,7 +629,7 @@ SYSCALL_DEFINE2(pidfd_open, pid_t, pid, unsigned int, flags)
>  	int fd;
>  	struct pid *p;
>  
> -	if (flags & ~PIDFD_NONBLOCK)
> +	if (flags & ~(PIDFD_NONBLOCK | PIDFD_THREAD))
>  		return -EINVAL;
>  
>  	if (pid <= 0)
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 9561a3962ca6..919cd33a0405 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2051,7 +2051,8 @@ bool do_notify_parent(struct task_struct *tsk, int sig)
>  	WARN_ON_ONCE(!tsk->ptrace &&
>  	       (tsk->group_leader != tsk || !thread_group_empty(tsk)));
>  	/*
> -	 * tsk is a group leader and has no threads, wake up the pidfd waiters.
> +	 * tsk is a group leader and has no threads, wake up the !PIDFD_THREAD
> +	 * waiters.
>  	 */
>  	if (thread_group_empty(tsk))
>  		do_notify_pidfd(tsk);
> @@ -3926,6 +3927,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
>  		prepare_kill_siginfo(sig, &kinfo);
>  	}
>  
> +	/* TODO: respect PIDFD_THREAD */

So I've been thinking about this at the end of last week. Do we need to
give userspace a way to send a thread-group wide signal even when a
PIDFD_THREAD pidfd is passed? Or should we just not worry about this
right now and wait until someone needs this?

>  	ret = kill_pid_info(sig, &kinfo, pid);

Otherwise this looks good to me!

On 01/29, Christian Brauner wrote:
>
> On Mon, Jan 29, 2024 at 12:23:15PM +0100, Oleg Nesterov wrote:
> > @@ -3926,6 +3927,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
> >  		prepare_kill_siginfo(sig, &kinfo);
> >  	}
> >
> > +	/* TODO: respect PIDFD_THREAD */
>
> So I've been thinking about this at the end of last week. Do we need to
> give userspace a way to send a thread-group wide signal even when a
> PIDFD_THREAD pidfd is passed? Or should we just not worry about this
> right now and wait until someone needs this?

I don't know. I am fine either way, but I think this needs a separate
patch and another discussion in any case. Anyway should be trivial,
pidfd_send_signal() has the "flags" argument.

On a related note, should copy_process(CLONE_PIDFD | CLONE_THREAD) add
PIDFD_THREAD flag "automatically" depending on CLONE_THREAD? Or do we
want another CLONE_PIDFD_THREAD flag so that PIDFD_THREAD can be used
without CLONE_THREAD? Again, I do not know, needs another discussion.

> Otherwise this looks good to me!

OK, thanks, I'll send v2 in a minute. The patch is the same, I only
updated the comments.

Oleg.

On Mon, Jan 29, 2024 at 02:41:11PM +0100, Christian Brauner wrote:
> On Mon, Jan 29, 2024 at 12:23:15PM +0100, Oleg Nesterov wrote:
> > --- a/kernel/signal.c
> > +++ b/kernel/signal.c
> > @@ -2051,7 +2051,8 @@ bool do_notify_parent(struct task_struct *tsk, int sig)
> >  	WARN_ON_ONCE(!tsk->ptrace &&
> >  	       (tsk->group_leader != tsk || !thread_group_empty(tsk)));
> >  	/*
> > -	 * tsk is a group leader and has no threads, wake up the pidfd waiters.
> > +	 * tsk is a group leader and has no threads, wake up the !PIDFD_THREAD
> > +	 * waiters.
> >  	 */
> >  	if (thread_group_empty(tsk))
> >  		do_notify_pidfd(tsk);
> > @@ -3926,6 +3927,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
> >  		prepare_kill_siginfo(sig, &kinfo);
> >  	}
> >  
> > +	/* TODO: respect PIDFD_THREAD */
> 
> So I've been thinking about this at the end of last week. Do we need to
> give userspace a way to send a thread-group wide signal even when a
> PIDFD_THREAD pidfd is passed? Or should we just not worry about this
> right now and wait until someone needs this?

I don't need it currently, but it would have been handy for some of
the tests I wrote.

Should I fix those up and send them too on top of Oleg's v2?

Tycho

On Mon, Jan 29, 2024 at 07:31:35AM -0700, Tycho Andersen wrote:
> On Mon, Jan 29, 2024 at 02:41:11PM +0100, Christian Brauner wrote:
> > On Mon, Jan 29, 2024 at 12:23:15PM +0100, Oleg Nesterov wrote:
> > > --- a/kernel/signal.c
> > > +++ b/kernel/signal.c
> > > @@ -2051,7 +2051,8 @@ bool do_notify_parent(struct task_struct *tsk, int sig)
> > >  	WARN_ON_ONCE(!tsk->ptrace &&
> > >  	       (tsk->group_leader != tsk || !thread_group_empty(tsk)));
> > >  	/*
> > > -	 * tsk is a group leader and has no threads, wake up the pidfd waiters.
> > > +	 * tsk is a group leader and has no threads, wake up the !PIDFD_THREAD
> > > +	 * waiters.
> > >  	 */
> > >  	if (thread_group_empty(tsk))
> > >  		do_notify_pidfd(tsk);
> > > @@ -3926,6 +3927,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
> > >  		prepare_kill_siginfo(sig, &kinfo);
> > >  	}
> > >  
> > > +	/* TODO: respect PIDFD_THREAD */
> > 
> > So I've been thinking about this at the end of last week. Do we need to
> > give userspace a way to send a thread-group wide signal even when a
> > PIDFD_THREAD pidfd is passed? Or should we just not worry about this
> > right now and wait until someone needs this?
> 
> I don't need it currently, but it would have been handy for some of
> the tests I wrote.
> 
> Should I fix those up and send them too on top of Oleg's v2?

Sure, I don't mind.