On Thu, 2024-12-05 at 22:22 -0800, Andrii Nakryiko wrote: > On Thu, Dec 5, 2024 at 8:07 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: > > > > On Thu, 2024-12-05 at 17:44 -0800, Alexei Starovoitov wrote: > > > On Thu, Dec 5, 2024 at 4:29 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: > > > > > > > > so I went ahead and the fix does look simple: > > > > https://github.com/eddyz87/bpf/tree/skb-pull-data-global-func-bug > > > > > > Looks simple enough to me. > > > Ship it for bpf tree. > > > If we can come up with something better we can do it later in bpf-next. > > > > > > I very much prefer to avoid complexity as much as possible. > > > > Sent the patch-set for "simple". > > It is better then "dumb" by any metric anyways. > > Will try what Andrii suggests, as allowing calling global sub-programs > > from non-sleepable context sounds interesting. > > > > I haven't looked at your patches yet, but keep in mind another gotcha > with subprograms: they can be freplace'd by another BPF program > (clearly freplace programs were a successful reduction of > complexity... ;) If there would be no general objections for the patch-set I posted, I'll do a v2 with an additional flag in bpf_prog_aux/bpf_func_info_aux to be checked when freplace attachment is done. > What this means in practice is whatever deductions you get out of > analyzing any specific original subprogram might be violated by > freplace program if we don't enforce them during freplace attachment. > > > Anyways, I came here to say that I think I have a much simpler > solution that won't require big changes to the BPF verifier: tags. We > can shift the burden to the user having to declare the intent upfront > through subprog tags. And then, during verification of that global > subprog, the verifier can enforce that only explicitly declared side > effects can be enacted by the subprogram's code (taking into account > lazy dead code detection logic). I considered tags, but didn't like it much for something so easily computable. Please take a look at the patch, the change for check_cfg() is 32 lines. > We already take advantage of declarative tags for global subprog args > (__arg_trusted, etc), we can do the same for the function itself. We > can have __subprog_invalidates_all_pkt_pointers tag (and yes, I do > insist on this laconic name, of course), and during verification of > subprogram we just make sure that subprog was annotated as such, if > one of those fancy helpers is called directly in subprog itself or > transitively through any of *actually* called subprogs. > > All this will preserve the lazy approach we have with no need to look > ahead into subprog's implementation. I'd keep the concept of global > subprog completely and exhaustively described with its type signature > and associated tags as much as possible. > > P.S. We still need to keep in mind freplace complications, of course.
