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

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

[Maxim Khmelevskii]

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>
---
 .../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:
-- 
2.54.0

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:

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

[Maxim Khmelevskii]

On 23/07/2026 18:34, Kumar Kartikeya Dwivedi wrote:
> 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:

 > Wouldn't this still be flaky?

Yes, but it seems like the best solution to me.

 > Could we order these tests and wait for a RCU gp
to pass before finishing each one?

I don't think so, because an RCU gp does not guarantee that htab dtor 
was called.


Let me try to explain these points and please correct me if they are wrong.

That is how htab frees memory:

const struct bpf_map_ops htab_map_ops = {
...
     .map_free = htab_map_free,
...

static void htab_map_free(struct bpf_map *map)
|-> void bpf_mem_alloc_destroy(struct bpf_mem_alloc *ma)
     wait for bpf_mem_refill

static void bpf_mem_refill(struct irq_work *work)
|-> static void free_bulk(struct bpf_mem_cache *c)
     |-> static void do_call_rcu_ttrace(struct bpf_mem_cache *c)
         |-> static inline void call_rcu_tasks_trace(struct rcu_head 
*rhp, rcu_callback_t func)
             register callback __free_rcu

static void __free_rcu(struct rcu_head *head)
|-> static int free_all(struct bpf_mem_cache *c, struct llist_node 
*llnode, bool percpu)
        here the htab destructor is called

If this is correct, htab free happens after RCU gp and task RCU gp, 
which makes the option with waiting for the RCU gp after each test 
pointless. What kind of guarantees kern_sync_rcu gives right now?

Polling on refs seems better because it improves chances for the test to 
pass without introducing major changes and it works with any RCU flavor, 
also the same hack is used already in bpf_testmod_exit.

Considering all of this, what do you think is the way forward?