[RFC PATCH] pidfs: don't report autoreaped tasks readable before exit info exists

[RFC PATCH] pidfs: don't report autoreaped tasks readable before exit info exists

[fujo]

pidfd_poll() reports a task readable as soon as task->exit_state is set
and the thread group is empty.  For a task that is going to be
auto-reaped (CLONE_AUTOREAP, or a parent that ignores SIGCHLD) this
happens before release_task() runs and before pidfs_exit() publishes
PIDFS_ATTR_BIT_EXIT.

A woken poller that immediately calls PIDFD_GET_INFO with
PIDFD_INFO_EXIT can therefore get an answer without PIDFD_INFO_EXIT.
For CLONE_AUTOREAP this breaks the documented use of polling the pidfd
and retrieving the exit status via PIDFD_GET_INFO: the task is
auto-reaped and cannot be waited for, so the exit status is lost for
that poll.

Do not report a task that is pending auto-reap readable until its exit
information has been published, and wake the pidfd waitqueue from
pidfs_exit() after PIDFS_ATTR_BIT_EXIT has been set so that pollers
woken early sleep again and are woken once the information exists.

Reproducer: clone3(CLONE_PIDFD | CLONE_AUTOREAP); the child creates a
thread that exits after 500us and then exits itself; the parent polls
the pidfd and immediately calls PIDFD_GET_INFO with PIDFD_INFO_EXIT.
On current mainline 18/300 iterations (SMP 2) and 78/300 (SMP 1) return
POLLIN without PIDFD_INFO_EXIT; with this change 0/300 in both cases.
The in-tree pidfd_autoreap_test global.autoreap_multithreaded case
fails without and passes with this change.

Signed-off-by: fujo <hooyan2024@gmail.com>
---
 fs/pidfs.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/fs/pidfs.c b/fs/pidfs.c
index a6a643f15..5245131ea 100644
--- a/fs/pidfs.c
+++ b/fs/pidfs.c
@@ -320,8 +320,26 @@ 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 && !delay_group_leader(task))
-		poll_flags = EPOLLIN | EPOLLRDNORM;
+	else if (task->exit_state && !delay_group_leader(task)) {
+		struct pidfs_attr *pattr = READ_ONCE(pid->attr);
+		bool pending_autoreap;
+
+		/*
+		 * A task that is going to be auto-reaped reaches EXIT_DEAD (or
+		 * carries CLONE_AUTOREAP) before release_task() publishes
+		 * PIDFS_ATTR_BIT_EXIT.  Don't report readable until the exit
+		 * information is actually available, or a poller can observe
+		 * POLLIN and then get PIDFD_GET_INFO without PIDFD_INFO_EXIT.
+		 * ptraced tasks are not auto-reaped, so they keep the old
+		 * behavior.
+		 */
+		pending_autoreap = (!task->ptrace && task->signal->autoreap) ||
+				   task->exit_state == EXIT_DEAD;
+		if (!pending_autoreap ||
+		    (!IS_ERR_OR_NULL(pattr) &&
+		     test_bit(PIDFS_ATTR_BIT_EXIT, &pattr->attr_mask)))
+			poll_flags = EPOLLIN | EPOLLRDNORM;
+	}
 
 	return poll_flags;
 }
@@ -790,6 +808,13 @@ void pidfs_exit(struct task_struct *tsk)
 	/* Ensure that PIDFD_GET_INFO sees either all or nothing. */
 	smp_wmb();
 	set_bit(PIDFS_ATTR_BIT_EXIT, &attr->attr_mask);
+
+	/*
+	 * Pollers may have been woken before the exit information above was
+	 * published.  Wake them again now that PIDFD_GET_INFO can return
+	 * PIDFD_INFO_EXIT.
+	 */
+	do_notify_pidfd(tsk);
 }
 
 #ifdef CONFIG_COREDUMP
-- 
2.53.0

Re: [RFC PATCH] pidfs: don't report autoreaped tasks readable before exit info exists

[Oleg Nesterov]

Sorry, I don't understand...

Christian, can you take a look?

On 10/17, fujo wrote:
>
> pidfd_poll() reports a task readable as soon as task->exit_state is set
> and the thread group is empty.  For a task that is going to be
> auto-reaped (CLONE_AUTOREAP, or a parent that ignores SIGCHLD) this
> happens before release_task() runs and before pidfs_exit() publishes
> PIDFS_ATTR_BIT_EXIT.

But this doesn't depend on CLONE_AUTOREAP/etc; unless the exiting task
is delay_group_leader(), exit_notify() always calls do_notify_pidfd().
The pollers get EPOLLIN.

Then release_task()->pidfs_exit() publishes PIDFS_ATTR_BIT_EXIT, and
another wake_up_all(&pid->wait_pidfd) wakes the pollers again, after
that the pollers should get EPOLLIN | EPOLLHUP.

Please add more details, in particular about your test-case.

As for tools/testing/selftests/pidfd/pidfd_autoreap_test.c
At first glance it is wrong, it should use pfd.events = POLLHUP

Oleg.

> A woken poller that immediately calls PIDFD_GET_INFO with
> PIDFD_INFO_EXIT can therefore get an answer without PIDFD_INFO_EXIT.
> For CLONE_AUTOREAP this breaks the documented use of polling the pidfd
> and retrieving the exit status via PIDFD_GET_INFO: the task is
> auto-reaped and cannot be waited for, so the exit status is lost for
> that poll.
>
> Do not report a task that is pending auto-reap readable until its exit
> information has been published, and wake the pidfd waitqueue from
> pidfs_exit() after PIDFS_ATTR_BIT_EXIT has been set so that pollers
> woken early sleep again and are woken once the information exists.
>
> Reproducer: clone3(CLONE_PIDFD | CLONE_AUTOREAP); the child creates a
> thread that exits after 500us and then exits itself; the parent polls
> the pidfd and immediately calls PIDFD_GET_INFO with PIDFD_INFO_EXIT.
> On current mainline 18/300 iterations (SMP 2) and 78/300 (SMP 1) return
> POLLIN without PIDFD_INFO_EXIT; with this change 0/300 in both cases.
> The in-tree pidfd_autoreap_test global.autoreap_multithreaded case
> fails without and passes with this change.
>
> Signed-off-by: fujo <hooyan2024@gmail.com>
> ---
>  fs/pidfs.c | 29 +++++++++++++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/fs/pidfs.c b/fs/pidfs.c
> index a6a643f15..5245131ea 100644
> --- a/fs/pidfs.c
> +++ b/fs/pidfs.c
> @@ -320,8 +320,26 @@ 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 && !delay_group_leader(task))
> -		poll_flags = EPOLLIN | EPOLLRDNORM;
> +	else if (task->exit_state && !delay_group_leader(task)) {
> +		struct pidfs_attr *pattr = READ_ONCE(pid->attr);
> +		bool pending_autoreap;
> +
> +		/*
> +		 * A task that is going to be auto-reaped reaches EXIT_DEAD (or
> +		 * carries CLONE_AUTOREAP) before release_task() publishes
> +		 * PIDFS_ATTR_BIT_EXIT.  Don't report readable until the exit
> +		 * information is actually available, or a poller can observe
> +		 * POLLIN and then get PIDFD_GET_INFO without PIDFD_INFO_EXIT.
> +		 * ptraced tasks are not auto-reaped, so they keep the old
> +		 * behavior.
> +		 */
> +		pending_autoreap = (!task->ptrace && task->signal->autoreap) ||
> +				   task->exit_state == EXIT_DEAD;
> +		if (!pending_autoreap ||
> +		    (!IS_ERR_OR_NULL(pattr) &&
> +		     test_bit(PIDFS_ATTR_BIT_EXIT, &pattr->attr_mask)))
> +			poll_flags = EPOLLIN | EPOLLRDNORM;
> +	}
>
>  	return poll_flags;
>  }
> @@ -790,6 +808,13 @@ void pidfs_exit(struct task_struct *tsk)
>  	/* Ensure that PIDFD_GET_INFO sees either all or nothing. */
>  	smp_wmb();
>  	set_bit(PIDFS_ATTR_BIT_EXIT, &attr->attr_mask);
> +
> +	/*
> +	 * Pollers may have been woken before the exit information above was
> +	 * published.  Wake them again now that PIDFD_GET_INFO can return
> +	 * PIDFD_INFO_EXIT.
> +	 */
> +	do_notify_pidfd(tsk);
>  }
>
>  #ifdef CONFIG_COREDUMP
> --
> 2.53.0
>