On Fri, Dec 6, 2024 at 8:08 AM Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote: > > On Thu, Dec 5, 2024 at 10:23 PM Andrii Nakryiko > <andrii.nakryiko@xxxxxxxxx> 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... ;) > > > > 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). > > > > 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. > > tags for args was an aid to the verifier. Nothing is broken without them. > Here it's about correctness. > So we cannot use tags to solve this case. Hm.. Just like without an arg tag, verifier would conservatively assume that `struct task_struct *task` global subprog argument is just some opaque memory, not really a task, and would verify that argument and code working with it as such. If a user did something that required extra task_struct semantics, then that would be a verification error. Unless the user added __arg_trusted, of course. Same thing here. We *assume* that global subprog doesn't have this packet pointers side effect. If later during verification it turns out it does have this effect -- this is an error and subprog gets rejected. Unless the user provided __subprog_invalidates_all_pkt_pointers, of course. Same thing. I'm not saying we shouldn't fix the issue, I'm saying we should fix it in a different place and add a tag to enable this side effect.
