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

[Eduard Zingerman <eddyz87@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2096 bytes --]

On Thu, 2025-11-13 at 10:40 +0000, Puranjay Mohan wrote:

[...]

> 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;
> +		}
> +	}

Instead of having this logic in two places, how about moving the
desc->imm setup down to sort_kfunc_descs_by_imm_off()?
I think it the only consumer of desc->imm in verifier.c.
E.g. as in the diff attached.

> +
>  	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;
>  		}
> +		desc->imm = call_imm;
>  	}
> -	desc->imm = call_imm;
>  	desc->addr = addr;
>  	return 0;
>  }

[-- Attachment #2: kfunc-desc-imm.diff --]
[-- Type: text/x-patch, Size: 2667 bytes --]

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 1268fa075d4c..7ffe526c34cb 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3391,16 +3391,44 @@ static int kfunc_desc_cmp_by_imm_off(const void *a, const void *b)
 	return 0;
 }
 
-static void sort_kfunc_descs_by_imm_off(struct bpf_prog *prog)
+static int set_kfunc_desc_imm(struct bpf_verifier_env *env, struct bpf_kfunc_desc *desc)
+{
+	unsigned long call_imm;
+
+	if (bpf_jit_supports_far_kfunc_call()) {
+		call_imm = desc->func_id;
+		return 0;
+	} else {
+		call_imm = BPF_CALL_IMM(desc->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",
+				desc->func_id);
+			return -EINVAL;
+		}
+	}
+	desc->imm = call_imm;
+	return 0;
+}
+
+static int sort_kfunc_descs_by_imm_off(struct bpf_verifier_env *env)
 {
 	struct bpf_kfunc_desc_tab *tab;
+	int i, err;
 
-	tab = prog->aux->kfunc_tab;
+	tab = env->prog->aux->kfunc_tab;
 	if (!tab)
-		return;
+		return 0;
+
+	for (i = 0; i < tab->nr_descs; i++) {
+		err = set_kfunc_desc_imm(env, &tab->descs[i]);
+		if (err)
+			return err;
+	}
 
 	sort(tab->descs, tab->nr_descs, sizeof(tab->descs[0]),
 	     kfunc_desc_cmp_by_imm_off, NULL);
+	return 0;
 }
 
 bool bpf_prog_has_kfunc_call(const struct bpf_prog *prog)
@@ -22320,10 +22348,10 @@ static int specialize_kfunc(struct bpf_verifier_env *env, struct bpf_kfunc_desc
 	bool is_rdonly;
 	u32 func_id = desc->func_id;
 	u16 offset = desc->offset;
-	unsigned long addr = desc->addr, call_imm;
+	unsigned long addr = desc->addr;
 
 	if (offset) /* return if module BTF is used */
-		goto set_imm;
+		return 0;
 
 	if (bpf_dev_bound_kfunc_id(func_id)) {
 		xdp_kfunc = bpf_dev_bound_resolve_kfunc(prog, func_id);
@@ -22351,19 +22379,6 @@ static int specialize_kfunc(struct bpf_verifier_env *env, struct bpf_kfunc_desc
 		if (!env->insn_aux_data[insn_idx].non_sleepable)
 			addr = (unsigned long)bpf_dynptr_from_file_sleepable;
 	}
-
-set_imm:
-	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->imm = call_imm;
 	desc->addr = addr;
 	return 0;
 }
@@ -23441,7 +23456,9 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
 		}
 	}
 
-	sort_kfunc_descs_by_imm_off(env->prog);
+	ret = sort_kfunc_descs_by_imm_off(env);
+	if (ret)
+		return ret;
 
 	return 0;
 }