Re: [RFC PATCH v1 08/10] bpf: verifier: refactor kfunc specialization

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

On 10/3/25 23:08, Eduard Zingerman wrote:
> On Fri, 2025-10-03 at 17:04 +0100, Mykyta Yatsenko wrote:
>
> [...]
>
>> @@ -3354,18 +3344,29 @@ 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;
>> +
>> +	call_imm = kfunc_call_imm(addr, func_id);
>> +	/* Check whether the relative offset overflows desc->imm */
>> +	if ((unsigned long)(s32)call_imm != call_imm) {
> This error was previously reported only when !bpf_jit_supports_far_kfunc_call().
>
>> +		verbose(env, "address of kernel function %s is out of range\n",
>> +			func_name);
>> +		return -EINVAL;
>> +	}
>> +
>>   	desc = &tab->descs[tab->nr_descs++];
>>   	desc->func_id = func_id;
>> -	desc->imm = call_imm;
> Nit: no need to move this assignment.
>
>>   	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->imm = call_imm;
>> +	desc->func_model = func_model;
>> +	sort(tab->descs, tab->nr_descs, sizeof(tab->descs[0]),
>> +	     kfunc_desc_cmp_by_id_off, NULL);
>> +	return 0;
>>   }
>>   
>>   static int kfunc_desc_cmp_by_imm_off(const void *a, const void *b)
>> @@ -21822,21 +21823,32 @@ static int fixup_call_args(struct bpf_verifier_env *env)
>>   	return err;
>>   }
>>   
>> +static unsigned long kfunc_call_imm(unsigned long func_addr, u32 func_id)
>> +{
>> +	if (bpf_jit_supports_far_kfunc_call())
>> +		return func_id;
>> +
>> +	return BPF_CALL_IMM(func_addr);
>> +}
>> +
>>   /* replace a generic kfunc with a specialized version if necessary */
>> -static void specialize_kfunc(struct bpf_verifier_env *env,
>> -			     u32 func_id, u16 offset, unsigned long *addr)
>> +static void specialize_kfunc(struct bpf_verifier_env *env, struct bpf_kfunc_desc *desc)
>>   {
>> +	struct bpf_prog_aux *prog_aux = env->prog->aux;
>> +	struct bpf_kfunc_desc_tab *tab = prog_aux->kfunc_tab;
>>   	struct bpf_prog *prog = env->prog;
>>   	bool seen_direct_write;
>>   	void *xdp_kfunc;
>>   	bool is_rdonly;
>> +	u32 func_id = desc->func_id;
>> +	u16 offset = desc->offset;
>> +	unsigned long call_imm;
>> +	unsigned long addr = 0;
>>   
>>   	if (bpf_dev_bound_kfunc_id(func_id)) {
>>   		xdp_kfunc = bpf_dev_bound_resolve_kfunc(prog, func_id);
>> -		if (xdp_kfunc) {
>> -			*addr = (unsigned long)xdp_kfunc;
>> -			return;
>> -		}
>> +		if (xdp_kfunc)
>> +			addr = (unsigned long)xdp_kfunc;
>>   		/* fallback to default kfunc when not supported by netdev */
>>   	}
> Note: right after this line there is:
>
> 	if (offset)      // this checks if kernel or module BTF is used
> 		return;
>
> The refactoring changes behavior at this point:
> previously if `offset != 0` the `addr` computed for dev bound kfunc
> would be assigned to `desc->addr`, after the refactoring this is not
> the case.
>
> On the other hand, bpf_dev_bound_kfunc_id() looks up func_id in set8
> xdp_metadata_kfunc_ids, that contains functions only defined for
> kernel BTF.
>
> Hence, I suggest moving this `if (offset) return` as the first check
> in the function, to avoid confusion.
Thanks for checking, your suggestion makes sense.
>
>>   
>> @@ -21848,21 +21860,28 @@ static void specialize_kfunc(struct bpf_verifier_env *env,
>>   		is_rdonly = !may_access_direct_pkt_data(env, NULL, BPF_WRITE);
>>   
>>   		if (is_rdonly)
>> -			*addr = (unsigned long)bpf_dynptr_from_skb_rdonly;
>> +			addr = (unsigned long)bpf_dynptr_from_skb_rdonly;
>>   
>>   		/* restore env->seen_direct_write to its original value, since
>>   		 * may_access_direct_pkt_data mutates it
>>   		 */
>>   		env->seen_direct_write = seen_direct_write;
>> +	} else if (func_id == special_kfunc_list[KF_bpf_set_dentry_xattr]) {
>> +		if (bpf_lsm_has_d_inode_locked(prog))
>> +			addr = (unsigned long)bpf_set_dentry_xattr_locked;
>> +	} else if (func_id == special_kfunc_list[KF_bpf_remove_dentry_xattr]) {
>> +		if (bpf_lsm_has_d_inode_locked(prog))
>> +			addr = (unsigned long)bpf_remove_dentry_xattr_locked;
>>   	}
>>   
>> -	if (func_id == special_kfunc_list[KF_bpf_set_dentry_xattr] &&
>> -	    bpf_lsm_has_d_inode_locked(prog))
>> -		*addr = (unsigned long)bpf_set_dentry_xattr_locked;
>> +	if (!addr) /* Nothing to patch with */
>> +		return;
>>   
>> -	if (func_id == special_kfunc_list[KF_bpf_remove_dentry_xattr] &&
>> -	    bpf_lsm_has_d_inode_locked(prog))
>> -		*addr = (unsigned long)bpf_remove_dentry_xattr_locked;
>> +	call_imm = kfunc_call_imm(addr, func_id);
>> +	desc->imm = call_imm;
> Nit:	desc->imm = kfunc_call_imm(addr, func_id);
>
>> +	desc->addr = addr;
>> +	sort(tab->descs, tab->nr_descs, sizeof(tab->descs[0]),
>> +	     kfunc_desc_cmp_by_id_off, NULL);
> Why sorting again?
> Neither `func_id` nor `offset` fields change.
yes, this is a mistake.
>
>>   }
>>   
>>   static void __fixup_collection_insert_kfunc(struct bpf_insn_aux_data *insn_aux,
>> @@ -21885,7 +21904,7 @@ static void __fixup_collection_insert_kfunc(struct bpf_insn_aux_data *insn_aux,
>>   static int fixup_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>>   			    struct bpf_insn *insn_buf, int insn_idx, int *cnt)
>>   {
>> -	const struct bpf_kfunc_desc *desc;
>> +	struct bpf_kfunc_desc *desc;
>>   
>>   	if (!insn->imm) {
>>   		verbose(env, "invalid kernel function call not eliminated in verifier pass\n");
>> @@ -21905,6 +21924,8 @@ static int fixup_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>>   		return -EFAULT;
>>   	}
>>   
>> +	specialize_kfunc(env, desc);
>> +
>>   	if (!bpf_jit_supports_far_kfunc_call())
>>   		insn->imm = BPF_CALL_IMM(desc->addr);
>>   	if (insn->off)