linux-kernel - Re: [PATCH bpf-next] bpf/selftests: Fix send

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20230310061452.GB1022987@maniforge>
Date:   Fri, 10 Mar 2023 00:14:52 -0600
From:   David Vernet <void@...ifault.com>
To:     bpf@...r.kernel.org
Cc:     ast@...nel.org, daniel@...earbox.net, andrii@...nel.org,
        martin.lau@...ux.dev, song@...nel.org, yhs@...a.com,
        john.fastabend@...il.com, kpsingh@...nel.org, sdf@...gle.com,
        haoluo@...gle.com, jolsa@...nel.org, linux-kernel@...r.kernel.org,
        kernel-team@...a.com
Subject: Re: [PATCH bpf-next] bpf/selftests: Fix send_signal tracepoint tests

On Fri, Mar 10, 2023 at 12:10:48AM -0600, David Vernet wrote:
> The send_signal tracepoint tests are non-deterministically failing in
> CI. The test works as follows:
> 
> 1. Two pairs of file descriptors are created using the pipe() function.
>    One pair is used to communicate between a parent process -> child
>    process, and the other for the reverse direction.
> 
> 2. A child is fork()'ed. The child process registers a signal handler,
>    notifies its parent that the signal handler is registered, and then
>    and waits for its parent to have enabled a BPF program that sends a
>    signal.
> 
> 3. The parent opens and loads a BPF skeleton with programs that send
>    signals to the child process. The different programs are triggered by
>    different perf events (either NMI or normal perf), or by regular
>    tracepoints. The signal is delivered to the child whenever the child
>    triggers the program.
> 
> 4. The child's signal handler is invoked, which sets a flag saying that
>    the signal handler was reached. The child then signals to the parent
>    that it received the signal, and the test ends.
> 
> The perf testcases (send_signal_perf{_thread} and
> send_signal_nmi{_thread}) work 100% of the time, but the tracepoint
> testcases fail non-deterministically because the tracepoint is not
> always being fired for the child.
> 
> There are two tracepoint programs registered in the test:
> 'tracepoint/sched/sched_switch', and
> 'tracepoint/syscalls/sys_enter_nanosleep'. The child never intentionally
> blocks, nor sleeps, so neither tracepoint is guaranteed to be triggered.
> To fix this, we can have the child trigger the nanosleep program with a
> usleep().
> 
> Before this patch, the test would fail locally every 2-3 runs. Now, it
> doesn't fail after more than 1000 runs.
> 
> Signed-off-by: David Vernet <void@...ifault.com>
> ---
>  tools/testing/selftests/bpf/prog_tests/send_signal.c | 5 ++++-
>  1 file changed, 4 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 d63a20fbed33..61cc83fca53c 100644
> --- a/tools/testing/selftests/bpf/prog_tests/send_signal.c
> +++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c
> @@ -64,8 +64,11 @@ static void test_send_signal_common(struct perf_event_attr *attr,
>  		ASSERT_EQ(read(pipe_p2c[0], buf, 1), 1, "pipe_read");
>  
>  		/* wait a little for signal handler */
> -		for (int i = 0; i < 1000000000 && !sigusr1_received; i++)
> +		for (int i = 0; i < 1000000000 && !sigusr1_received; i++) {
>  			j /= i + j + 1;
> +			if (!attr)
> +				ASSERT_EQ(usleep(1), 0, "nanosleep_tp");

As soon as I sent this out, it occurred to me that having an ASSERT_EQ
like this is not a good idea. usleep() could be interrupted by a signal
and return EINTR, and the whole point of this test is to send signals to
the child. Let me resend this as v2 without the ASSERT_EQ.

> +		}
>  
>  		buf[0] = sigusr1_received ? '2' : '0';
>  		ASSERT_EQ(sigusr1_received, 1, "sigusr1_received");
> -- 
> 2.39.0
>