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

On Sat, Apr 20, 2024 at 2:10 AM Benjamin Tissoires bentiss@kernel.org 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