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.
