netdev - Re: [PATCH bpf-next 2/2] selftests/bpf: expand test_tc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAH3MdRUMPfZEku3z11JAFtewud-kJjrXoNcTnKa8uKJd-aS4Jg@mail.gmail.com>
Date:   Mon, 22 Apr 2019 16:56:02 -0700
From:   Y Song <ys114321@...il.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     netdev <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Alan Maguire <alan.maguire@...cle.com>,
        Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH bpf-next 2/2] selftests/bpf: expand test_tc_tunnel with
 SIT encap

On Mon, Apr 22, 2019 at 4:47 PM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> On Mon, Apr 22, 2019 at 7:40 PM Y Song <ys114321@...il.com> wrote:
> >
> > On Mon, Apr 22, 2019 at 7:58 AM Willem de Bruijn
> > <willemdebruijn.kernel@...il.com> wrote:
> > >
> > > From: Willem de Bruijn <willemb@...gle.com>
> > >
> > > So far, all BPF tc tunnel testcases encapsulate in the same network
> > > protocol. Add an encap testcase that requires updating skb->protocol.
> > >
> > > The 6in4 tunnel encapsulates an IPv6 packet inside an IPv4 tunnel.
> > > Verify that bpf_skb_net_grow correctly updates skb->protocol to
> > > select the right protocol handler in __netif_receive_skb_core.
> > >
> > > The BPF program should also manually update the link layer header to
> > > encode the right network protocol.
> > >
> > > Signed-off-by: Willem de Bruijn <willemb@...gle.com>
> > > ---
> > >  tools/testing/selftests/bpf/config            |  1 +
> > >  .../selftests/bpf/progs/test_tc_tunnel.c      | 55 +++++++++++++++++--
> > >  tools/testing/selftests/bpf/test_tc_tunnel.sh | 20 ++++++-
> > >  3 files changed, 71 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/config b/tools/testing/selftests/bpf/config
> > > index 8c976476f6fdc..f7a0744db31e1 100644
> > > --- a/tools/testing/selftests/bpf/config
> > > +++ b/tools/testing/selftests/bpf/config
> > > @@ -33,3 +33,4 @@ CONFIG_MPLS=y
> > >  CONFIG_NET_MPLS_GSO=m
> > >  CONFIG_MPLS_ROUTING=m
> > >  CONFIG_MPLS_IPTUNNEL=m
> > > +CONFIG_IPV6_SIT=m
> > > diff --git a/tools/testing/selftests/bpf/progs/test_tc_tunnel.c b/tools/testing/selftests/bpf/progs/test_tc_tunnel.c
> > > index ab56a6a72b7a5..94ae1caab2bfc 100644
> > > --- a/tools/testing/selftests/bpf/progs/test_tc_tunnel.c
> > > +++ b/tools/testing/selftests/bpf/progs/test_tc_tunnel.c
> > > @@ -77,17 +77,43 @@ static __always_inline int encap_ipv4(struct __sk_buff *skb, __u8 encap_proto,
> > >         struct v4hdr h_outer;
> > >         struct tcphdr tcph;
> > >         int olen, l2_len;
> > > +       int tcp_off;
> > >         __u64 flags;
> > >
> > > -       if (bpf_skb_load_bytes(skb, ETH_HLEN, &iph_inner,
> > > -                              sizeof(iph_inner)) < 0)
> > > -               return TC_ACT_OK;
> > > +       if (encap_proto == IPPROTO_IPV6) {
> > > +               const __u32 saddr = (192 << 24) | (168 << 16) | (1 << 8) | 1;
> > > +               const __u32 daddr = (192 << 24) | (168 << 16) | (1 << 8) | 2;
> > > +               struct ipv6hdr iph6_inner;
> > > +
> > > +               if (bpf_skb_load_bytes(skb, ETH_HLEN, &iph6_inner,
> > > +                                      sizeof(iph6_inner)) < 0)
> > > +                       return TC_ACT_OK;
> > > +
> > > +               /* convert to viable ipv4 header */
> > > +               memset(&iph_inner, 0, sizeof(iph_inner));
> > > +               iph_inner.version = 4;
> > > +               iph_inner.ihl = 5;
> > > +               iph_inner.tot_len = bpf_htons(sizeof(iph6_inner) +
> > > +                                   bpf_ntohs(iph6_inner.payload_len));
> > > +               iph_inner.ttl = iph6_inner.hop_limit - 1;
> > > +               iph_inner.protocol = iph6_inner.nexthdr;
> > > +               iph_inner.saddr = __bpf_constant_htonl(saddr);
> > > +               iph_inner.daddr = __bpf_constant_htonl(daddr);
> >
> > The code seems correctly. But maybe some variable renaming or
> > comments can help improve readability.
> >
> > For example, here iph_inner (ipv4) intends to represent the
> > inner ipv6 and iph_inner.protocol is assigned to iph6_inner.nexthdr
> > although it is correctly handled later with h_outer.ip logic.
>
> Thanks for the review. Yes, I added this feature to an already complex
> test with 20 variants. Tried to keep the changes as few and local as
> possible.
>
> Would it help if I expand the /* convert to viable ipv4 header */
> comment? To better explain why we convert to an ipv4 header here
> (because all other encap options encap the same protocol, so
> encap_ipv4() expects an iphdr instead of an ipv6hdr).

Thanks. Comments are fine as long as it explains its purpose here.