On Fri, Feb 28, 2025 at 3:23 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Fri, 2025-02-28 at 15:18 -0800, Andrii Nakryiko wrote:
>
> [...]
>
> > > /* non-recursive DFS pseudo code
> > > @@ -17183,9 +17187,20 @@ static int visit_insn(int t, struct bpf_verifier_env *env)
> > > mark_prune_point(env, t);
> > > mark_jmp_point(env, t);
> > > }
> > > - if (bpf_helper_call(insn) && bpf_helper_changes_pkt_data(insn->imm))
> > > - mark_subprog_changes_pkt_data(env, t);
> > > - if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
> > > + if (bpf_helper_call(insn)) {
> > > + const struct bpf_func_proto *fp;
> > > +
> > > + ret = get_helper_proto(env, insn->imm, &fp);
> > > + /* If called in a non-sleepable context program will be
> > > + * rejected anyway, so we should end up with precise
> > > + * sleepable marks on subprogs, except for dead code
> > > + * elimination.
> >
> > TBH, I'm worried that we are regressing to doing all these side effect
> > analyses disregarding dead code elimination. It's not something
> > hypothetical to have an .rodata variable controlling whether, say, to
> > do bpf_probe_read_user() (non-sleepable) vs bpf_copy_from_user()
> > (sleepable) inside global subprog, depending on some outside
> > configuration (e.g., whether we'll be doing SEC("iter.s/task") or it's
> > actually profiler logic called inside SEC("perf_event"), all
> > controlled by user-space). We do have use cases like this in
> > production already, and this dead code elimination is important in
> > such cases. Probably can be worked around with more global functions
> > and stuff like that, but still, it's worrying we are giving up on such
> > an important part of the BPF CO-RE approach - disabling parts of code
> > "dynamically" before loading BPF programs.
>
> There were two alternatives on the table last time:
> - add support for tags on global functions;
I was supportive of this, I believe
> - verify global subprogram call tree in post-order,
> in order to have the flags ready when needed.
Remind me of the details here? we'd start validating the main prog,
suspend that process when encountering global func, go validate global
func, once done, come back to main prog, right?
Alternatively, we could mark expected properties (restrictions) of
global subprogs as we encounter them, right? E.g, if we come to global
func call inside rcu_read_{lock,unlock}() region, we'd mark it
internally as "needs to be non-sleepable".
>
> Both were rejected back than.
> But we still can reconsider :)
>
yep, though I'm not really feeling hopeful
> > > + */
> > > + if (ret == 0 && fp->might_sleep)
> > > + mark_subprog_sleepable(env, t);
> > > + if (bpf_helper_changes_pkt_data(insn->imm))
> > > + mark_subprog_changes_pkt_data(env, t);
> > > + } else if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
> > > struct bpf_kfunc_call_arg_meta meta;
> > >
> > > ret = fetch_kfunc_meta(env, insn, &meta, NULL);
>
> [...]
>
