From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mathieu Xhonneux Subject: Re: [PATCH net-next v2 4/5] ipv6: sr: Add seg6local action End.BPF Date: Mon, 30 Apr 2018 22:06:18 +0200 Message-ID: References: <223e33f96b2cb9868a55a686c5d9321162a67ddb.1524591163.git.m.xhonneux@gmail.com> <20180427.105919.1774690223194208745.davem@davemloft.net> <20180428000141.f63iw36nfr3bbz2r@ast-mbp> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Cc: David Miller , netdev@vger.kernel.org, David Lebrun , Daniel Borkmann To: Alexei Starovoitov Return-path: Received: from mail-lf0-f46.google.com ([209.85.215.46]:40067 "EHLO mail-lf0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755964AbeD3UGU (ORCPT ); Mon, 30 Apr 2018 16:06:20 -0400 Received: by mail-lf0-f46.google.com with SMTP id j16-v6so13791225lfb.7 for ; Mon, 30 Apr 2018 13:06:19 -0700 (PDT) In-Reply-To: <20180428000141.f63iw36nfr3bbz2r@ast-mbp> Sender: netdev-owner@vger.kernel.org List-ID: 2018-04-28 2:01 GMT+02:00 Alexei Starovoitov : > > On Fri, Apr 27, 2018 at 10:59:19AM -0400, David Miller wrote: > > From: Mathieu Xhonneux > > 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.