Re: [PATCH bpf-next v1 4/5] selftests/bpf: Exercise unsafe obj drops from tracing progs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Replacing bpf_obj_free_fields() with bpf_obj_cancel_fields() in map update and delete paths breaks BTF special field initialization and isolation, leading to cross-key reference hijacking in hash maps and un-clearable fields in array maps.
--

commit 0b4e0636a1d77531077ad0491c76e6b6595a28c6
Author: Justin Suess <utilityemal77@gmail.com>

bpf: Cancel special fields on map value recycle

This commit introduces bpf_obj_cancel_fields() to defer the destruction
of special fields (like referenced kptrs and graph roots) until map
teardown, rather than destroying them during map update or delete operations.
It updates array and hash map paths to use this cancellation helper,
explicitly relaxing the eager-free semantics for these fields. Although tests
were added in related commits (like selftests/bpf: Exercise kptr map update
lifetime) asserting that the refcount remains elevated after an update, they
failed to account for memory reuse.

> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
> index e6271a2bf6d64..4e8d6fe6999bf 100644
> --- a/kernel/bpf/arraymap.c
> +++ b/kernel/bpf/arraymap.c
> @@ -384,7 +384,7 @@ static long array_map_update_elem(struct bpf_map *map, void *key, void *value,
>  	if (array->map.map_type == BPF_MAP_TYPE_PERCPU_ARRAY) {
>  		val = this_cpu_ptr(array->pptrs[index & array->index_mask]);
>  		copy_map_value(map, val, value);
> -		bpf_obj_free_fields(array->map.record, val);
> +		bpf_obj_cancel_fields(array->map.record, val);
>  	} else {
>  		val = array->value +
>  			(u64)array->elem_size * (index & array->index_mask);
> @@ -392,7 +392,7 @@ static long array_map_update_elem(struct bpf_map *map, void *key, void *value,
>  			copy_map_value_locked(map, val, value, false);
>  		else
>  			copy_map_value(map, val, value);
> -		bpf_obj_free_fields(array->map.record, val);
> +		bpf_obj_cancel_fields(array->map.record, val);

[Severity: Critical]
Since array_map_update_elem() updates the element in place, does this mean
array elements permanently retain old references?

If copy_map_value() skips copying over special fields, and the new
bpf_obj_cancel_fields() no longer clears them, it seems the old kptrs
are left intact on the array element. Wouldn't this make it impossible
for userspace to clear a kptr from an array map once it is set?

>  	}
>  	return 0;
>  }

[ ... ]

> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index b4366cad3cfa5..a9b2b882b90f2 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c

[ ... ]

> @@ -844,11 +848,11 @@ static void check_and_cancel_fields(struct bpf_htab *htab,
>  		int cpu;
>  
>  		for_each_possible_cpu(cpu)
> -			bpf_obj_free_fields(htab->map.record, per_cpu_ptr(pptr, cpu));
> +			bpf_obj_cancel_fields(htab->map.record, per_cpu_ptr(pptr, cpu));
>  	} else {
>  		void *map_value = htab_elem_value(elem, htab->map.key_size);
>  
> -		bpf_obj_free_fields(htab->map.record, map_value);
> +		bpf_obj_cancel_fields(htab->map.record, map_value);
>  	}
>  }

[Severity: Critical]
Can this lead to cross-key reference hijacking when hash map elements
are reused?

When an element is deleted or updated in a hash map, its memory is pushed
back to the bpf_mem_alloc freelist (or extra_elems for preallocated maps).
Since the special fields are no longer cleared, the pushed element retains
its old kptr and lock state.

If alloc_htab_elem() later reuses this memory for a newly inserted key,
and copy_map_value() intentionally skips copying over special fields,
will the new key inherit the old key's reference? A BPF program could then
look up the new key and extract the reference via bpf_kptr_xchg(), breaking
isolation guarantees.

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