On Mon, Aug 1, 2022 at 7:12 PM Joanne Koong <joannelkoong@xxxxxxxxx> wrote:
>
> On Mon, Aug 1, 2022 at 4:33 PM Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
> >
> > (consider cross-posting network-related stuff to netdev@)
>
> Great, I will start cc-ing netdev@
>
> >
> > On Tue, 26 Jul 2022 11:47:04 -0700 Joanne Koong wrote:
> > > Add skb dynptrs, which are dynptrs whose underlying pointer points
> > > to a skb. The dynptr acts on skb data. skb dynptrs have two main
> > > benefits. One is that they allow operations on sizes that are not
> > > statically known at compile-time (eg variable-sized accesses).
> > > Another is that parsing the packet data through dynptrs (instead of
> > > through direct access of skb->data and skb->data_end) can be more
> > > ergonomic and less brittle (eg does not need manual if checking for
> > > being within bounds of data_end).
> >
> > Is there really a need for dynptr_from_{skb,xdp} to be different
> > function IDs? I was hoping this work would improve portability of
> > networking BPF programs across the hooks.
>
> Awesome, I like this idea of having just one generic API named
> something like bpf_dynptr_from_packet. I'll add this for v2!
>
Thinking about this some more, I don't think we get a lot of benefits
from combining it into one API (bpf_dynptr_from_packet) instead of 2
separate APIs (bpf_dynptr_from_skb / bpf_dynptr_from_xdp). The
bpf_dynptr_write behavior will be inconsistent (eg bpf_dynptr_write
into xdp frags will work whereas bpf_dynptr_write into skb frags will
fail). Martin also pointed out that he'd prefer bpf_dynptr_write() to
succeed for writing into frags and invalidate data slices (instead of
failing the write and always keeping data slices valid), which we
can't do if we combine xdp + skb, without always (needlessly)
invalidating xdp data slices whenever there's a write. Additionally,
in the verifier, there's no organic mapping between prog type -> prog
ctx, so we'll have to hardcode some mapping between prog type -> skb
vs. xdp ctx. I think for these reasons it makes more sense to have 2
separate APIs, instead of having 1 API that both hooks can call.
> >
> > > For bpf prog types that don't support writes on skb data, the dynptr is
> > > read-only (writes and data slices are not permitted). For reads on the
> > > dynptr, this includes reading into data in the non-linear paged buffers
> > > but for writes and data slices, if the data is in a paged buffer, the
> > > user must first call bpf_skb_pull_data to pull the data into the linear
> > > portion.
> > >
> > > Additionally, any helper calls that change the underlying packet buffer
> > > (eg bpf_skb_pull_data) invalidates any data slices of the associated
> > > dynptr.
> >
> > Grepping the verifier did not help me find that, would you mind
> > pointing me to the code?
>
> The base reg type of a skb data slice will be PTR_TO_PACKET - this
> gets set in this patch in check_helper_call() in verifier.c:
>
> + if (func_id == BPF_FUNC_dynptr_data &&
> + meta.type == BPF_DYNPTR_TYPE_SKB)
> + regs[BPF_REG_0].type = PTR_TO_PACKET | ret_flag;
>
> Anytime there is a helper call that changes the underlying packet
> buffer [0], the verifier iterates through the registers and marks all
> PTR_TO_PACKET reg types as unknown, which invalidates them. The dynptr
> data slice will be invalidated since its base reg type is
> PTR_TO_PACKET
>
> The stack trace is:
> check_helper_call() -> clear_all_pkt_pointers() ->
> __clear_all_pkt_pointers() -> mark_reg_unknown()
>
>
> I will add this explanation to the commit message for v2 since it is non-obvious
>
>
> [0] https://elixir.bootlin.com/linux/latest/source/kernel/bpf/verifier.c#L7143
>
> [1] https://elixir.bootlin.com/linux/latest/source/kernel/bpf/verifier.c#L6489
>
>
> >
> > > Right now, skb dynptrs can only be constructed from skbs that are
> > > the bpf program context - as such, there does not need to be any
> > > reference tracking or release on skb dynptrs.
