[PATCH bpf-next] selftests/bpf: Fix failure path in send_signal test

[PATCH bpf-next] selftests/bpf: Fix failure path in send_signal test

[Alexei Starovoitov]

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

Re: [PATCH bpf-next] selftests/bpf: Fix failure path in send_signal test

[bot+bpf-ci]

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

Re: [PATCH bpf-next] selftests/bpf: Fix failure path in send_signal test

[Yonghong Song]

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>

Re: [PATCH bpf-next] selftests/bpf: Fix failure path in send_signal test

[Andrii Nakryiko]

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
>

Re: [PATCH bpf-next] selftests/bpf: Fix failure path in send_signal test

[Alexei Starovoitov]

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

Re: [PATCH bpf-next] selftests/bpf: Fix failure path in send_signal test

[Andrii Nakryiko]

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