[ovs-dev] [PATCH v2 1/1] userspace: Add IPv6 extension header parsing for tunnels
echaudro at redhat.com
Tue Apr 3 08:53:02 UTC 2018
On 30/03/18 18:35, William Tu wrote:
> On Wed, Mar 28, 2018 at 9:52 AM, Eelco Chaudron <echaudro at 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 at 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.
>>>>> - 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
> 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...
More information about the dev