* [PATCH bpf-next 0/2] update skb->protocol in bpf_skb_net_grow
@ 2019-04-22 14:50 Willem de Bruijn
2019-04-22 14:50 ` [PATCH bpf-next 1/2] bpf: " Willem de Bruijn
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Willem de Bruijn @ 2019-04-22 14:50 UTC (permalink / raw)
To: netdev; +Cc: ast, daniel, alan.maguire, Willem de Bruijn
From: Willem de Bruijn <willemb@google.com>
Expand the tc tunnel encap support with protocols that convert the
network layer protocol, such as 6in4. This is analogous to existing
support in bpf_skb_proto_6_to_4.
Patch 1 implements the straightforward logic
Patch 2 tests it with a 6in4 tunnel
Willem de Bruijn (2):
bpf: update skb->protocol in bpf_skb_net_grow
selftests/bpf: expand test_tc_tunnel with SIT encap
net/core/filter.c | 8 +++
tools/testing/selftests/bpf/config | 1 +
.../selftests/bpf/progs/test_tc_tunnel.c | 55 +++++++++++++++++--
tools/testing/selftests/bpf/test_tc_tunnel.sh | 20 ++++++-
4 files changed, 79 insertions(+), 5 deletions(-)
--
2.21.0.593.g511ec345e18-goog
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH bpf-next 1/2] bpf: update skb->protocol in bpf_skb_net_grow 2019-04-22 14:50 [PATCH bpf-next 0/2] update skb->protocol in bpf_skb_net_grow Willem de Bruijn @ 2019-04-22 14:50 ` Willem de Bruijn 2019-04-22 23:35 ` Y Song 2019-04-22 14:50 ` [PATCH bpf-next 2/2] selftests/bpf: expand test_tc_tunnel with SIT encap Willem de Bruijn 2019-04-23 11:04 ` [PATCH bpf-next 0/2] update skb->protocol in bpf_skb_net_grow Alan Maguire 2 siblings, 1 reply; 8+ messages in thread From: Willem de Bruijn @ 2019-04-22 14:50 UTC (permalink / raw) To: netdev; +Cc: ast, daniel, alan.maguire, Willem de Bruijn From: Willem de Bruijn <willemb@google.com> Some tunnels, like sit, change the network protocol of packet. If so, update skb->protocol to match the new type. Signed-off-by: Willem de Bruijn <willemb@google.com> --- net/core/filter.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/net/core/filter.c b/net/core/filter.c index 1833926a63fc1..bd4d498fabfa4 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -3047,6 +3047,14 @@ static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff, skb_set_transport_header(skb, mac_len + nh_len); } + + /* Match skb->protocol to new outer l3 protocol */ + if (skb->protocol == htons(ETH_P_IP) && + flags & BPF_F_ADJ_ROOM_ENCAP_L3_IPV6) + skb->protocol = htons(ETH_P_IPV6); + else if (skb->protocol == htons(ETH_P_IPV6) && + flags & BPF_F_ADJ_ROOM_ENCAP_L3_IPV4) + skb->protocol = htons(ETH_P_IP); } if (skb_is_gso(skb)) { -- 2.21.0.593.g511ec345e18-goog ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: update skb->protocol in bpf_skb_net_grow 2019-04-22 14:50 ` [PATCH bpf-next 1/2] bpf: " Willem de Bruijn @ 2019-04-22 23:35 ` Y Song 0 siblings, 0 replies; 8+ messages in thread From: Y Song @ 2019-04-22 23:35 UTC (permalink / raw) To: Willem de Bruijn Cc: netdev, Alexei Starovoitov, Daniel Borkmann, alan.maguire, Willem de Bruijn On Mon, Apr 22, 2019 at 7:58 AM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > From: Willem de Bruijn <willemb@google.com> > > Some tunnels, like sit, change the network protocol of packet. > If so, update skb->protocol to match the new type. > > Signed-off-by: Willem de Bruijn <willemb@google.com> Acked-by: Yonghong Song <yhs@fb.com> > --- > net/core/filter.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/net/core/filter.c b/net/core/filter.c > index 1833926a63fc1..bd4d498fabfa4 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -3047,6 +3047,14 @@ static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff, > > skb_set_transport_header(skb, mac_len + nh_len); > } > + > + /* Match skb->protocol to new outer l3 protocol */ > + if (skb->protocol == htons(ETH_P_IP) && > + flags & BPF_F_ADJ_ROOM_ENCAP_L3_IPV6) > + skb->protocol = htons(ETH_P_IPV6); > + else if (skb->protocol == htons(ETH_P_IPV6) && > + flags & BPF_F_ADJ_ROOM_ENCAP_L3_IPV4) > + skb->protocol = htons(ETH_P_IP); > } > > if (skb_is_gso(skb)) { > -- > 2.21.0.593.g511ec345e18-goog > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH bpf-next 2/2] selftests/bpf: expand test_tc_tunnel with SIT encap 2019-04-22 14:50 [PATCH bpf-next 0/2] update skb->protocol in bpf_skb_net_grow Willem de Bruijn 2019-04-22 14:50 ` [PATCH bpf-next 1/2] bpf: " Willem de Bruijn @ 2019-04-22 14:50 ` Willem de Bruijn 2019-04-22 23:40 ` Y Song 2019-04-23 11:04 ` [PATCH bpf-next 0/2] update skb->protocol in bpf_skb_net_grow Alan Maguire 2 siblings, 1 reply; 8+ messages in thread From: Willem de Bruijn @ 2019-04-22 14:50 UTC (permalink / raw) To: netdev; +Cc: ast, daniel, alan.maguire, Willem de Bruijn From: Willem de Bruijn <willemb@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 <willemb@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); + + tcp_off = sizeof(iph6_inner); + } else { + if (bpf_skb_load_bytes(skb, ETH_HLEN, &iph_inner, + sizeof(iph_inner)) < 0) + return TC_ACT_OK; + + tcp_off = sizeof(iph_inner); + } /* filter only packets we want */ if (iph_inner.ihl != 5 || iph_inner.protocol != IPPROTO_TCP) return TC_ACT_OK; - if (bpf_skb_load_bytes(skb, ETH_HLEN + sizeof(iph_inner), + if (bpf_skb_load_bytes(skb, ETH_HLEN + tcp_off, &tcph, sizeof(tcph)) < 0) return TC_ACT_OK; @@ -129,6 +155,7 @@ static __always_inline int encap_ipv4(struct __sk_buff *skb, __u8 encap_proto, l2_len); break; case IPPROTO_IPIP: + case IPPROTO_IPV6: break; default: return TC_ACT_OK; @@ -164,6 +191,17 @@ static __always_inline int encap_ipv4(struct __sk_buff *skb, __u8 encap_proto, BPF_F_INVALIDATE_HASH) < 0) return TC_ACT_SHOT; + /* if changing outer proto type, update eth->h_proto */ + if (encap_proto == IPPROTO_IPV6) { + struct ethhdr eth; + + if (bpf_skb_load_bytes(skb, 0, ð, sizeof(eth)) < 0) + return TC_ACT_SHOT; + eth.h_proto = bpf_htons(ETH_P_IP); + if (bpf_skb_store_bytes(skb, 0, ð, sizeof(eth), 0) < 0) + return TC_ACT_SHOT; + } + return TC_ACT_OK; } @@ -325,6 +363,15 @@ int __encap_udp_eth(struct __sk_buff *skb) return TC_ACT_OK; } +SEC("encap_sit_none") +int __encap_sit_none(struct __sk_buff *skb) +{ + if (skb->protocol == __bpf_constant_htons(ETH_P_IPV6)) + return encap_ipv4(skb, IPPROTO_IPV6, ETH_P_IP); + else + return TC_ACT_OK; +} + SEC("encap_ip6tnl_none") int __encap_ip6tnl_none(struct __sk_buff *skb) { diff --git a/tools/testing/selftests/bpf/test_tc_tunnel.sh b/tools/testing/selftests/bpf/test_tc_tunnel.sh index d4d8d5d3b06e1..ff0d31d38061f 100755 --- a/tools/testing/selftests/bpf/test_tc_tunnel.sh +++ b/tools/testing/selftests/bpf/test_tc_tunnel.sh @@ -97,6 +97,9 @@ if [[ "$#" -eq "0" ]]; then echo "ip6ip6" $0 ipv6 ip6tnl none 100 + echo "sit" + $0 ipv6 sit none 100 + for mac in none mpls eth ; do echo "ip gre $mac" $0 ipv4 gre $mac 100 @@ -211,11 +214,20 @@ else targs="" fi +# tunnel address family differs from inner for SIT +if [[ "${tuntype}" == "sit" ]]; then + link_addr1="${ns1_v4}" + link_addr2="${ns2_v4}" +else + link_addr1="${addr1}" + link_addr2="${addr2}" +fi + # serverside, insert decap module # server is still running # client can connect again ip netns exec "${ns2}" ip link add name testtun0 type "${ttype}" \ - ${tmode} remote "${addr1}" local "${addr2}" $targs + ${tmode} remote "${link_addr1}" local "${link_addr2}" $targs expect_tun_fail=0 @@ -260,6 +272,12 @@ else server_listen fi +# bpf_skb_net_shrink does not take tunnel flags yet, cannot update L3. +if [[ "${tuntype}" == "sit" ]]; then + echo OK + exit 0 +fi + # serverside, use BPF for decap ip netns exec "${ns2}" ip link del dev testtun0 ip netns exec "${ns2}" tc qdisc add dev veth2 clsact -- 2.21.0.593.g511ec345e18-goog ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: expand test_tc_tunnel with SIT encap 2019-04-22 14:50 ` [PATCH bpf-next 2/2] selftests/bpf: expand test_tc_tunnel with SIT encap Willem de Bruijn @ 2019-04-22 23:40 ` Y Song 2019-04-22 23:47 ` Willem de Bruijn 0 siblings, 1 reply; 8+ messages in thread From: Y Song @ 2019-04-22 23:40 UTC (permalink / raw) To: Willem de Bruijn Cc: netdev, Alexei Starovoitov, Daniel Borkmann, alan.maguire, Willem de Bruijn On Mon, Apr 22, 2019 at 7:58 AM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > From: Willem de Bruijn <willemb@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 <willemb@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. > + > + tcp_off = sizeof(iph6_inner); > + } else { > + if (bpf_skb_load_bytes(skb, ETH_HLEN, &iph_inner, > + sizeof(iph_inner)) < 0) > + return TC_ACT_OK; > + > + tcp_off = sizeof(iph_inner); > + } > > /* filter only packets we want */ > if (iph_inner.ihl != 5 || iph_inner.protocol != IPPROTO_TCP) > return TC_ACT_OK; > > - if (bpf_skb_load_bytes(skb, ETH_HLEN + sizeof(iph_inner), > + if (bpf_skb_load_bytes(skb, ETH_HLEN + tcp_off, > &tcph, sizeof(tcph)) < 0) > return TC_ACT_OK; > > @@ -129,6 +155,7 @@ static __always_inline int encap_ipv4(struct __sk_buff *skb, __u8 encap_proto, > l2_len); > break; > case IPPROTO_IPIP: > + case IPPROTO_IPV6: > break; > default: > return TC_ACT_OK; > @@ -164,6 +191,17 @@ static __always_inline int encap_ipv4(struct __sk_buff *skb, __u8 encap_proto, > BPF_F_INVALIDATE_HASH) < 0) > return TC_ACT_SHOT; > > + /* if changing outer proto type, update eth->h_proto */ > + if (encap_proto == IPPROTO_IPV6) { > + struct ethhdr eth; > + > + if (bpf_skb_load_bytes(skb, 0, ð, sizeof(eth)) < 0) > + return TC_ACT_SHOT; > + eth.h_proto = bpf_htons(ETH_P_IP); > + if (bpf_skb_store_bytes(skb, 0, ð, sizeof(eth), 0) < 0) > + return TC_ACT_SHOT; > + } > + > return TC_ACT_OK; > } > > @@ -325,6 +363,15 @@ int __encap_udp_eth(struct __sk_buff *skb) > return TC_ACT_OK; > } > > +SEC("encap_sit_none") > +int __encap_sit_none(struct __sk_buff *skb) > +{ > + if (skb->protocol == __bpf_constant_htons(ETH_P_IPV6)) > + return encap_ipv4(skb, IPPROTO_IPV6, ETH_P_IP); > + else > + return TC_ACT_OK; > +} > + > SEC("encap_ip6tnl_none") > int __encap_ip6tnl_none(struct __sk_buff *skb) > { [...] > -- > 2.21.0.593.g511ec345e18-goog > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: expand test_tc_tunnel with SIT encap 2019-04-22 23:40 ` Y Song @ 2019-04-22 23:47 ` Willem de Bruijn 2019-04-22 23:56 ` Y Song 0 siblings, 1 reply; 8+ messages in thread From: Willem de Bruijn @ 2019-04-22 23:47 UTC (permalink / raw) To: Y Song Cc: netdev, Alexei Starovoitov, Daniel Borkmann, Alan Maguire, Willem de Bruijn On Mon, Apr 22, 2019 at 7:40 PM Y Song <ys114321@gmail.com> wrote: > > On Mon, Apr 22, 2019 at 7:58 AM Willem de Bruijn > <willemdebruijn.kernel@gmail.com> wrote: > > > > From: Willem de Bruijn <willemb@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 <willemb@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). ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf-next 2/2] selftests/bpf: expand test_tc_tunnel with SIT encap 2019-04-22 23:47 ` Willem de Bruijn @ 2019-04-22 23:56 ` Y Song 0 siblings, 0 replies; 8+ messages in thread From: Y Song @ 2019-04-22 23:56 UTC (permalink / raw) To: Willem de Bruijn Cc: netdev, Alexei Starovoitov, Daniel Borkmann, Alan Maguire, Willem de Bruijn On Mon, Apr 22, 2019 at 4:47 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > On Mon, Apr 22, 2019 at 7:40 PM Y Song <ys114321@gmail.com> wrote: > > > > On Mon, Apr 22, 2019 at 7:58 AM Willem de Bruijn > > <willemdebruijn.kernel@gmail.com> wrote: > > > > > > From: Willem de Bruijn <willemb@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 <willemb@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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf-next 0/2] update skb->protocol in bpf_skb_net_grow 2019-04-22 14:50 [PATCH bpf-next 0/2] update skb->protocol in bpf_skb_net_grow Willem de Bruijn 2019-04-22 14:50 ` [PATCH bpf-next 1/2] bpf: " Willem de Bruijn 2019-04-22 14:50 ` [PATCH bpf-next 2/2] selftests/bpf: expand test_tc_tunnel with SIT encap Willem de Bruijn @ 2019-04-23 11:04 ` Alan Maguire 2 siblings, 0 replies; 8+ messages in thread From: Alan Maguire @ 2019-04-23 11:04 UTC (permalink / raw) To: Willem de Bruijn; +Cc: netdev, ast, daniel, alan.maguire, Willem de Bruijn On Mon, 22 Apr 2019, Willem de Bruijn wrote: > From: Willem de Bruijn <willemb@google.com> > > Expand the tc tunnel encap support with protocols that convert the > network layer protocol, such as 6in4. This is analogous to existing > support in bpf_skb_proto_6_to_4. > > Patch 1 implements the straightforward logic > Patch 2 tests it with a 6in4 tunnel > > Willem de Bruijn (2): > bpf: update skb->protocol in bpf_skb_net_grow > selftests/bpf: expand test_tc_tunnel with SIT encap > For the series Reviewed-by: Alan Maguire <alan.maguire@oracle.com> Tested-by: Alan Maguire <alan.maguire@oracle.com> > net/core/filter.c | 8 +++ > tools/testing/selftests/bpf/config | 1 + > .../selftests/bpf/progs/test_tc_tunnel.c | 55 +++++++++++++++++-- > tools/testing/selftests/bpf/test_tc_tunnel.sh | 20 ++++++- > 4 files changed, 79 insertions(+), 5 deletions(-) > > -- > 2.21.0.593.g511ec345e18-goog > > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-04-23 11:04 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-04-22 14:50 [PATCH bpf-next 0/2] update skb->protocol in bpf_skb_net_grow Willem de Bruijn 2019-04-22 14:50 ` [PATCH bpf-next 1/2] bpf: " Willem de Bruijn 2019-04-22 23:35 ` Y Song 2019-04-22 14:50 ` [PATCH bpf-next 2/2] selftests/bpf: expand test_tc_tunnel with SIT encap Willem de Bruijn 2019-04-22 23:40 ` Y Song 2019-04-22 23:47 ` Willem de Bruijn 2019-04-22 23:56 ` Y Song 2019-04-23 11:04 ` [PATCH bpf-next 0/2] update skb->protocol in bpf_skb_net_grow Alan Maguire
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.