Re: [PATCH net-next v2 4/5] ipv6: sr: Add seg6local action End.BPF

[Mathieu Xhonneux <m.xhonneux@gmail.com>]

2018-04-28 2:01 GMT+02:00 Alexei Starovoitov <alexei.starovoitov@gmail.com>:
>
> On Fri, Apr 27, 2018 at 10:59:19AM -0400, David Miller wrote:
> > From: Mathieu Xhonneux <m.xhonneux@gmail.com>
> > Date: Tue, 24 Apr 2018 18:44:15 +0100
> >
> > > This patch adds the End.BPF action to the LWT seg6local infrastructure.
> > > This action works like any other seg6local End action, meaning that an IPv6
> > > header with SRH is needed, whose DA has to be equal to the SID of the
> > > action. It will also advance the SRH to the next segment, the BPF program
> > > does not have to take care of this.
> >
> > I'd like to see some BPF developers review this change.
> >
> > But on my side I wonder if, instead of validating the whole thing afterwards,
> > we should make the helpers accessible by the eBPF program validate the changes
> > as they are made.
>
> Looking at the code I don't think it's possible to keep it valid all the time
> while building, so seg6_validate_srh() after the program run seems necessary.


Indeed, e.g. to add a TLV in the SRH one needs to call
bpf_lwt_seg6_adjust_srh (to add some room for the TLV), then
bpf_lwt_seg6_store_bytes() (to fill the space with the TLV). Between
those two calls, the SRH is in an invalid state.

>
>
> I think the whole set should be targeting bpf-next tree.
> Please fix kbuild errors, rebase and document new helper in man-page style.
> Things like:
> +       test_btf_haskv.o test_btf_nokv.o test_lwt_seg6local.o
> +>>>>>>> selftests/bpf: test for seg6local End.BPF action
> should be fixed properly.


Oops, I didn't catch this one, thanks. I'll send a v3 towards bpf-next.