Re: [PATCH bpf-next v1 5/5] selftests/bpf: Exercise kptr map update lifetime

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] The newly added selftests incorrectly assert that map updates preserve old kptr references, codifying severe preexisting kernel bugs where bpf_obj_free_fields() is missing during map updates. This leads to leaked kptrs and cross-key uninitialized memory corruption in hash maps.
--

commit 4d2aa8d889c01038cf4926ada20dda3564090fb2
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

selftests/bpf: Exercise kptr map update lifetime

Add focused map_kptr coverage for BPF-side map updates that touch values
containing referenced kptrs. The new syscall programs stash the testmod
refcounted object in various maps, then update the same map from BPF.

> diff --git a/tools/testing/selftests/bpf/progs/map_kptr.c b/tools/testing/selftests/bpf/progs/map_kptr.c
> index e708ffbe1f610..3fbefc568e0a6 100644
> --- a/tools/testing/selftests/bpf/progs/map_kptr.c
> +++ b/tools/testing/selftests/bpf/progs/map_kptr.c

[ ... ]

> +SEC("syscall")
> +int test_array_map_update_kptr(void *ctx)
> +{
> +	struct map_value init = {}, *v;
> +	int key = 0, ret;
> +
> +	v = bpf_map_lookup_elem(&array_map, &key);
> +	if (!v)
> +		return 1;
> +	ret = stash_ref_ptr(v);
> +	if (ret)
> +		return ret;
> +	ret = check_refs(3);
> +	if (ret)
> +		return ret;
> +	ret = bpf_map_update_elem(&array_map, &key, &init, BPF_EXIST);
> +	if (ret)
> +		return 4;
> +	return check_refs(3);
> +}
> +
> +#define DEFINE_HASH_UPDATE_KPTR_TEST(name, map)			\
> +SEC("syscall")							\
> +int name(void *ctx)						\
> +{								\
> +	struct map_value init = {}, *v;				\
> +	int key = 0, ret;					\
> +								\
> +	ret = bpf_map_update_elem(&map, &key, &init, BPF_NOEXIST); \
> +	if (ret)						\
> +		return 1;					\
> +	v = bpf_map_lookup_elem(&map, &key);			\
> +	if (!v)							\
> +		return 2;					\
> +	ret = stash_ref_ptr(v);					\
> +	if (ret)						\
> +		return ret;					\
> +	ret = check_refs(3);					\
> +	if (ret)						\
> +		return ret;					\
> +	ret = bpf_map_update_elem(&map, &key, &init, BPF_EXIST); \
> +	if (ret)						\
> +		return 4;					\
> +	return check_refs(3);					\
> +}

[Severity: Critical]
This is a pre-existing issue, but does this selftest actually codify an
existing memory leak and uninitialized memory corruption in the kernel?

Looking at array_map_update_elem() and htab_map_update_elem() in the kernel,
they incorrectly use bpf_obj_cancel_fields() instead of
bpf_obj_free_fields() when overwriting elements. Because
bpf_obj_cancel_fields() explicitly skips BPF_KPTR_REF, the old kptr
references are never freed.

For hash maps specifically, htab_map_update_elem() allocates a new element
(l_new), and copy_map_value() explicitly skips copying special BTF fields
like kptrs. This means l_new receives uninitialized garbage memory for its
kptr field before being inserted into the map. Meanwhile, the old element is
placed in the freelist without its kptr being freed, leaking the reference.

By asserting check_refs(3) after the map update, is the test just measuring
this leak and ignoring the uninitialized garbage now present in the map,
rather than confirming that the kptr was gracefully preserved?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608144841.1732406-1-memxor@gmail.com?part=5