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

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 <[email protected]>
>> Reviewed-by: Ilya Leoshkevich <[email protected]>
>> Signed-off-by: Maxim Khmelevskii <[email protected]>
>> ---
> 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?