[ovs-dev,v2,1/1] userspace: Add IPv6 extension header parsing for tunnels

Message ID 550161ff4e3892d21bcbe9b8f0fb2bbe8e2025b5.1518104026.git.echaudro@redhat.com
State Changes Requested
Delegated to: Ian Stokes
Headers show
Series
  • [ovs-dev,v2,1/1] userspace: Add IPv6 extension header parsing for tunnels
Related show

Commit Message

Eelco Chaudron Feb. 8, 2018, 3:40 p.m.
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(+)

Comments

Eric Garver Feb. 8, 2018, 6:42 p.m. | #1
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
William Tu March 9, 2018, 2:59 p.m. | #2
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
Eelco Chaudron March 28, 2018, 4:52 p.m. | #3
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
William Tu March 30, 2018, 4:35 p.m. | #4
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
Eelco Chaudron April 3, 2018, 8:53 a.m. | #5
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

Patch

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));