Re: [PATCH bpf-next v1 1/2] bpf: Summarize sleepable global subprogs

[Eduard Zingerman <eddyz87@gmail.com>]

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);

[...]