Re: [PATCH bpf-next v2] bpf: verifier: initialize imm in kfunc_tab in add_kfunc_call()

[Mykyta Yatsenko <mykyta.yatsenko5@gmail.com>]

On 11/13/25 10:40, Puranjay Mohan wrote:
> Metadata about a kfunc call is added to the kfunc_tab in
> add_kfunc_call() but the call instruction itself could get removed by
> opt_remove_dead_code() later if it is not reachable.
>
> If the call instruction is removed, specialize_kfunc() is never called
> for it and the desc->imm in the kfunc_tab is never initialized for this
> kfunc call. In this case, sort_kfunc_descs_by_imm_off(env->prog); in
> do_misc_fixups() doesn't sort the table correctly.
> This is a problem from s390 as its JIT uses this table to find the
> addresses for kfuncs, and if this table is not sorted properly, JIT can
> fail to find addresses for valid kfunc calls.
>
> This was exposed by:
>
> commit d869d56ca848 ("bpf: verifier: refactor kfunc specialization")
>
> as before this commit, desc->imm was initialised in add_kfunc_call().
>
> Initialize desc->imm in add_kfunc_call(), it will be overwritten with new
> imm in specialize_kfunc() if the instruction is not removed.
>
> Signed-off-by: Puranjay Mohan <puranjay@kernel.org>
> ---
>
> Changes in v1->v2:
> v1: https://lore.kernel.org/all/20251111160949.45623-1-puranjay@kernel.org/
> - Removed fixes tag as the broken commit is not upstream yet.
> - Initialize desc->imm with the correct value for both with and without
>    bpf_jit_supports_far_kfunc_call() for completeness.
> - Don't re-initialize desc->imm to func_id in specialize_kfunc() as it
>    it already have that value, it only needs to be updated in the
>    !bpf_jit_supports_far_kfunc_call() case where the imm can change.
>
> This bug is not triggered by the CI currently, I am working on another
> set for non-sleepbale arena allocations and as part of that I am adding
> a new selftest that triggers this bug.
>
> Selftest: https://github.com/kernel-patches/bpf/pull/10242/commits/1f681f022c6d685fd76695e5eafbe9d9ab4c0002
> CI run: https://github.com/kernel-patches/bpf/actions/runs/19238699806/job/54996376908
>
> ---
>
>   kernel/bpf/verifier.c | 20 +++++++++++++++-----
>   1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 1268fa075d4c..31136f9c418b 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -3273,7 +3273,7 @@ static int add_kfunc_call(struct bpf_verifier_env *env, u32 func_id, s16 offset)
>   	struct bpf_kfunc_desc *desc;
>   	const char *func_name;
>   	struct btf *desc_btf;
> -	unsigned long addr;
> +	unsigned long addr, call_imm;
>   	int err;
>   
>   	prog_aux = env->prog->aux;
> @@ -3369,8 +3369,20 @@ static int add_kfunc_call(struct bpf_verifier_env *env, u32 func_id, s16 offset)
>   	if (err)
>   		return err;
>   
> +	if (bpf_jit_supports_far_kfunc_call()) {
> +		call_imm = func_id;
> +	} else {
> +		call_imm = BPF_CALL_IMM(addr);
> +		/* Check whether the relative offset overflows desc->imm */
> +		if ((unsigned long)(s32)call_imm != call_imm) {
> +			verbose(env, "address of kernel func_id %u is out of range\n", func_id);
> +			return -EINVAL;
> +		}
> +	}
> +
>   	desc = &tab->descs[tab->nr_descs++];
>   	desc->func_id = func_id;
> +	desc->imm = call_imm;
>   	desc->offset = offset;
>   	desc->addr = addr;
>   	desc->func_model = func_model;
> @@ -22353,17 +22365,15 @@ static int specialize_kfunc(struct bpf_verifier_env *env, struct bpf_kfunc_desc
>   	}
>   
>   set_imm:
> -	if (bpf_jit_supports_far_kfunc_call()) {
> -		call_imm = func_id;
> -	} else {
> +	if (!bpf_jit_supports_far_kfunc_call()) {
>   		call_imm = BPF_CALL_IMM(addr);
>   		/* Check whether the relative offset overflows desc->imm */
>   		if ((unsigned long)(s32)call_imm != call_imm) {
>   			verbose(env, "address of kernel func_id %u is out of range\n", func_id);
>   			return -EINVAL;
>   		}
Not a big deal, but maybe extracting this piece of code into a separate 
function will
make it better a little bit. It makes it easier to debug verifier when 
any concrete error
is produced only once, then you know where to put the breakpoint, less 
chances to miss something.
> +		desc->imm = call_imm;
>   	}
> -	desc->imm = call_imm;
>   	desc->addr = addr;
>   	return 0;
>   }