On Mon, Apr 22, 2019 at 4:47 PM Willem de Bruijn <willemdebruijn.ker...@gmail.com> wrote: > > On Mon, Apr 22, 2019 at 7:40 PM Y Song <ys114...@gmail.com> wrote: > > > > On Mon, Apr 22, 2019 at 7:58 AM Willem de Bruijn > > <willemdebruijn.ker...@gmail.com> wrote: > > > > > > From: Willem de Bruijn <will...@google.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 <will...@google.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.