Re: [PATCH bpf v1 2/8] selftests/bpf: Test killing a loader during instruction rewrites

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Race condition causing false test failures on extremely fast hardware or optimized verifiers.
--

commit e86296a4aae66b4d0778df9e0379e5e9bc8a6bff
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Subject: selftests/bpf: Test killing a loader during instruction rewrites

This commit adds a selftest to verify that a child process stuck in the
verifier's quadratic instruction rewrite phase can be successfully terminated
by a fatal signal.

> diff --git a/tools/testing/selftests/bpf/prog_tests/prog_load_signal.c b/tools/testing/selftests/bpf/prog_tests/prog_load_signal.c
> new file mode 100644
> index 0000000000000..0f78db3bccc76
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/prog_load_signal.c
[ ... ]
> +	if (!pid) {
> +		close(pipefd[0]);
> +		if (write(pipefd[1], "x", 1) != 1)
> +			_exit(1);
> +		close(pipefd[1]);
> +		prog_fd = bpf_prog_load(BPF_PROG_TYPE_SOCKET_FILTER, NULL, "GPL",
> +					insns, NOP_CNT + 2, NULL);
> +		if (prog_fd >= 0)
> +			close(prog_fd);
> +		_exit(prog_fd < 0);
> +	}

[Severity: Medium]
Could the child process exit too quickly on very fast hardware or with an
optimized verifier? If the child completes the load before the parent sends
SIGKILL, it will exit normally and become a zombie.

[ ... ]
> +	/*
> +	 * Allow linear verification to finish before sending SIGKILL. The nop
> +	 * removal pass is quadratic, so four control-load times still leaves a
> +	 * wide window in which an affected kernel is rewriting instructions.
> +	 */
> +	kill_delay = MAX(control_time * 4, MIN_KILL_DELAY_NS);
> +	sleep_ns(kill_delay);

Here, kill_delay enforces a minimum delay of 100ms (MIN_KILL_DELAY_NS). Is it
possible that on modern CPUs with high cache bandwidth, the verifier's 32,768
instruction rewrite could finish in under 100ms?

If control_time is very small, the parent will sleep for 100ms, which might
give the child ample time to finish bpf_prog_load() and exit.

[ ... ]
> +	start = monotonic_ns();
> +	ret = waitpid_timeout(pid, &status, REAP_TIMEOUT_NS);
> +	if (!ASSERT_EQ(ret, pid, "prog_load_killable")) {
> +		fprintf(stderr, "control load %llu us, child still alive %llu us after SIGKILL\n",
> +			control_time / 1000, (monotonic_ns() - start) / 1000);
> +		goto cleanup;
> +	}
> +	pid = -1;
> +	ASSERT_TRUE(WIFSIGNALED(status) && WTERMSIG(status) == SIGKILL,
> +		    "killed_by_sigkill");

If the child finished early and exited normally as described above, waitpid
will retrieve a normal exit status. This assertion will then incorrectly fail
because the child wasn't killed by the signal.

Could this lead to flaky test failures on fast CI machines?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260905070003.3193366-1-memxor@gmail.com?part=2