Re: [PATCH RFC v2 1/3] pidfs: improve multi-threaded exec and premature thread-group leader exit polling

[Oleg Nesterov <oleg@redhat.com>]

On 03/18, Christian Brauner wrote:
>
> @@ -746,8 +751,23 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
>  	 * sub-thread or delay_group_leader(), wake up the
>  	 * PIDFD_THREAD waiters.
>  	 */
> -	if (!thread_group_empty(tsk))
> -		do_notify_pidfd(tsk);
> +	if (!thread_group_empty(tsk)) {
> +		if (delay_group_leader(tsk)) {
> +			struct pid *pid;
> +
> +			/*
> +			 * This is a thread-group leader exiting before
> +			 * all of its subthreads have exited allow pidfd
> +			 * polling to detect this case and delay exit
> +			 * notification until the last thread has
> +			 * exited.
> +			 */
> +			pid = task_pid(tsk);
> +			WRITE_ONCE(pid->delayed_leader, 1);

This is racy, tsk->exit_state is already set so pidfd_poll() can see
task->exit_state && !pid->delayed_leader.

But this is minor. I can't understand all these complications,
probably because I barely slept tonight ;) I will re-read this patch
again tomorrow, but could you explain why we can't simply use the
trivial patch below?

Oleg.
---

diff --git a/fs/pidfs.c b/fs/pidfs.c
index d980f779c213..8a95920aed98 100644
--- a/fs/pidfs.c
+++ b/fs/pidfs.c
@@ -210,7 +210,6 @@ static void pidfd_show_fdinfo(struct seq_file *m, struct file *f)
 static __poll_t pidfd_poll(struct file *file, struct poll_table_struct *pts)
 {
 	struct pid *pid = pidfd_pid(file);
-	bool thread = file->f_flags & PIDFD_THREAD;
 	struct task_struct *task;
 	__poll_t poll_flags = 0;
 
@@ -223,7 +222,7 @@ static __poll_t pidfd_poll(struct file *file, struct poll_table_struct *pts)
 	task = pid_task(pid, PIDTYPE_PID);
 	if (!task)
 		poll_flags = EPOLLIN | EPOLLRDNORM | EPOLLHUP;
-	else if (task->exit_state && (thread || thread_group_empty(task)))
+	else if (task->exit_state && !delay_group_leader(task))
 		poll_flags = EPOLLIN | EPOLLRDNORM;
 
 	return poll_flags;
diff --git a/kernel/exit.c b/kernel/exit.c
index 9916305e34d3..356ca41d313b 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -746,7 +746,7 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
 	 * sub-thread or delay_group_leader(), wake up the
 	 * PIDFD_THREAD waiters.
 	 */
-	if (!thread_group_empty(tsk))
+	if (!delay_group_leader(tsk))
 		do_notify_pidfd(tsk);
 
 	if (unlikely(tsk->ptrace)) {