Re: [PATCH v2 6/6] selftests/bpf: Add a call_srcu() re-entry reproducer

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing control flow branch after ASSERT_OK failure for bpf_map_delete_elem in test_rcu_reentry.
- [Medium] The BPF test program unconditionally marks itself as successful even if it fails to trigger the nested RCU callback.
--

--- Patch [6]: [PATCH v2 6/6] selftests/bpf: Add a call_srcu() re-entry reproducer ---
commit 60310d36c31afce9ef7c76534688f89f2ffa0f29
Author: Puranjay Mohan <puranjay@xxxxxxxxxx>

selftests/bpf: Add a call_srcu() re-entry reproducer

This commit adds a BPF selftest to exercise call_srcu() re-entry safety. It
uses an fentry program on rcu_segcblist_enqueue() to perform a task-storage
delete, which defers work via call_rcu_tasks_trace() and re-enters the
enqueue on the same CPU.

> diff --git a/tools/testing/selftests/bpf/prog_tests/rcu_reentry.c b/tools/testing/selftests/bpf/prog_tests/rcu_reentry.c
> new file mode 100644
> index 0000000000000..f6ecd93be30f4
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/rcu_reentry.c

[ ... ]

> @@ -0,0 +1,58 @@
> +// SPDX-License-Identifier: GPL-2.0

[ ... ]

> +	/* Arm the handler for this thread, then trigger call_rcu_tasks_trace(). */
> +	skel->bss->target_pid = syscall(__NR_gettid);
> +	err = bpf_map_delete_elem(map_fd, &pidfd);
> +	ASSERT_OK(err, "boot_delete");
> +
> +	/* Only Tree SRCU enqueues via rcu_segcblist_enqueue(); skip elsewhere. */
> +	if (!skel->bss->hits) {

[Severity: Medium]
Should this ASSERT_OK() call be placed inside an if statement?

If bpf_map_delete_elem() fails, the boolean return value of ASSERT_OK() is
ignored and the code falls through. The test will evaluate if (!skel->bss->hits)
and incorrectly skip instead of properly aborting.

> +		test__skip();
> +		goto out;
> +	}
> +	ASSERT_EQ(skel->bss->reentered, 1, "reentry_deferred");
> +out:
> +	if (pidfd >= 0)
> +		close(pidfd);
> +	rcu_reentry__destroy(skel);
> +}

> diff --git a/tools/testing/selftests/bpf/progs/rcu_reentry.c b/tools/testing/selftests/bpf/progs/rcu_reentry.c
> new file mode 100644
> index 0000000000000..d92a927ff51c0
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/rcu_reentry.c

[ ... ]

> @@ -0,0 +1,45 @@
> +// SPDX-License-Identifier: GPL-2.0

[ ... ]

> +SEC("fentry/rcu_segcblist_enqueue")
> +int BPF_PROG(reenter)
> +{
> +	struct task_struct *cur;
> +
> +	if (reentered || !target_pid)
> +		return 0;
> +
> +	cur = bpf_get_current_task_btf();
> +	if (!cur || cur->pid != target_pid)
> +		return 0;
> +
> +	/* Re-enter via a task-storage delete, which calls call_rcu_tasks_trace(). */
> +	__sync_fetch_and_add(&hits, 1);
> +	bpf_task_storage_get(&task_stg, cur, 0, BPF_LOCAL_STORAGE_GET_F_CREATE);
> +	bpf_task_storage_delete(&task_stg, cur);
> +
> +	reentered = 1;

[Severity: Medium]
Will this unconditionally mark the test as passed even if the BPF program
failed to trigger the nested callback?

If bpf_task_storage_get() fails (e.g., due to memory limits), the subsequent
bpf_task_storage_delete() will safely fail without triggering the nested
call_rcu_tasks_trace(). The tested deadlock path is never exercised, but
reentered is still set to 1, which will cause the userspace test to assert
success.

> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803134839.2103051-1-puranjay@xxxxxxxxxx?part=6