On Fri, Dec 6, 2024 at 2:44 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote: > > 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. The tags would be that generalizable side effect declaration approach, so seems worth it to set a uniform approach. > Please take a look at the patch, the change for check_cfg() is 32 lines. I did, actually. And I already explained what I don't like about it: eagerness. check_cfg() is not the right place for this, if we want to support dead code elimination and BPF CO-RE-based feature gating. Which your patches clearly violate, so I don't like them, sorry. We made this eagerness mistake with global subprogs verification previously, and had to switch it to lazy on-demand global subprog validation. I think we should preserve this lazy approach going forward. > > > 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. > >
