| Message ID | 20240315-hid-bpf-sleepable-v4-4-5658f2540564@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | sleepable bpf_timer (was: allow HID-BPF to do device IOs) | expand |
On Fri, 2024-03-15 at 15:29 +0100, Benjamin Tissoires wrote: [...] > @@ -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); > } I was curious why 'env->cur_state &&' check was needed and found that removing it caused an error in the following fragment: static int do_misc_fixups(struct bpf_verifier_env *env) { ... 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); ... } ... } When do_misc_fixups() is done env->cur_state is NULL. Current implementation would use GFP_ATOMIC allocation even for sleepable callbacks, where GFP_KERNEL is sufficient. Is this is something we want to address? > > /* The non-sleepable programs and sleepable programs with explicit bpf_rcu_read_lock()
On Tue, Mar 19, 2024 at 12:54 AM Eduard Zingerman <eddyz87@gmail.com> wrote: > > On Fri, 2024-03-15 at 15:29 +0100, Benjamin Tissoires wrote: > [...] > > > @@ -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); > > } > > I was curious why 'env->cur_state &&' check was needed and found that > removing it caused an error in the following fragment: > > static int do_misc_fixups(struct bpf_verifier_env *env) > { > ... > 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); > ... > } > ... > } > > When do_misc_fixups() is done env->cur_state is NULL. > Current implementation would use GFP_ATOMIC allocation even for > sleepable callbacks, where GFP_KERNEL is sufficient. > Is this is something we want to address? I honestly have no idea of the impact there. AFAICT, if env->cur_state is not set, we don't even know if the callback will be sleepable or not, so if there is a small penalty, then it's the safest option, no? Cheers, Benjamin > > > > > /* The non-sleepable programs and sleepable programs with explicit bpf_rcu_read_lock() >
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 53f85e114a33..0be07da38f8a 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() @@ -9493,7 +9496,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]; @@ -16937,6 +16941,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> --- 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(-)