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

[Eduard Zingerman <eddyz87@gmail.com>]

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.

>  
> @@ -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.

>  }
>  
>  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)