Re: [PATCH bpf v3 4/4] selftests/bpf: Add tests to verify freeing the special fields when update hash and local storage maps

[Leon Hwang <leon.hwang@linux.dev>]

On 2025/10/28 00:34, Amery Hung wrote:
> On Sun, Oct 26, 2025 at 8:42 AM Leon Hwang <leon.hwang@linux.dev> wrote:
>>
>> Add tests to verify that updating hash and local storage maps decrements
>> refcount when BPF_KPTR_REF objects are involved.
>>
>> The tests perform the following steps:
>>
>> 1. Call update_elem() to insert an initial value.
>> 2. Use bpf_refcount_acquire() to increment the refcount.
>> 3. Store the node pointer in the map value.
>> 4. Add the node to a linked list.
>> 5. Probe-read the refcount and verify it is *2*.
>> 6. Call update_elem() again to trigger refcount decrement.
>> 7. Probe-read the refcount and verify it is *1*.
>>
>> Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
>> ---
>>  .../bpf/prog_tests/refcounted_kptr.c          | 178 +++++++++++++++++-
>>  .../selftests/bpf/progs/refcounted_kptr.c     | 160 ++++++++++++++++
>>  2 files changed, 337 insertions(+), 1 deletion(-)
>>

[...]

>> @@ -44,3 +44,179 @@ void test_refcounted_kptr_wrong_owner(void)
>>         ASSERT_OK(opts.retval, "rbtree_wrong_owner_remove_fail_a2 retval");
>>         refcounted_kptr__destroy(skel);
>>  }
>> +
>> +static void test_refcnt_leak(void *values, size_t values_sz, u64 flags, struct bpf_map *map,
>> +                            struct bpf_program *prog_leak, struct bpf_program *prog_check)
>> +{

[...]

>> +}
>
> Just use syscall BPF programs across different subtests, and you can
> share this test_refcnt_leak() across subtests.
>
> It also saves you some code setting up bpf_test_run_opts. You can just
> call bpf_prog_test_run_opts(prog_fd, NULL) as you don't pass any input
> from ctx.
>
>> +
>> +static void test_percpu_hash_refcount_leak(void)
>> +{

[...]

>> +out:
>> +       close(cgroup);
>> +       refcounted_kptr__destroy(skel);
>> +       if (client_fd >= 0)
>> +               close(client_fd);
>> +       if (server_fd >= 0)
>> +               close(server_fd);
>> +}
>
> Then, you won't need to set up server, connection.... just to
> read/write cgroup local storage. Just call test_refcnt_leak() that
> runs the two BPF syscall programs for cgroup local storage.
>

[...]

>
>
> And in syscall BPF program, you can simply get the cgroup through the
> current task
>
> SEC("syscall")
> int syscall_prog(void *ctx)
> {
>         struct task_struct *task = bpf_get_current_task_btf();
>
>         v = bpf_cgrp_storage_get(&cgrp_strg, task->cgroups->dfl_cgrp, 0,
>                                BPF_LOCAL_STORAGE_GET_F_CREATE);
>         ...
> }
>

Hi Amery,

Thanks for the suggestion.

I tried your approach, but the verifier rejected it with the following
error:

0: R1=ctx() R10=fp0
; task = bpf_get_current_task_btf(); @ refcounted_kptr.c:686
0: (85) call bpf_get_current_task_btf#158     ; R0=trusted_ptr_task_struct()
; v = bpf_cgrp_storage_get(&cgrp_strg, task->cgroups->dfl_cgrp, 0, @
refcounted_kptr.c:687
1: (79) r1 = *(u64 *)(r0 +4856)       ; R0=trusted_ptr_task_struct()
R1=untrusted_ptr_css_set()
2: (79) r2 = *(u64 *)(r1 +96)         ; R1=untrusted_ptr_css_set()
R2=untrusted_ptr_cgroup()
3: (18) r1 = 0xffffa1b442a4b800       ; R1=map_ptr(map=cgrp_strg,ks=4,vs=16)
5: (b7) r3 = 0                        ; R3=0
6: (b7) r4 = 1                        ; R4=1
7: (85) call bpf_cgrp_storage_get#210
R2 type=untrusted_ptr_ expected=ptr_, trusted_ptr_, rcu_ptr_
processed 7 insns (limit 1000000) max_states_per_insn 0 total_states 0
peak_states 0 mark_read 0

The issue is that dereferencing task->cgroups->dfl_cgrp results in an
untrusted pointer, which bpf_cgrp_storage_get() doesn't accept in
syscall programs. I will investigate the issue later.

However, while searching for 'task->cgroups->dfl_cgrp' usage in the
selftests, I found that bpf_cgrp_storage_get() works fine in fentry
programs. This approach also has the benefit of avoiding the cgroup
setup and client/server infrastructure.

I'll respin the selftest using fentry programs instead.

Thanks,
Leon

[...]