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

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

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

[Oleg Nesterov]

On 10/17, fujo wrote:
>
> 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 will withdraw the kernel patch and send a selftests-only v2.

Good ;)

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

Well, I'm not quite sure which documentation you are referring to, but
of course it always makes sense to fix or improve the unclear doc.

Oleg.

[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: [PATCH v2] selftests/pidfd: wait for POLLHUP before reading exit info

[Oleg Nesterov]

On 10/17, fujo wrote:
>
> @@ -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");

This chunk doesn't look right, POLLIN makes more sense in this case.
Unless I am confused, this poll(timeout = 0) tries to verify that the
child does not exit and report POLLIN, with or without POLLHUP.

Oleg.

[PATCH v3] 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.

The zero-timeout liveness check in autokill_pidfd_open_no_effect keeps
POLLIN; it verifies that the child has not exited yet.

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: [PATCH v2] selftests/pidfd: wait for POLLHUP before reading exit info

[fujo]

Hi Oleg,

> This chunk doesn't look right, POLLIN makes more sense in this case.
> Unless I am confused, this poll(timeout = 0) tries to verify that the
> child does not exit and report POLLIN, with or without POLLHUP.

You are right, thanks.  That poll() is a liveness check, not an
exit-info wait, so v3 keeps POLLIN there and only switches the polls
that are followed by PIDFD_GET_INFO.

About the documentation: there is no in-tree KAPI file for this; the
wording I referred to is the CLONE_AUTOREAP series description ("poll
the child's exit via poll() and retrieve exit status via
PIDFD_GET_INFO").  If you or Christian think a clarifying sentence
belongs somewhere in the tree, I'm happy to send it, but I didn't want
to guess where.

Thanks,
fujo

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

[Oleg Nesterov]

On 10/18, fujo wrote:
>
> You are right, thanks.  That poll() is a liveness check, not an
> exit-info wait, so v3 keeps POLLIN there and only switches the polls
> that are followed by PIDFD_GET_INFO.

Great, I'll try to review V3 tomorrow.

> About the documentation: there is no in-tree KAPI file for this; the
> wording I referred to is the CLONE_AUTOREAP series description ("poll
> the child's exit via poll() and retrieve exit status via
> PIDFD_GET_INFO").  If you or Christian think a clarifying sentence
> belongs somewhere in the tree, I'm happy to send it, but I didn't want
> to guess where.

So it seems that (please correct me if I am wrong) you were confused by
some wording in a changelog or cover letter. If yes, too late to fix it.

But perhaps you can check (and possibly fix/improve) the man page(s) ?
Though, to be honest, I have no idea where all of this is documented...
But surely it must be documented somewhere, right ? ;)

Thanks,

Oleg.