| Message ID | 20240408-hid-bpf-sleepable-v6-4-0499ddd91b94@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | sleepable bpf_timer (was: allow HID-BPF to do device IOs) | expand |
On Mon, 2024-04-08 at 10:09 +0200, Benjamin Tissoires wrote: > Now that we have bpf_timer_set_sleepable_cb() available and working, we > can tag the attached callback as sleepable, and let the verifier check > in the correct context the calls and kfuncs. > > Signed-off-by: Benjamin Tissoires <bentiss@kernel.org> > > --- I think this patch is fine with one nit regarding in_sleepable(). Acked-by: Eduard Zingerman <eddyz87@gmail.com> > @@ -5279,7 +5281,8 @@ static int map_kptr_match_type(struct bpf_verifier_env *env, > > static bool in_sleepable(struct bpf_verifier_env *env) > { > - return env->prog->sleepable; > + return env->prog->sleepable || > + (env->cur_state && env->cur_state->in_sleepable); > } Sorry, I already raised this before. As far as I understand the 'env->cur_state' check is needed because this function is used from do_misc_fixups(): if (is_storage_get_function(insn->imm)) { if (!in_sleepable(env) || env->insn_aux_data[i + delta].storage_get_func_atomic) insn_buf[0] = BPF_MOV64_IMM(BPF_REG_5, (__force __s32)GFP_ATOMIC); else insn_buf[0] = BPF_MOV64_IMM(BPF_REG_5, (__force __s32)GFP_KERNEL); insn_buf[1] = *insn; cnt = 2; ... } For a timer callback function 'env->prog->sleepable' would be false. Which means that inside sleepable callback function GFP_ATOMIC would be used in cases where GFP_KERNEL would be sufficient. An alternative would be to check (and set) sleepable flag not for a full program but for a subprogram. Whether or not this is something worth addressing I don't know. [...]
On Mon, Apr 8, 2024 at 3:36 PM Eduard Zingerman <eddyz87@gmail.com> wrote: > > On Mon, 2024-04-08 at 10:09 +0200, Benjamin Tissoires wrote: > > Now that we have bpf_timer_set_sleepable_cb() available and working, we > > can tag the attached callback as sleepable, and let the verifier check > > in the correct context the calls and kfuncs. > > > > Signed-off-by: Benjamin Tissoires <bentiss@kernel.org> > > > > --- > > I think this patch is fine with one nit regarding in_sleepable(). > Acked-by: Eduard Zingerman <eddyz87@gmail.com> > > > @@ -5279,7 +5281,8 @@ static int map_kptr_match_type(struct bpf_verifier_env *env, > > > > static bool in_sleepable(struct bpf_verifier_env *env) > > { > > - return env->prog->sleepable; > > + return env->prog->sleepable || > > + (env->cur_state && env->cur_state->in_sleepable); > > } > > Sorry, I already raised this before. > As far as I understand the 'env->cur_state' check is needed because > this function is used from do_misc_fixups(): > > if (is_storage_get_function(insn->imm)) { > if (!in_sleepable(env) || > env->insn_aux_data[i + delta].storage_get_func_atomic) > insn_buf[0] = BPF_MOV64_IMM(BPF_REG_5, (__force __s32)GFP_ATOMIC); > else > insn_buf[0] = BPF_MOV64_IMM(BPF_REG_5, (__force __s32)GFP_KERNEL); > insn_buf[1] = *insn; > cnt = 2; > ... > } > > For a timer callback function 'env->prog->sleepable' would be false. > Which means that inside sleepable callback function GFP_ATOMIC would > be used in cases where GFP_KERNEL would be sufficient. > An alternative would be to check (and set) sleepable flag not for a > full program but for a subprogram. At this point all subprograms are still part of the main program. jit_subprogs() hasn't been called yet. So there is only one 'prog' object. So cannot really set prog->sleepable for callback subprog. But you've raised a good point. We can remove "!in_sleepable(env)" part in do_misc_fixups() with: - if (in_sleepable(env) && is_storage_get_function(func_id)) - env->insn_aux_data[insn_idx].storage_get_func_atomic = true; + if (is_storage_get_function(func_id)) + env->insn_aux_data[insn_idx].storage_get_func_atomic = !in_sleepable(env);
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h index 7cb1b75eee38..14e4ee67b694 100644 --- a/include/linux/bpf_verifier.h +++ b/include/linux/bpf_verifier.h @@ -426,6 +426,7 @@ struct bpf_verifier_state { * while they are still in use. */ bool used_as_loop_entry; + bool in_sleepable; /* first and last insn idx of this verifier state */ u32 first_insn_idx; diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 00ac3a3a5f01..b75a8aca6ec9 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -1434,6 +1434,7 @@ static int copy_verifier_state(struct bpf_verifier_state *dst_state, } dst_state->speculative = src->speculative; dst_state->active_rcu_lock = src->active_rcu_lock; + dst_state->in_sleepable = src->in_sleepable; dst_state->curframe = src->curframe; dst_state->active_lock.ptr = src->active_lock.ptr; dst_state->active_lock.id = src->active_lock.id; @@ -2407,7 +2408,7 @@ static void init_func_state(struct bpf_verifier_env *env, /* Similar to push_stack(), but for async callbacks */ static struct bpf_verifier_state *push_async_cb(struct bpf_verifier_env *env, int insn_idx, int prev_insn_idx, - int subprog) + int subprog, bool is_sleepable) { struct bpf_verifier_stack_elem *elem; struct bpf_func_state *frame; @@ -2434,6 +2435,7 @@ static struct bpf_verifier_state *push_async_cb(struct bpf_verifier_env *env, * Initialize it similar to do_check_common(). */ elem->st.branches = 1; + elem->st.in_sleepable = is_sleepable; frame = kzalloc(sizeof(*frame), GFP_KERNEL); if (!frame) goto err; @@ -5279,7 +5281,8 @@ static int map_kptr_match_type(struct bpf_verifier_env *env, static bool in_sleepable(struct bpf_verifier_env *env) { - return env->prog->sleepable; + return env->prog->sleepable || + (env->cur_state && env->cur_state->in_sleepable); } /* The non-sleepable programs and sleepable programs with explicit bpf_rcu_read_lock() @@ -9497,7 +9500,8 @@ static int push_callback_call(struct bpf_verifier_env *env, struct bpf_insn *ins /* there is no real recursion here. timer callbacks are async */ env->subprog_info[subprog].is_async_cb = true; async_cb = push_async_cb(env, env->subprog_info[subprog].start, - insn_idx, subprog); + insn_idx, subprog, + is_bpf_timer_set_sleepable_cb_impl_kfunc(insn->imm)); if (!async_cb) return -EFAULT; callee = async_cb->frame[0]; @@ -16947,6 +16951,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 */
Now that we have bpf_timer_set_sleepable_cb() available and working, we can tag the attached callback as sleepable, and let the verifier check in the correct context the calls and kfuncs. Signed-off-by: Benjamin Tissoires <bentiss@kernel.org> --- no changes in v6 no changes in v5 changes in v4: - use a function parameter to forward the sleepable information new in v3 (split from v2 02/10) --- include/linux/bpf_verifier.h | 1 + kernel/bpf/verifier.c | 13 ++++++++++--- 2 files changed, 11 insertions(+), 3 deletions(-)