[PATCH net-next 0/2] openvswitch: add IPv6 tunneling support

[PATCH net-next 0/2] openvswitch: add IPv6 tunneling support

[Jiri Benc]

This builds on the previous work that added IPv6 support to lwtunnels and
adds IPv6 tunneling support to ovs; it is now pretty straightforward. 

To use IPv6 tunneling, there needs to be a metadata based tunnel net_device
created and added to the ovs bridge. Currently, only vxlan is supported by
the kernel, with geneve to follow shortly. There's no need nor intent to add
a support for this into the vport-vxlan compat layer.

Jiri Benc (2):
  openvswitch: add tunnel protocol to sw_flow_key
  openvswitch: netlink attributes for IPv6 tunneling

 include/uapi/linux/openvswitch.h |   2 +
 net/openvswitch/flow.c           |   4 +-
 net/openvswitch/flow.h           |   1 +
 net/openvswitch/flow_netlink.c   | 125 +++++++++++++++++++++++++++------------
 net/openvswitch/flow_table.c     |   2 +-
 5 files changed, 93 insertions(+), 41 deletions(-)

-- 
1.8.3.1

[PATCH net-next 1/2] openvswitch: add tunnel protocol to sw_flow_key

[Jiri Benc]

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 <jbenc@redhat.com>
---
 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(-)

diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index c8db44ab2ee7..0ea128eeeab2 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -698,8 +698,7 @@ int ovs_flow_key_extract(const struct ip_tunnel_info *tun_info,
 {
 	/* Extract metadata from packet. */
 	if (tun_info) {
-		if (ip_tunnel_info_af(tun_info) != AF_INET)
-			return -EINVAL;
+		key->tun_proto = ip_tunnel_info_af(tun_info);
 		memcpy(&key->tun_key, &tun_info->key, sizeof(key->tun_key));
 
 		if (tun_info->options_len) {
@@ -714,6 +713,7 @@ int ovs_flow_key_extract(const struct ip_tunnel_info *tun_info,
 			key->tun_opts_len = 0;
 		}
 	} else  {
+		key->tun_proto = 0;
 		key->tun_opts_len = 0;
 		memset(&key->tun_key, 0, sizeof(key->tun_key));
 	}
diff --git a/net/openvswitch/flow.h b/net/openvswitch/flow.h
index fe527d2dd4b7..5688e33e2de6 100644
--- a/net/openvswitch/flow.h
+++ b/net/openvswitch/flow.h
@@ -63,6 +63,7 @@ struct sw_flow_key {
 		u32	skb_mark;	/* SKB mark. */
 		u16	in_port;	/* Input switch port (or DP_MAX_PORTS). */
 	} __packed phy; /* Safe when right after 'tun_key'. */
+	u8 tun_proto;			/* Protocol of encapsulating tunnel. */
 	u32 ovs_flow_hash;		/* Datapath computed hash value.  */
 	u32 recirc_id;			/* Recirculation ID.  */
 	struct {
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index 5c030a4d7338..03ba070c3256 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -643,6 +643,7 @@ static int ipv4_tun_from_nlattr(const struct nlattr *attr,
 	}
 
 	SW_FLOW_KEY_PUT(match, tun_key.tun_flags, tun_flags, is_mask);
+	SW_FLOW_KEY_PUT(match, tun_proto, AF_INET, is_mask);
 
 	if (rem > 0) {
 		OVS_NLERR(log, "IPv4 tunnel attribute has %d unknown bytes.",
@@ -1194,7 +1195,7 @@ int ovs_nla_get_match(struct net *net, struct sw_flow_match *match,
 			/* The userspace does not send tunnel attributes that
 			 * are 0, but we should not wildcard them nonetheless.
 			 */
-			if (match->key->tun_key.u.ipv4.dst)
+			if (match->key->tun_proto)
 				SW_FLOW_KEY_MEMSET_FIELD(match, tun_key,
 							 0xff, true);
 
@@ -1367,7 +1368,7 @@ static int __ovs_nla_put_key(const struct sw_flow_key *swkey,
 	if (nla_put_u32(skb, OVS_KEY_ATTR_PRIORITY, output->phy.priority))
 		goto nla_put_failure;
 
-	if ((swkey->tun_key.u.ipv4.dst || is_mask)) {
+	if ((swkey->tun_proto || is_mask)) {
 		const void *opts = NULL;
 
 		if (output->tun_key.tun_flags & TUNNEL_OPTIONS_PRESENT)
@@ -1913,6 +1914,8 @@ static int validate_and_copy_set_tun(const struct nlattr *attr,
 
 	tun_info = &tun_dst->u.tun_info;
 	tun_info->mode = IP_TUNNEL_INFO_TX;
+	if (key.tun_proto == AF_INET6)
+		tun_info->mode |= IP_TUNNEL_INFO_IPV6;
 	tun_info->key = key.tun_key;
 
 	/* We need to store the options in the action itself since
diff --git a/net/openvswitch/flow_table.c b/net/openvswitch/flow_table.c
index f2ea83ba4763..95dbcedf0bd4 100644
--- a/net/openvswitch/flow_table.c
+++ b/net/openvswitch/flow_table.c
@@ -427,7 +427,7 @@ static u32 flow_hash(const struct sw_flow_key *key,
 
 static int flow_key_start(const struct sw_flow_key *key)
 {
-	if (key->tun_key.u.ipv4.dst)
+	if (key->tun_proto)
 		return 0;
 	else
 		return rounddown(offsetof(struct sw_flow_key, phy),
-- 
1.8.3.1

[PATCH net-next 2/2] openvswitch: netlink attributes for IPv6 tunneling

[Jiri Benc]

Add netlink attributes for IPv6 tunnel addresses. This enables IPv6 support
for tunnels.

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.

Signed-off-by: Jiri Benc <jbenc@redhat.com>
---
 include/uapi/linux/openvswitch.h |   2 +
 net/openvswitch/flow_netlink.c   | 120 +++++++++++++++++++++++++++------------
 2 files changed, 85 insertions(+), 37 deletions(-)

diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index 32e07d8cbaf4..4036e1b1980f 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -349,6 +349,8 @@ enum ovs_tunnel_key_attr {
 	OVS_TUNNEL_KEY_ATTR_TP_SRC,		/* be16 src Transport Port. */
 	OVS_TUNNEL_KEY_ATTR_TP_DST,		/* be16 dst Transport Port. */
 	OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS,		/* Nested OVS_VXLAN_EXT_* */
+	OVS_TUNNEL_KEY_ATTR_IPV6_SRC,		/* struct in6_addr src IPv6 address. */
+	OVS_TUNNEL_KEY_ATTR_IPV6_DST,		/* struct in6_addr dst IPv6 address. */
 	__OVS_TUNNEL_KEY_ATTR_MAX
 };
 
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index 03ba070c3256..73df3cda1070 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -262,8 +262,8 @@ size_t ovs_tun_key_attr_size(void)
 	 * updating this function.
 	 */
 	return    nla_total_size(8)    /* OVS_TUNNEL_KEY_ATTR_ID */
-		+ nla_total_size(4)    /* OVS_TUNNEL_KEY_ATTR_IPV4_SRC */
-		+ nla_total_size(4)    /* OVS_TUNNEL_KEY_ATTR_IPV4_DST */
+		+ nla_total_size(16)   /* OVS_TUNNEL_KEY_ATTR_IPV[46]_SRC */
+		+ nla_total_size(16)   /* OVS_TUNNEL_KEY_ATTR_IPV[46]_DST */
 		+ nla_total_size(1)    /* OVS_TUNNEL_KEY_ATTR_TOS */
 		+ nla_total_size(1)    /* OVS_TUNNEL_KEY_ATTR_TTL */
 		+ nla_total_size(0)    /* OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT */
@@ -323,6 +323,8 @@ static const struct ovs_len_tbl ovs_tunnel_key_lens[OVS_TUNNEL_KEY_ATTR_MAX + 1]
 	[OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS]   = { .len = OVS_ATTR_VARIABLE },
 	[OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS]    = { .len = OVS_ATTR_NESTED,
 						.next = ovs_vxlan_ext_key_lens },
+	[OVS_TUNNEL_KEY_ATTR_IPV6_SRC]      = { .len = sizeof(struct in6_addr) },
+	[OVS_TUNNEL_KEY_ATTR_IPV6_DST]      = { .len = sizeof(struct in6_addr) },
 };
 
 /* The size of the argument for each %OVS_KEY_ATTR_* Netlink attribute.  */
@@ -542,14 +544,14 @@ static int vxlan_tun_opt_from_nlattr(const struct nlattr *attr,
 	return 0;
 }
 
-static int ipv4_tun_from_nlattr(const struct nlattr *attr,
-				struct sw_flow_match *match, bool is_mask,
-				bool log)
+static int ip_tun_from_nlattr(const struct nlattr *attr,
+			      struct sw_flow_match *match, bool is_mask,
+			      bool log)
 {
 	struct nlattr *a;
 	int rem;
 	bool ttl = false;
-	__be16 tun_flags = 0;
+	__be16 tun_flags = 0, ipv4 = false, ipv6 = false;
 	int opts_type = 0;
 
 	nla_for_each_nested(a, attr, rem) {
@@ -578,10 +580,22 @@ static int ipv4_tun_from_nlattr(const struct nlattr *attr,
 		case OVS_TUNNEL_KEY_ATTR_IPV4_SRC:
 			SW_FLOW_KEY_PUT(match, tun_key.u.ipv4.src,
 					nla_get_in_addr(a), is_mask);
+			ipv4 = true;
 			break;
 		case OVS_TUNNEL_KEY_ATTR_IPV4_DST:
 			SW_FLOW_KEY_PUT(match, tun_key.u.ipv4.dst,
 					nla_get_in_addr(a), is_mask);
+			ipv4 = true;
+			break;
+		case OVS_TUNNEL_KEY_ATTR_IPV6_SRC:
+			SW_FLOW_KEY_PUT(match, tun_key.u.ipv6.dst,
+					nla_get_in6_addr(a), is_mask);
+			ipv6 = true;
+			break;
+		case OVS_TUNNEL_KEY_ATTR_IPV6_DST:
+			SW_FLOW_KEY_PUT(match, tun_key.u.ipv6.dst,
+					nla_get_in6_addr(a), is_mask);
+			ipv6 = true;
 			break;
 		case OVS_TUNNEL_KEY_ATTR_TOS:
 			SW_FLOW_KEY_PUT(match, tun_key.tos,
@@ -636,29 +650,42 @@ static int ipv4_tun_from_nlattr(const struct nlattr *attr,
 			opts_type = type;
 			break;
 		default:
-			OVS_NLERR(log, "Unknown IPv4 tunnel attribute %d",
+			OVS_NLERR(log, "Unknown IP tunnel attribute %d",
 				  type);
 			return -EINVAL;
 		}
 	}
 
 	SW_FLOW_KEY_PUT(match, tun_key.tun_flags, tun_flags, is_mask);
-	SW_FLOW_KEY_PUT(match, tun_proto, AF_INET, is_mask);
+	SW_FLOW_KEY_PUT(match, tun_proto, ipv6 ? AF_INET6 : AF_INET, is_mask);
 
 	if (rem > 0) {
-		OVS_NLERR(log, "IPv4 tunnel attribute has %d unknown bytes.",
+		OVS_NLERR(log, "IP tunnel attribute has %d unknown bytes.",
 			  rem);
 		return -EINVAL;
 	}
 
+	if (ipv4 && ipv6) {
+		OVS_NLERR(log, "Mixed IPv4 and IPv6 tunnel attributes");
+		return -EINVAL;
+	}
+
 	if (!is_mask) {
-		if (!match->key->tun_key.u.ipv4.dst) {
+		if (!ipv4 && !ipv6) {
+			OVS_NLERR(log, "IP tunnel dst address not specified");
+			return -EINVAL;
+		}
+		if (ipv4 && !match->key->tun_key.u.ipv4.dst) {
 			OVS_NLERR(log, "IPv4 tunnel dst address is zero");
 			return -EINVAL;
 		}
+		if (ipv6 && ipv6_addr_any(&match->key->tun_key.u.ipv6.dst)) {
+			OVS_NLERR(log, "IPv6 tunnel dst address is zero");
+			return -EINVAL;
+		}
 
 		if (!ttl) {
-			OVS_NLERR(log, "IPv4 tunnel TTL not specified.");
+			OVS_NLERR(log, "IP tunnel TTL not specified.");
 			return -EINVAL;
 		}
 	}
@@ -683,21 +710,36 @@ static int vxlan_opt_to_nlattr(struct sk_buff *skb,
 	return 0;
 }
 
-static int __ipv4_tun_to_nlattr(struct sk_buff *skb,
-				const struct ip_tunnel_key *output,
-				const void *tun_opts, int swkey_tun_opts_len)
+static int __ip_tun_to_nlattr(struct sk_buff *skb,
+			      const struct ip_tunnel_key *output,
+			      const void *tun_opts, int swkey_tun_opts_len,
+			      unsigned short tun_proto)
 {
 	if (output->tun_flags & TUNNEL_KEY &&
 	    nla_put_be64(skb, OVS_TUNNEL_KEY_ATTR_ID, output->tun_id))
 		return -EMSGSIZE;
-	if (output->u.ipv4.src &&
-	    nla_put_in_addr(skb, OVS_TUNNEL_KEY_ATTR_IPV4_SRC,
-			    output->u.ipv4.src))
-		return -EMSGSIZE;
-	if (output->u.ipv4.dst &&
-	    nla_put_in_addr(skb, OVS_TUNNEL_KEY_ATTR_IPV4_DST,
-			    output->u.ipv4.dst))
-		return -EMSGSIZE;
+	switch (tun_proto) {
+	case AF_INET:
+		if (output->u.ipv4.src &&
+		    nla_put_in_addr(skb, OVS_TUNNEL_KEY_ATTR_IPV4_SRC,
+				    output->u.ipv4.src))
+			return -EMSGSIZE;
+		if (output->u.ipv4.dst &&
+		    nla_put_in_addr(skb, OVS_TUNNEL_KEY_ATTR_IPV4_DST,
+				    output->u.ipv4.dst))
+			return -EMSGSIZE;
+		break;
+	case AF_INET6:
+		if (!ipv6_addr_any(&output->u.ipv6.src) &&
+		    nla_put_in6_addr(skb, OVS_TUNNEL_KEY_ATTR_IPV6_SRC,
+				     &output->u.ipv6.src))
+			return -EMSGSIZE;
+		if (!ipv6_addr_any(&output->u.ipv6.dst) &&
+		    nla_put_in6_addr(skb, OVS_TUNNEL_KEY_ATTR_IPV6_DST,
+				     &output->u.ipv6.dst))
+			return -EMSGSIZE;
+		break;
+	}
 	if (output->tos &&
 	    nla_put_u8(skb, OVS_TUNNEL_KEY_ATTR_TOS, output->tos))
 		return -EMSGSIZE;
@@ -731,9 +773,10 @@ static int __ipv4_tun_to_nlattr(struct sk_buff *skb,
 	return 0;
 }
 
-static int ipv4_tun_to_nlattr(struct sk_buff *skb,
-			      const struct ip_tunnel_key *output,
-			      const void *tun_opts, int swkey_tun_opts_len)
+static int ip_tun_to_nlattr(struct sk_buff *skb,
+			    const struct ip_tunnel_key *output,
+			    const void *tun_opts, int swkey_tun_opts_len,
+			    unsigned short tun_proto)
 {
 	struct nlattr *nla;
 	int err;
@@ -742,7 +785,8 @@ static int ipv4_tun_to_nlattr(struct sk_buff *skb,
 	if (!nla)
 		return -EMSGSIZE;
 
-	err = __ipv4_tun_to_nlattr(skb, output, tun_opts, swkey_tun_opts_len);
+	err = __ip_tun_to_nlattr(skb, output, tun_opts, swkey_tun_opts_len,
+				 tun_proto);
 	if (err)
 		return err;
 
@@ -754,9 +798,10 @@ int ovs_nla_put_egress_tunnel_key(struct sk_buff *skb,
 				  const struct ip_tunnel_info *egress_tun_info,
 				  const void *egress_tun_opts)
 {
-	return __ipv4_tun_to_nlattr(skb, &egress_tun_info->key,
-				    egress_tun_opts,
-				    egress_tun_info->options_len);
+	return __ip_tun_to_nlattr(skb, &egress_tun_info->key,
+				  egress_tun_opts,
+				  egress_tun_info->options_len,
+				  ip_tunnel_info_af(egress_tun_info));
 }
 
 static int metadata_from_nlattrs(struct net *net, struct sw_flow_match *match,
@@ -807,8 +852,8 @@ static int metadata_from_nlattrs(struct net *net, struct sw_flow_match *match,
 		*attrs &= ~(1 << OVS_KEY_ATTR_SKB_MARK);
 	}
 	if (*attrs & (1 << OVS_KEY_ATTR_TUNNEL)) {
-		if (ipv4_tun_from_nlattr(a[OVS_KEY_ATTR_TUNNEL], match,
-					 is_mask, log) < 0)
+		if (ip_tun_from_nlattr(a[OVS_KEY_ATTR_TUNNEL], match,
+				       is_mask, log) < 0)
 			return -EINVAL;
 		*attrs &= ~(1 << OVS_KEY_ATTR_TUNNEL);
 	}
@@ -1374,8 +1419,8 @@ static int __ovs_nla_put_key(const struct sw_flow_key *swkey,
 		if (output->tun_key.tun_flags & TUNNEL_OPTIONS_PRESENT)
 			opts = TUN_METADATA_OPTS(output, swkey->tun_opts_len);
 
-		if (ipv4_tun_to_nlattr(skb, &output->tun_key, opts,
-				       swkey->tun_opts_len))
+		if (ip_tun_to_nlattr(skb, &output->tun_key, opts,
+				     swkey->tun_opts_len, swkey->tun_proto))
 			goto nla_put_failure;
 	}
 
@@ -1878,7 +1923,7 @@ static int validate_and_copy_set_tun(const struct nlattr *attr,
 	int err = 0, start, opts_type;
 
 	ovs_match_init(&match, &key, NULL);
-	opts_type = ipv4_tun_from_nlattr(nla_data(attr), &match, false, log);
+	opts_type = ip_tun_from_nlattr(nla_data(attr), &match, false, log);
 	if (opts_type < 0)
 		return opts_type;
 
@@ -2377,10 +2422,11 @@ static int set_action_to_attr(const struct nlattr *a, struct sk_buff *skb)
 		if (!start)
 			return -EMSGSIZE;
 
-		err = ipv4_tun_to_nlattr(skb, &tun_info->key,
-					 tun_info->options_len ?
+		err = ip_tun_to_nlattr(skb, &tun_info->key,
+				       tun_info->options_len ?
 					     ip_tunnel_info_opts(tun_info) : NULL,
-					 tun_info->options_len);
+				       tun_info->options_len,
+				       ip_tunnel_info_af(tun_info));
 		if (err)
 			return err;
 		nla_nest_end(skb, start);
-- 
1.8.3.1

Re: [PATCH net-next 1/2] openvswitch: add tunnel protocol to sw_flow_key

[Pravin Shelar]

On Tue, Sep 29, 2015 at 10:52 AM, Jiri Benc <jbenc@redhat.com> wrote:
> 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 <jbenc@redhat.com>
> ---
>  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(-)
>
...

> diff --git a/net/openvswitch/flow.h b/net/openvswitch/flow.h
> index fe527d2dd4b7..5688e33e2de6 100644
> --- a/net/openvswitch/flow.h
> +++ b/net/openvswitch/flow.h
> @@ -63,6 +63,7 @@ struct sw_flow_key {
>                 u32     skb_mark;       /* SKB mark. */
>                 u16     in_port;        /* Input switch port (or DP_MAX_PORTS). */
>         } __packed phy; /* Safe when right after 'tun_key'. */
> +       u8 tun_proto;                   /* Protocol of encapsulating tunnel. */

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.

Re: [ovs-dev] [PATCH net-next 1/2] openvswitch: add tunnel protocol to sw_flow_key

[Jesse Gross]

On Tue, Sep 29, 2015 at 10:52 AM, Jiri Benc <jbenc@redhat.com> wrote:
> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> index 5c030a4d7338..03ba070c3256 100644
> --- a/net/openvswitch/flow_netlink.c
> +++ b/net/openvswitch/flow_netlink.c
> @@ -643,6 +643,7 @@ static int ipv4_tun_from_nlattr(const struct nlattr *attr,
>         }
>
>         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.

Re: [PATCH net-next 2/2] openvswitch: netlink attributes for IPv6 tunneling

[Jesse Gross]

On Tue, Sep 29, 2015 at 10:52 AM, Jiri Benc <jbenc@redhat.com> wrote:
> 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.

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.

Re: [PATCH net-next 1/2] openvswitch: add tunnel protocol to sw_flow_key

[Jiri Benc]

On Tue, 29 Sep 2015 13:41:34 -0700, Pravin Shelar wrote:
> 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.

> 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

Re: [ovs-dev] [PATCH net-next 1/2] openvswitch: add tunnel protocol to sw_flow_key

[Jiri Benc]

On Tue, 29 Sep 2015 19:08:44 -0700, Jesse Gross wrote:
> On Tue, Sep 29, 2015 at 10:52 AM, Jiri Benc <jbenc@redhat.com> wrote:
> > diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> > index 5c030a4d7338..03ba070c3256 100644
> > --- a/net/openvswitch/flow_netlink.c
> > +++ b/net/openvswitch/flow_netlink.c
> > @@ -643,6 +643,7 @@ static int ipv4_tun_from_nlattr(const struct nlattr *attr,
> >         }
> >
> >         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.

I think you're right, this is a special case. I'll fix it.

Thanks,

 Jiri

-- 
Jiri Benc

Re: [PATCH net-next 2/2] openvswitch: netlink attributes for IPv6 tunneling

[Jiri Benc]

On Tue, 29 Sep 2015 20:05:00 -0700, Jesse Gross wrote:
> 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.

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

I'm not making it more broken. We currently (i.e. right now, in the
current net.git) have two APIs for tunnel specification in the ovs
kernel datapath: the old one, which is translated by the compat layer
to create a net_device, and the lwtunnel one, which requires user space
to create a (metadata) tunnel net_device and add it to the datapath.
I'm simply not adding more code to the first, legacy interface, which
seems to be the correct thing to do.

 Jiri

-- 
Jiri Benc

Re: [PATCH net-next 1/2] openvswitch: add tunnel protocol to sw_flow_key

[Pravin Shelar]

On Wed, Sep 30, 2015 at 12:09 AM, Jiri Benc <jbenc@redhat.com> wrote:
> On Tue, 29 Sep 2015 13:41:34 -0700, Pravin Shelar wrote:
>> 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.


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

Re: [PATCH net-next 2/2] openvswitch: netlink attributes for IPv6 tunneling

[Jesse Gross]

On Wed, Sep 30, 2015 at 12:28 AM, Jiri Benc <jbenc@redhat.com> wrote:
> On Tue, 29 Sep 2015 20:05:00 -0700, Jesse Gross wrote:
>> 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).

Yes but that is a bug. From the perspective of the intended use of
this function, I don't think there is any difference between compat
and non-compat users.

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

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.

Re: [ovs-dev] [PATCH net-next 1/2] openvswitch: add tunnel protocol to sw_flow_key

[Jesse Gross]

On Wed, Sep 30, 2015 at 1:13 PM, Pravin Shelar <pshelar@nicira.com> wrote:
> On Wed, Sep 30, 2015 at 12:09 AM, Jiri Benc <jbenc@redhat.com> wrote:
>> On Tue, 29 Sep 2015 13:41:34 -0700, Pravin Shelar wrote:
>>> 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.

I think he meant for non-OVS users.

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

Re: [ovs-dev] [PATCH net-next 1/2] openvswitch: add tunnel protocol to sw_flow_key

[Jiri Benc]

On Wed, 30 Sep 2015 13:25:12 -0700, Jesse Gross wrote:
> On Wed, Sep 30, 2015 at 1:13 PM, Pravin Shelar <pshelar@nicira.com> wrote:
> > On Wed, Sep 30, 2015 at 12:09 AM, Jiri Benc <jbenc@redhat.com> wrote:
> >> On Tue, 29 Sep 2015 13:41:34 -0700, Pravin Shelar wrote:
> >>> 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.
> 
> I think he meant for non-OVS users.

Yes, I didn't mean masking in ovs, I meant that we'd need to hide the
bit from other users, for example in net/ipv4/ip_tunnel_core.c.
Currently, the information about ip_tunnel_key protocol is stored
outside the structure. Changing this would mean quite big changes in
the lwtunnel code (or, rather, IP users of lwtunnel) which doesn't seem
worth it just because of ovs. Especially when ovs can store the
information just fine without impact on memory footprint.

I don't see any real advantage in storing the protocol inside
ip_tunnel_key, this looks like it would be just a change for the change.

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

User space can set and get those bits in LWTUNNEL_IP_FLAGS netlink
attribute when using lwtunnel+routing rules. It would make sense to
move them to uapi but that's for a different patch(set).

 Jiri

-- 
Jiri Benc

Re: [PATCH net-next 2/2] openvswitch: netlink attributes for IPv6 tunneling

[Jiri Benc]

On Wed, 30 Sep 2015 13:18:40 -0700, Jesse Gross wrote:
> 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.

 Jiri

-- 
Jiri Benc

Re: [PATCH net-next 2/2] openvswitch: netlink attributes for IPv6 tunneling

[Jesse Gross]

On Wed, Sep 30, 2015 at 2:05 PM, Jiri Benc <jbenc@redhat.com> wrote:
> On Wed, 30 Sep 2015 13:18:40 -0700, Jesse Gross wrote:
>> 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.

Can you at least update the existing code for IPv6 so that this
doesn't introduce another lurking issue when the bug is fixed?