Re: [PATCH bpf-next] selftests/bpf: Fix flaky refcount asserts in map_kptr_race

Re: [PATCH bpf-next] selftests/bpf: Fix flaky refcount asserts in map_kptr_race

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The polling loop in `wait_for_refs` continues retrying after a fatal error in `read_refs`, causing unnecessary delays and test log spam.
- [Medium] A Time-Of-Check to Time-Of-Use (TOCTOU) race exists between `wait_for_refs()` and the subsequent `ASSERT_EQ()`, which can still cause flaky test failures.
--

--- Patch [1]: [PATCH bpf-next] selftests/bpf: Fix flaky refcount asserts in map_kptr_race ---
commit a9dbb8ca45d040e8f920633939d7db5945ecaca2
Author: Maxim Khmelevskii <max@linux.ibm.com>

selftests/bpf: Fix flaky refcount asserts in map_kptr_race
    
bpf_testmod shares objects across tests, including the prog_test_struct.
Refcounter release is deferred behind an RCU grace period, so it may still
be pending. This patch adds wait_for_refs to prevent flaky failures when
other tests have contaminated the refcount.

> diff --git a/tools/testing/selftests/bpf/prog_tests/map_kptr_race.c b/tools/testing/selftests/bpf/prog_tests/map_kptr_race.c
> index 506ed55e85289..5a543b3465a80 100644
> --- a/tools/testing/selftests/bpf/prog_tests/map_kptr_race.c
> +++ b/tools/testing/selftests/bpf/prog_tests/map_kptr_race.c
> @@ -28,6 +28,15 @@ static int read_refs(struct map_kptr_race *skel)
>  	return skel->bss->num_of_refs;
>  }
>  
> +static void wait_for_refs(struct map_kptr_race *skel)
> +{
> +	for (int i = 0; i < 500; i++) {
> +		if (read_refs(skel) == 2)
> +			return;
> +		usleep(10 * 1000);
> +	}
> +}
> +

[Severity: Medium]
Since read_refs() uses ASSERT_OK() which logs an error message on failure
and returns -1, does this loop introduce a regression by failing to handle
fatal errors correctly?

If read_refs() returns a negative error code (for example, if
bpf_prog_test_run_opts fails), it appears this loop will ignore it and
retry 500 times.

This would result in 500 redundant assertion failures in the test output
and an unnecessary 5-second stall before the test finally fails.

Could we add a check to break early if read_refs() returns a value < 0?

[ ... ]

> @@ -73,6 +82,7 @@ static void test_htab_leak(void)
>  		sched_yield();
>  
>  	ASSERT_EQ(watcher->bss->map_freed, 1, "map_freed");
> +	wait_for_refs(watcher);
>  	ASSERT_EQ(read_refs(watcher), 2, "htab refcount");
>  

[Severity: Medium]
Is there a potential time-of-check to time-of-use regression here that could
still result in flaky tests?

wait_for_refs() waits for the condition and returns, but then the shared
state is read again via ASSERT_EQ(read_refs(watcher), 2).

Because tests run in parallel and share the bpf_testmod kernel module,
another test could increment the refcount in the unprotected window between
wait_for_refs() returning and read_refs() executing inside the assertion.
This same pattern also appears in test_percpu_htab_leak() and 
test_sk_ls_leak().

Could wait_for_refs() return the final refcount value or boolean success
status so the double-read can be avoided?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723114042.559926-2-max@linux.ibm.com?part=1

Re: [PATCH bpf-next] selftests/bpf: Fix flaky refcount asserts in map_kptr_race

[Kumar Kartikeya Dwivedi]

On Thu Jul 23, 2026 at 1:40 PM CEST, Maxim Khmelevskii wrote:
> bpf_testmod shares objects across tests; prog_test_struct that holds
> cnt.refs.counter is one of them. Refcounter release is deferred behind
> an RCU grace period, so it may still be pending when this test asserts.
> wait_for_refs prevents flaky failures when other tests have contaminated
> the refcount.
>
> Fixed tests:
>  - test_htab_leak
>  - test_percpu_htab_leak
>  - test_sk_ls_leak
>
> Reported-by: Ilya Leoshkevich <iii@linux.ibm.com>
> Reviewed-by: Ilya Leoshkevich <iii@linux.ibm.com>
> Signed-off-by: Maxim Khmelevskii <max@linux.ibm.com>
> ---

Wouldn't this still be flaky? Could we order these tests and wait for a RCU gp
to pass before finishing each one? We have kern_sync_rcu(). Either way, I don't
think the current approach is the right way forward.

pw-bot: cr

>  .../testing/selftests/bpf/prog_tests/map_kptr_race.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/map_kptr_race.c b/tools/testing/selftests/bpf/prog_tests/map_kptr_race.c
> index 506ed55e8528..5a543b3465a8 100644
> --- a/tools/testing/selftests/bpf/prog_tests/map_kptr_race.c
> +++ b/tools/testing/selftests/bpf/prog_tests/map_kptr_race.c
> @@ -28,6 +28,15 @@ static int read_refs(struct map_kptr_race *skel)
>  	return skel->bss->num_of_refs;
>  }
>
> +static void wait_for_refs(struct map_kptr_race *skel)
> +{
> +	for (int i = 0; i < 500; i++) {
> +		if (read_refs(skel) == 2)
> +			return;
> +		usleep(10 * 1000);
> +	}
> +}
> +
>  static void test_htab_leak(void)
>  {
>  	LIBBPF_OPTS(bpf_test_run_opts, opts,
> @@ -73,6 +82,7 @@ static void test_htab_leak(void)
>  		sched_yield();
>
>  	ASSERT_EQ(watcher->bss->map_freed, 1, "map_freed");
> +	wait_for_refs(watcher);
>  	ASSERT_EQ(read_refs(watcher), 2, "htab refcount");
>
>  out_watcher:
> @@ -134,6 +144,7 @@ static void test_percpu_htab_leak(void)
>  		sched_yield();
>
>  	ASSERT_EQ(watcher->bss->map_freed, 1, "map_freed");
> +	wait_for_refs(watcher);
>  	ASSERT_EQ(read_refs(watcher), 2, "percpu_htab refcount");
>
>  out_watcher:
> @@ -195,6 +206,7 @@ static void test_sk_ls_leak(void)
>  		sched_yield();
>
>  	ASSERT_EQ(watcher->bss->map_freed, 1, "map_freed");
> +	wait_for_refs(watcher);
>  	ASSERT_EQ(read_refs(watcher), 2, "sk_ls refcount");
>
>  out_watcher: