* [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
* [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 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
* 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.