Re: [PATCH bpf-next v3 08/10] bpf: verifier: refactor kfunc specialization

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

On 10/21/25 00:38, Eduard Zingerman wrote:
> On Mon, 2025-10-20 at 23:25 +0100, Mykyta Yatsenko wrote:
>
> [...]
>
>> @@ -3375,18 +3366,25 @@ static int add_kfunc_call(struct bpf_verifier_env *env, u32 func_id, s16 offset)
>>   			return err;
>>   	}
>>   
>> +	err = btf_distill_func_proto(&env->log, desc_btf,
>> +				     func_proto, func_name,
>> +				     &func_model);
>> +	if (err)
>> +		return err;
>> +
>> +	err = kfunc_call_imm(env, addr, func_id, &call_imm);
>> +	if (err)
>> +		return err;
>> +
> Sorry, I should have asked in v1/v2. Is there a reason to call this
> function two times? In other words, it looks like doing the following
> on top of your changes is sufficient and removes the need to call
> kfunc_call_imm twice:
Yeah, I think we can skip the first call, I got confused initially by the
sort_kfunc_descs_by_imm_off(), which relies on imm to be set, that's why 
in v1 I
also re-sorted array after modifying imm.
It looks like initially we sort by func_id and offset and then modify 
sorting order to imm,
offset, we patch the imm before changing sort order, so it's all good, 
thanks!
>
>      ---- 8< -----------------------------------------
>
>      diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>      index 0418768d13e4..f509f9e0383d 100644
>      --- a/kernel/bpf/verifier.c
>      +++ b/kernel/bpf/verifier.c
>      @@ -3373,13 +3373,8 @@ static int add_kfunc_call(struct bpf_verifier_env *env, u32 func_id, s16 offset)
>              if (err)
>                      return err;
>       
>      -       err = kfunc_call_imm(env, addr, func_id, &call_imm);
>      -       if (err)
>      -               return err;
>      -
>              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;
>      @@ -21892,8 +21887,10 @@ static int specialize_kfunc(struct bpf_verifier_env *env, struct bpf_kfunc_desc
>              unsigned long addr = 0;
>              int err;
>       
>      +       addr = desc->addr;
>      +
>              if (offset) /* return if module BTF is used */
>      -               return 0;
>      +               goto fill_imm;
>       
>              if (bpf_dev_bound_kfunc_id(func_id)) {
>                      xdp_kfunc = bpf_dev_bound_resolve_kfunc(prog, func_id);
>      @@ -21922,9 +21919,7 @@ static int specialize_kfunc(struct bpf_verifier_env *env, struct bpf_kfunc_desc
>                              addr = (unsigned long)bpf_dynptr_from_file_sleepable;
>              }
>       
>      -       if (!addr) /* Nothing to patch with */
>      -               return 0;
>      -
>      +fill_imm:
>              err = kfunc_call_imm(env, addr, func_id, &desc->imm);
>              if (err)
>                      return err;
>
>      ----------------------------------------- >8 ----
>
> The desc->imm field is used only from:
> - kfunc_desc_cmp_by_imm_off():
>    - invoked from do_misc_fixups() at the very end,
>      after all specialize_kfunc() calls are done.
> - bpf_jit_find_kfunc_model(), from some jits.
>
> So, that should be safe.
> Selftests are passing after this change as well.
>
>>   	desc = &tab->descs[tab->nr_descs++];
>>   	desc->func_id = func_id;
>>   	desc->imm = call_imm;
>>   	desc->offset = offset;
>>   	desc->addr = addr;
>> -	err = btf_distill_func_proto(&env->log, desc_btf,
>> -				     func_proto, func_name,
>> -				     &desc->func_model);
>> -	if (!err)
>> -		sort(tab->descs, tab->nr_descs, sizeof(tab->descs[0]),
>> -		     kfunc_desc_cmp_by_id_off, NULL);
>> -	return err;
>> +	desc->func_model = func_model;
>> +	sort(tab->descs, tab->nr_descs, sizeof(tab->descs[0]),
>> +	     kfunc_desc_cmp_by_id_off, NULL);
>> +	return 0;
>>   }
>
> [...]