openvswitch: add IPv6 tunneling support

Store tunnel protocol (AF_INET or AF_INET6) in sw_flow_key. This field now also acts as an indicator whether the flow contains tunnel data (this was previously indicated by tun_key.u.ipv4.dst being set but with IPv6 addresses in an union with IPv4 ones this won't work anymore).

The new field was added to a hole in sw_flow_key.

Signed-off-by Jiri Benc <[email protected]>

---
 net/openvswitch/flow.c         | 4 ++--
 net/openvswitch/flow.h         | 1 +
 net/openvswitch/flow_netlink.c | 7 +++++--
 net/openvswitch/flow_table.c   | 2 +-
 4 files changed, 9 insertions(+), 5 deletions(-)

When compat code for tunnel configuration is used, IPv6 tun_info will be rejected by ovs_tunnel_get_egress_info. As the consequence, only the new way of tunnel config supports IPv6.

We can add rather add TUNNEL_IPV6 flag to distinguish IPv4 and IPv6 tunnel keys. This can be stored in ip_tunnel_key.tun_flags.

That also saves space in flow key.

Quoted 7 lines · Jiri Benc

net/openvswitch/flow_netlink.c +0 -0

index 5c030a4d7338..03ba070c3256 100644
@@ -643,6 +643,7 @@ static int ipv4_tun_from_nlattr(const struct nlattr *attr,
        }
 

net/openvswitch/flow_netlink.c static int ipv4_tun_from_nlattr(const struct nlattr *attr, · line 646

Quoted 2 lines

net/openvswitch/flow_netlink.c +1 -0

        SW_FLOW_KEY_PUT(match, tun_key.tun_flags, tun_flags, is_mask);
+       SW_FLOW_KEY_PUT(match, tun_proto, AF_INET, is_mask);

I don't think this is right in the case of the mask. It will cause the the mask to be the value AF_INET - instead you want to set the mask to be 0xff.

This appears to me to be a bug in the existing code. ovs_tunnel_get_egress_info() as a general mechanism is still in use and should work with both the old and new configuration methods.

However, I agree that it doesn't look like it will work currently with tunnel devices. I think we need to fix this rather than making it more broken.

Quoted 2 lines · Pravin Shelar

We can add rather add TUNNEL_IPV6 flag to distinguish IPv4 and IPv6 tunnel keys. This can be stored in ip_tunnel_key.tun_flags.

Not really. This was my original approach, too, but openvswitch is not the only user of struct ip_tunnel_key, and in the lwtunnel core, tun_flags are handled in the way that makes this impractical. Most importantly, the tun_flags value is directly taken from/stored to LWTUNNEL_IP_FLAGS/LWTUNNEL_IP6_FLAGS netlink attributes in net/ipv4/ip_tunnel_core.c. This would mean complicated masking, etc.

Quoted 1 line

That also saves space in flow key.

The field was added to a 2 byte hole in the struct sw_flow_key (leaving still 1 byte free), thus there's no additional space used.

Jiri

--
Jiri Benc

Quoted 3 lines · Jesse Gross

This appears to me to be a bug in the existing code. ovs_tunnel_get_egress_info() as a general mechanism is still in use and should work with both the old and new configuration methods.

It's currently used only from the compat layer (the API that the user space that is unaware of lwtunnels use).

I don't understand what it would be good for with lwtunnel based tunnels. The metadata_dst is created in the validate_and_copy_set_tun function (net/openvswitch/flow_netlink.c) and used to specify egress encapsulation metadata. The ovs_tunnel_get_egress_info function is not needed.

Quoted 11 lines · Jiri Benc

Quoted 2 lines · Pravin Shelar

We can add rather add TUNNEL_IPV6 flag to distinguish IPv4 and IPv6 tunnel keys. This can be stored in ip_tunnel_key.tun_flags.

Not really. This was my original approach, too, but openvswitch is not the only user of struct ip_tunnel_key, and in the lwtunnel core, tun_flags are handled in the way that makes this impractical. Most importantly, the tun_flags value is directly taken from/stored to LWTUNNEL_IP_FLAGS/LWTUNNEL_IP6_FLAGS netlink attributes in net/ipv4/ip_tunnel_core.c. This would mean complicated masking, etc.

How is it impractical ? Userspace can set flag for IPv6 tunnel info. That should be easy.

IPv6 bit can not be masked anyways so I do not see problem with masking this flag due to the new bit.

Since this field is exposed to userspace. TUNNEL_* flags needs to be moved to uapi header.

Quoted 12 lines · Pravin Shelar

Quoted 11 lines · Jiri Benc

Quoted 2 lines · Pravin Shelar

We can add rather add TUNNEL_IPV6 flag to distinguish IPv4 and IPv6 tunnel keys. This can be stored in ip_tunnel_key.tun_flags.

Not really. This was my original approach, too, but openvswitch is not the only user of struct ip_tunnel_key, and in the lwtunnel core, tun_flags are handled in the way that makes this impractical. Most importantly, the tun_flags value is directly taken from/stored to LWTUNNEL_IP_FLAGS/LWTUNNEL_IP6_FLAGS netlink attributes in net/ipv4/ip_tunnel_core.c. This would mean complicated masking, etc.

Quoted 5 lines

How is it impractical ? Userspace can set flag for IPv6 tunnel info. That should be easy.

IPv6 bit can not be masked anyways so I do not see problem with masking this flag due to the new bit.

I think he meant for non-OVS users.

Quoted 2 lines

Since this field is exposed to userspace. TUNNEL_* flags needs to be moved to uapi header.

This doesn't really seem all that desirable to me. It's nice to be able to change these as necessary and in the particular case of IPv6, it seems like something that the kernel can manage by itself (as is done in this patch and I think the same strategy would apply regardless of the particular representation).

This function is used to report back information that is the result of the encapsulation process, such as the UDP source port chosen. Take a look at net/openvswitch/actions.c:output_userspace(), particularly the OVS_USERSPACE_ATTR_EGRESS_TUN_PORT case.

Quoted 4 lines · Jesse Gross

This function is used to report back information that is the result of the encapsulation process, such as the UDP source port chosen. Take a look at net/openvswitch/actions.c:output_userspace(), particularly the OVS_USERSPACE_ATTR_EGRESS_TUN_PORT case.

I see. I think it should be addressed separately from this patchset, though, as the function needs to be completely rewritten even for IPv4 and IPv6 can be handled alongside it.

I'll change the patch description in v2, the current wording is not correct. I don't think that fixing the bug should be a prerequisite for this patchset, the problem is already there and this patchset doesn't change that.