Re: [RFC PATCH bpf-next 5/6] bpf: Allow pro/epilogue to call kfunc

[Eduard Zingerman <eddyz87@gmail.com>]

On Tue, 2024-08-13 at 11:49 -0700, Martin KaFai Lau wrote:
> From: Martin KaFai Lau <martin.lau@kernel.org>
> 
> The existing prologue has been able to call bpf helper but not a kfunc.
> This patch allows the prologue/epilogue to call the kfunc.

[...]

> Once the insn->off is determined (either reuse an existing one
> or an unused one is found), it will call the existing add_kfunc_call()
> and everything else should fall through.
> 
> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
> ---

fwiw, don't see any obvious problems with this patch.

Reviewed-by: Eduard Zingerman <eddyz87@gmail.com>

>  kernel/bpf/verifier.c | 116 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 113 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 5e995b7884fb..2873e1083402 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -2787,6 +2787,61 @@ static struct btf *find_kfunc_desc_btf(struct bpf_verifier_env *env, s16 offset)
>  	return btf_vmlinux ?: ERR_PTR(-ENOENT);
>  }
>  
> +static int find_kfunc_desc_btf_offset(struct bpf_verifier_env *env, struct btf *btf,
> +				      struct module *module, s16 *offset)
> +{
> +	struct bpf_kfunc_btf_tab *tab;
> +	struct bpf_kfunc_btf *b;
> +	s16 new_offset = S16_MAX;
> +	u32 i;
> +
> +	if (btf_is_vmlinux(btf)) {
> +		*offset = 0;
> +		return 0;
> +	}
> +
> +	tab = env->prog->aux->kfunc_btf_tab;
> +	if (!tab) {
> +		tab = kzalloc(sizeof(*tab), GFP_KERNEL);
> +		if (!tab)
> +			return -ENOMEM;
> +		env->prog->aux->kfunc_btf_tab = tab;
> +	}
> +
> +	b = tab->descs;
> +	for (i = tab->nr_descs; i > 0; i--) {

Question: why iterating in reverse here?

> +		if (b[i - 1].btf == btf) {
> +			*offset = b[i - 1].offset;
> +			return 0;
> +		}
> +		/* Search new_offset from backward S16_MAX, S16_MAX-1, ...
> +		 * tab->nr_descs max out at MAX_KFUNC_BTFS which is
> +		 * smaller than S16_MAX, so it will be able to find
> +		 * a non-zero new_offset to use.
> +		 */
> +		if (new_offset == b[i - 1].offset)
> +			new_offset--;
> +	}
> +
> +	if (tab->nr_descs == MAX_KFUNC_BTFS) {
> +		verbose(env, "too many different module BTFs\n");
> +		return -E2BIG;
> +	}
> +
> +	if (!try_module_get(module))
> +		return -ENXIO;
> +
> +	b = &tab->descs[tab->nr_descs++];
> +	btf_get(btf);
> +	b->btf = btf;
> +	b->module = module;
> +	b->offset = new_offset;
> +	*offset = new_offset;
> +	sort(tab->descs, tab->nr_descs, sizeof(tab->descs[0]),
> +	     kfunc_btf_cmp_by_off, NULL);
> +	return 0;
> +}
> +
>  static int add_kfunc_call(struct bpf_verifier_env *env, u32 func_id, s16 offset)
>  {
>  	const struct btf_type *func, *func_proto;

[...]