| Message ID | 550161ff4e3892d21bcbe9b8f0fb2bbe8e2025b5.1518104026.git.echaudro@redhat.com |
|---|---|
| State | Superseded |
| Delegated to: | Ian Stokes |
| Headers | show |
| Series | [ovs-dev,v2,1/1] userspace: Add IPv6 extension header parsing for tunnels | expand |
On Thu, Feb 08, 2018 at 04:40:38PM +0100, Eelco Chaudron wrote: > While OVS userspace datapath (OVS-DPDK) supports GREv6, it does not > inter-operate with a native Linux ip6gretap tunnel. This is because > the Linux driver uses IPv6 optional headers for the Tunnel > Encapsulation Limit (RFC 2473, section 6.6). Maybe worth noting that the kernel started adding these headers in 4.12. Specifically, it was introduced by 89a23c8b528b ("ip6_tunnel: Fix missing tunnel encapsulation limit option") > > OVS userspace simply does not parse these IPv6 extension headers > inside netdev_tnl_ip_extract_tnl_md(), as such popping the tunnel > leaves extra bytes resulting in a mangled decapsulated frame. > > The change below will parse the IPv6 "next header" chain and return > the offset to the upper layer protocol. > > v1->v2 > - Remove netdev_tnl_ip6_get_upperlayer_offset() and reused existing > parse_ipv6_ext_hdrs() function. > > Signed-off-by: Eelco Chaudron <echaudro@redhat.com> > --- > lib/netdev-native-tnl.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c > index fb5eab033..4f520a0f9 100644 > --- a/lib/netdev-native-tnl.c > +++ b/lib/netdev-native-tnl.c > @@ -115,6 +115,10 @@ netdev_tnl_ip_extract_tnl_md(struct dp_packet *packet, struct flow_tnl *tnl, > *hlen += IP_HEADER_LEN; > > } else if (IP_VER(ip->ip_ihl_ver) == 6) { > + const void *ip6_data; > + size_t ip6_size; > + uint8_t nw_proto; > + uint8_t nw_frag; > ovs_be32 tc_flow = get_16aligned_be32(&ip6->ip6_flow); > > memcpy(tnl->ipv6_src.s6_addr, ip6->ip6_src.be16, sizeof ip6->ip6_src); > @@ -125,6 +129,20 @@ netdev_tnl_ip_extract_tnl_md(struct dp_packet *packet, struct flow_tnl *tnl, > > *hlen += IPV6_HEADER_LEN; > > + ip6_data = ip6 + 1; > + ip6_size = l3_size - IPV6_HEADER_LEN; > + nw_proto = ip6->ip6_nxt; > + nw_frag = 0; > + > + if (!parse_ipv6_ext_hdrs(&ip6_data, &ip6_size, &nw_proto, &nw_frag) || > + nw_frag != 0) { > + VLOG_WARN_RL(&err_rl, > + "ipv6 packet has unsupported extension headers"); > + return NULL; > + } > + > + *hlen += l3_size - IPV6_HEADER_LEN - ip6_size; > + > } else { > VLOG_WARN_RL(&err_rl, "ipv4 packet has invalid version (%d)", > IP_VER(ip->ip_ihl_ver)); > -- > 2.14.3 > Thanks Eelco. I tested this by applying it on top of a non-upstream gre6 test case I had previously written [0]. If this gets applied I can post that testcase to the list. Tested-by: Eric Garver <e@erig.me> [0] https://github.com/erig0/ovs/commits/ff2e39278b66b3fe937dba63b32e341098901c50
On Thu, Feb 8, 2018 at 10:42 AM, Eric Garver <e@erig.me> wrote: > On Thu, Feb 08, 2018 at 04:40:38PM +0100, Eelco Chaudron wrote: >> While OVS userspace datapath (OVS-DPDK) supports GREv6, it does not >> inter-operate with a native Linux ip6gretap tunnel. This is because >> the Linux driver uses IPv6 optional headers for the Tunnel >> Encapsulation Limit (RFC 2473, section 6.6). > > Maybe worth noting that the kernel started adding these headers in > 4.12. Specifically, it was introduced by 89a23c8b528b ("ip6_tunnel: Fix > missing tunnel encapsulation limit option") > >> >> OVS userspace simply does not parse these IPv6 extension headers >> inside netdev_tnl_ip_extract_tnl_md(), as such popping the tunnel >> leaves extra bytes resulting in a mangled decapsulated frame. >> >> The change below will parse the IPv6 "next header" chain and return >> the offset to the upper layer protocol. >> >> v1->v2 >> - Remove netdev_tnl_ip6_get_upperlayer_offset() and reused existing >> parse_ipv6_ext_hdrs() function. >> I also hit this issue recent. I tested this patch and it works OK. However, do you think its simpler if we do below: --- a/lib/netdev-native-tnl.c +++ b/lib/netdev-native-tnl.c @@ -146,8 +146,8 @@ netdev_tnl_ip_extract_tnl_md(struct dp_packet *packet, struct flow_tnl *tnl, tnl->ip_tos = ntohl(tc_flow) >> 20; tnl->ip_ttl = ip6->ip6_hlim; - *hlen += IPV6_HEADER_LEN; + *hlen += packet->l4_ofs - packet->l3_ofs; Since parse_ipv6_ext_hdrs() already called at packet parsing stage, the l4_ofs has the extension header's length. Regards, William >> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> >> --- >> lib/netdev-native-tnl.c | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> >> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c >> index fb5eab033..4f520a0f9 100644 >> --- a/lib/netdev-native-tnl.c >> +++ b/lib/netdev-native-tnl.c >> @@ -115,6 +115,10 @@ netdev_tnl_ip_extract_tnl_md(struct dp_packet *packet, struct flow_tnl *tnl, >> *hlen += IP_HEADER_LEN; >> >> } else if (IP_VER(ip->ip_ihl_ver) == 6) { >> + const void *ip6_data; >> + size_t ip6_size; >> + uint8_t nw_proto; >> + uint8_t nw_frag; >> ovs_be32 tc_flow = get_16aligned_be32(&ip6->ip6_flow); >> >> memcpy(tnl->ipv6_src.s6_addr, ip6->ip6_src.be16, sizeof ip6->ip6_src); >> @@ -125,6 +129,20 @@ netdev_tnl_ip_extract_tnl_md(struct dp_packet *packet, struct flow_tnl *tnl, >> >> *hlen += IPV6_HEADER_LEN; >> >> + ip6_data = ip6 + 1; >> + ip6_size = l3_size - IPV6_HEADER_LEN; >> + nw_proto = ip6->ip6_nxt; >> + nw_frag = 0; >> + >> + if (!parse_ipv6_ext_hdrs(&ip6_data, &ip6_size, &nw_proto, &nw_frag) || >> + nw_frag != 0) { >> + VLOG_WARN_RL(&err_rl, >> + "ipv6 packet has unsupported extension headers"); >> + return NULL; >> + } >> + >> + *hlen += l3_size - IPV6_HEADER_LEN - ip6_size; >> + >> } else { >> VLOG_WARN_RL(&err_rl, "ipv4 packet has invalid version (%d)", >> IP_VER(ip->ip_ihl_ver)); >> -- >> 2.14.3 >> > > Thanks Eelco. I tested this by applying it on top of a non-upstream gre6 > test case I had previously written [0]. If this gets applied I can post > that testcase to the list. > > Tested-by: Eric Garver <e@erig.me> > > [0] https://github.com/erig0/ovs/commits/ff2e39278b66b3fe937dba63b32e341098901c50 > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 03/09/2018 03:59 PM, William Tu wrote: > On Thu, Feb 8, 2018 at 10:42 AM, Eric Garver <e@erig.me> wrote: >> On Thu, Feb 08, 2018 at 04:40:38PM +0100, Eelco Chaudron wrote: >>> While OVS userspace datapath (OVS-DPDK) supports GREv6, it does not >>> inter-operate with a native Linux ip6gretap tunnel. This is because >>> the Linux driver uses IPv6 optional headers for the Tunnel >>> Encapsulation Limit (RFC 2473, section 6.6). >> Maybe worth noting that the kernel started adding these headers in >> 4.12. Specifically, it was introduced by 89a23c8b528b ("ip6_tunnel: Fix >> missing tunnel encapsulation limit option") >> >>> OVS userspace simply does not parse these IPv6 extension headers >>> inside netdev_tnl_ip_extract_tnl_md(), as such popping the tunnel >>> leaves extra bytes resulting in a mangled decapsulated frame. >>> >>> The change below will parse the IPv6 "next header" chain and return >>> the offset to the upper layer protocol. >>> >>> v1->v2 >>> - Remove netdev_tnl_ip6_get_upperlayer_offset() and reused existing >>> parse_ipv6_ext_hdrs() function. >>> > I also hit this issue recent. I tested this patch and it works OK. > However, do you think its simpler if we do below: > --- a/lib/netdev-native-tnl.c > +++ b/lib/netdev-native-tnl.c > @@ -146,8 +146,8 @@ netdev_tnl_ip_extract_tnl_md(struct dp_packet > *packet, struct flow_tnl *tnl, > tnl->ip_tos = ntohl(tc_flow) >> 20; > tnl->ip_ttl = ip6->ip6_hlim; > > - *hlen += IPV6_HEADER_LEN; > + *hlen += packet->l4_ofs - packet->l3_ofs; > > Since parse_ipv6_ext_hdrs() already called at packet parsing stage, > the l4_ofs has the extension header's length. Are you sure this path is taken for all packets? Quickly looking at the code I see this path only with connection tracking actions. Or did I miss something? >>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> >>> --- >>> lib/netdev-native-tnl.c | 18 ++++++++++++++++++ >>> 1 file changed, 18 insertions(+) >>> >>> diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c >>> index fb5eab033..4f520a0f9 100644 >>> --- a/lib/netdev-native-tnl.c >>> +++ b/lib/netdev-native-tnl.c >>> @@ -115,6 +115,10 @@ netdev_tnl_ip_extract_tnl_md(struct dp_packet *packet, struct flow_tnl *tnl, >>> *hlen += IP_HEADER_LEN; >>> >>> } else if (IP_VER(ip->ip_ihl_ver) == 6) { >>> + const void *ip6_data; >>> + size_t ip6_size; >>> + uint8_t nw_proto; >>> + uint8_t nw_frag; >>> ovs_be32 tc_flow = get_16aligned_be32(&ip6->ip6_flow); >>> >>> memcpy(tnl->ipv6_src.s6_addr, ip6->ip6_src.be16, sizeof ip6->ip6_src); >>> @@ -125,6 +129,20 @@ netdev_tnl_ip_extract_tnl_md(struct dp_packet *packet, struct flow_tnl *tnl, >>> >>> *hlen += IPV6_HEADER_LEN; >>> >>> + ip6_data = ip6 + 1; >>> + ip6_size = l3_size - IPV6_HEADER_LEN; >>> + nw_proto = ip6->ip6_nxt; >>> + nw_frag = 0; >>> + >>> + if (!parse_ipv6_ext_hdrs(&ip6_data, &ip6_size, &nw_proto, &nw_frag) || >>> + nw_frag != 0) { >>> + VLOG_WARN_RL(&err_rl, >>> + "ipv6 packet has unsupported extension headers"); >>> + return NULL; >>> + } >>> + >>> + *hlen += l3_size - IPV6_HEADER_LEN - ip6_size; >>> + >>> } else { >>> VLOG_WARN_RL(&err_rl, "ipv4 packet has invalid version (%d)", >>> IP_VER(ip->ip_ihl_ver)); >>> -- >>> 2.14.3 >>> >> Thanks Eelco. I tested this by applying it on top of a non-upstream gre6 >> test case I had previously written [0]. If this gets applied I can post >> that testcase to the list. >> >> Tested-by: Eric Garver <e@erig.me> >> >> [0] https://github.com/erig0/ovs/commits/ff2e39278b66b3fe937dba63b32e341098901c50 >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Wed, Mar 28, 2018 at 9:52 AM, Eelco Chaudron <echaudro@redhat.com> wrote: > On 03/09/2018 03:59 PM, William Tu wrote: >> >> On Thu, Feb 8, 2018 at 10:42 AM, Eric Garver <e@erig.me> wrote: >>> >>> On Thu, Feb 08, 2018 at 04:40:38PM +0100, Eelco Chaudron wrote: >>>> >>>> While OVS userspace datapath (OVS-DPDK) supports GREv6, it does not >>>> inter-operate with a native Linux ip6gretap tunnel. This is because >>>> the Linux driver uses IPv6 optional headers for the Tunnel >>>> Encapsulation Limit (RFC 2473, section 6.6). >>> >>> Maybe worth noting that the kernel started adding these headers in >>> 4.12. Specifically, it was introduced by 89a23c8b528b ("ip6_tunnel: Fix >>> missing tunnel encapsulation limit option") >>> >>>> OVS userspace simply does not parse these IPv6 extension headers >>>> inside netdev_tnl_ip_extract_tnl_md(), as such popping the tunnel >>>> leaves extra bytes resulting in a mangled decapsulated frame. >>>> >>>> The change below will parse the IPv6 "next header" chain and return >>>> the offset to the upper layer protocol. >>>> >>>> v1->v2 >>>> - Remove netdev_tnl_ip6_get_upperlayer_offset() and reused existing >>>> parse_ipv6_ext_hdrs() function. >>>> >> I also hit this issue recent. I tested this patch and it works OK. >> However, do you think its simpler if we do below: >> --- a/lib/netdev-native-tnl.c >> +++ b/lib/netdev-native-tnl.c >> @@ -146,8 +146,8 @@ netdev_tnl_ip_extract_tnl_md(struct dp_packet >> *packet, struct flow_tnl *tnl, >> tnl->ip_tos = ntohl(tc_flow) >> 20; >> tnl->ip_ttl = ip6->ip6_hlim; >> >> - *hlen += IPV6_HEADER_LEN; >> + *hlen += packet->l4_ofs - packet->l3_ofs; >> >> Since parse_ipv6_ext_hdrs() already called at packet parsing stage, >> the l4_ofs has the extension header's length. > > Are you sure this path is taken for all packets? Quickly looking at the code > I see this path only with connection tracking actions. Or did I miss > something? > in the miniflow_extract, it calls parse_ipv6_ext_hdrs__(&data, &size, &nw_proto, &nw_frag) so the 'size' is updated with the ipv6 extension length. William
On 30/03/18 18:35, William Tu wrote: > On Wed, Mar 28, 2018 at 9:52 AM, Eelco Chaudron <echaudro@redhat.com> wrote: >> On 03/09/2018 03:59 PM, William Tu wrote: >>> On Thu, Feb 8, 2018 at 10:42 AM, Eric Garver <e@erig.me> wrote: >>>> On Thu, Feb 08, 2018 at 04:40:38PM +0100, Eelco Chaudron wrote: >>>>> While OVS userspace datapath (OVS-DPDK) supports GREv6, it does not >>>>> inter-operate with a native Linux ip6gretap tunnel. This is because >>>>> the Linux driver uses IPv6 optional headers for the Tunnel >>>>> Encapsulation Limit (RFC 2473, section 6.6). >>>> Maybe worth noting that the kernel started adding these headers in >>>> 4.12. Specifically, it was introduced by 89a23c8b528b ("ip6_tunnel: Fix >>>> missing tunnel encapsulation limit option") >>>> >>>>> OVS userspace simply does not parse these IPv6 extension headers >>>>> inside netdev_tnl_ip_extract_tnl_md(), as such popping the tunnel >>>>> leaves extra bytes resulting in a mangled decapsulated frame. >>>>> >>>>> The change below will parse the IPv6 "next header" chain and return >>>>> the offset to the upper layer protocol. >>>>> >>>>> v1->v2 >>>>> - Remove netdev_tnl_ip6_get_upperlayer_offset() and reused existing >>>>> parse_ipv6_ext_hdrs() function. >>>>> >>> I also hit this issue recent. I tested this patch and it works OK. >>> However, do you think its simpler if we do below: >>> --- a/lib/netdev-native-tnl.c >>> +++ b/lib/netdev-native-tnl.c >>> @@ -146,8 +146,8 @@ netdev_tnl_ip_extract_tnl_md(struct dp_packet >>> *packet, struct flow_tnl *tnl, >>> tnl->ip_tos = ntohl(tc_flow) >> 20; >>> tnl->ip_ttl = ip6->ip6_hlim; >>> >>> - *hlen += IPV6_HEADER_LEN; >>> + *hlen += packet->l4_ofs - packet->l3_ofs; >>> >>> Since parse_ipv6_ext_hdrs() already called at packet parsing stage, >>> the l4_ofs has the extension header's length. >> Are you sure this path is taken for all packets? Quickly looking at the code >> I see this path only with connection tracking actions. Or did I miss >> something? >> > in the miniflow_extract, it calls > parse_ipv6_ext_hdrs__(&data, &size, &nw_proto, &nw_frag) > so the 'size' is updated with the ipv6 extension length. That was really stupid of me missing the static one :( I'll rework/test your suggestion and send out a V3 later this week (early next week). If some one does not like William's suggestion, please speak up now... //Eelco
diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c index fb5eab033..4f520a0f9 100644 --- a/lib/netdev-native-tnl.c +++ b/lib/netdev-native-tnl.c @@ -115,6 +115,10 @@ netdev_tnl_ip_extract_tnl_md(struct dp_packet *packet, struct flow_tnl *tnl, *hlen += IP_HEADER_LEN; } else if (IP_VER(ip->ip_ihl_ver) == 6) { + const void *ip6_data; + size_t ip6_size; + uint8_t nw_proto; + uint8_t nw_frag; ovs_be32 tc_flow = get_16aligned_be32(&ip6->ip6_flow); memcpy(tnl->ipv6_src.s6_addr, ip6->ip6_src.be16, sizeof ip6->ip6_src); @@ -125,6 +129,20 @@ netdev_tnl_ip_extract_tnl_md(struct dp_packet *packet, struct flow_tnl *tnl, *hlen += IPV6_HEADER_LEN; + ip6_data = ip6 + 1; + ip6_size = l3_size - IPV6_HEADER_LEN; + nw_proto = ip6->ip6_nxt; + nw_frag = 0; + + if (!parse_ipv6_ext_hdrs(&ip6_data, &ip6_size, &nw_proto, &nw_frag) || + nw_frag != 0) { + VLOG_WARN_RL(&err_rl, + "ipv6 packet has unsupported extension headers"); + return NULL; + } + + *hlen += l3_size - IPV6_HEADER_LEN - ip6_size; + } else { VLOG_WARN_RL(&err_rl, "ipv4 packet has invalid version (%d)", IP_VER(ip->ip_ihl_ver));
While OVS userspace datapath (OVS-DPDK) supports GREv6, it does not inter-operate with a native Linux ip6gretap tunnel. This is because the Linux driver uses IPv6 optional headers for the Tunnel Encapsulation Limit (RFC 2473, section 6.6). OVS userspace simply does not parse these IPv6 extension headers inside netdev_tnl_ip_extract_tnl_md(), as such popping the tunnel leaves extra bytes resulting in a mangled decapsulated frame. The change below will parse the IPv6 "next header" chain and return the offset to the upper layer protocol. v1->v2 - Remove netdev_tnl_ip6_get_upperlayer_offset() and reused existing parse_ipv6_ext_hdrs() function. Signed-off-by: Eelco Chaudron <echaudro@redhat.com> --- lib/netdev-native-tnl.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)