Re: [PATCH bpf-next 2/2] selftests/bpf: Test bpf_send_signal_task() from kernel thread context

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The newly added `send_signal_kworker` program silently runs during existing remote tests, sending the signal instead of the intended programs (like perf or NMI) and invalidating their test coverage.
- [Low] Multi-line comments do not follow the BPF subsystem style guidelines.
--

commit 2bcfd3bbad998237f70d4a407001c43f11ecd4c9
Author: Aditya Sharma <adi.sharma@zohomail.in>
selftests/bpf: Test bpf_send_signal_task() from kernel thread context

This commit adds a new test to verify bpf_send_signal_task() from a kernel
thread context. It attaches a BPF program to a workqueue tracepoint to
ensure signals can be properly sent when the caller is a kworker.

> diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c
> index 7ac4d5a488aa5..f9dd23db1ef98 100644
> --- a/tools/testing/selftests/bpf/prog_tests/send_signal.c
> +++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c
[ ... ]
> +	if (pid == 0) {
> +		/* install signal handler and notify parent */
> +		sa.sa_sigaction = sigusr1_siginfo_handler;
> +		sa.sa_flags = SA_RESTART | SA_SIGINFO;
> +		ASSERT_NEQ(sigaction(SIGUSR1, &sa, NULL), -1, "sigaction");
> +
> +		close(pipe_c2p[0]); /* close read */
> +		close(pipe_p2c[1]); /* close write */
> +
> +		/* notify parent signal handler is installed */
> +		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");
> +
> +		/* the signal is sent from workqueue context, so nothing has
> +		 * to be triggered from here
> +		 */

[Severity: Low]
This isn't a bug, but does this multi-line comment follow the BPF subsystem
style guidelines? The BPF subsystem mandates that multi-line comments must
have the opening /* on its own line.

> +		while (!sigusr1_received)
> +			sleep(1);
> +
> +		buf[0] = sigusr1_received;
> +
> +		ASSERT_EQ(sigusr1_received, 8, "sigusr1_received");
> +		ASSERT_EQ(write(pipe_c2p[1], buf, 1), 1, "pipe_write");
[ ... ]
> diff --git a/tools/testing/selftests/bpf/progs/test_send_signal_kern.c b/tools/testing/selftests/bpf/progs/test_send_signal_kern.c
> index 176a355e30624..d1c000c308857 100644
> --- a/tools/testing/selftests/bpf/progs/test_send_signal_kern.c
> +++ b/tools/testing/selftests/bpf/progs/test_send_signal_kern.c
[ ... ]
> @@ -60,6 +60,32 @@ int send_signal_tp_sched(void *ctx)
>  	return bpf_send_signal_test(ctx);
>  }
>  
> +/* Send a signal to a task other than current, from a context where current
> + * is a kernel thread. No filtering on current is possible here, so this is
> + * driven entirely by target_pid.
> + */

[Severity: Low]
This isn't a bug, but similar to the comment in send_signal.c, should the
opening /* be on its own line here as well to match the BPF style
guidelines?

> +SEC("tp_btf/workqueue_execute_start")
> +int send_signal_kworker(void *ctx)
> +{
> +	struct task_struct *target_task;
> +	int ret;
> +
> +	if (status != 0 || target_pid == 0)
> +		return 0;

[Severity: Medium]
Could this condition cause the kworker program to silently run during other
remote tests and invalidate their test coverage?

During existing remote tests, both the pid and target_pid global variables
are set to non-zero values. Because this program is auto-attached by
test_send_signal_kern__attach() and only checks target_pid == 0, it appears
it will run and send the signal when background workqueue events occur
during those other tests.

If this program delivers the signal and sets status = 1, it could preempt
the intended test programs (like send_signal_tp or send_signal_perf),
causing the remote tests to spuriously pass without actually exercising
their specific BPF programs.

Would it make sense to also verify pid == 0 here to guarantee this program
remains inert when the other tests are running?

> +
> +	target_task = bpf_task_from_pid(target_pid);
> +	if (!target_task)
> +		return 0;
> +
> +	ret = bpf_send_signal_task(target_task, sig, PIDTYPE_TGID, 8);
> +	bpf_task_release(target_task);
> +
> +	if (ret == 0)
> +		status = 1;
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902150439.692725-1-adi.sharma@zohomail.in?part=2