Re: [PATCH bpf-next v5 3/6] bpf/helpers: introduce bpf_timer_set_sleepable_cb() kfunc

[Kumar Kartikeya Dwivedi]

On Fri, 22 Mar 2024 at 15:57, Benjamin Tissoires <bentiss@xxxxxxxxxx> wrote:
>
> In this patch, bpf_timer_set_sleepable_cb() is functionally equivalent
> to bpf_timer_set_callback(), to the exception that it enforces
> the timer to be started with BPF_F_TIMER_SLEEPABLE.
>
> But given that bpf_timer_set_callback() is a helper when
> bpf_timer_set_sleepable_cb() is a kfunc, we need to teach the verifier
> about its attached callback.
> Marking that callback as sleepable will be done in a separate patch
>
> Acked-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
> Signed-off-by: Benjamin Tissoires <bentiss@xxxxxxxxxx>
>
> ---
> [...]
>
> @@ -19548,6 +19582,28 @@ static int fixup_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>                    desc->func_id == special_kfunc_list[KF_bpf_rdonly_cast]) {
>                 insn_buf[0] = BPF_MOV64_REG(BPF_REG_0, BPF_REG_1);
>                 *cnt = 1;
> +       } else if (is_bpf_timer_set_sleepable_cb_impl_kfunc(desc->func_id)) {
> +               /* The verifier will process callback_fn as many times as necessary
> +                * with different maps and the register states prepared by
> +                * set_timer_callback_state will be accurate.
> +                *
> +                * The following use case is valid:
> +                *   map1 is shared by prog1, prog2, prog3.
> +                *   prog1 calls bpf_timer_init for some map1 elements
> +                *   prog2 calls bpf_timer_set_callback for some map1 elements.
> +                *     Those that were not bpf_timer_init-ed will return -EINVAL.
> +                *   prog3 calls bpf_timer_start for some map1 elements.
> +                *     Those that were not both bpf_timer_init-ed and
> +                *     bpf_timer_set_callback-ed will return -EINVAL.
> +                */
> +               struct bpf_insn ld_addrs[2] = {
> +                       BPF_LD_IMM64(BPF_REG_3, (long)env->prog->aux),
> +               };
> +
> +               insn_buf[0] = ld_addrs[0];
> +               insn_buf[1] = ld_addrs[1];
> +               insn_buf[2] = *insn;
> +               *cnt = 3;
>         }

Would be better to handle the fixup of all kfuncs in one place, i.e.
fixup_kfunc_call.

>         return 0;
>  }
>
> --
> 2.44.0
>
>