[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
>

[PATCH v2] selftests/pidfd: wait for POLLHUP before reading exit info

[fujo]

The CLONE_AUTOREAP tests poll for POLLIN and then immediately call
PIDFD_GET_INFO with PIDFD_INFO_EXIT.  PIDFD_INFO_EXIT is only published
from release_task() -> pidfs_exit(), which runs after the pidfd already
reported POLLIN for the exited task.  A poller woken by
do_notify_pidfd() can therefore read a mask without PIDFD_INFO_EXIT;
under KASAN the autoreap_multithreaded case fails this way.

POLLHUP is returned once the task linkage has been removed from
struct pid, which happens after pidfs_exit() has published the exit
information.  Wait for POLLHUP and check it in revents.

With this change all tests pass ten consecutive runs on an unpatched
kernel.

Signed-off-by: fujo <hooyan2024@gmail.com>
---
 .../selftests/pidfd/pidfd_autoreap_test.c     | 36 +++++++++----------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/tools/testing/selftests/pidfd/pidfd_autoreap_test.c b/tools/testing/selftests/pidfd/pidfd_autoreap_test.c
index 1c586482f..b4f3699ef 100644
--- a/tools/testing/selftests/pidfd/pidfd_autoreap_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_autoreap_test.c
@@ -149,7 +149,7 @@ TEST(autoreap_rejects_thread)
 
 /*
  * Basic test: create an autoreap child, let it exit, verify:
- * - pidfd becomes readable (poll returns POLLIN)
+ * - pidfd becomes readable (poll returns POLLHUP)
  * - PIDFD_GET_INFO returns the correct exit code
  * - waitpid() returns -1/ECHILD (no zombie)
  */
@@ -172,10 +172,10 @@ TEST(autoreap_basic)
 
 	/* Wait for the child to exit via pidfd poll. */
 	pfd.fd = pidfd;
-	pfd.events = POLLIN;
+	pfd.events = POLLHUP;
 	ret = poll(&pfd, 1, 5000);
 	ASSERT_EQ(ret, 1);
-	ASSERT_TRUE(pfd.revents & POLLIN);
+	ASSERT_TRUE(pfd.revents & POLLHUP);
 
 	/* Verify exit info via PIDFD_GET_INFO. */
 	ret = ioctl(pidfd, PIDFD_GET_INFO, &info);
@@ -225,10 +225,10 @@ TEST(autoreap_signaled)
 
 	/* Wait for exit via pidfd. */
 	pfd.fd = pidfd;
-	pfd.events = POLLIN;
+	pfd.events = POLLHUP;
 	ret = poll(&pfd, 1, 5000);
 	ASSERT_EQ(ret, 1);
-	ASSERT_TRUE(pfd.revents & POLLIN);
+	ASSERT_TRUE(pfd.revents & POLLHUP);
 
 	/* Verify signal info. */
 	ret = ioctl(pidfd, PIDFD_GET_INFO, &info);
@@ -334,10 +334,10 @@ TEST(autoreap_reparent)
 
 	/* Wait for it to exit via pidfd poll. */
 	pfd.fd = pidfd;
-	pfd.events = POLLIN;
+	pfd.events = POLLHUP;
 	ret = poll(&pfd, 1, 5000);
 	ASSERT_EQ(ret, 1);
-	ASSERT_TRUE(pfd.revents & POLLIN);
+	ASSERT_TRUE(pfd.revents & POLLHUP);
 
 	/*
 	 * The grandchild should have been autoreaped even though
@@ -421,10 +421,10 @@ TEST(autoreap_multithreaded)
 
 	/* Wait for the process to fully exit via pidfd poll. */
 	pfd.fd = pidfd;
-	pfd.events = POLLIN;
+	pfd.events = POLLHUP;
 	ret = poll(&pfd, 1, 5000);
 	ASSERT_EQ(ret, 1);
-	ASSERT_TRUE(pfd.revents & POLLIN);
+	ASSERT_TRUE(pfd.revents & POLLHUP);
 
 	/* Verify exit info. */
 	ret = ioctl(pidfd, PIDFD_GET_INFO, &info);
@@ -512,7 +512,7 @@ TEST(autoreap_no_inherit)
 
 	/* Wait for the autoreap child to exit. */
 	pfd.fd = pidfd;
-	pfd.events = POLLIN;
+	pfd.events = POLLHUP;
 	ret = poll(&pfd, 1, 5000);
 	ASSERT_EQ(ret, 1);
 
@@ -571,7 +571,7 @@ TEST(nnp_sets_no_new_privs)
 
 	/* Wait for child to exit. */
 	pfd.fd = pidfd;
-	pfd.events = POLLIN;
+	pfd.events = POLLHUP;
 	ret = poll(&pfd, 1, 5000);
 	ASSERT_EQ(ret, 1);
 
@@ -631,7 +631,7 @@ TEST(autoreap_no_new_privs_unset)
 	ASSERT_GE(pidfd, 0);
 
 	pfd.fd = pidfd;
-	pfd.events = POLLIN;
+	pfd.events = POLLHUP;
 	ret = poll(&pfd, 1, 5000);
 	ASSERT_EQ(ret, 1);
 
@@ -695,10 +695,10 @@ TEST(autokill_basic)
 
 	/* Wait for the child to die via the pidfd_open'd fd. */
 	pfd.fd = pollfd_fd;
-	pfd.events = POLLIN;
+	pfd.events = POLLHUP;
 	ret = poll(&pfd, 1, 5000);
 	ASSERT_EQ(ret, 1);
-	ASSERT_TRUE(pfd.revents & POLLIN);
+	ASSERT_TRUE(pfd.revents & POLLHUP);
 
 	/* Child should be autoreaped — no zombie. */
 	usleep(100000);
@@ -799,7 +799,7 @@ TEST(autokill_pidfd_open_no_effect)
 
 	/* Verify child is still alive by polling the clone3 pidfd. */
 	pfd.fd = pidfd;
-	pfd.events = POLLIN;
+	pfd.events = POLLHUP;
 	ret = poll(&pfd, 1, 0);
 	ASSERT_EQ(ret, 0) {
 		TH_LOG("Child died after closing pidfd_open fd — should still be alive");
@@ -813,10 +813,10 @@ TEST(autokill_pidfd_open_no_effect)
 	close(pidfd);
 
 	pfd.fd = open_fd;
-	pfd.events = POLLIN;
+	pfd.events = POLLHUP;
 	ret = poll(&pfd, 1, 5000);
 	ASSERT_EQ(ret, 1);
-	ASSERT_TRUE(pfd.revents & POLLIN);
+	ASSERT_TRUE(pfd.revents & POLLHUP);
 
 	/* Child should be autoreaped — no zombie. */
 	usleep(100000);
@@ -884,7 +884,7 @@ TEST(autokill_without_nnp_with_cap)
 
 	/* Wait for child to exit. */
 	pfd.fd = pidfd;
-	pfd.events = POLLIN;
+	pfd.events = POLLHUP;
 	ret = poll(&pfd, 1, 5000);
 	ASSERT_EQ(ret, 1);
 
-- 
2.53.0

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

[fujo]

Hi Oleg,

thanks, and sorry for the unclear report.  You are right that the early
wakeup is not specific to CLONE_AUTOREAP: exit_notify() always calls
do_notify_pidfd() for an exiting non-delay_group_leader, so pollers are
woken and pidfd_poll() returns EPOLLIN as soon as exit_state is set.

What I observed is the window between that first wakeup and
release_task() -> pidfs_exit() publishing PIDFS_ATTR_BIT_EXIT: poll()
returns EPOLLIN to userspace at that point (it does not wait for the
later wake_up_all() after the unhash), so a parent that follows the
"poll and then PIDFD_GET_INFO" sequence from the series description can
read a mask without PIDFD_INFO_EXIT.  The second wakeup only helps
pollers that are still asleep when it happens.

Test-case details, as requested:

- clone3(CLONE_PIDFD | CLONE_AUTOREAP, exit_signal = 0, pidfd = &pidfd)
- mode 0: the child creates one thread which exits after 500us and then
  exits itself with 42; mode 1: the child exits immediately
- parent: poll(pidfd, POLLIN, 5000), then
  ioctl(pidfd, PIDFD_GET_INFO, {.mask = PIDFD_INFO_EXIT}) and counts
  results whose mask lacks PIDFD_INFO_EXIT
- x86_64 mainline 58161ead29e5 (v7.3-rc7), defconfig + KASAN + DEBUG_INFO
  + DEBUG_KMEMLEAK, QEMU, 2G:
    mode 0, SMP 2: 18/300
    mode 0, SMP 1: 78/300
    mode 1, SMP 2:  8/300
- the in-tree pidfd_autoreap_test global.autoreap_multithreaded case
  fails the same way: its poll() gets POLLIN and PIDFD_GET_INFO returns
  mask 0x7 (PID|CREDS|CGROUPID) without EXIT.

Your POLLHUP remark resolves it: PIDFD_INFO_EXIT is published before the
task linkage is removed, so POLLHUP is the right condition to wait for.
I changed the autoreap tests to pfd.events = POLLHUP and to check
POLLHUP in revents; on the *unpatched* kernel they now pass ten
consecutive runs (19/19 each), and a POLLHUP variant of the reproducer
gets 300/300 with PIDFD_INFO_EXIT.  So the kernel behaves as designed
and the tests were wrong.

I will withdraw the kernel patch and send a selftests-only v2.  Do you
think the CLONE_AUTOREAP cover letter or some KAPI comment should also
say explicitly that POLLHUP, not POLLIN, guarantees that
PIDFD_INFO_EXIT is available?  The "poll the pidfd and retrieve the exit
status via PIDFD_GET_INFO" wording is what led me there.

Thanks,
fujo