Re: [PATCH bpf-next v2 13/16] bpf: wq: add bpf_wq_set_callback_impl

[Alexei Starovoitov]

On Sat, Apr 20, 2024 at 2:10 AM Benjamin Tissoires <bentiss@xxxxxxxxxx> wrote:
> @@ -11018,6 +11027,7 @@ enum special_kfunc_type {
>         KF_bpf_percpu_obj_drop_impl,
>         KF_bpf_throw,
>         KF_bpf_iter_css_task_new,
> +       KF_bpf_wq_set_callback_impl,
>  };
>
>  BTF_SET_START(special_kfunc_set)
> @@ -11044,6 +11054,7 @@ BTF_ID(func, bpf_throw)
>  #ifdef CONFIG_CGROUPS
>  BTF_ID(func, bpf_iter_css_task_new)
>  #endif
> +BTF_ID(func, bpf_wq_set_callback_impl)
>  BTF_SET_END(special_kfunc_set)
>
>  BTF_ID_LIST(special_kfunc_list)
> @@ -11074,6 +11085,7 @@ BTF_ID(func, bpf_iter_css_task_new)
>  #else
>  BTF_ID_UNUSED
>  #endif
> +BTF_ID(func, bpf_wq_set_callback_impl)

This is broken on !CONFIG_CGROUPS.
KF_bpf_wq_set_callback_impl in enum special_kfunc_type won't
match special_kfunc_list.
I moved this line up while applying.

>
>  static bool is_kfunc_ret_null(struct bpf_kfunc_call_arg_meta *meta)
>  {
> @@ -11402,12 +11414,28 @@ static bool is_sync_callback_calling_kfunc(u32 btf_id)
>         return btf_id == special_kfunc_list[KF_bpf_rbtree_add_impl];
>  }
>
> +static bool is_async_callback_calling_kfunc(u32 btf_id)
> +{
> +       return btf_id == special_kfunc_list[KF_bpf_wq_set_callback_impl];
> +}
> +
>  static bool is_bpf_throw_kfunc(struct bpf_insn *insn)
>  {
>         return bpf_pseudo_kfunc_call(insn) && insn->off == 0 &&
>                insn->imm == special_kfunc_list[KF_bpf_throw];
>  }
>
> +static bool is_bpf_wq_set_callback_impl_kfunc(u32 btf_id)
> +{
> +       return btf_id == special_kfunc_list[KF_bpf_wq_set_callback_impl];
> +}
> +
> +static bool is_callback_calling_kfunc(u32 btf_id)
> +{
> +       return is_sync_callback_calling_kfunc(btf_id) ||
> +              is_async_callback_calling_kfunc(btf_id);
> +}
> +
>  static bool is_rbtree_lock_required_kfunc(u32 btf_id)
>  {
>         return is_bpf_rbtree_api_kfunc(btf_id);
> @@ -12219,6 +12247,16 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
>                 }
>         }
>
> +       if (is_bpf_wq_set_callback_impl_kfunc(meta.func_id)) {
> +               err = push_callback_call(env, insn, insn_idx, meta.subprogno,
> +                                        set_timer_callback_state);
> +               if (err) {
> +                       verbose(env, "kfunc %s#%d failed callback verification\n",
> +                               func_name, meta.func_id);
> +                       return err;
> +               }
> +       }
> +
>         rcu_lock = is_kfunc_bpf_rcu_read_lock(&meta);
>         rcu_unlock = is_kfunc_bpf_rcu_read_unlock(&meta);
>
> @@ -16982,6 +17020,9 @@ static bool states_equal(struct bpf_verifier_env *env,
>         if (old->active_rcu_lock != cur->active_rcu_lock)
>                 return false;
>
> +       if (old->in_sleepable != cur->in_sleepable)
> +               return false;
> +
>         /* for states to be equal callsites have to be the same
>          * and all frame states need to be equivalent
>          */
> @@ -19653,6 +19694,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_wq_set_callback_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.
> +                */

Also removed this comment. It's not correct here.

> +               struct bpf_insn ld_addrs[2] = {
> +                       BPF_LD_IMM64(BPF_REG_4, (long)env->prog->aux),
> +               };
> +
> +               insn_buf[0] = ld_addrs[0];
> +               insn_buf[1] = ld_addrs[1];
> +               insn_buf[2] = *insn;
> +               *cnt = 3;
>         }
>         return 0;
>  }
>
> --
> 2.44.0
>