Re: [PATCH bpf-next 10/17] bpf: Recognize pointers to functions in read-only maps

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] bpf: verifier incorrectly rejects valid programs and triggers WARN_ONCE when reading uncalled function pointers
- [Medium] bpf: map user state is not restored on JIT failure, breaking libbpf log reloads
--

commit ac5645576aa732578e623a1ca8af9bcff7aa9194
Author: Alexei Starovoitov <ast@kernel.org>

bpf: Recognize pointers to functions in read-only maps

Compilers put pointers to functions into read-only data: tables of
functions, struct ops, vtables, where they're mixed with sizes,
alignments and other data. Rust vtables are like that.
Let callx call through them.

> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 868350977f0cb..273f74068068c 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
[ ... ]
> @@ -3029,6 +3029,11 @@ void __bpf_free_used_maps(struct bpf_prog_aux *aux,
>  			map->ops->map_poke_untrack(map, aux);
>  		if (sleepable)
>  			atomic64_dec(&map->sleepable_refcnt);
> +		/*
> +		 * The program that didn't load is not a user of the map. libbpf
> +		 * loads the program again to get the log of the verifier.
> +		 */
> +		cmpxchg(&map->user, (unsigned long)aux, 0);

[Severity: Medium]
Will this properly clear the map's user state if the BPF_MAP_USER_PATCHED
flag was set before a failure?

If jit_subprogs() sets the BPF_MAP_USER_PATCHED flag, map->user becomes
me | BPF_MAP_USER_PATCHED. If a failure happens afterward, this cmpxchg()
strictly expects map->user to equal aux, which will fail to match.

This can leave the map permanently locked in the patched state, causing
libbpf's subsequent reload attempt (to obtain the verifier log) to fail
with -EBUSY.

>  		bpf_map_put(map);
>  	}
>  }
> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> index 2add8001c3ec3..e568b9b790b5c 100644
> --- a/kernel/bpf/fixups.c
> +++ b/kernel/bpf/fixups.c
[ ... ]
> @@ -1285,6 +1289,57 @@ static int jit_subprogs(struct bpf_verifier_env *env)
>  		cond_resched();
>  	}
>  
> +	/*
> +	 * The addresses of all functions are final. Replace the offsets of
> +	 * functions with them in the maps of the program, see
> +	 * resolve_func_ptrs(). The program must be the only user of such map.
> +	 * From now on no other program can use it, see bpf_map_claim().
> +	 */
> +	for (i = 0; i < env->func_ptr_cnt; i++) {
> +		struct bpf_func_ptr *ptr = &env->func_ptrs[i];
> +		unsigned long me = (unsigned long)prog->aux;
> +		u64 addr, old, new = 0;
> +
> +		/* pointers are sorted by map */
> +		if ((!i || ptr->map != ptr[-1].map) &&
> +		    cmpxchg(&ptr->map->user, me, me | BPF_MAP_USER_PATCHED) != me) {
> +			verbose(env, "map '%s' is used by another program\n", ptr->map->name);
> +			err = -EBUSY;
> +			goto out_free;
> +		}
> +
> +		/* it's the address of the value of the map whatever the offset is */
> +		err = ptr->map->ops->map_direct_value_addr(ptr->map, &addr, 0);
> +		if (verifier_bug_if(err, env, "no value of map '%s'", ptr->map->name)) {
> +			err = -EFAULT;
> +			goto out_free;
> +		}
> +		addr += ptr->map_off;
> +
> +		if (ptr->xlated_off != BPF_FUNC_PTR_DELETED) {
> +			subprog = bpf_find_subprog(env, ptr->xlated_off);
> +			if (verifier_bug_if(subprog <= 0, env, "no function at insn %u",
> +					    ptr->xlated_off)) {
> +				err = -EFAULT;
> +				goto out_free;
> +			}
> +			new = (unsigned long)func[subprog]->bpf_func;
> +		} else if (verifier_bug_if(ptr->used, env, "function of map '%s' offset %u is removed",
> +					   ptr->map->name, ptr->map_off)) {

[Severity: High]
Does eagerly setting ptr->used = true in check_func_ptr_read() lead to a
verifier bug here if the function is never actually called?

If a program simply reads the function pointer from the map but does not
invoke callx, check_func_ptr_read() still flags it as used. Since the
subprogram is never called, dead-code elimination can remove it.

When jit_subprogs() later evaluates the deleted but "used" pointer, it
incorrectly triggers a WARN_ONCE and returns -EFAULT, rejecting a valid
program.

> +			/* the program that reads the pointer might call the function */
> +			err = -EFAULT;
> +			goto out_free;
> +		}
> +		/* else the function is dead code, nothing calls it, the pointer is NULL */
> +
> +		old = (u64)ptr->orig_off * sizeof(struct bpf_insn);
> +		if (verifier_bug_if(cmpxchg64((u64 *)(unsigned long)addr, old, new) != old, env,
> +				    "map '%s' offset %u changed", ptr->map->name, ptr->map_off)) {
> +			err = -EFAULT;
> +			goto out_free;
> +		}
> +	}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260922011323.1298619-1-alexei.starovoitov@gmail.com?part=10