* [PATCH bpf-next] selftests/bpf: Fix failure path in send_signal test
@ 2025-11-13 7:05 Alexei Starovoitov
2025-11-13 7:31 ` bot+bpf-ci
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Alexei Starovoitov @ 2025-11-13 7:05 UTC (permalink / raw)
To: bpf; +Cc: daniel, andrii, martin.lau
From: Alexei Starovoitov <ast@kernel.org>
When test_send_signal_kern__open_and_load() fails parent closes the
pipe which cases ASSERT_EQ(read(pipe_p2c...)) to fail, but child
continues and enters infinite loop, while parent is stuck in wait(NULL).
Fix the issue by killing the child before jumping to skel_open_load_failure.
The bug was discovered while compiling all of selftests with -O1 instead of -O2
which caused progs/test_send_signal_kern.c to fail to load.
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
tools/testing/selftests/bpf/prog_tests/send_signal.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c
index 1702aa592c2c..61521dc76c3c 100644
--- a/tools/testing/selftests/bpf/prog_tests/send_signal.c
+++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c
@@ -110,8 +110,10 @@ static void test_send_signal_common(struct perf_event_attr *attr,
close(pipe_p2c[0]); /* close read */
skel = test_send_signal_kern__open_and_load();
- if (!ASSERT_OK_PTR(skel, "skel_open_and_load"))
+ if (!ASSERT_OK_PTR(skel, "skel_open_and_load")) {
+ kill(pid, SIGKILL);
goto skel_open_load_failure;
+ }
/* boost with a high priority so we got a higher chance
* that if an interrupt happens, the underlying task
--
2.47.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: Fix failure path in send_signal test
2025-11-13 7:05 [PATCH bpf-next] selftests/bpf: Fix failure path in send_signal test Alexei Starovoitov
@ 2025-11-13 7:31 ` bot+bpf-ci
2025-11-13 21:59 ` Yonghong Song
2025-11-14 22:20 ` Andrii Nakryiko
2 siblings, 0 replies; 6+ messages in thread
From: bot+bpf-ci @ 2025-11-13 7:31 UTC (permalink / raw)
To: alexei.starovoitov, bpf
Cc: daniel, andrii, martin.lau, ast, andrii, daniel, martin.lau,
eddyz87, yonghong.song, clm, ihor.solodrai
[-- Attachment #1: Type: text/plain, Size: 1923 bytes --]
> diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c
> index 1702aa592..61521dc76 100644
> --- a/tools/testing/selftests/bpf/prog_tests/send_signal.c
> +++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c
> @@ -110,8 +110,10 @@ static void test_send_signal_common(struct perf_event_attr *attr,
> close(pipe_p2c[0]); /* close read */
>
> skel = test_send_signal_kern__open_and_load();
> - if (!ASSERT_OK_PTR(skel, "skel_open_and_load"))
> + if (!ASSERT_OK_PTR(skel, "skel_open_and_load")) {
> + kill(pid, SIGKILL);
> goto skel_open_load_failure;
> + }
The fix addresses the skel_open_and_load failure path, but does this same
issue affect the other early failure paths in test_send_signal_common()?
Looking at the code after this hunk:
if (!attr) {
err = test_send_signal_kern__attach(skel);
if (!ASSERT_OK(err, "skel_attach")) {
err = -1;
goto destroy_skel;
}
} else {
[ ... ]
if (!ASSERT_GE(pmu_fd, 0, "perf_event_open")) {
err = -1;
goto destroy_skel;
}
skel->links.send_signal_perf =
bpf_program__attach_perf_event(skel->progs.send_signal_perf, pmu_fd);
if (!ASSERT_OK_PTR(skel->links.send_signal_perf, "attach_perf_event"))
goto disable_pmu;
}
/* wait until child signal handler installed */
ASSERT_EQ(read(pipe_c2p[0], buf, 1), 1, "pipe_read");
All three failure paths (attach, perf_event_open, attach_perf_event) occur
before reading from the child at pipe_c2p[0]. They goto labels that
eventually reach skel_open_load_failure without killing the child first.
Won't these paths trigger the same infinite loop bug described in the
commit message?
---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/19323536860
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: Fix failure path in send_signal test
2025-11-13 7:05 [PATCH bpf-next] selftests/bpf: Fix failure path in send_signal test Alexei Starovoitov
2025-11-13 7:31 ` bot+bpf-ci
@ 2025-11-13 21:59 ` Yonghong Song
2025-11-14 22:20 ` Andrii Nakryiko
2 siblings, 0 replies; 6+ messages in thread
From: Yonghong Song @ 2025-11-13 21:59 UTC (permalink / raw)
To: Alexei Starovoitov, bpf; +Cc: daniel, andrii, martin.lau
On 11/12/25 11:05 PM, Alexei Starovoitov wrote:
> From: Alexei Starovoitov <ast@kernel.org>
>
> When test_send_signal_kern__open_and_load() fails parent closes the
> pipe which cases ASSERT_EQ(read(pipe_p2c...)) to fail, but child
> continues and enters infinite loop, while parent is stuck in wait(NULL).
>
> Fix the issue by killing the child before jumping to skel_open_load_failure.
>
> The bug was discovered while compiling all of selftests with -O1 instead of -O2
> which caused progs/test_send_signal_kern.c to fail to load.
>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
I can reproduce the hang with -O1 and this patch fixed it. So
Tested-by: Yonghong Song <yonghong.song@linux.dev>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: Fix failure path in send_signal test
2025-11-13 7:05 [PATCH bpf-next] selftests/bpf: Fix failure path in send_signal test Alexei Starovoitov
2025-11-13 7:31 ` bot+bpf-ci
2025-11-13 21:59 ` Yonghong Song
@ 2025-11-14 22:20 ` Andrii Nakryiko
2025-11-14 22:25 ` Alexei Starovoitov
2 siblings, 1 reply; 6+ messages in thread
From: Andrii Nakryiko @ 2025-11-14 22:20 UTC (permalink / raw)
To: Alexei Starovoitov; +Cc: bpf, daniel, andrii, martin.lau
On Wed, Nov 12, 2025 at 11:05 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> From: Alexei Starovoitov <ast@kernel.org>
>
> When test_send_signal_kern__open_and_load() fails parent closes the
> pipe which cases ASSERT_EQ(read(pipe_p2c...)) to fail, but child
> continues and enters infinite loop, while parent is stuck in wait(NULL).
>
> Fix the issue by killing the child before jumping to skel_open_load_failure.
>
> The bug was discovered while compiling all of selftests with -O1 instead of -O2
> which caused progs/test_send_signal_kern.c to fail to load.
>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---
> tools/testing/selftests/bpf/prog_tests/send_signal.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c
> index 1702aa592c2c..61521dc76c3c 100644
> --- a/tools/testing/selftests/bpf/prog_tests/send_signal.c
> +++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c
> @@ -110,8 +110,10 @@ static void test_send_signal_common(struct perf_event_attr *attr,
> close(pipe_p2c[0]); /* close read */
>
> skel = test_send_signal_kern__open_and_load();
> - if (!ASSERT_OK_PTR(skel, "skel_open_and_load"))
> + if (!ASSERT_OK_PTR(skel, "skel_open_and_load")) {
> + kill(pid, SIGKILL);
> goto skel_open_load_failure;
> + }
this is only a partial solution, as rightfully pointed out by AI. The
real solution, IMO, is to make child die by itself if parent's pipe is
closed (which is what we do in parent on cleanup). If you run these
tests with -v, you'll actually see what happens on child side:
#374/7 send_signal/send_signal_tracepoint_remote:OK
test_send_signal_common:PASS:pipe_c2p 0 nsec
test_send_signal_common:PASS:pipe_p2c 0 nsec
test_send_signal_common:PASS:fork 0 nsec
test_send_signal_common:PASS:fork 0 nsec
test_send_signal_common:PASS:sigaction 0 nsec
test_send_signal_common:PASS:pipe_write 0 nsec
test_send_signal_common:FAIL:pipe_read unexpected pipe_read: actual 0
!= expected 1
So a really simple and more robust solution is:
diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c
b/tools/testing/selftests/bpf/prog_tests/send_signal.c
index 1702aa592c2c..589a7bf3532a 100644
--- a/tools/testing/selftests/bpf/prog_tests/send_signal.c
+++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c
@@ -76,7 +76,8 @@ static void test_send_signal_common(struct
perf_event_attr *attr,
ASSERT_EQ(write(pipe_c2p[1], buf, 1), 1, "pipe_write");
/* make sure parent enabled bpf program to send_signal */
- ASSERT_EQ(read(pipe_p2c[0], buf, 1), 1, "pipe_read");
+ if (!ASSERT_EQ(read(pipe_p2c[0], buf, 1), 1, "pipe_read"))
+ goto child_cleanup;
/* wait a little for signal handler */
for (int i = 0; i < 1000000000 && !sigusr1_received; i++) {
@@ -101,6 +102,7 @@ static void test_send_signal_common(struct
perf_event_attr *attr,
if (!remote)
ASSERT_OK(setpriority(PRIO_PROCESS, 0,
old_prio), "setpriority");
+child_cleanup:
close(pipe_c2p[1]);
close(pipe_p2c[0]);
exit(0);
pw-bot: cr
>
> /* boost with a high priority so we got a higher chance
> * that if an interrupt happens, the underlying task
> --
> 2.47.3
>
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: Fix failure path in send_signal test
2025-11-14 22:20 ` Andrii Nakryiko
@ 2025-11-14 22:25 ` Alexei Starovoitov
2025-11-15 1:02 ` Andrii Nakryiko
0 siblings, 1 reply; 6+ messages in thread
From: Alexei Starovoitov @ 2025-11-14 22:25 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: bpf, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau
On Fri, Nov 14, 2025 at 2:20 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Nov 12, 2025 at 11:05 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > From: Alexei Starovoitov <ast@kernel.org>
> >
> > When test_send_signal_kern__open_and_load() fails parent closes the
> > pipe which cases ASSERT_EQ(read(pipe_p2c...)) to fail, but child
> > continues and enters infinite loop, while parent is stuck in wait(NULL).
> >
> > Fix the issue by killing the child before jumping to skel_open_load_failure.
> >
> > The bug was discovered while compiling all of selftests with -O1 instead of -O2
> > which caused progs/test_send_signal_kern.c to fail to load.
> >
> > Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> > ---
> > tools/testing/selftests/bpf/prog_tests/send_signal.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c
> > index 1702aa592c2c..61521dc76c3c 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/send_signal.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c
> > @@ -110,8 +110,10 @@ static void test_send_signal_common(struct perf_event_attr *attr,
> > close(pipe_p2c[0]); /* close read */
> >
> > skel = test_send_signal_kern__open_and_load();
> > - if (!ASSERT_OK_PTR(skel, "skel_open_and_load"))
> > + if (!ASSERT_OK_PTR(skel, "skel_open_and_load")) {
> > + kill(pid, SIGKILL);
> > goto skel_open_load_failure;
> > + }
>
>
> this is only a partial solution, as rightfully pointed out by AI. The
> real solution, IMO, is to make child die by itself if parent's pipe is
> closed (which is what we do in parent on cleanup). If you run these
> tests with -v, you'll actually see what happens on child side:
You're looking at the old patch.
> #374/7 send_signal/send_signal_tracepoint_remote:OK
> test_send_signal_common:PASS:pipe_c2p 0 nsec
> test_send_signal_common:PASS:pipe_p2c 0 nsec
> test_send_signal_common:PASS:fork 0 nsec
> test_send_signal_common:PASS:fork 0 nsec
> test_send_signal_common:PASS:sigaction 0 nsec
> test_send_signal_common:PASS:pipe_write 0 nsec
> test_send_signal_common:FAIL:pipe_read unexpected pipe_read: actual 0
> != expected 1
>
>
> So a really simple and more robust solution is:
Not really. It would still miss all other unhandled ASSERTs and gotos
in the parent.
See v2 for robust sigkill.
> diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c
> b/tools/testing/selftests/bpf/prog_tests/send_signal.c
> index 1702aa592c2c..589a7bf3532a 100644
> --- a/tools/testing/selftests/bpf/prog_tests/send_signal.c
> +++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c
> @@ -76,7 +76,8 @@ static void test_send_signal_common(struct
> perf_event_attr *attr,
> ASSERT_EQ(write(pipe_c2p[1], buf, 1), 1, "pipe_write");
>
> /* make sure parent enabled bpf program to send_signal */
> - ASSERT_EQ(read(pipe_p2c[0], buf, 1), 1, "pipe_read");
> + if (!ASSERT_EQ(read(pipe_p2c[0], buf, 1), 1, "pipe_read"))
> + goto child_cleanup;
>
> /* wait a little for signal handler */
> for (int i = 0; i < 1000000000 && !sigusr1_received; i++) {
> @@ -101,6 +102,7 @@ static void test_send_signal_common(struct
> perf_event_attr *attr,
> if (!remote)
> ASSERT_OK(setpriority(PRIO_PROCESS, 0,
> old_prio), "setpriority");
>
> +child_cleanup:
> close(pipe_c2p[1]);
> close(pipe_p2c[0]);
> exit(0);
>
> pw-bot: cr
>
>
> >
> > /* boost with a high priority so we got a higher chance
> > * that if an interrupt happens, the underlying task
> > --
> > 2.47.3
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: Fix failure path in send_signal test
2025-11-14 22:25 ` Alexei Starovoitov
@ 2025-11-15 1:02 ` Andrii Nakryiko
0 siblings, 0 replies; 6+ messages in thread
From: Andrii Nakryiko @ 2025-11-15 1:02 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: bpf, Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau
On Fri, Nov 14, 2025 at 2:25 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Fri, Nov 14, 2025 at 2:20 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Wed, Nov 12, 2025 at 11:05 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > From: Alexei Starovoitov <ast@kernel.org>
> > >
> > > When test_send_signal_kern__open_and_load() fails parent closes the
> > > pipe which cases ASSERT_EQ(read(pipe_p2c...)) to fail, but child
> > > continues and enters infinite loop, while parent is stuck in wait(NULL).
> > >
> > > Fix the issue by killing the child before jumping to skel_open_load_failure.
> > >
> > > The bug was discovered while compiling all of selftests with -O1 instead of -O2
> > > which caused progs/test_send_signal_kern.c to fail to load.
> > >
> > > Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> > > ---
> > > tools/testing/selftests/bpf/prog_tests/send_signal.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c
> > > index 1702aa592c2c..61521dc76c3c 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/send_signal.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c
> > > @@ -110,8 +110,10 @@ static void test_send_signal_common(struct perf_event_attr *attr,
> > > close(pipe_p2c[0]); /* close read */
> > >
> > > skel = test_send_signal_kern__open_and_load();
> > > - if (!ASSERT_OK_PTR(skel, "skel_open_and_load"))
> > > + if (!ASSERT_OK_PTR(skel, "skel_open_and_load")) {
> > > + kill(pid, SIGKILL);
> > > goto skel_open_load_failure;
> > > + }
> >
> >
> > this is only a partial solution, as rightfully pointed out by AI. The
> > real solution, IMO, is to make child die by itself if parent's pipe is
> > closed (which is what we do in parent on cleanup). If you run these
> > tests with -v, you'll actually see what happens on child side:
>
> You're looking at the old patch.
oops :)
>
> > #374/7 send_signal/send_signal_tracepoint_remote:OK
> > test_send_signal_common:PASS:pipe_c2p 0 nsec
> > test_send_signal_common:PASS:pipe_p2c 0 nsec
> > test_send_signal_common:PASS:fork 0 nsec
> > test_send_signal_common:PASS:fork 0 nsec
> > test_send_signal_common:PASS:sigaction 0 nsec
> > test_send_signal_common:PASS:pipe_write 0 nsec
> > test_send_signal_common:FAIL:pipe_read unexpected pipe_read: actual 0
> > != expected 1
> >
> >
> > So a really simple and more robust solution is:
>
> Not really. It would still miss all other unhandled ASSERTs and gotos
> in the parent.
I'd be actually more concerned about that sigusr1_received loop in the
child, that one we can get stuck in for a long while, so ok, SIGKILL
it is.
> See v2 for robust sigkill.
>
> > diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c
> > b/tools/testing/selftests/bpf/prog_tests/send_signal.c
> > index 1702aa592c2c..589a7bf3532a 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/send_signal.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c
> > @@ -76,7 +76,8 @@ static void test_send_signal_common(struct
> > perf_event_attr *attr,
> > ASSERT_EQ(write(pipe_c2p[1], buf, 1), 1, "pipe_write");
> >
> > /* make sure parent enabled bpf program to send_signal */
> > - ASSERT_EQ(read(pipe_p2c[0], buf, 1), 1, "pipe_read");
> > + if (!ASSERT_EQ(read(pipe_p2c[0], buf, 1), 1, "pipe_read"))
> > + goto child_cleanup;
> >
> > /* wait a little for signal handler */
> > for (int i = 0; i < 1000000000 && !sigusr1_received; i++) {
> > @@ -101,6 +102,7 @@ static void test_send_signal_common(struct
> > perf_event_attr *attr,
> > if (!remote)
> > ASSERT_OK(setpriority(PRIO_PROCESS, 0,
> > old_prio), "setpriority");
> >
> > +child_cleanup:
> > close(pipe_c2p[1]);
> > close(pipe_p2c[0]);
> > exit(0);
> >
> > pw-bot: cr
> >
> >
> > >
> > > /* boost with a high priority so we got a higher chance
> > > * that if an interrupt happens, the underlying task
> > > --
> > > 2.47.3
> > >
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-11-15 1:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-11-13 7:05 [PATCH bpf-next] selftests/bpf: Fix failure path in send_signal test Alexei Starovoitov
2025-11-13 7:31 ` bot+bpf-ci
2025-11-13 21:59 ` Yonghong Song
2025-11-14 22:20 ` Andrii Nakryiko
2025-11-14 22:25 ` Alexei Starovoitov
2025-11-15 1:02 ` Andrii Nakryiko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox