[PATCH v3 0/3] pidfds for non thread group leaders

[PATCH v3 0/3] pidfds for non thread group leaders

[Tycho Andersen]

From: Tycho Andersen <tandersen@netflix.com>

Hi all,

Finally picking this back up. I looked for a location where we could put
the do_notify_pidfd() call so that we'd only need one for an
embarassingly long time, but couldn't find anything that worked. So, we
have a new call in a similar location in v1, just using the actual
helper. Perhaps this makes sense, since we're adding a new feature.

Hopefully all the tests even pass this time! I looked for a bit but
couldn't figure out why they don't exit(0) when something failed. I'll
keep looking at that.

Thoughts welcome,

Tycho

Tycho Andersen (3):
  pidfd: allow pidfd_open() on non-thread-group leaders
  selftests/pidfd: add non-thread-group leader tests
  clone: allow CLONE_THREAD | CLONE_PIDFD together

 include/linux/sched/signal.h                  |   1 +
 kernel/exit.c                                 |  11 +
 kernel/fork.c                                 |   7 +-
 kernel/pid.c                                  |  11 +-
 kernel/signal.c                               |   2 +-
 tools/testing/selftests/pidfd/.gitignore      |   1 +
 tools/testing/selftests/pidfd/Makefile        |   3 +-
 .../selftests/pidfd/pidfd_non_tgl_test.c      | 600 ++++++++++++++++++
 8 files changed, 619 insertions(+), 17 deletions(-)
 create mode 100644 tools/testing/selftests/pidfd/pidfd_non_tgl_test.c


base-commit: 610347effc2ecb5ededf5037e82240b151f883ab
-- 
2.34.1

[PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders

[Tycho Andersen]

From: Tycho Andersen <tandersen@netflix.com>

We are using the pidfd family of syscalls with the seccomp userspace
notifier. When some thread triggers a seccomp notification, we want to do
some things to its context (munge fd tables via pidfd_getfd(), maybe write
to its memory, etc.). However, threads created with ~CLONE_FILES or
~CLONE_VM mean that we can't use the pidfd family of syscalls for this
purpose, since their fd table or mm are distinct from the thread group
leader's. In this patch, we relax this restriction for pidfd_open().

In order to avoid dangling poll() users we need to notify pidfd waiters
when individual threads die, but once we do that all the other machinery
seems to work ok viz. the tests. But I suppose there are more cases than
just this one.

Signed-off-by: Tycho Andersen <tandersen@netflix.com>
--
v2: unify pidfd notification to all go through do_notify_pidfd() inside of
    __exit_signals() suggested by Oleg.
    Link to v1: https://lore.kernel.org/all/20231130163946.277502-1-tycho@tycho.pizza/
v3: go back to two separate call sites, the exiting one in
    do_notify_parent(), and a new one in release_task(), when a thread is
    not the thread group leader.
---
 include/linux/sched/signal.h |  1 +
 kernel/exit.c                | 11 +++++++++++
 kernel/fork.c                |  4 +---
 kernel/pid.c                 | 11 +----------
 kernel/signal.c              |  2 +-
 5 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index 4b7664c56208..d752f003a69a 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -333,6 +333,7 @@ extern int kill_pid_usb_asyncio(int sig, int errno, sigval_t addr, struct pid *,
 extern int kill_pgrp(struct pid *pid, int sig, int priv);
 extern int kill_pid(struct pid *pid, int sig, int priv);
 extern __must_check bool do_notify_parent(struct task_struct *, int);
+extern void do_notify_pidfd(struct task_struct *task);
 extern void __wake_up_parent(struct task_struct *p, struct task_struct *parent);
 extern void force_sig(int);
 extern void force_fatal_sig(int);
diff --git a/kernel/exit.c b/kernel/exit.c
index 3988a02efaef..90117d7861f4 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -256,6 +256,17 @@ void release_task(struct task_struct *p)
 	write_lock_irq(&tasklist_lock);
 	ptrace_release_task(p);
 	thread_pid = get_pid(p->thread_pid);
+
+	/*
+	 * If we're not the leader, notify any waiters on our pidfds. Note that
+	 * we don't want to notify the leader until /everyone/ in the thread
+	 * group is dead, viz. the condition below.
+	 *
+	 * We have to do this here, since __exit_signal() will
+	 * __unhash_processes(), and break do_notify_pidfd()'s lookup.
+	 */
+	if (!thread_group_leader(p))
+		do_notify_pidfd(p);
 	__exit_signal(p);
 
 	/*
diff --git a/kernel/fork.c b/kernel/fork.c
index 47ff3b35352e..44969cd472f0 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2157,8 +2157,6 @@ static int __pidfd_prepare(struct pid *pid, unsigned int flags, struct file **re
  * Allocate a new file that stashes @pid and reserve a new pidfd number in the
  * caller's file descriptor table. The pidfd is reserved but not installed yet.
  *
- * The helper verifies that @pid is used as a thread group leader.
- *
  * If this function returns successfully the caller is responsible to either
  * call fd_install() passing the returned pidfd and pidfd file as arguments in
  * order to install the pidfd into its file descriptor table or they must use
@@ -2176,7 +2174,7 @@ 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))
+	if (!pid)
 		return -EINVAL;
 
 	return __pidfd_prepare(pid, flags, ret);
diff --git a/kernel/pid.c b/kernel/pid.c
index b52b10865454..b55c0adf457b 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -552,11 +552,6 @@ struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags)
  * Return the task associated with @pidfd. The function takes a reference on
  * the returned task. The caller is responsible for releasing that reference.
  *
- * Currently, the process identified by @pidfd is always a thread-group leader.
- * This restriction currently exists for all aspects of pidfds including pidfd
- * creation (CLONE_PIDFD cannot be used with CLONE_THREAD) and pidfd polling
- * (only supports thread group leaders).
- *
  * Return: On success, the task_struct associated with the pidfd.
  *	   On error, a negative errno number will be returned.
  */
@@ -615,11 +610,7 @@ int pidfd_create(struct pid *pid, unsigned int flags)
  * @flags: flags to pass
  *
  * This creates a new pid file descriptor with the O_CLOEXEC flag set for
- * the process identified by @pid. Currently, the process identified by
- * @pid must be a thread-group leader. This restriction currently exists
- * for all aspects of pidfds including pidfd creation (CLONE_PIDFD cannot
- * be used with CLONE_THREAD) and pidfd polling (only supports thread group
- * leaders).
+ * the process identified by @pid.
  *
  * Return: On success, a cloexec pidfd is returned.
  *         On error, a negative errno number will be returned.
diff --git a/kernel/signal.c b/kernel/signal.c
index c9c57d053ce4..3e3c9d0fa3a5 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2019,7 +2019,7 @@ int send_sigqueue(struct sigqueue *q, struct pid *pid, enum pid_type type)
 	return ret;
 }
 
-static void do_notify_pidfd(struct task_struct *task)
+void do_notify_pidfd(struct task_struct *task)
 {
 	struct pid *pid;
 
-- 
2.34.1

Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders

[Oleg Nesterov]

Too late for me, but I don't understand this patch after a quick glance.
perhaps I missed something...

On 01/23, Tycho Andersen wrote:
>
> @@ -256,6 +256,17 @@ void release_task(struct task_struct *p)
>  	write_lock_irq(&tasklist_lock);
>  	ptrace_release_task(p);
>  	thread_pid = get_pid(p->thread_pid);
> +
> +	/*
> +	 * If we're not the leader, notify any waiters on our pidfds. Note that
> +	 * we don't want to notify the leader until /everyone/ in the thread
> +	 * group is dead, viz. the condition below.
> +	 *
> +	 * We have to do this here, since __exit_signal() will
> +	 * __unhash_processes(), and break do_notify_pidfd()'s lookup.
> +	 */
> +	if (!thread_group_leader(p))
> +		do_notify_pidfd(p);

This doesn't look consistent.

If the task is a group leader do_notify_pidfd() is called by exit_notify()
when it becomes a zombie (if no other threads), before it is reaped by its
parent (unless autoreap).

If it is a sub-thread, it is called by release_task() above. Note that a
sub-thread can become a zombie too if it is traced.

>  	__exit_signal(p);

and,  do_notify_pidfd() is called before __exit_signal() which does
__unhash_process() -> detach_pid(PIDTYPE_PID).

Doesn't this mean that pidfd_poll() can hang? thread_group_exited()
won't return true after do_notify_pidfd() above, not to mention that
thread_group_empty() is not possible if !thread_group_leader().

So. When do we want to do do_notify_pidfd() ? Whe the task (leader or not)
becomes a zombie (passes exit_notify) or when it is reaped by release_task?

Either way pidfd_poll() needs more changes with this patch and it can't
use thread_group_exited(). If do_notify_pidfd() is called by release_task()
after __exit_signal(), it can just check pid_has_task(PIDTYPE_PID).

Oleg.

Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders

[Tycho Andersen]

On Tue, Jan 23, 2024 at 08:56:08PM +0100, Oleg Nesterov wrote:
> Too late for me, but I don't understand this patch after a quick glance.
> perhaps I missed something...

Thanks for taking a look.

> On 01/23, Tycho Andersen wrote:
> >
> > @@ -256,6 +256,17 @@ void release_task(struct task_struct *p)
> >  	write_lock_irq(&tasklist_lock);
> >  	ptrace_release_task(p);
> >  	thread_pid = get_pid(p->thread_pid);
> > +
> > +	/*
> > +	 * If we're not the leader, notify any waiters on our pidfds. Note that
> > +	 * we don't want to notify the leader until /everyone/ in the thread
> > +	 * group is dead, viz. the condition below.
> > +	 *
> > +	 * We have to do this here, since __exit_signal() will
> > +	 * __unhash_processes(), and break do_notify_pidfd()'s lookup.
> > +	 */
> > +	if (!thread_group_leader(p))
> > +		do_notify_pidfd(p);
> 
> This doesn't look consistent.
> 
> If the task is a group leader do_notify_pidfd() is called by exit_notify()
> when it becomes a zombie (if no other threads), before it is reaped by its
> parent (unless autoreap).

There is another path, also in release_task(), that I was trying to
mirror since it deals explicitly with sub-threads but,

> If it is a sub-thread, it is called by release_task() above. Note that a
> sub-thread can become a zombie too if it is traced.

I didn't know about this.

> >  	__exit_signal(p);
> 
> and,  do_notify_pidfd() is called before __exit_signal() which does
> __unhash_process() -> detach_pid(PIDTYPE_PID).
> 
> Doesn't this mean that pidfd_poll() can hang? thread_group_exited()
> won't return true after do_notify_pidfd() above, not to mention that
> thread_group_empty() is not possible if !thread_group_leader().

I was wondering about this too, but the test_non_tgl_poll_exit test in
the next patch tests exactly this and works as expected.

> So. When do we want to do do_notify_pidfd() ? Whe the task (leader or not)
> becomes a zombie (passes exit_notify) or when it is reaped by release_task?

It seems like we'd want it when exit_notify() is called in principle,
since that's when the pid actually dies. When it is reaped is "mostly
unrelated". Something like,

1. in the "normal" exit_notify() paths via do_notify_parent()
2. if none of those cases are true (aka the final else in
   exit_notify()) and the thread is not ptraced
3. via release_task() finally if this was the thread group leader and
   it died before some sub-thread

then in pidfd_poll(), we can do:

    if (!tsk || (tsk->exit_state >= 0) || thread_group_exited())
        do_notify_pidfd();

?

> Either way pidfd_poll() needs more changes with this patch and it can't
> use thread_group_exited(). If do_notify_pidfd() is called by release_task()
> after __exit_signal(), it can just check pid_has_task(PIDTYPE_PID).

I suppose this is why my test works, since pid_task(PIDTYPE_PID) is null
after release_task(). But if we want it to happen earlier, we'll have
to do something like the above.

Tycho

Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders

[Oleg Nesterov]

I am already sleeping. I'll try to reply to other parts of your email
tomorrow but I am not sure, I will be very busy with family duties.

On 01/23, Tycho Andersen wrote:
>
> > >  	__exit_signal(p);
> >
> > and,  do_notify_pidfd() is called before __exit_signal() which does
> > __unhash_process() -> detach_pid(PIDTYPE_PID).
> >
> > Doesn't this mean that pidfd_poll() can hang? thread_group_exited()
> > won't return true after do_notify_pidfd() above, not to mention that
> > thread_group_empty() is not possible if !thread_group_leader().
>
> I was wondering about this too, but the test_non_tgl_poll_exit test in
> the next patch tests exactly this and works as expected.

Well, if release_task() completes __exit_signal() before the woken task
does thread_group_exited(), pid_task(PIDTYPE_PID) will return 0 and
pidfd_poll() won't hang.

But to be honest I can't understand test_non_tgl_poll_exit() at all. I don't
even understand why the process/thread created by fork_task_with_thread()
should ever exit. And why it creates the "writer" child... Never mind, too
late for me to read the code.

Oleg.

Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders

[Oleg Nesterov]

On 01/23, Oleg Nesterov wrote:
>
> But to be honest I can't understand test_non_tgl_poll_exit() at all. I don't
> even understand why the process/thread created by fork_task_with_thread()
> should ever exit. And why it creates the "writer" child... Never mind, too
> late for me to read the code.

Ah, OK, it passes thread_wait_exit to fork_task_with_thread(), and this "fn"
reads sk_pair and does exit() which is actually exit_group().

Oleg.

Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders

[Oleg Nesterov]

Add Eric.

On 01/23, Tycho Andersen wrote:
>
> On Tue, Jan 23, 2024 at 08:56:08PM +0100, Oleg Nesterov wrote:
>
> > So. When do we want to do do_notify_pidfd() ? Whe the task (leader or not)
> > becomes a zombie (passes exit_notify) or when it is reaped by release_task?
>
> It seems like we'd want it when exit_notify() is called in principle,
> since that's when the pid actually dies.

No the pid "dies" after this task is reaped, until then its nr is still
in use and pid_task(PIDTYPE_PID) returns the exiting/exited task.

> When it is reaped is "mostly unrelated".

Then why pidfd_poll() can't simply check !task || task->exit_state ?

Nevermind. So, currently pidfd_poll() succeeds when the leader can be
reaped, iow the whole thread group has exited. But even if you are the
parent, you can't expect that wait(WNOHANG) must succeed, the leader
can be traced. I guess it is too late to change this behaviour.

What if we add the new PIDFD_THREAD flag? With this flag

	- sys_pidfd_open() doesn't require the must be a group leader

	- pidfd_poll() succeeds when the task passes exit_notify() and
	  becomes a zombie, even if it is a leader and has other threads.


Please the the incomplete/untested patch below.

	- The change in exit_notify() is sub-optimal, we can do better
	  to avoid 2 do_notify_pidfd() calls from exit_notify(). But
	  so far this is only for discussion, lets keep it simple.

	- __pidfd_prepare() needs some minor cleanups regardless of
	  this change, I'll send the patch...

What do you think?

And why is thread_group_exited() exported?

Oleg.

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	// or anything else not used by anon_inode's
 
 #endif /* _UAPI_LINUX_PIDFD_H */
diff --git a/kernel/exit.c b/kernel/exit.c
index dfb963d2f862..9f8526b7d717 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -752,6 +752,10 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
 		autoreap = true;
 	}
 
+	/* unnecessary if do_notify_parent() was already called,
+	   we can do better */
+	do_notify_pidfd(tsk);
+
 	if (autoreap) {
 		tsk->exit_state = EXIT_DEAD;
 		list_add(&tsk->ptrace_entry, &dead);
diff --git a/kernel/fork.c b/kernel/fork.c
index c981fa6171c1..38f2c7423fb4 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>
@@ -2068,12 +2069,27 @@ static void pidfd_show_fdinfo(struct seq_file *m, struct file *f)
 }
 #endif
 
+static bool xxx_exited(struct pid *pid, int excl)
+{
+	struct task_struct *task;
+	bool exited;
+
+	rcu_read_lock();
+	task = pid_task(pid, PIDTYPE_PID);
+	exited = !task ||
+		(READ_ONCE(task->exit_state) && (excl || 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;
+	int excl = file->f_flags & PIDFD_THREAD;
 	__poll_t poll_flags = 0;
 
 	poll_wait(file, &pid->wait_pidfd, pts);
@@ -2083,7 +2099,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 (xxx_exited(pid, excl))
 		poll_flags = EPOLLIN | EPOLLRDNORM;
 
 	return poll_flags;
@@ -2129,7 +2145,9 @@ static int __pidfd_prepare(struct pid *pid, unsigned int flags, struct file **re
 {
 	int pidfd;
 	struct file *pidfd_file;
+	unsigned excl = flags & PIDFD_THREAD;
 
+	flags &= ~PIDFD_THREAD;
 	if (flags & ~(O_NONBLOCK | O_RDWR | O_CLOEXEC))
 		return -EINVAL;
 
@@ -2144,6 +2162,7 @@ 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 */
+	pidfd_file->f_flags |= excl;
 	*ret = pidfd_file;
 	return pidfd;
 }
@@ -2176,7 +2195,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))
+	unsigned excl = flags & PIDFD_THREAD;
+
+	if (!pid || !pid_has_task(pid, excl ? PIDTYPE_PID : PIDTYPE_TGID))
 		return -EINVAL;
 
 	return __pidfd_prepare(pid, flags, ret);
diff --git a/kernel/pid.c b/kernel/pid.c
index b52b10865454..5257197f9493 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)

Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders

[Christian Brauner]

> > When it is reaped is "mostly unrelated".
> 
> Then why pidfd_poll() can't simply check !task || task->exit_state ?
> 
> Nevermind. So, currently pidfd_poll() succeeds when the leader can be

Hm, the comment right above mentions:

        /*
         * Inform pollers only when the whole thread group exits.
         * If the thread group leader exits before all other threads in the
         * group, then poll(2) should block, similar to the wait(2) family.
         */

> reaped, iow the whole thread group has exited. But even if you are the
> parent, you can't expect that wait(WNOHANG) must succeed, the leader
> can be traced. I guess it is too late to change this behaviour.

Hm, why is that an issue though? And if it is an issue why shouldn't we
be able to change it? Because a program would rely on WNOHANG to hang on
a ptraced leader? That seems esoteric imho. I might just misunderstand.

> 
> What if we add the new PIDFD_THREAD flag? With this flag
> 
> 	- sys_pidfd_open() doesn't require the must be a group leader

Yes.

> 
> 	- pidfd_poll() succeeds when the task passes exit_notify() and
> 	  becomes a zombie, even if it is a leader and has other threads.

Iiuc, if an existing user creates a pidfd for a thread-group leader and
then polls that pidfd they would currently only get notified if the
thread-group is empty and the leader has exited.

If we now start notifying when the thread-group leader exits but the
thread-group isn't empty then this would be a fairly big api change and
I would expect this to cause regressions as that surely is something
that userspace relies on. Am I understand this right?

Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders

[Oleg Nesterov]

On 01/25, Christian Brauner wrote:
>
> > > When it is reaped is "mostly unrelated".
> >
> > Then why pidfd_poll() can't simply check !task || task->exit_state ?
> >
> > Nevermind. So, currently pidfd_poll() succeeds when the leader can be
>
> Hm, the comment right above mentions:
>
>         /*
>          * Inform pollers only when the whole thread group exits.
>          * If the thread group leader exits before all other threads in the
>          * group, then poll(2) should block, similar to the wait(2) family.
>          */
> > reaped, iow the whole thread group has exited.

Yes, but the comment doesn't contradict with what I have said?


> > But even if you are the
> > parent, you can't expect that wait(WNOHANG) must succeed, the leader
> > can be traced. I guess it is too late to change this behaviour.
>
> Hm, why is that an issue though?

Well, I didn't say this is a problem. I simply do not know how/why people
use pidfd_poll().

I mostly tried to explain why do I think that do_notify_pidfd() should
be always called from exit_notify() path, not by release_task(), even
if the task is not a leader.

> Because a program would rely on WNOHANG to hang on
> a ptraced leader? That seems esoteric imho.

To me it would be usefule, but lets not discuss this now. The "patch"
I sent doesn't change the current behaviour.

> > What if we add the new PIDFD_THREAD flag? With this flag
> >
> > 	- sys_pidfd_open() doesn't require the must be a group leader
>
> Yes.
>
> >
> > 	- pidfd_poll() succeeds when the task passes exit_notify() and
> > 	  becomes a zombie, even if it is a leader and has other threads.
>
> Iiuc, if an existing user creates a pidfd for a thread-group leader and
> then polls that pidfd they would currently only get notified if the
> thread-group is empty and the leader has exited.
>
> If we now start notifying when the thread-group leader exits but the
> thread-group isn't empty then this would be a fairly big api change

Hmm... again, this patch doesn't (shouldn't) change the current behavior.

Please note "with this flag" above. If sys_pidfd_open() was called
without PIDFD_THREAD, then sys_pidfd_open() still requires that the
target task must be a group leader, and pidfd_poll() won't succeed
until the leader exits and thread_group_empty() is true.

Oleg.

Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders

[Tycho Andersen]

On Thu, Jan 25, 2024 at 06:51:14PM +0100, Oleg Nesterov wrote:
> > > What if we add the new PIDFD_THREAD flag? With this flag
> > >
> > > 	- sys_pidfd_open() doesn't require the must be a group leader
> >
> > Yes.
> >
> > >
> > > 	- pidfd_poll() succeeds when the task passes exit_notify() and
> > > 	  becomes a zombie, even if it is a leader and has other threads.
> >
> > Iiuc, if an existing user creates a pidfd for a thread-group leader and
> > then polls that pidfd they would currently only get notified if the
> > thread-group is empty and the leader has exited.
> >
> > If we now start notifying when the thread-group leader exits but the
> > thread-group isn't empty then this would be a fairly big api change
> 
> Hmm... again, this patch doesn't (shouldn't) change the current behavior.
> 
> Please note "with this flag" above. If sys_pidfd_open() was called
> without PIDFD_THREAD, then sys_pidfd_open() still requires that the
> target task must be a group leader, and pidfd_poll() won't succeed
> until the leader exits and thread_group_empty() is true.

Thanks for sending your patch, I'll take a look at it (probably
tomorrow at this rate).

One of the things I don't like about PIDFD_THREAD is that it's hard to
tell whether an arbitrary thread is a leader or not. Right now we do
it by parsing /proc/pid/status, which shows all the stuff from
do_task_stat() that we don't care about but which is quite expensive
to compute. (Maybe there's a better way?)

With PIDFD_THREAD we could could do it twice, once with the flag, get
EINVAL, and then do it again. But ideally we wouldn't have to.

Still, if that's the only way that makes sense, that's fine.

Tycho

Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders

[Oleg Nesterov]

On 01/25, Tycho Andersen wrote:
>
> One of the things I don't like about PIDFD_THREAD is that it's hard to
> tell whether an arbitrary thread is a leader or not. Right now we do
> it by parsing /proc/pid/status, which shows all the stuff from
> do_task_stat() that we don't care about but which is quite expensive
> to compute. (Maybe there's a better way?)
>
> With PIDFD_THREAD we could could do it twice, once with the flag, get
> EINVAL, and then do it again. But ideally we wouldn't have to.

Too late for me, most probably I misunderstood.

If you want the PIDFD_THREAD behaviour, you can always use this flag
without any check...

Could you spell?

Oleg.

Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders

[Oleg Nesterov]

On 01/25, Oleg Nesterov wrote:
>
> On 01/25, Tycho Andersen wrote:
> >
> > One of the things I don't like about PIDFD_THREAD is that it's hard to
> > tell whether an arbitrary thread is a leader or not. Right now we do
> > it by parsing /proc/pid/status, which shows all the stuff from
> > do_task_stat() that we don't care about but which is quite expensive
> > to compute. (Maybe there's a better way?)
> >
> > With PIDFD_THREAD we could could do it twice, once with the flag, get
> > EINVAL, and then do it again. But ideally we wouldn't have to.
>
> Too late for me, most probably I misunderstood.
>
> If you want the PIDFD_THREAD behaviour, you can always use this flag
> without any check...
>
> Could you spell?

Just in case, we can even add PIDFD_AUTO (modulo naming) which acts as
PIDFD_THREAD if the target task is not a leader or 0 (current behaviour)
otherwise. Trivial.

Oleg.

Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders

[Tycho Andersen]

On Thu, Jan 25, 2024 at 07:30:46PM +0100, Oleg Nesterov wrote:
> On 01/25, Oleg Nesterov wrote:
> >
> > On 01/25, Tycho Andersen wrote:
> > >
> > > One of the things I don't like about PIDFD_THREAD is that it's hard to
> > > tell whether an arbitrary thread is a leader or not. Right now we do
> > > it by parsing /proc/pid/status, which shows all the stuff from
> > > do_task_stat() that we don't care about but which is quite expensive
> > > to compute. (Maybe there's a better way?)
> > >
> > > With PIDFD_THREAD we could could do it twice, once with the flag, get
> > > EINVAL, and then do it again. But ideally we wouldn't have to.
> >
> > Too late for me, most probably I misunderstood.
> >
> > If you want the PIDFD_THREAD behaviour, you can always use this flag
> > without any check...

Sorry, I hadn't read the patch. If it's ok to use PIDFD_THREAD on a
leader, then we can just always specify it. (We don't care about the
behavior of pidfd_poll().)

> > Could you spell?
> 
> Just in case, we can even add PIDFD_AUTO (modulo naming) which acts as
> PIDFD_THREAD if the target task is not a leader or 0 (current behaviour)
> otherwise. Trivial.

Yep, or given the above, maybe it'll work as-is, thank you.

Tycho

Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders

[Christian Brauner]

On Thu, Jan 25, 2024 at 11:36:50AM -0700, Tycho Andersen wrote:
> On Thu, Jan 25, 2024 at 07:30:46PM +0100, Oleg Nesterov wrote:
> > On 01/25, Oleg Nesterov wrote:
> > >
> > > On 01/25, Tycho Andersen wrote:
> > > >
> > > > One of the things I don't like about PIDFD_THREAD is that it's hard to
> > > > tell whether an arbitrary thread is a leader or not. Right now we do
> > > > it by parsing /proc/pid/status, which shows all the stuff from
> > > > do_task_stat() that we don't care about but which is quite expensive
> > > > to compute. (Maybe there's a better way?)
> > > >
> > > > With PIDFD_THREAD we could could do it twice, once with the flag, get
> > > > EINVAL, and then do it again. But ideally we wouldn't have to.
> > >
> > > Too late for me, most probably I misunderstood.
> > >
> > > If you want the PIDFD_THREAD behaviour, you can always use this flag
> > > without any check...
> 
> Sorry, I hadn't read the patch. If it's ok to use PIDFD_THREAD on a
> leader, then we can just always specify it. (We don't care about the
> behavior of pidfd_poll().)
> 
> > > Could you spell?
> > 
> > Just in case, we can even add PIDFD_AUTO (modulo naming) which acts as
> > PIDFD_THREAD if the target task is not a leader or 0 (current behaviour)
> > otherwise. Trivial.
> 
> Yep, or given the above, maybe it'll work as-is, thank you.

Yes, let's rather do the explicit PIDFD_THREAD.

Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders

[Christian Brauner]

On Thu, Jan 25, 2024 at 06:51:14PM +0100, Oleg Nesterov wrote:
> On 01/25, Christian Brauner wrote:
> >
> > > > When it is reaped is "mostly unrelated".
> > >
> > > Then why pidfd_poll() can't simply check !task || task->exit_state ?
> > >
> > > Nevermind. So, currently pidfd_poll() succeeds when the leader can be
> >
> > Hm, the comment right above mentions:
> >
> >         /*
> >          * Inform pollers only when the whole thread group exits.
> >          * If the thread group leader exits before all other threads in the
> >          * group, then poll(2) should block, similar to the wait(2) family.
> >          */
> > > reaped, iow the whole thread group has exited.
> 
> Yes, but the comment doesn't contradict with what I have said?

No, it doesn't. I'm trying to understand what you are suggesting though.
Are you saying !task || tas->exit_state is enough and we shouldn't use
the helper that was added in commit 38fd525a4c61 ("exit: Factor
thread_group_exited out of pidfd_poll"). If so what does that buy us
open-coding the check instead of using that helper? Is there an actual
bug here?

> > > But even if you are the
> > > parent, you can't expect that wait(WNOHANG) must succeed, the leader
> > > can be traced. I guess it is too late to change this behaviour.
> >
> > Hm, why is that an issue though?
> 
> Well, I didn't say this is a problem. I simply do not know how/why people
> use pidfd_poll().

Sorry, I just have a hard time understanding what you wanted then. :)

"I guess it is too late to change this behavior." made it sound like a)
there's a problem and b) that you would prefer to change behavior. Thus,
it seems that wait(WNOHANG) hanging when a traced leader of an empty
thread-group has exited is a problem in your eyes.

> 
> I mostly tried to explain why do I think that do_notify_pidfd() should
> be always called from exit_notify() path, not by release_task(), even
> if the task is not a leader.
> 
> > Because a program would rely on WNOHANG to hang on
> > a ptraced leader? That seems esoteric imho.
> 
> To me it would be usefule, but lets not discuss this now. The "patch"

Ok, that's good then. I would expect that at least stuff like rr makes
use of pidfd and they might rely on this behavior - although I haven't
checked their code.

> I sent doesn't change the current behaviour.

Yeah, I got that but it would still be useful to understand the wider
context you were adressing.

> 
> > > What if we add the new PIDFD_THREAD flag? With this flag
> > >
> > > 	- sys_pidfd_open() doesn't require the must be a group leader
> >
> > Yes.
> >
> > >
> > > 	- pidfd_poll() succeeds when the task passes exit_notify() and
> > > 	  becomes a zombie, even if it is a leader and has other threads.
> >
> > Iiuc, if an existing user creates a pidfd for a thread-group leader and
> > then polls that pidfd they would currently only get notified if the
> > thread-group is empty and the leader has exited.
> >
> > If we now start notifying when the thread-group leader exits but the
> > thread-group isn't empty then this would be a fairly big api change
> 
> Hmm... again, this patch doesn't (shouldn't) change the current behavior.
> 
> Please note "with this flag" above. If sys_pidfd_open() was called
> without PIDFD_THREAD, then sys_pidfd_open() still requires that the
> target task must be a group leader, and pidfd_poll() won't succeed
> until the leader exits and thread_group_empty() is true.

Yeah, I missed the PIDFD_THREAD flag suggestion. Sorry about that. Btw,
I'm not sure whether you remember that but when we originally did the
pidfd work you and I discussed thread support and already decided back
then that having a flag like PIDFD_THREAD would likely be the way to go.

The PIDFD_THREAD flag would be would be interesting because we could
make pidfd_send_signal() support this flag as well to allow sending a
signal to a specific thread. That's something that I had also wanted to
support. And I've been asked for this a few times already. What do you
think?

Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders

[Oleg Nesterov]

On 01/26, Christian Brauner wrote:
>
> No, it doesn't. I'm trying to understand what you are suggesting though.
> Are you saying !task || tas->exit_state is enough

If PIDFD_THREAD then I think it is enough. Otherwise we still need
!task || (exit_state && thread_group_empty)

> and we shouldn't use
> the helper that was added in commit 38fd525a4c61 ("exit: Factor
> thread_group_exited out of pidfd_poll"). If so what does that buy us
> open-coding the check instead of using that helper? Is there an actual
> bug here?

The patch adds the new xxx_exited(task, excl) helper which checks

	!task || (exit_state && (excl || thread_group_empty))

yes, the naming is not good.

> > Well, I didn't say this is a problem. I simply do not know how/why people
> > use pidfd_poll().
>
> Sorry, I just have a hard time understanding what you wanted then. :)
>
> "I guess it is too late to change this behavior." made it sound like a)
> there's a problem and b) that you would prefer to change behavior. Thus,
> it seems that wait(WNOHANG) hanging when a traced leader of an empty
> thread-group has exited is a problem in your eyes.

Again, I mostly tried to argue with do_notify_pidfd() called by realese_task().

I think that with PIDFD_THREAD set pidfd_poll() should succeed right
after the exiting thread becomes a zombie (passes exit_notify), whether
it is a leader or not.

Let me quote part of my reply to Tycho's patch

	> +	/*
	> +	 * If we're not the leader, notify any waiters on our pidfds. Note that
	> +	 * we don't want to notify the leader until /everyone/ in the thread
	> +	 * group is dead, viz. the condition below.
	> +	 *
	> +	 * We have to do this here, since __exit_signal() will
	> +	 * __unhash_processes(), and break do_notify_pidfd()'s lookup.
	> +	 */
	> +	if (!thread_group_leader(p))
	> +		do_notify_pidfd(p);

	This doesn't look consistent.

	If the task is a group leader do_notify_pidfd() is called by exit_notify()
	when it becomes a zombie (if no other threads), before it is reaped by its
	parent (unless autoreap).

	If it is a sub-thread, it is called by release_task() above. Note that a
	sub-thread can become a zombie too if it is traced.

Not to mention that this is racy.

I would not mind if we simply move do_notify_pidfd() from exit_notify() to
release_task() and do it regardless of thread_group_leader(). And in some
sense this looks more logical to me.

But as I said:

	- I do not know how/why people actually use poll(pidfd)

	- it is too late to change the current behaviour

Sorry for confusion.

> I'm not sure whether you remember that but when we originally did the
> pidfd work you and I discussed thread support and already decided back
> then that having a flag like PIDFD_THREAD would likely be the way to go.

All I can recall is that, yes, we had some discussions about pidfd in
the past ;)

> The PIDFD_THREAD flag would be would be interesting because we could
> make pidfd_send_signal() support this flag

Agreed,

Oleg.

Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders

[Christian Brauner]

> Please the the incomplete/untested patch below.
> 
> 	- The change in exit_notify() is sub-optimal, we can do better
> 	  to avoid 2 do_notify_pidfd() calls from exit_notify(). But
> 	  so far this is only for discussion, lets keep it simple.
> 
> 	- __pidfd_prepare() needs some minor cleanups regardless of
> 	  this change, I'll send the patch...
> 
> What do you think?
> 
> And why is thread_group_exited() exported?
> 
> Oleg.
> 
> 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	// or anything else not used by anon_inode's

I like it!

The only request I would have is to not alias O_EXCL and PIDFD_THREAD.
Because it doesn't map as clearly as NONBLOCK did.

>  
>  #endif /* _UAPI_LINUX_PIDFD_H */
> diff --git a/kernel/exit.c b/kernel/exit.c
> index dfb963d2f862..9f8526b7d717 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -752,6 +752,10 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
>  		autoreap = true;
>  	}
>  
> +	/* unnecessary if do_notify_parent() was already called,
> +	   we can do better */
> +	do_notify_pidfd(tsk);
> +
>  	if (autoreap) {
>  		tsk->exit_state = EXIT_DEAD;
>  		list_add(&tsk->ptrace_entry, &dead);
> diff --git a/kernel/fork.c b/kernel/fork.c
> index c981fa6171c1..38f2c7423fb4 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>
> @@ -2068,12 +2069,27 @@ static void pidfd_show_fdinfo(struct seq_file *m, struct file *f)
>  }
>  #endif
>  
> +static bool xxx_exited(struct pid *pid, int excl)
> +{
> +	struct task_struct *task;
> +	bool exited;
> +
> +	rcu_read_lock();
> +	task = pid_task(pid, PIDTYPE_PID);
> +	exited = !task ||
> +		(READ_ONCE(task->exit_state) && (excl || 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;
> +	int excl = file->f_flags & PIDFD_THREAD;
>  	__poll_t poll_flags = 0;
>  
>  	poll_wait(file, &pid->wait_pidfd, pts);
> @@ -2083,7 +2099,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 (xxx_exited(pid, excl))
>  		poll_flags = EPOLLIN | EPOLLRDNORM;
>  
>  	return poll_flags;
> @@ -2129,7 +2145,9 @@ static int __pidfd_prepare(struct pid *pid, unsigned int flags, struct file **re
>  {
>  	int pidfd;
>  	struct file *pidfd_file;
> +	unsigned excl = flags & PIDFD_THREAD;
>  
> +	flags &= ~PIDFD_THREAD;
>  	if (flags & ~(O_NONBLOCK | O_RDWR | O_CLOEXEC))
>  		return -EINVAL;
>  
> @@ -2144,6 +2162,7 @@ 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 */
> +	pidfd_file->f_flags |= excl;
>  	*ret = pidfd_file;
>  	return pidfd;
>  }
> @@ -2176,7 +2195,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))
> +	unsigned excl = flags & PIDFD_THREAD;
> +
> +	if (!pid || !pid_has_task(pid, excl ? PIDTYPE_PID : PIDTYPE_TGID))
>  		return -EINVAL;
>  
>  	return __pidfd_prepare(pid, flags, ret);
> diff --git a/kernel/pid.c b/kernel/pid.c
> index b52b10865454..5257197f9493 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)
> 

Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders

[Oleg Nesterov]

On 01/26, Christian Brauner wrote:
>
> > --- 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	// or anything else not used by anon_inode's
>
> I like it!
>
> The only request I would have is to not alias O_EXCL and PIDFD_THREAD.
> Because it doesn't map as clearly as NONBLOCK did.

But it would be nice to have PIDFD_THREAD in file->f_flags. Where else
can we keep it?

I chose O_EXCL because it can only be used at open time, it can never
be used or changed after anon_inode_getfile(), so we can safely do

	pidfd_file->f_flags |= PIDFD_THREAD;

in __pidfd_prepare() and then check in pidfd_poll/pidfd_send_signal.

What do you suggest instead?

Oleg.

Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders

[Christian Brauner]

On Fri, Jan 26, 2024 at 03:33:50PM +0100, Oleg Nesterov wrote:
> On 01/26, Christian Brauner wrote:
> >
> > > --- 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	// or anything else not used by anon_inode's
> >
> > I like it!
> >
> > The only request I would have is to not alias O_EXCL and PIDFD_THREAD.
> > Because it doesn't map as clearly as NONBLOCK did.
> 
> But it would be nice to have PIDFD_THREAD in file->f_flags. Where else
> can we keep it?

No, I did just mean that the uapi value doesn't necessarily have to
reflect what we do internally. IOW, we can still raise O_EXCL internally
in ->f_flags but there's no need to expose it as O_EXCL to userspace. We
often have internal and external flag spaces. If you prefer it your way
I'm not going argue.

> I chose O_EXCL because it can only be used at open time, it can never
> be used or changed after anon_inode_getfile(), so we can safely do
> 
> 	pidfd_file->f_flags |= PIDFD_THREAD;
> 
> in __pidfd_prepare() and then check in pidfd_poll/pidfd_send_signal.
> 
> What do you suggest instead?

(Long-term and unrelated to this here, I think we will need to consider
not just stashing struct pid in pidfd_file->private_data but instead
struct pid with additional data because there's various functionality
that users would like that requires additional state to be stored and we
can't or don't want to do that in struct pid directly.)

Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders

[Tycho Andersen]

Hi Oleg,

On Thu, Jan 25, 2024 at 03:08:31PM +0100, Oleg Nesterov wrote:
> What do you think?

Thank you, it passes all my tests.

> +	/* unnecessary if do_notify_parent() was already called,
> +	   we can do better */
> +	do_notify_pidfd(tsk);

"do better" here could be something like,

diff --git a/kernel/exit.c b/kernel/exit.c
index efe8f1d3a6af..7e545393f2f5 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -742,6 +742,7 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
 	bool autoreap;
 	struct task_struct *p, *n;
 	LIST_HEAD(dead);
+	bool needs_notify = true;
 
 	write_lock_irq(&tasklist_lock);
 	forget_original_parent(tsk, &dead);
@@ -756,16 +757,21 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
 				!ptrace_reparented(tsk) ?
 			tsk->exit_signal : SIGCHLD;
 		autoreap = do_notify_parent(tsk, sig);
+		needs_notify = false;
 	} else if (thread_group_leader(tsk)) {
-		autoreap = thread_group_empty(tsk) &&
-			do_notify_parent(tsk, tsk->exit_signal);
+		autoreap = false;
+		if (thread_group_empty(tsk)) {
+			autoreap = do_notify_parent(tsk, tsk->exit_signal);
+			needs_notify = false;
+		}
 	} else {
 		autoreap = true;
 	}
 
 	/* unnecessary if do_notify_parent() was already called,
 	   we can do better */
-	do_notify_pidfd(tsk);
+	if (needs_notify)
+		do_notify_pidfd(tsk);
 
 	if (autoreap) {
 		tsk->exit_state = EXIT_DEAD;


but even with that, there's other calls in the tree to
do_notify_parent() that might double notify.

This brings up another interesting behavior that I noticed while
testing this, if you do a poll() on pidfd, followed quickly by a
pidfd_getfd() on the same thread you just got an event on, you can
sometimes get an EBADF from __pidfd_fget() instead of the more
expected ESRCH higher up the stack.

I wonder if it makes sense to abuse ->f_flags to add a PIDFD_NOTIFIED?
Then we can refuse further pidfd syscall operations in a sane way, and
also "do better" above by checking this flag from do_pidfd_notify()
before doing it again?

Tycho

Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders

[Oleg Nesterov]

Hi Tycho,

On 01/26, Tycho Andersen wrote:
>
> On Thu, Jan 25, 2024 at 03:08:31PM +0100, Oleg Nesterov wrote:
> > What do you think?
>
> Thank you, it passes all my tests.

Great, thanks!

OK, I'll make v2 on top of the recent
"pidfd: cleanup the usage of __pidfd_prepare's flags"

but we need to finish our discussion with Christian about the
usage of O_EXCL.

As for clone(CLONE_PIDFD | CLONE_THREAD), this is trivial but
I think this needs another discussion too, lets do this later.

> > +	/* unnecessary if do_notify_parent() was already called,
> > +	   we can do better */
> > +	do_notify_pidfd(tsk);
>
> "do better" here could be something like,
>
> [...snip...]

No, no, please see below.

For the moment, please forget about PIDFD_THREAD, lets discuss
the current behaviour.

> but even with that, there's other calls in the tree to
> do_notify_parent() that might double notify.

Yes, and we can't avoid this. Well, perhaps do_notify_parent()
can do something like

	if (ptrace_reparented())
		do_notify_pidfd();

so that only the "final" do_notify_parent() does do_notify_pidfd()
but this needs another discussion and in fact I don't think this
would be right or make much sense. Lets forget this for now.

Now. Even without PIDFD_THREAD, I think it makes sense to change
do_notify_parent() to do

	if (thread_group_empty(tsk))
		do_notify_pidfd(tsk);

thread_group_empty(tsk) can only be true if tsk is a group leader
and it is the last thread. And this is exactly what pidfd_poll()
currently needs.

In fact I'd even prefer to do this in a separate patch for the
documentation purposes.

Now, PIDFD_THREAD can just add

	if (!thread_group_empty(tsk))
		do_notify_pidfd(tsk);

right after "tsk->exit_state = EXIT_ZOMBIE", that is all.

This also preserves the do_notify_pidfd/__wake_up_parent ordering.
Not that I think this is important, just for consistency.

> This brings up another interesting behavior that I noticed while
> testing this, if you do a poll() on pidfd, followed quickly by a
> pidfd_getfd() on the same thread you just got an event on, you can
> sometimes get an EBADF from __pidfd_fget() instead of the more
> expected ESRCH higher up the stack.

exit_notify() is called after exit_files(). pidfd_getfd() returns
ESRCH if the exiting thread completes release_task(), otherwise it
returns EBADF because ->files == NULL. This too doesn't really
depend on PIDFD_THREAD.

> I wonder if it makes sense to abuse ->f_flags to add a PIDFD_NOTIFIED?
> Then we can refuse further pidfd syscall operations in a sane way, and

But how? We only have "struct pid *", how can we find all files
"attached" to this pid?

> also "do better" above by checking this flag from do_pidfd_notify()
> before doing it again?

and even it was possible, I don't think it makes a lot of sense, see
also above.

but perhaps I understood you...

Oleg.

Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders

[Christian Brauner]

On Sat, Jan 27, 2024 at 11:54:32AM +0100, Oleg Nesterov wrote:
> Hi Tycho,
> 
> On 01/26, Tycho Andersen wrote:
> >
> > On Thu, Jan 25, 2024 at 03:08:31PM +0100, Oleg Nesterov wrote:
> > > What do you think?
> >
> > Thank you, it passes all my tests.
> 
> Great, thanks!
> 
> OK, I'll make v2 on top of the recent
> "pidfd: cleanup the usage of __pidfd_prepare's flags"
> 
> but we need to finish our discussion with Christian about the
> usage of O_EXCL.

Just write the patch exactly like you want. If it's as a little a detail
as the uapi flag value we can just always change that when applying.
There's no reason to introduce artificial delays because of my
preference here..

> 
> As for clone(CLONE_PIDFD | CLONE_THREAD), this is trivial but
> I think this needs another discussion too, lets do this later.
> 
> > > +	/* unnecessary if do_notify_parent() was already called,
> > > +	   we can do better */
> > > +	do_notify_pidfd(tsk);
> >
> > "do better" here could be something like,
> >
> > [...snip...]
> 
> No, no, please see below.
> 
> For the moment, please forget about PIDFD_THREAD, lets discuss
> the current behaviour.
> 
> > but even with that, there's other calls in the tree to
> > do_notify_parent() that might double notify.
> 
> Yes, and we can't avoid this. Well, perhaps do_notify_parent()
> can do something like
> 
> 	if (ptrace_reparented())
> 		do_notify_pidfd();
> 
> so that only the "final" do_notify_parent() does do_notify_pidfd()
> but this needs another discussion and in fact I don't think this
> would be right or make much sense. Lets forget this for now.
> 
> Now. Even without PIDFD_THREAD, I think it makes sense to change
> do_notify_parent() to do
> 
> 	if (thread_group_empty(tsk))
> 		do_notify_pidfd(tsk);
> 
> thread_group_empty(tsk) can only be true if tsk is a group leader
> and it is the last thread. And this is exactly what pidfd_poll()
> currently needs.
> 
> In fact I'd even prefer to do this in a separate patch for the
> documentation purposes.
> 
> Now, PIDFD_THREAD can just add
> 
> 	if (!thread_group_empty(tsk))
> 		do_notify_pidfd(tsk);
> 
> right after "tsk->exit_state = EXIT_ZOMBIE", that is all.

Sounds good.

> 
> This also preserves the do_notify_pidfd/__wake_up_parent ordering.
> Not that I think this is important, just for consistency.
> 
> > This brings up another interesting behavior that I noticed while
> > testing this, if you do a poll() on pidfd, followed quickly by a
> > pidfd_getfd() on the same thread you just got an event on, you can
> > sometimes get an EBADF from __pidfd_fget() instead of the more
> > expected ESRCH higher up the stack.
> 
> exit_notify() is called after exit_files(). pidfd_getfd() returns
> ESRCH if the exiting thread completes release_task(), otherwise it
> returns EBADF because ->files == NULL. This too doesn't really
> depend on PIDFD_THREAD.
> 
> > I wonder if it makes sense to abuse ->f_flags to add a PIDFD_NOTIFIED?
> > Then we can refuse further pidfd syscall operations in a sane way, and
> 
> But how? We only have "struct pid *", how can we find all files
> "attached" to this pid?

We can't. There's some use-cases that would make this desirable but that
would mean we would need another data structure to track this. I once
toyed with a patch for this but never got so excited about it to
actually finish it.

Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders

[Tycho Andersen]

On Sat, Jan 27, 2024 at 11:54:32AM +0100, Oleg Nesterov wrote:
> Hi Tycho,
> 
> On 01/26, Tycho Andersen wrote:
> >
> > On Thu, Jan 25, 2024 at 03:08:31PM +0100, Oleg Nesterov wrote:
> > > What do you think?
> >
> > Thank you, it passes all my tests.
> 
> Great, thanks!
> 
> OK, I'll make v2 on top of the recent
> "pidfd: cleanup the usage of __pidfd_prepare's flags"
> 
> but we need to finish our discussion with Christian about the
> usage of O_EXCL.
> 
> As for clone(CLONE_PIDFD | CLONE_THREAD), this is trivial but
> I think this needs another discussion too, lets do this later.
> 
> > > +	/* unnecessary if do_notify_parent() was already called,
> > > +	   we can do better */
> > > +	do_notify_pidfd(tsk);
> >
> > "do better" here could be something like,
> >
> > [...snip...]
> 
> No, no, please see below.
> 
> For the moment, please forget about PIDFD_THREAD, lets discuss
> the current behaviour.
> 
> > but even with that, there's other calls in the tree to
> > do_notify_parent() that might double notify.
> 
> Yes, and we can't avoid this. Well, perhaps do_notify_parent()
> can do something like
> 
> 	if (ptrace_reparented())
> 		do_notify_pidfd();
> 
> so that only the "final" do_notify_parent() does do_notify_pidfd()
> but this needs another discussion and in fact I don't think this
> would be right or make much sense. Lets forget this for now.

It seems like (and the current pidfd_test enforces for some cases) we
want exactly one notification for a task dying. I don't understand
how we guarantee this now, with all of these calls.

> > This brings up another interesting behavior that I noticed while
> > testing this, if you do a poll() on pidfd, followed quickly by a
> > pidfd_getfd() on the same thread you just got an event on, you can
> > sometimes get an EBADF from __pidfd_fget() instead of the more
> > expected ESRCH higher up the stack.
> 
> exit_notify() is called after exit_files(). pidfd_getfd() returns
> ESRCH if the exiting thread completes release_task(), otherwise it
> returns EBADF because ->files == NULL. This too doesn't really
> depend on PIDFD_THREAD.

Yup, understood. It just seems like an inconsistency we might want to
fix.

> > I wonder if it makes sense to abuse ->f_flags to add a PIDFD_NOTIFIED?
> > Then we can refuse further pidfd syscall operations in a sane way, and
> 
> But how? We only have "struct pid *", how can we find all files
> "attached" to this pid?

Yeah, we'd need some other linkage as Christian points out. But if
there is a predicate we can write that says whether this task has been
notified or not, it's not necessary. I just don't understand what that
is. But maybe your patch will make it clearer.

Tycho

Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders

[Oleg Nesterov]

On 01/27, Tycho Andersen wrote:
>
> It seems like (and the current pidfd_test enforces for some cases)

Which pidfd_test ?

> we
> want exactly one notification for a task dying.

This can't be right. EVERY user of poll_wait() or wait_event/etc
must handle/tolerate the false wakeups.

> I don't understand
> how we guarantee this now, with all of these calls.

I don't understand why do we need or even want to guarantee this.

The extra wakeup must be always fine correctness-wise. Sure, it
would be nice to avoid the unnecessary wakeups, and perhaps we
can change wake_up_all() to pass a key to, say, only wake_up the
PIDFD_THREAD waiters from exit_notify(). But certainly this is
outside the scope of PIDFD_THREAD change we discuss.

The changes in do_notify_parent() (I have already sent the patch) and
in exit_notify() (proposed in my previous email) just ensure that,
with the minimal changes, we avoid 2 do_notify_pidfd's from the same
exit_notify() path.

> > exit_notify() is called after exit_files(). pidfd_getfd() returns
> > ESRCH if the exiting thread completes release_task(), otherwise it
> > returns EBADF because ->files == NULL. This too doesn't really
> > depend on PIDFD_THREAD.
>
> Yup, understood. It just seems like an inconsistency we might want to
> fix.

Not sure this worth "fixing"...

Oleg.

Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders

[Tycho Andersen]

On Sat, Jan 27, 2024 at 05:31:39PM +0100, Oleg Nesterov wrote:
> On 01/27, Tycho Andersen wrote:
> >
> > It seems like (and the current pidfd_test enforces for some cases)
> 
> Which pidfd_test ?

I was thinking of poll_pidfd() in pidfd_test.c, but,

> > we
> > want exactly one notification for a task dying.
> 
> This can't be right. EVERY user of poll_wait() or wait_event/etc
> must handle/tolerate the false wakeups.

you're right, it doesn't enforce exactly once.

> > I don't understand
> > how we guarantee this now, with all of these calls.
> 
> I don't understand why do we need or even want to guarantee this.
> 
> The extra wakeup must be always fine correctness-wise. Sure, it
> would be nice to avoid the unnecessary wakeups, and perhaps we
> can change wake_up_all() to pass a key to, say, only wake_up the
> PIDFD_THREAD waiters from exit_notify(). But certainly this is
> outside the scope of PIDFD_THREAD change we discuss.
> 
> The changes in do_notify_parent() (I have already sent the patch) and
> in exit_notify() (proposed in my previous email) just ensure that,
> with the minimal changes, we avoid 2 do_notify_pidfd's from the same
> exit_notify() path.

Sounds good.

> > > exit_notify() is called after exit_files(). pidfd_getfd() returns
> > > ESRCH if the exiting thread completes release_task(), otherwise it
> > > returns EBADF because ->files == NULL. This too doesn't really
> > > depend on PIDFD_THREAD.
> >
> > Yup, understood. It just seems like an inconsistency we might want to
> > fix.
> 
> Not sure this worth "fixing"...

Yep, maybe not. Just wanted to point it out.

Tycho

Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders

[Oleg Nesterov]

On 01/27, Tycho Andersen wrote:
>
> > > > exit_notify() is called after exit_files(). pidfd_getfd() returns
> > > > ESRCH if the exiting thread completes release_task(), otherwise it
> > > > returns EBADF because ->files == NULL. This too doesn't really
> > > > depend on PIDFD_THREAD.
> > >
> > > Yup, understood. It just seems like an inconsistency we might want to
> > > fix.
> >
> > Not sure this worth "fixing"...
>
> Yep, maybe not. Just wanted to point it out.

On the second thought I am starting to understand your concern...

Indeed, in this case -EBADF is technically correct but it can confuse
the user which doesn't or can't know that this task/thread is exiting,
because EBADF looks as if the "int fd" argument was wrong.

Sorry I missed your point before.

Oleg.

Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders

[Tycho Andersen]

On Sat, Jan 27, 2024 at 08:31:27PM +0100, Oleg Nesterov wrote:
> On 01/27, Tycho Andersen wrote:
> >
> > > > > exit_notify() is called after exit_files(). pidfd_getfd() returns
> > > > > ESRCH if the exiting thread completes release_task(), otherwise it
> > > > > returns EBADF because ->files == NULL. This too doesn't really
> > > > > depend on PIDFD_THREAD.
> > > >
> > > > Yup, understood. It just seems like an inconsistency we might want to
> > > > fix.
> > >
> > > Not sure this worth "fixing"...
> >
> > Yep, maybe not. Just wanted to point it out.
> 
> On the second thought I am starting to understand your concern...
> 
> Indeed, in this case -EBADF is technically correct but it can confuse
> the user which doesn't or can't know that this task/thread is exiting,
> because EBADF looks as if the "int fd" argument was wrong.
> 
> Sorry I missed your point before.

No worries. I realized it's not so hard to fix with your new
xxx_exited() helper from the PIDFD_THREAD patch, so maybe worth
cleaning up after all?

Tycho

Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders

[Oleg Nesterov]

On 01/27, Tycho Andersen wrote:
>
> On Sat, Jan 27, 2024 at 08:31:27PM +0100, Oleg Nesterov wrote:
> >
> > On the second thought I am starting to understand your concern...
> >
> > Indeed, in this case -EBADF is technically correct but it can confuse
> > the user which doesn't or can't know that this task/thread is exiting,
> > because EBADF looks as if the "int fd" argument was wrong.
> >
> > Sorry I missed your point before.
>
> No worries. I realized it's not so hard to fix with your new
> xxx_exited() helper from the PIDFD_THREAD patch, so maybe worth
> cleaning up after all?

OK, lets discuss this later.

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, will be busy tomorrow (family duties ;)

Oleg.

[RFC PATCH] pidfd: implement PIDFD_THREAD flag for pidfd_open()

[Oleg Nesterov]

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

Re: [RFC PATCH] pidfd: implement PIDFD_THREAD flag for pidfd_open()

[Christian Brauner]

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!

Re: [RFC PATCH] pidfd: implement PIDFD_THREAD flag for pidfd_open()

[Tycho Andersen]

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

Re: [RFC PATCH] pidfd: implement PIDFD_THREAD flag for pidfd_open()

[Christian Brauner]

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.

Re: [RFC PATCH] pidfd: implement PIDFD_THREAD flag for pidfd_open()

[Oleg Nesterov]

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.

Re: [RFC PATCH] pidfd: implement PIDFD_THREAD flag for pidfd_open()

[Andy Lutomirski]

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

Re: [RFC PATCH] pidfd: implement PIDFD_THREAD flag for pidfd_open()

[Oleg Nesterov]

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.

Re: [RFC PATCH] pidfd: implement PIDFD_THREAD flag for pidfd_open()

[Oleg Nesterov]

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.

Re: [RFC PATCH] pidfd: implement PIDFD_THREAD flag for pidfd_open()

[Andy Lutomirski]

> 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

Re: [RFC PATCH] pidfd: implement PIDFD_THREAD flag for pidfd_open()

[Christian Brauner]

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?

Re: [RFC PATCH] pidfd: implement PIDFD_THREAD flag for pidfd_open()

[Andy Lutomirski]

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

Re: [RFC PATCH] pidfd: implement PIDFD_THREAD flag for pidfd_open()

[Christian Brauner]

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.

Re: [RFC PATCH] pidfd: implement PIDFD_THREAD flag for pidfd_open()

[Christian Brauner]

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?

Re: [RFC PATCH] pidfd: implement PIDFD_THREAD flag for pidfd_open()

[Andy Lutomirski]

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.

[PATCH v3 2/3] selftests/pidfd: add non-thread-group leader tests

[Tycho Andersen]

From: Tycho Andersen <tandersen@netflix.com>

This adds a family of tests for various behaviors for pidfds of
non-thread-group leaders.

Signed-off-by: Tycho Andersen <tandersen@netflix.com>
---
v2: unchanged
v3: add some tests for the situation that Christian described in one of the
    previous threads, see the comments for a link + details.
---
 tools/testing/selftests/pidfd/.gitignore      |   1 +
 tools/testing/selftests/pidfd/Makefile        |   3 +-
 .../selftests/pidfd/pidfd_non_tgl_test.c      | 583 ++++++++++++++++++
 3 files changed, 586 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/pidfd/.gitignore b/tools/testing/selftests/pidfd/.gitignore
index 973198a3ec3d..e7532e84a34a 100644
--- a/tools/testing/selftests/pidfd/.gitignore
+++ b/tools/testing/selftests/pidfd/.gitignore
@@ -6,3 +6,4 @@ pidfd_wait
 pidfd_fdinfo_test
 pidfd_getfd_test
 pidfd_setns_test
+pidfd_non_tgl_test
diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile
index d731e3e76d5b..50e3aa9de05a 100644
--- a/tools/testing/selftests/pidfd/Makefile
+++ b/tools/testing/selftests/pidfd/Makefile
@@ -2,7 +2,8 @@
 CFLAGS += -g $(KHDR_INCLUDES) -pthread -Wall
 
 TEST_GEN_PROGS := pidfd_test pidfd_fdinfo_test pidfd_open_test \
-	pidfd_poll_test pidfd_wait pidfd_getfd_test pidfd_setns_test
+	pidfd_poll_test pidfd_wait pidfd_getfd_test pidfd_setns_test \
+	pidfd_non_tgl_test
 
 include ../lib.mk
 
diff --git a/tools/testing/selftests/pidfd/pidfd_non_tgl_test.c b/tools/testing/selftests/pidfd/pidfd_non_tgl_test.c
new file mode 100644
index 000000000000..c0624b127fab
--- /dev/null
+++ b/tools/testing/selftests/pidfd/pidfd_non_tgl_test.c
@@ -0,0 +1,583 @@
+// SPDX-License-Identifier: GPL-2.0
+#define _GNU_SOURCE
+#include <sys/socket.h>
+#include <limits.h>
+#include <string.h>
+#include <signal.h>
+#include <syscall.h>
+#include <sched.h>
+#include <poll.h>
+
+#include "../kselftest.h"
+#include "pidfd.h"
+
+static int sys_waitid(int which, pid_t pid, siginfo_t *info, int options,
+		      struct rusage *ru)
+{
+	return syscall(__NR_waitid, which, pid, info, options, ru);
+}
+
+// glibc defaults to 8MB stacks
+#define STACK_SIZE (8 * 1024 * 1024)
+static char stack[STACK_SIZE];
+
+static int thread_sleep(void *)
+{
+	while (1)
+		sleep(100);
+	return 1;
+}
+
+static int fork_task_with_thread(int (*fn)(void *), int sk_pair[2],
+				 pid_t *tgl, pid_t *thread, int *tgl_pidfd,
+				 int *thread_pidfd)
+{
+	*tgl_pidfd = *thread_pidfd = -1;
+
+	*tgl = fork();
+	if (*tgl < 0) {
+		perror("fork");
+		return -1;
+	}
+
+	if (!*tgl) {
+		int flags = CLONE_THREAD | CLONE_VM | CLONE_SIGHAND;
+		pid_t t;
+
+		t = clone(fn, stack + STACK_SIZE, flags, sk_pair);
+		if (t < 0) {
+			perror("clone");
+			exit(1);
+		}
+
+		close(sk_pair[1]);
+
+		if (write(sk_pair[0], &t, sizeof(t)) != sizeof(t)) {
+			perror("read");
+			exit(1);
+		}
+
+		// wait to get killed for various reasons by the tests.
+		while (1)
+			sleep(100);
+	}
+
+	errno = 0;
+	if (read(sk_pair[1], thread, sizeof(*thread)) != sizeof(*thread)) {
+		perror("read");
+		goto cleanup;
+	}
+
+	*tgl_pidfd = sys_pidfd_open(*tgl, 0);
+	if (*tgl_pidfd < 0) {
+		perror("pidfd_open tgl");
+		goto cleanup;
+	}
+
+	*thread_pidfd = sys_pidfd_open(*thread, 0);
+	if (*thread_pidfd < 0) {
+		perror("pidfd");
+		goto cleanup;
+	}
+
+	return 0;
+
+cleanup:
+	kill(*tgl, SIGKILL);
+	if (*tgl_pidfd >= 0)
+		close(*tgl_pidfd);
+	if (*thread_pidfd >= 0)
+		close(*thread_pidfd);
+	return -1;
+}
+
+static int test_non_tgl_basic(void)
+{
+	pid_t tgl, thread;
+	int sk_pair[2], status;
+	int tgl_pidfd = -1, thread_pidfd = -1;
+	int ret = KSFT_FAIL;
+
+	if (socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sk_pair) < 0) {
+		ksft_print_msg("socketpair failed %s\n", strerror(errno));
+		return KSFT_FAIL;
+	}
+
+	if (fork_task_with_thread(thread_sleep, sk_pair, &tgl, &thread,
+				  &tgl_pidfd, &thread_pidfd) < 0) {
+		return KSFT_FAIL;
+	}
+
+	/*
+	 * KILL of a thread should still kill everyone
+	 */
+	if (sys_pidfd_send_signal(thread_pidfd, SIGKILL, NULL, 0)) {
+		perror("pidfd_send_signal");
+		goto cleanup;
+	}
+
+	errno = 0;
+	if (waitpid(tgl, &status, 0) != tgl) {
+		perror("waitpid tgl");
+		goto cleanup;
+	}
+
+	if (!WIFSIGNALED(status) || WTERMSIG(status) != SIGKILL) {
+		ksft_print_msg("bad exit status %x\n", status);
+		goto cleanup;
+	}
+
+	ret = KSFT_PASS;
+
+cleanup:
+	close(sk_pair[0]);
+	close(sk_pair[1]);
+	close(tgl_pidfd);
+	close(thread_pidfd);
+	return ret;
+}
+
+static int thread_exec(void *arg)
+{
+	int *sk_pair = arg;
+	pid_t thread;
+
+	if (read(sk_pair[0], &thread, sizeof(thread)) != sizeof(thread)) {
+		perror("read");
+		exit(1);
+	}
+
+	execlp("/bin/true", "/bin/true", NULL);
+	return 1;
+}
+
+static int test_non_tgl_exec(void)
+{
+	pid_t tgl, thread;
+	int sk_pair[2];
+	int tgl_pidfd = -1, thread_pidfd = -1;
+	int ret = KSFT_FAIL, ready;
+	struct pollfd pollfd;
+
+	if (socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sk_pair) < 0) {
+		ksft_print_msg("socketpair failed %s\n", strerror(errno));
+		return KSFT_FAIL;
+	}
+
+	if (fork_task_with_thread(thread_exec, sk_pair, &tgl, &thread,
+				  &tgl_pidfd, &thread_pidfd) < 0) {
+		return KSFT_FAIL;
+	}
+
+	if (write(sk_pair[1], &thread, sizeof(thread)) != sizeof(thread)) {
+		perror("read");
+		goto cleanup;
+	}
+
+	// thread will exec(), so this pidfd should eventually be dead (i.e.
+	// poll should return).
+	pollfd.fd = thread_pidfd;
+	pollfd.events = POLLIN;
+
+	ready = poll(&pollfd, 1, -1);
+	if (ready == -1) {
+		perror("poll");
+		goto cleanup;
+	}
+
+	if (ready != 1) {
+		ksft_print_msg("bad poll result %d\n", ready);
+		goto cleanup;
+	}
+
+	if (pollfd.revents != POLLIN) {
+		ksft_print_msg("bad poll revents: %x\n", pollfd.revents);
+		goto cleanup;
+	}
+
+	errno = 0;
+	if (sys_pidfd_getfd(thread_pidfd, 0, 0) > 0) {
+		ksft_print_msg("got a real fd");
+		goto cleanup;
+	}
+
+	if (errno != ESRCH) {
+		ksft_print_msg("polling invalid thread didn't give ESRCH");
+		goto cleanup;
+	}
+
+	ret = KSFT_PASS;
+
+cleanup:
+	close(sk_pair[0]);
+	close(sk_pair[1]);
+	close(tgl_pidfd);
+	close(thread_pidfd);
+	kill(tgl, SIGKILL);
+	waitpid(tgl, NULL, 0);
+	return ret;
+}
+
+int thread_wait_exit(void *arg)
+{
+	int *sk_pair = arg;
+	pid_t thread;
+
+	if (read(sk_pair[0], &thread, sizeof(thread)) != sizeof(thread)) {
+		perror("read");
+		exit(1);
+	}
+
+	return 0;
+}
+
+static int test_non_tgl_poll_exit(void)
+{
+	pid_t tgl, thread, writer;
+	int sk_pair[2], ready;
+	int tgl_pidfd = -1, thread_pidfd = -1;
+	int ret = KSFT_FAIL;
+	struct pollfd pollfd;
+
+	if (socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sk_pair) < 0) {
+		ksft_print_msg("socketpair failed %s\n", strerror(errno));
+		return KSFT_FAIL;
+	}
+
+	if (fork_task_with_thread(thread_wait_exit, sk_pair, &tgl, &thread,
+				  &tgl_pidfd, &thread_pidfd) < 0) {
+		return KSFT_FAIL;
+	}
+
+	writer = fork();
+	if (writer < 0) {
+		perror("fork");
+		return KSFT_FAIL;
+	}
+
+	if (writer == 0) {
+		sleep(3);
+		if (write(sk_pair[1], &thread, sizeof(thread)) != sizeof(thread)) {
+			perror("read");
+			exit(1);
+		}
+		exit(0);
+	}
+
+	// thread will exit, so this pidfd should eventually be dead (i.e.
+	// poll should return).
+	pollfd.fd = thread_pidfd;
+	pollfd.events = POLLIN;
+
+	ready = poll(&pollfd, 1, -1);
+	if (ready == -1) {
+		perror("poll");
+		goto cleanup;
+	}
+
+	if (ready != 1) {
+		ksft_print_msg("bad poll result %d\n", ready);
+		goto cleanup;
+	}
+
+	if (pollfd.revents != POLLIN) {
+		ksft_print_msg("bad poll revents: %x\n", pollfd.revents);
+		goto cleanup;
+	}
+
+	if (pollfd.fd != thread_pidfd) {
+		ksft_print_msg("bad poll fd: %x\n", pollfd.fd);
+		goto cleanup;
+	}
+
+	ret = KSFT_PASS;
+
+cleanup:
+	close(sk_pair[0]);
+	close(sk_pair[1]);
+	close(tgl_pidfd);
+	close(thread_pidfd);
+	kill(tgl, SIGKILL);
+	waitpid(tgl, NULL, 0);
+	waitpid(thread, NULL, 0);
+	return ret;
+}
+
+
+static int test_non_tgl_exit_poll(void)
+{
+	pid_t tgl, thread;
+	int sk_pair[2];
+	int tgl_pidfd = -1, thread_pidfd = -1;
+	int ret = KSFT_FAIL, ready;
+	struct pollfd pollfd;
+	siginfo_t siginfo;
+
+	if (socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sk_pair) < 0) {
+		ksft_print_msg("socketpair failed %s\n", strerror(errno));
+		return KSFT_FAIL;
+	}
+
+	if (fork_task_with_thread(thread_wait_exit, sk_pair, &tgl, &thread,
+				  &tgl_pidfd, &thread_pidfd) < 0) {
+		return KSFT_FAIL;
+	}
+
+	if (write(sk_pair[1], &thread, sizeof(thread)) != sizeof(thread)) {
+		perror("write");
+		goto cleanup;
+	}
+
+	// thread will exit, so this pidfd should eventually be dead (i.e.
+	// poll should return).
+	pollfd.fd = thread_pidfd;
+	pollfd.events = POLLIN;
+
+	ready = poll(&pollfd, 1, -1);
+	if (ready == -1) {
+		perror("poll");
+		goto cleanup;
+	}
+
+	if (ready != 1) {
+		ksft_print_msg("bad poll result %d\n", ready);
+		goto cleanup;
+	}
+
+	if (pollfd.revents != POLLIN) {
+		ksft_print_msg("bad poll revents: %x\n", pollfd.revents);
+		goto cleanup;
+	}
+
+	errno = 0;
+	if (sys_pidfd_getfd(thread_pidfd, 0, 0) > 0) {
+		ksft_print_msg("got a real pidfd");
+		goto cleanup;
+	}
+
+	if (errno != ESRCH) {
+		ksft_print_msg("polling invalid thread didn't give ESRCH");
+		goto cleanup;
+	}
+
+	// ok, but the thread group *leader* should still be alive
+	pollfd.fd = tgl_pidfd;
+	ready = poll(&pollfd, 1, 1);
+	if (ready == -1) {
+		perror("poll");
+		goto cleanup;
+	}
+
+	if (ready != 0) {
+		ksft_print_msg("polling leader returned something?! %x", pollfd.revents);
+		goto cleanup;
+	}
+
+	errno = 0;
+	if (sys_waitid(P_PIDFD, thread_pidfd, &siginfo, WEXITED, NULL) == 0) {
+		ksft_print_msg("got a real wait value");
+		goto cleanup;
+	}
+
+	if (errno != ECHILD) {
+		ksft_print_msg("waiting invalid thread didn't give ECHILD: %d\n", errno);
+		goto cleanup;
+	}
+
+	close(thread_pidfd);
+
+	ret = KSFT_PASS;
+
+cleanup:
+	kill(tgl, SIGKILL);
+	waitpid(tgl, NULL, 0);
+	waitpid(thread, NULL, 0);
+	return ret;
+}
+
+/*
+ * This is a test for the first half of the scenario in
+ * https://lore.kernel.org/all/20231207-avancieren-unbezahlbar-9258f45ec3ec@brauner/
+ *
+ * i.e we expect:
+ *
+ *      waitid(P_PIDFD, tg_pidfd)               waitid(P_PIDFD, t_pidfd)
+ *      tg_pid[PIDTYPE_TGID] == tsk1            t_pid[PIDTYPE_TGID] == NULL
+ *      => succeeds                             => fails
+ */
+static int test_non_tgl_waitid_exit(void)
+{
+	pid_t tgl, thread, writer;
+	int sk_pair[2], status;
+	int tgl_pidfd = -1, thread_pidfd = -1;
+	int ret = KSFT_FAIL;
+	siginfo_t siginfo;
+
+	if (socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sk_pair) < 0) {
+		ksft_print_msg("socketpair failed %s\n", strerror(errno));
+		return KSFT_FAIL;
+	}
+
+	if (fork_task_with_thread(thread_wait_exit, sk_pair, &tgl, &thread,
+				  &tgl_pidfd, &thread_pidfd) < 0) {
+		return KSFT_FAIL;
+	}
+
+	writer = fork();
+	if (writer < 0) {
+		perror("fork");
+		return KSFT_FAIL;
+	}
+
+	if (writer == 0) {
+		sleep(3);
+		if (write(sk_pair[1], &thread, sizeof(thread)) != sizeof(thread)) {
+			perror("read");
+			exit(1);
+		}
+		exit(0);
+	}
+
+	errno = 0;
+	if (sys_waitid(P_PIDFD, thread_pidfd, &siginfo, __WALL | WEXITED, NULL) == 0
+			|| errno != ECHILD) {
+		ksft_print_msg("waitid didn't give ECHILD");
+		goto cleanup;
+	}
+	close(thread_pidfd);
+
+	kill(tgl, SIGKILL);
+
+	errno = 0;
+	if (sys_waitid(P_PIDFD, tgl_pidfd, &siginfo, __WALL | WEXITED, NULL) != 0) {
+		perror("waitid");
+		ksft_print_msg("waitid failed %d\n", errno);
+		goto cleanup;
+	}
+	close(tgl_pidfd);
+
+	if (siginfo.si_code != CLD_KILLED || siginfo.si_status != SIGKILL) {
+		ksft_print_msg("bad exit code %x status %x\n", siginfo.si_code, siginfo.si_status);
+		goto cleanup;
+	}
+
+	ret = KSFT_PASS;
+
+cleanup:
+	kill(tgl, SIGKILL);
+	waitpid(tgl, &status, 0);
+	waitpid(writer, NULL, 0);
+	return ret;
+}
+
+/*
+ * This is a test for the first second half of the scenario in
+ * https://lore.kernel.org/all/20231207-avancieren-unbezahlbar-9258f45ec3ec@brauner/
+ *
+ * i.e after the exec, we expect:
+ *
+ *      waitid(P_PIDFD, tg_pidfd)		waitid(P_PIDFD, t_pid)
+ *      tg_pid[PIDTYPE_TGID] == tsk2		t_pid[PIDTYPE_TGID] == NULL
+ *      => succeeds				=> fails
+ */
+static int test_non_tgl_waitid_exec(void)
+{
+	pid_t tgl, thread, writer;
+	int sk_pair[2], status;
+	int tgl_pidfd = -1, thread_pidfd = -1;
+	int ret = KSFT_FAIL;
+	siginfo_t siginfo;
+
+	if (socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sk_pair) < 0) {
+		ksft_print_msg("socketpair failed %s\n", strerror(errno));
+		return KSFT_FAIL;
+	}
+
+	if (fork_task_with_thread(thread_exec, sk_pair, &tgl, &thread,
+				  &tgl_pidfd, &thread_pidfd) < 0) {
+		return KSFT_FAIL;
+	}
+
+	writer = fork();
+	if (writer < 0) {
+		perror("fork");
+		return KSFT_FAIL;
+	}
+
+	if (writer == 0) {
+		sleep(3);
+		if (write(sk_pair[1], &thread, sizeof(thread)) != sizeof(thread)) {
+			perror("read");
+			exit(1);
+		}
+		exit(0);
+	}
+
+	errno = 0;
+	if (sys_waitid(P_PIDFD, thread_pidfd, &siginfo, __WALL | WEXITED, NULL) == 0
+			|| errno != ECHILD) {
+		ksft_print_msg("waitid didn't give ECHILD");
+		goto cleanup;
+	}
+	close(thread_pidfd);
+
+	kill(tgl, SIGKILL);
+
+	errno = 0;
+	if (sys_waitid(P_PIDFD, tgl_pidfd, &siginfo, __WALL | WEXITED, NULL) != 0) {
+		perror("waitid");
+		ksft_print_msg("waitid failed %d\n", errno);
+		goto cleanup;
+	}
+	close(tgl_pidfd);
+
+	if (siginfo.si_code != CLD_KILLED || siginfo.si_status != SIGKILL) {
+		ksft_print_msg("bad exit code %x status %x\n", siginfo.si_code, siginfo.si_status);
+		goto cleanup;
+	}
+
+	ret = KSFT_PASS;
+cleanup:
+	kill(tgl, SIGKILL);
+	kill(thread, SIGKILL);
+	waitpid(tgl, &status, 0);
+	waitpid(thread, &status, 0);
+	waitpid(writer, &status, 0);
+	return ret;
+}
+
+#define T(x) { x, #x }
+struct pidfd_non_tgl_test {
+	int (*fn)();
+	const char *name;
+} tests[] = {
+	T(test_non_tgl_basic),
+	T(test_non_tgl_exec),
+	T(test_non_tgl_poll_exit),
+	T(test_non_tgl_exit_poll),
+	T(test_non_tgl_waitid_exit),
+	T(test_non_tgl_waitid_exec),
+};
+#undef T
+
+int main(int argc, char *argv[])
+{
+	int i, ret = EXIT_SUCCESS;
+
+	for (i = 0; i < ARRAY_SIZE(tests); i++) {
+		switch (tests[i].fn()) {
+		case KSFT_PASS:
+			ksft_test_result_pass("%s\n", tests[i].name);
+			break;
+		case KSFT_SKIP:
+			ksft_test_result_skip("%s\n", tests[i].name);
+			break;
+		default:
+			ret = EXIT_FAILURE;
+			ksft_test_result_fail("%s\n", tests[i].name);
+			break;
+		}
+	}
+
+	return ret;
+}
-- 
2.34.1

[PATCH v3 3/3] clone: allow CLONE_THREAD | CLONE_PIDFD together

[Tycho Andersen]

From: Tycho Andersen <tandersen@netflix.com>

This removes the restriction of CLONE_THREAD | CLONE_PIDFD being specified
together. Assuming the previous patch sorts out all the thorny issues this
should be safe. I've left it as a separate patch since it is not strictly
necessary as a usecase for us, but might be nice? Perhaps we want to wait
until someone actually needs it though.

Signed-off-by: Tycho Andersen <tandersen@netflix.com>
---
 kernel/fork.c                                   |  3 +--
 .../selftests/pidfd/pidfd_non_tgl_test.c        | 17 +++++++++++++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index 44969cd472f0..25fccf7c08a7 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2296,9 +2296,8 @@ __latent_entropy struct task_struct *copy_process(
 		/*
 		 * - CLONE_DETACHED is blocked so that we can potentially
 		 *   reuse it later for CLONE_PIDFD.
-		 * - CLONE_THREAD is blocked until someone really needs it.
 		 */
-		if (clone_flags & (CLONE_DETACHED | CLONE_THREAD))
+		if (clone_flags & CLONE_DETACHED)
 			return ERR_PTR(-EINVAL);
 	}
 
diff --git a/tools/testing/selftests/pidfd/pidfd_non_tgl_test.c b/tools/testing/selftests/pidfd/pidfd_non_tgl_test.c
index c0624b127fab..a34208c2307e 100644
--- a/tools/testing/selftests/pidfd/pidfd_non_tgl_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_non_tgl_test.c
@@ -546,6 +546,22 @@ static int test_non_tgl_waitid_exec(void)
 	return ret;
 }
 
+static int test_clone_thread_pidfd(void)
+{
+	pid_t pid;
+	int flags = CLONE_THREAD | CLONE_VM | CLONE_SIGHAND | CLONE_PIDFD;
+	int pidfd;
+
+	pid = clone(thread_sleep, stack + STACK_SIZE, flags, NULL, &pidfd);
+	if (pid < 0) {
+		perror("clone");
+		return KSFT_FAIL;
+	}
+
+	close(pidfd);
+	return KSFT_PASS;
+}
+
 #define T(x) { x, #x }
 struct pidfd_non_tgl_test {
 	int (*fn)();
@@ -557,6 +573,7 @@ struct pidfd_non_tgl_test {
 	T(test_non_tgl_exit_poll),
 	T(test_non_tgl_waitid_exit),
 	T(test_non_tgl_waitid_exec),
+	T(test_clone_thread_pidfd),
 };
 #undef T
 
-- 
2.34.1