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

[Oleg Nesterov <oleg@redhat.com>]

Christian,

All the comments look misleading (and overcomplicated) to me.

See below, but first lets recall the commit 64bef697d33b75fc06c5789
("pidfd: implement PIDFD_THREAD flag for pidfd_open()") which says

    pidfd: implement PIDFD_THREAD flag for pidfd_open()

    With this flag:

            ....

            - pidfd_poll() succeeds when the task exits and becomes a
              zombie (iow, passes exit_notify()), even if it is a leader
              and thread-group is not empty.

This patch simply reverts this behaviour, the exiting leader will not
report the exit if it has sub-threads (alive or not). And afaics your
V1 tried to do the same. And this eliminates the

              This means that the behaviour of pidfd_poll(PIDFD_THREAD,
              pid-of-group-leader) is not well defined if it races with
              exec() from its sub-thread; ...

problem mentioned in the changelog. That is all.

IOW, with this change PIDFD_THREAD has no effect.

Except the pid_has_task() checks in sys_pidfd_open() paths, without
PIDFD_THREAD the target task must be a group leader.

On 03/20, Christian Brauner wrote:
>
> @@ -218,12 +218,32 @@ static __poll_t pidfd_poll(struct file *file, struct poll_table_struct *pts)

Your forgot to remove the no longer used

	bool thread = file->f_flags & PIDFD_THREAD;

above ;)

>  	/*
>  	 * Depending on PIDFD_THREAD, inform pollers when the thread
>  	 * or the whole thread-group exits.

See above (and below), this no longer depends on PIDFD_THREAD.

> +	else if (task->exit_state && !delay_group_leader(task))
>  		poll_flags = EPOLLIN | EPOLLRDNORM;

So with this change:

If the exiting task is a sub-thread, report EPOLLIN as before.
delay_group_leader() can't be true. In this case PIDFD_THREAD
must be set.

If the exiting task is a leader, we do not care about PIDFD_THREAD.
We report EPOLLIN only if it is the last/only thread.

> diff --git a/kernel/exit.c b/kernel/exit.c
> index 9916305e34d3..ce5cdad5ba9c 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -271,6 +271,9 @@ void release_task(struct task_struct *p)
>  		 * If we were the last child thread and the leader has
>  		 * exited already, and the leader's parent ignores SIGCHLD,
>  		 * then we are the one who should release the leader.
> +		 *
> +		 * This will also wake PIDFD_THREAD pidfds for the
> +		 * thread-group leader that already exited.
>  		 */
>  		zap_leader = do_notify_parent(leader, leader->exit_signal);

Again, this doesn't depend on PIDFD_THREAD.

> @@ -743,10 +746,13 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
>
>  	tsk->exit_state = EXIT_ZOMBIE;
>  	/*
> -	 * sub-thread or delay_group_leader(), wake up the
> -	 * PIDFD_THREAD waiters.
> +	 * Wake up PIDFD_THREAD waiters if this is a proper subthread
> +	 * exit. If this is a premature thread-group leader exit delay
> +	 * the notification until the last subthread exits. If a
> +	 * subthread should exec before then no notification will be
> +	 * generated.
>  	 */
> -	if (!thread_group_empty(tsk))
> +	if (!delay_group_leader(tsk))
>  		do_notify_pidfd(tsk);

The same...

> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2180,8 +2180,10 @@ 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
> -	 * non-PIDFD_THREAD waiters.
> +	 * This is a thread-group leader without subthreads so wake up
> +	 * the non-PIDFD_THREAD waiters. This also wakes the
> +	 * PIDFD_THREAD waiters for the thread-group leader in case it
> +	 * exited prematurely from release_task().
>  	 */

This too.

Oleg.