[PATCH bpf-next v5 2/2] selftests/bpf: Add test to verify freeing the special fields when update [lru_,]percpu_hash maps

Leon Hwang posted 2 patches 1 month, 2 weeks ago
There is a newer version of this series
[PATCH bpf-next v5 2/2] selftests/bpf: Add test to verify freeing the special fields when update [lru_,]percpu_hash maps
Posted by Leon Hwang 1 month, 2 weeks ago
Add test to verify that updating [lru_,]percpu_hash 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          | 57 ++++++++++++++++++
 .../selftests/bpf/progs/refcounted_kptr.c     | 60 +++++++++++++++++++
 2 files changed, 117 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/refcounted_kptr.c b/tools/testing/selftests/bpf/prog_tests/refcounted_kptr.c
index d6bd5e16e6372..b95551768d27b 100644
--- a/tools/testing/selftests/bpf/prog_tests/refcounted_kptr.c
+++ b/tools/testing/selftests/bpf/prog_tests/refcounted_kptr.c
@@ -44,3 +44,60 @@ void test_refcounted_kptr_wrong_owner(void)
 	ASSERT_OK(opts.retval, "rbtree_wrong_owner_remove_fail_a2 retval");
 	refcounted_kptr__destroy(skel);
 }
+
+void test_percpu_hash_kptr_refcount_leak(void)
+{
+	struct refcounted_kptr *skel;
+	int cpu_nr, fd, err, key = 0;
+	struct bpf_map *map;
+	size_t values_sz;
+	u64 *values;
+	LIBBPF_OPTS(bpf_test_run_opts, opts,
+		    .data_in = &pkt_v4,
+		    .data_size_in = sizeof(pkt_v4),
+		    .repeat = 1,
+	);
+
+	cpu_nr = libbpf_num_possible_cpus();
+	if (!ASSERT_GT(cpu_nr, 0, "libbpf_num_possible_cpus"))
+		return;
+
+	values = calloc(cpu_nr, sizeof(u64));
+	if (!ASSERT_OK_PTR(values, "calloc values"))
+		return;
+
+	skel = refcounted_kptr__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "refcounted_kptr__open_and_load")) {
+		free(values);
+		return;
+	}
+
+	values_sz = cpu_nr * sizeof(u64);
+	memset(values, 0, values_sz);
+
+	map = skel->maps.percpu_hash;
+	err = bpf_map__update_elem(map, &key, sizeof(key), values, values_sz, 0);
+	if (!ASSERT_OK(err, "bpf_map__update_elem"))
+		goto out;
+
+	fd = bpf_program__fd(skel->progs.percpu_hash_refcount_leak);
+	err = bpf_prog_test_run_opts(fd, &opts);
+	if (!ASSERT_OK(err, "bpf_prog_test_run_opts"))
+		goto out;
+	if (!ASSERT_EQ(opts.retval, 2, "opts.retval"))
+		goto out;
+
+	err = bpf_map__update_elem(map, &key, sizeof(key), values, values_sz, 0);
+	if (!ASSERT_OK(err, "bpf_map__update_elem"))
+		goto out;
+
+	fd = bpf_program__fd(skel->progs.check_percpu_hash_refcount);
+	err = bpf_prog_test_run_opts(fd, &opts);
+	ASSERT_OK(err, "bpf_prog_test_run_opts");
+	ASSERT_EQ(opts.retval, 1, "opts.retval");
+
+out:
+	refcounted_kptr__destroy(skel);
+	free(values);
+}
+
diff --git a/tools/testing/selftests/bpf/progs/refcounted_kptr.c b/tools/testing/selftests/bpf/progs/refcounted_kptr.c
index 893a4fdb4b6e9..87b0cc018f840 100644
--- a/tools/testing/selftests/bpf/progs/refcounted_kptr.c
+++ b/tools/testing/selftests/bpf/progs/refcounted_kptr.c
@@ -568,4 +568,64 @@ int BPF_PROG(rbtree_sleepable_rcu_no_explicit_rcu_lock,
 	return 0;
 }
 
+private(kptr_ref) u64 ref;
+
+static int probe_read_refcount(void)
+{
+	u32 refcount;
+
+	bpf_probe_read_kernel(&refcount, sizeof(refcount), (void *) ref);
+	return refcount;
+}
+
+static int __insert_in_list(struct bpf_list_head *head, struct bpf_spin_lock *lock,
+			    struct node_data __kptr **node)
+{
+	struct node_data *n, *m;
+
+	n = bpf_obj_new(typeof(*n));
+	if (!n)
+		return -1;
+
+	m = bpf_refcount_acquire(n);
+	n = bpf_kptr_xchg(node, n);
+	if (n) {
+		bpf_obj_drop(n);
+		bpf_obj_drop(m);
+		return -2;
+	}
+
+	bpf_spin_lock(lock);
+	bpf_list_push_front(head, &m->l);
+	ref = (u64)(void *) &m->ref;
+	bpf_spin_unlock(lock);
+	return probe_read_refcount();
+}
+
+struct {
+	__uint(type, BPF_MAP_TYPE_PERCPU_HASH);
+	__type(key, int);
+	__type(value, struct map_value);
+	__uint(max_entries, 1);
+} percpu_hash SEC(".maps");
+
+SEC("tc")
+int percpu_hash_refcount_leak(void *ctx)
+{
+	struct map_value *v;
+	int key = 0;
+
+	v = bpf_map_lookup_elem(&percpu_hash, &key);
+	if (!v)
+		return 0;
+
+	return __insert_in_list(&head, &lock, &v->node);
+}
+
+SEC("tc")
+int check_percpu_hash_refcount(void *ctx)
+{
+	return probe_read_refcount();
+}
+
 char _license[] SEC("license") = "GPL";
-- 
2.51.1
Re: [PATCH bpf-next v5 2/2] selftests/bpf: Add test to verify freeing the special fields when update [lru_,]percpu_hash maps
Posted by bot+bpf-ci@kernel.org 1 month, 2 weeks ago
> diff --git a/tools/testing/selftests/bpf/progs/refcounted_kptr.c b/tools/testing/selftests/bpf/progs/refcounted_kptr.c
> index 893a4fdb4..87b0cc018 100644
> --- a/tools/testing/selftests/bpf/progs/refcounted_kptr.c
> +++ b/tools/testing/selftests/bpf/progs/refcounted_kptr.c

[ ... ]

> @@ -568,4 +568,64 @@ int BPF_PROG(rbtree_sleepable_rcu_no_explicit_rcu_lock,
>  	return 0;
>  }
>
> +private(kptr_ref) u64 ref;
> +
> +static int probe_read_refcount(void)
> +{
> +	u32 refcount;
> +
> +	bpf_probe_read_kernel(&refcount, sizeof(refcount), (void *) ref);
> +	return refcount;
> +}
> +
> +static int __insert_in_list(struct bpf_list_head *head, struct bpf_spin_lock *lock,
> +			    struct node_data __kptr **node)
> +{
> +	struct node_data *n, *m;
> +
> +	n = bpf_obj_new(typeof(*n));
> +	if (!n)
> +		return -1;
> +
> +	m = bpf_refcount_acquire(n);
> +	n = bpf_kptr_xchg(node, n);
> +	if (n) {
> +		bpf_obj_drop(n);
> +		bpf_obj_drop(m);
> +		return -2;
> +	}

In __insert_in_list(), after bpf_kptr_xchg() stores the new object in
the map and returns the old value in n, can the error path drop both
n and m? At this point, the new object (pointed to by m) is already
referenced by the map. Dropping m here would free an object that the
map still points to, leaving a dangling pointer.

The test itself never triggers this path since it initializes the map
with zeros first, so the kptr field starts as NULL. However, if this
test were extended or this pattern copied elsewhere, the incorrect
error handling could lead to a use-after-free.

> +
> +	bpf_spin_lock(lock);
> +	bpf_list_push_front(head, &m->l);
> +	ref = (u64)(void *) &m->ref;
> +	bpf_spin_unlock(lock);
> +	return probe_read_refcount();
> +}

[ ... ]


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/19072261328
Re: [PATCH bpf-next v5 2/2] selftests/bpf: Add test to verify freeing the special fields when update [lru_,]percpu_hash maps
Posted by Alexei Starovoitov 1 month, 2 weeks ago
On Tue, Nov 4, 2025 at 6:52 AM <bot+bpf-ci@kernel.org> wrote:
>
> > diff --git a/tools/testing/selftests/bpf/progs/refcounted_kptr.c b/tools/testing/selftests/bpf/progs/refcounted_kptr.c
> > index 893a4fdb4..87b0cc018 100644
> > --- a/tools/testing/selftests/bpf/progs/refcounted_kptr.c
> > +++ b/tools/testing/selftests/bpf/progs/refcounted_kptr.c
>
> [ ... ]
>
> > @@ -568,4 +568,64 @@ int BPF_PROG(rbtree_sleepable_rcu_no_explicit_rcu_lock,
> >       return 0;
> >  }
> >
> > +private(kptr_ref) u64 ref;
> > +
> > +static int probe_read_refcount(void)
> > +{
> > +     u32 refcount;
> > +
> > +     bpf_probe_read_kernel(&refcount, sizeof(refcount), (void *) ref);
> > +     return refcount;
> > +}
> > +
> > +static int __insert_in_list(struct bpf_list_head *head, struct bpf_spin_lock *lock,
> > +                         struct node_data __kptr **node)
> > +{
> > +     struct node_data *n, *m;
> > +
> > +     n = bpf_obj_new(typeof(*n));
> > +     if (!n)
> > +             return -1;
> > +
> > +     m = bpf_refcount_acquire(n);
> > +     n = bpf_kptr_xchg(node, n);
> > +     if (n) {
> > +             bpf_obj_drop(n);
> > +             bpf_obj_drop(m);
> > +             return -2;
> > +     }
>
> In __insert_in_list(), after bpf_kptr_xchg() stores the new object in
> the map and returns the old value in n, can the error path drop both
> n and m? At this point, the new object (pointed to by m) is already
> referenced by the map. Dropping m here would free an object that the
> map still points to, leaving a dangling pointer.

AI is wrong, but I bet it got confused by reuse of variable 'n'.
It's hard for humans too.
Leon,
please use a different var.
n = bpf_kptr_xchg(node, n); is a head scratcher.

Also see Yonghong's comment on v4 which I suspect applies to v5.

pw-bot: cr
Re: [PATCH bpf-next v5 2/2] selftests/bpf: Add test to verify freeing the special fields when update [lru_,]percpu_hash maps
Posted by Leon Hwang 1 month, 1 week ago

On 2025/11/5 01:37, Alexei Starovoitov wrote:
> On Tue, Nov 4, 2025 at 6:52 AM <bot+bpf-ci@kernel.org> wrote:
>>
>>> diff --git a/tools/testing/selftests/bpf/progs/refcounted_kptr.c b/tools/testing/selftests/bpf/progs/refcounted_kptr.c
>>> index 893a4fdb4..87b0cc018 100644
>>> --- a/tools/testing/selftests/bpf/progs/refcounted_kptr.c
>>> +++ b/tools/testing/selftests/bpf/progs/refcounted_kptr.c
>>
>> [ ... ]
>>
>>> @@ -568,4 +568,64 @@ int BPF_PROG(rbtree_sleepable_rcu_no_explicit_rcu_lock,
>>>       return 0;
>>>  }
>>>
>>> +private(kptr_ref) u64 ref;
>>> +
>>> +static int probe_read_refcount(void)
>>> +{
>>> +     u32 refcount;
>>> +
>>> +     bpf_probe_read_kernel(&refcount, sizeof(refcount), (void *) ref);
>>> +     return refcount;
>>> +}
>>> +
>>> +static int __insert_in_list(struct bpf_list_head *head, struct bpf_spin_lock *lock,
>>> +                         struct node_data __kptr **node)
>>> +{
>>> +     struct node_data *n, *m;
>>> +
>>> +     n = bpf_obj_new(typeof(*n));
>>> +     if (!n)
>>> +             return -1;
>>> +
>>> +     m = bpf_refcount_acquire(n);
>>> +     n = bpf_kptr_xchg(node, n);
>>> +     if (n) {
>>> +             bpf_obj_drop(n);
>>> +             bpf_obj_drop(m);
>>> +             return -2;
>>> +     }
>>
>> In __insert_in_list(), after bpf_kptr_xchg() stores the new object in
>> the map and returns the old value in n, can the error path drop both
>> n and m? At this point, the new object (pointed to by m) is already
>> referenced by the map. Dropping m here would free an object that the
>> map still points to, leaving a dangling pointer.
> 
> AI is wrong, but I bet it got confused by reuse of variable 'n'.
> It's hard for humans too.
> Leon,
> please use a different var.
> n = bpf_kptr_xchg(node, n); is a head scratcher.

No problem.

I'll update the variable names in the next revision.

> 
> Also see Yonghong's comment on v4 which I suspect applies to v5.

That was actually a misunderstanding — he didn't run the newly added tests.

Still, I'll update the test name to include "refcounted_kptr" to make it
clearer and help avoid such confusion in the future.

Thanks,
Leon