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;
- verify global subprogram call tree in post-order,
in order to have the flags ready when needed.
Both were rejected back than.
But we still can reconsider :)
> > + */
> > + 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);
[...]
