* Re: [PATCH bpf-next v2] bpf: verifier: initialize imm in kfunc_tab in add_kfunc_call()
2025-11-13 10:40 [PATCH bpf-next v2] bpf: verifier: initialize imm in kfunc_tab in add_kfunc_call() Puranjay Mohan
@ 2025-11-13 10:55 ` Kumar Kartikeya Dwivedi
2025-11-13 15:34 ` Mykyta Yatsenko
2025-11-13 19:57 ` Eduard Zingerman
2 siblings, 0 replies; 5+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2025-11-13 10:55 UTC (permalink / raw)
To: Puranjay Mohan
Cc: bpf, Puranjay Mohan, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Martin KaFai Lau, Eduard Zingerman, kernel-team
On Thu, 13 Nov 2025 at 11:40, Puranjay Mohan <puranjay@kernel.org> 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>
> ---
Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH bpf-next v2] bpf: verifier: initialize imm in kfunc_tab in add_kfunc_call()
2025-11-13 10:40 [PATCH bpf-next v2] bpf: verifier: initialize imm in kfunc_tab in add_kfunc_call() Puranjay Mohan
2025-11-13 10:55 ` Kumar Kartikeya Dwivedi
@ 2025-11-13 15:34 ` Mykyta Yatsenko
2025-11-13 19:57 ` Eduard Zingerman
2 siblings, 0 replies; 5+ messages in thread
From: Mykyta Yatsenko @ 2025-11-13 15:34 UTC (permalink / raw)
To: Puranjay Mohan, bpf
Cc: Puranjay Mohan, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Martin KaFai Lau, Eduard Zingerman,
Kumar Kartikeya Dwivedi, kernel-team
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;
> }
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH bpf-next v2] bpf: verifier: initialize imm in kfunc_tab in add_kfunc_call()
2025-11-13 10:40 [PATCH bpf-next v2] bpf: verifier: initialize imm in kfunc_tab in add_kfunc_call() Puranjay Mohan
2025-11-13 10:55 ` Kumar Kartikeya Dwivedi
2025-11-13 15:34 ` Mykyta Yatsenko
@ 2025-11-13 19:57 ` Eduard Zingerman
2025-11-14 11:32 ` Puranjay Mohan
2 siblings, 1 reply; 5+ messages in thread
From: Eduard Zingerman @ 2025-11-13 19:57 UTC (permalink / raw)
To: Puranjay Mohan, bpf
Cc: Puranjay Mohan, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, Martin KaFai Lau, Kumar Kartikeya Dwivedi,
kernel-team
[-- 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;
}
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH bpf-next v2] bpf: verifier: initialize imm in kfunc_tab in add_kfunc_call()
2025-11-13 19:57 ` Eduard Zingerman
@ 2025-11-14 11:32 ` Puranjay Mohan
0 siblings, 0 replies; 5+ messages in thread
From: Puranjay Mohan @ 2025-11-14 11:32 UTC (permalink / raw)
To: Eduard Zingerman, bpf
Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Martin KaFai Lau, Kumar Kartikeya Dwivedi, kernel-team
Eduard Zingerman <eddyz87@gmail.com> writes:
> 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.
This seems like the best way to move ahead with fixing this. I will send
v3 with your suggested diff.
>> +
>> 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;
>> }
> 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;
> }
^ permalink raw reply [flat|nested] 5+ messages in thread