Re: Packet pointer invalidation and subprograms

[Eduard Zingerman <eddyz87@gmail.com>]

On Thu, 2024-12-05 at 22:22 -0800, Andrii Nakryiko wrote:
> On Thu, Dec 5, 2024 at 8:07 PM Eduard Zingerman <eddyz87@gmail.com> 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@gmail.com> 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.