On Thu, Jul 4, 2024 at 3:24 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> Extract the part of check_helper_call() as a utility function allowing
> to query 'struct bpf_func_proto' for a specific helper function id.
>
> Signed-off-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
> ---
> kernel/bpf/verifier.c | 30 +++++++++++++++++++++++-------
> 1 file changed, 23 insertions(+), 7 deletions(-)
>
I'd write it differently (see below), but it's correct, so:
Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index d3927d819465..4869f1fb0a42 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -10261,6 +10261,24 @@ static void update_loop_inline_state(struct bpf_verifier_env *env, u32 subprogno
> state->callback_subprogno == subprogno);
> }
>
> +static int get_helper_proto(struct bpf_verifier_env *env, int func_id,
> + const struct bpf_func_proto **ptr)
> +{
> + const struct bpf_func_proto *result = NULL;
> +
> + if (func_id < 0 || func_id >= __BPF_FUNC_MAX_ID)
> + return -ERANGE;
> +
> + if (env->ops->get_func_proto)
> + result = env->ops->get_func_proto(func_id, env->prog);
> +
> + if (!result)
> + return -EINVAL;
result is a bit unnecessary. We could do either
*ptr = NULL;
if (env->ops->get_func_proto)
*ptr = env->ops->get_func_proto(func_id, env->prog);
return *ptr ? 0 : -EINVAL;
or just
if (!env->ops->get_func_proto)
return -EINVAL;
*ptr = env->ops->get_func_proto(func_id, env->prog);
return *ptr ? 0 : -EINVAL;
> +
> + *ptr = result;
> + return 0;
> +}
> +
> static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
> int *insn_idx_p)
> {
> @@ -10277,18 +10295,16 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>
> /* find function prototype */
> func_id = insn->imm;
> - if (func_id < 0 || func_id >= __BPF_FUNC_MAX_ID) {
> - verbose(env, "invalid func %s#%d\n", func_id_name(func_id),
> - func_id);
> + err = get_helper_proto(env, insn->imm, &fn);
> + if (err == -ERANGE) {
> + verbose(env, "invalid func %s#%d\n", func_id_name(func_id), func_id);
> return -EINVAL;
> }
>
> - if (env->ops->get_func_proto)
> - fn = env->ops->get_func_proto(func_id, env->prog);
> - if (!fn) {
> + if (err) {
nit: this is one block of error handling, I'd keep it "connected":
if (err == -ERANGE) {
} else if (err) {
}
> verbose(env, "program of this type cannot use helper %s#%d\n",
> func_id_name(func_id), func_id);
> - return -EINVAL;
> + return err;
> }
>
> /* eBPF programs must be GPL compatible to use GPL-ed functions */
> --
> 2.45.2
>
