Re: [PATCH bpf v2 06/11] bpf: Clear insn array jitted pointers on map reuse

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing release semantics in `bpf_insn_array_release` can allow stale JIT pointers to overwrite the re-initialized array due to CPU store reordering.
--

commit 9b6ea61f193d20b96c453fda13952db5c1dc1506
Author: Daniel Borkmann <daniel@iogearbox.net>

bpf: Clear insn array jitted pointers on map reuse

This commit fixes a bug where stale JIT pointers and jitted offsets are left in
an instruction array map when it is reused. It clears these fields during map
initialization in bpf_insn_array_init().

> diff --git a/kernel/bpf/bpf_insn_array.c b/kernel/bpf/bpf_insn_array.c
> index a2f84afe6f7c6..5874abd1f7438 100644
> --- a/kernel/bpf/bpf_insn_array.c
> +++ b/kernel/bpf/bpf_insn_array.c
> @@ -199,12 +199,16 @@ int bpf_insn_array_init(struct bpf_map *map, const struct bpf_prog *prog)
>  		return -EBUSY;
>  
>  	/*
> -	 * Reset all the map indexes to the original values.  This is needed,
> -	 * e.g., when a replay of verification with different log level should
> -	 * be performed.
> +	 * Reset the map to its pre-verification state. The xlated and jitted
> +	 * offsets and the jitted target pointers are recomputed by the verifier
> +	 * and the JIT for this program, so any values left by a previous owner
> +	 * must be cleared here.
>  	 */
> -	for (i = 0; i < map->max_entries; i++)
> +	for (i = 0; i < map->max_entries; i++) {
>  		values[i].xlated_off = values[i].orig_off;
> +		values[i].jitted_off = 0;
> +		insn_array->ips[i] = 0;
> +	}
>  
>  	return 0;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but while looking at how the map is
reused, I noticed a potential missing release semantics issue in
bpf_insn_array_release():

kernel/bpf/bpf_insn_array.c:bpf_insn_array_release() {
	struct bpf_insn_array *insn_array = cast_insn_array(map);

	atomic_set(&insn_array->used, 0);
}

Is it possible that without release semantics (like atomic_set_release), prior
stores to the map can be reordered after this flag is set?

If two threads concurrently load a BPF program using the same
BPF_MAP_TYPE_INSN_ARRAY map:

Thread 1 JITs the program and writes to ips via bpf_prog_update_insn_ptrs(),
but fails a late verification check like bpf_check_tail_call(). It then
releases the map by setting used = 0 via the relaxed atomic_set().

Thread 2 acquires the map using atomic_xchg() and zeroes ips during
initialization, as added in this patch.

Could CPU reordering on weakly-ordered architectures allow Thread 1's delayed
JIT stores to overwrite the zeros with stale pointers, causing Thread 2 to
subsequently use those stale pointers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914170326.249953-1-daniel@iogearbox.net?part=6