[PATCHv2 net-next 0/2] net: erspan: add support for openvswitch

[PATCHv2 net-next 0/2] net: erspan: add support for openvswitch

[William Tu]

The first patch refactors the originally erspan header definitions. 
Originally, the erspan fields are defined as a group into a __be16 field,
and use mask and offset to access each field.  This is more costly due to
calling ntohs/htons and error-prone.  The first patch changes it to use
bitfields.  The second patch introduces the new OVS tunnel key attribute
to program both v1 and v2 erspan tunnel for openvswitch.

William Tu (2):
  net: erspan: use bitfield instead of mask and offset
  openvswitch: add erspan version II support

 include/net/erspan.h             | 127 ++++++++++++++++++++++++++++----------
 include/uapi/linux/openvswitch.h |  13 +++-
 net/ipv4/ip_gre.c                |  38 +++++-------
 net/ipv6/ip6_gre.c               |  36 ++++-------
 net/openvswitch/flow_netlink.c   | 129 ++++++++++++++++++++++++++++++++++++---
 5 files changed, 253 insertions(+), 90 deletions(-)

--
v1->v2
  Fix compatibility issue suggested by Pravin.

[PATCHv2 net-next 1/2] net: erspan: use bitfield instead of mask and offset

[William Tu]

Originally the erspan fields are defined as a group into a __be16 field,
and use mask and offset to access each field.  This is more costly due to
calling ntohs/htons.  The patch changes it to use bitfields.

Signed-off-by: William Tu <u9012063@gmail.com>
---
 include/net/erspan.h | 127 ++++++++++++++++++++++++++++++++++++++-------------
 net/ipv4/ip_gre.c    |  38 ++++++---------
 net/ipv6/ip6_gre.c   |  36 ++++++---------
 3 files changed, 121 insertions(+), 80 deletions(-)

diff --git a/include/net/erspan.h b/include/net/erspan.h
index acdf6843095d..2b75821e2ebe 100644
--- a/include/net/erspan.h
+++ b/include/net/erspan.h
@@ -65,16 +65,30 @@
 #define GRA_MASK	0x0006
 #define O_MASK		0x0001
 
+#define HWID_OFFSET    4
+#define DIR_OFFSET     3
+
 /* ERSPAN version 2 metadata header */
 struct erspan_md2 {
 	__be32 timestamp;
 	__be16 sgt;	/* security group tag */
-	__be16 flags;
-#define P_OFFSET	15
-#define FT_OFFSET	10
-#define HWID_OFFSET	4
-#define DIR_OFFSET	3
-#define GRA_OFFSET	1
+#if defined(__LITTLE_ENDIAN_BITFIELD)
+	__u8	hwid_upper:2,
+		ft:5,
+		p:1;
+	__u8	o:1,
+		gra:2,
+		dir:1,
+		hwid:4;
+#elif defined(__BIG_ENDIAN_BITFIELD)
+	__u8	p:1,
+		ft:5,
+		hwid_upper:2;
+	__u8	hwid:4,
+		dir:1,
+		gra:2,
+		o:1;
+#endif
 };
 
 enum erspan_encap_type {
@@ -95,15 +109,62 @@ struct erspan_metadata {
 };
 
 struct erspan_base_hdr {
-	__be16 ver_vlan;
-#define VER_OFFSET  12
-	__be16 session_id;
-#define COS_OFFSET  13
-#define EN_OFFSET   11
-#define BSO_OFFSET  EN_OFFSET
-#define T_OFFSET    10
+#if defined(__LITTLE_ENDIAN_BITFIELD)
+	__u8	vlan_upper:4,
+		ver:4;
+	__u8	vlan:8;
+	__u8	session_id_upper:2,
+		t:1,
+		en:2,
+		cos:3;
+	__u8	session_id:8;
+#elif defined(__BIG_ENDIAN_BITFIELD)
+	__u8	ver: 4,
+		vlan_upper:4;
+	__u8	vlan:8;
+	__u8	cos:3,
+		en:2,
+		t:1,
+		session_id_upper:2;
+	__u8	session_id:8;
+#else
+#error "Please fix <asm/byteorder.h>"
+#endif
 };
 
+static inline void set_session_id(struct erspan_base_hdr *ershdr, u16 id)
+{
+	ershdr->session_id = id & 0xff;
+	ershdr->session_id_upper = (id >> 8) & 0x3;
+}
+
+static inline u16 get_session_id(const struct erspan_base_hdr *ershdr)
+{
+	return (ershdr->session_id_upper << 8) + ershdr->session_id;
+}
+
+static inline void set_vlan(struct erspan_base_hdr *ershdr, u16 vlan)
+{
+	ershdr->vlan = vlan & 0xff;
+	ershdr->vlan_upper = (vlan >> 8) & 0xf;
+}
+
+static inline u16 get_vlan(const struct erspan_base_hdr *ershdr)
+{
+	return (ershdr->vlan_upper << 8) + ershdr->vlan;
+}
+
+static inline void set_hwid(struct erspan_md2 *md2, u8 hwid)
+{
+	md2->hwid = hwid & 0xf;
+	md2->hwid_upper = (hwid >> 4) & 0x3;
+}
+
+static inline u8 get_hwid(const struct erspan_md2 *md2)
+{
+	return (md2->hwid_upper << 4) + md2->hwid;
+}
+
 static inline int erspan_hdr_len(int version)
 {
 	return sizeof(struct erspan_base_hdr) +
@@ -120,7 +181,7 @@ static inline u8 tos_to_cos(u8 tos)
 }
 
 static inline void erspan_build_header(struct sk_buff *skb,
-				__be32 id, u32 index,
+				u32 id, u32 index,
 				bool truncate, bool is_ipv4)
 {
 	struct ethhdr *eth = eth_hdr(skb);
@@ -154,12 +215,12 @@ static inline void erspan_build_header(struct sk_buff *skb,
 	memset(ershdr, 0, sizeof(*ershdr) + ERSPAN_V1_MDSIZE);
 
 	/* Build base header */
-	ershdr->ver_vlan = htons((vlan_tci & VLAN_MASK) |
-				 (ERSPAN_VERSION << VER_OFFSET));
-	ershdr->session_id = htons((u16)(ntohl(id) & ID_MASK) |
-			   ((tos_to_cos(tos) << COS_OFFSET) & COS_MASK) |
-			   (enc_type << EN_OFFSET & EN_MASK) |
-			   ((truncate << T_OFFSET) & T_MASK));
+	ershdr->ver = ERSPAN_VERSION;
+	ershdr->cos = tos_to_cos(tos);
+	ershdr->en = enc_type;
+	ershdr->t = truncate;
+	set_vlan(ershdr, vlan_tci);
+	set_session_id(ershdr, id);
 
 	/* Build metadata */
 	ersmd = (struct erspan_metadata *)(ershdr + 1);
@@ -187,7 +248,7 @@ static inline __be32 erspan_get_timestamp(void)
 }
 
 static inline void erspan_build_header_v2(struct sk_buff *skb,
-					  __be32 id, u8 direction, u16 hwid,
+					  u32 id, u8 direction, u16 hwid,
 					  bool truncate, bool is_ipv4)
 {
 	struct ethhdr *eth = eth_hdr(skb);
@@ -198,7 +259,6 @@ static inline void erspan_build_header_v2(struct sk_buff *skb,
 		__be16 tci;
 	} *qp;
 	u16 vlan_tci = 0;
-	u16 session_id;
 	u8 gra = 0; /* 100 usec */
 	u8 bso = 0; /* Bad/Short/Oversized */
 	u8 sgt = 0;
@@ -221,22 +281,23 @@ static inline void erspan_build_header_v2(struct sk_buff *skb,
 	memset(ershdr, 0, sizeof(*ershdr) + ERSPAN_V2_MDSIZE);
 
 	/* Build base header */
-	ershdr->ver_vlan = htons((vlan_tci & VLAN_MASK) |
-				 (ERSPAN_VERSION2 << VER_OFFSET));
-	session_id = (u16)(ntohl(id) & ID_MASK) |
-		     ((tos_to_cos(tos) << COS_OFFSET) & COS_MASK) |
-		     (bso << BSO_OFFSET & BSO_MASK) |
-		     ((truncate << T_OFFSET) & T_MASK);
-	ershdr->session_id = htons(session_id);
+	ershdr->ver = ERSPAN_VERSION2;
+	ershdr->cos = tos_to_cos(tos);
+	ershdr->en = bso;
+	ershdr->t = truncate;
+	set_vlan(ershdr, vlan_tci);
+	set_session_id(ershdr, id);
 
 	/* Build metadata */
 	md = (struct erspan_metadata *)(ershdr + 1);
 	md->u.md2.timestamp = erspan_get_timestamp();
 	md->u.md2.sgt = htons(sgt);
-	md->u.md2.flags = htons(((1 << P_OFFSET) & P_MASK) |
-				((hwid << HWID_OFFSET) & HWID_MASK) |
-				((direction << DIR_OFFSET) & DIR_MASK) |
-				((gra << GRA_OFFSET) & GRA_MASK));
+	md->u.md2.p = 1;
+	md->u.md2.ft = 0;
+	md->u.md2.dir = direction;
+	md->u.md2.gra = gra;
+	md->u.md2.o = 0;
+	set_hwid(&md->u.md2, hwid);
 }
 
 #endif
diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index b61f2285816d..6ec670fbbbdd 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -114,7 +114,7 @@ MODULE_PARM_DESC(log_ecn_error, "Log packets received with corrupted ECN");
 static struct rtnl_link_ops ipgre_link_ops __read_mostly;
 static int ipgre_tunnel_init(struct net_device *dev);
 static void erspan_build_header(struct sk_buff *skb,
-				__be32 id, u32 index,
+				u32 id, u32 index,
 				bool truncate, bool is_ipv4);
 
 static unsigned int ipgre_net_id __read_mostly;
@@ -273,12 +273,12 @@ static int erspan_rcv(struct sk_buff *skb, struct tnl_ptk_info *tpi,
 
 	iph = ip_hdr(skb);
 	ershdr = (struct erspan_base_hdr *)(skb->data + gre_hdr_len);
-	ver = (ntohs(ershdr->ver_vlan) & VER_MASK) >> VER_OFFSET;
+	ver = ershdr->ver;
 
 	/* The original GRE header does not have key field,
 	 * Use ERSPAN 10-bit session ID as key.
 	 */
-	tpi->key = cpu_to_be32(ntohs(ershdr->session_id) & ID_MASK);
+	tpi->key = cpu_to_be32(get_session_id(ershdr));
 	tunnel = ip_tunnel_lookup(itn, skb->dev->ifindex,
 				  tpi->flags | TUNNEL_KEY,
 				  iph->saddr, iph->daddr, tpi->key);
@@ -324,14 +324,8 @@ static int erspan_rcv(struct sk_buff *skb, struct tnl_ptk_info *tpi,
 			if (ver == 1) {
 				tunnel->index = ntohl(pkt_md->u.index);
 			} else {
-				u16 md2_flags;
-				u16 dir, hwid;
-
-				md2_flags = ntohs(pkt_md->u.md2.flags);
-				dir = (md2_flags & DIR_MASK) >> DIR_OFFSET;
-				hwid = (md2_flags & HWID_MASK) >> HWID_OFFSET;
-				tunnel->dir = dir;
-				tunnel->hwid = hwid;
+				tunnel->dir = pkt_md->u.md2.dir;
+				tunnel->hwid = get_hwid(&pkt_md->u.md2);
 			}
 
 		}
@@ -615,19 +609,14 @@ static void erspan_fb_xmit(struct sk_buff *skb, struct net_device *dev,
 	}
 
 	if (version == 1) {
-		erspan_build_header(skb, tunnel_id_to_key32(key->tun_id),
+		erspan_build_header(skb, ntohl(tunnel_id_to_key32(key->tun_id)),
 				    ntohl(md->u.index), truncate, true);
 	} else if (version == 2) {
-		u16 md2_flags;
-		u8 direction;
-		u16 hwid;
-
-		md2_flags = ntohs(md->u.md2.flags);
-		direction = (md2_flags & DIR_MASK) >> DIR_OFFSET;
-		hwid = (md2_flags & HWID_MASK) >> HWID_OFFSET;
-
-		erspan_build_header_v2(skb, tunnel_id_to_key32(key->tun_id),
-				       direction, hwid,	truncate, true);
+		erspan_build_header_v2(skb,
+				       ntohl(tunnel_id_to_key32(key->tun_id)),
+				       md->u.md2.dir,
+				       get_hwid(&md->u.md2),
+				       truncate, true);
 	} else {
 		goto err_free_rt;
 	}
@@ -733,10 +722,11 @@ static netdev_tx_t erspan_xmit(struct sk_buff *skb,
 
 	/* Push ERSPAN header */
 	if (tunnel->erspan_ver == 1)
-		erspan_build_header(skb, tunnel->parms.o_key, tunnel->index,
+		erspan_build_header(skb, ntohl(tunnel->parms.o_key),
+				    tunnel->index,
 				    truncate, true);
 	else
-		erspan_build_header_v2(skb, tunnel->parms.o_key,
+		erspan_build_header_v2(skb, ntohl(tunnel->parms.o_key),
 				       tunnel->dir, tunnel->hwid,
 				       truncate, true);
 
diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index db99446e0276..9f35087f93bc 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -512,8 +512,8 @@ static int ip6erspan_rcv(struct sk_buff *skb, int gre_hdr_len,
 
 	ipv6h = ipv6_hdr(skb);
 	ershdr = (struct erspan_base_hdr *)skb->data;
-	ver = (ntohs(ershdr->ver_vlan) & VER_MASK) >> VER_OFFSET;
-	tpi->key = cpu_to_be32(ntohs(ershdr->session_id) & ID_MASK);
+	ver = ershdr->ver;
+	tpi->key = cpu_to_be32(get_session_id(ershdr));
 
 	tunnel = ip6gre_tunnel_lookup(skb->dev,
 				      &ipv6h->saddr, &ipv6h->daddr, tpi->key,
@@ -564,14 +564,8 @@ static int ip6erspan_rcv(struct sk_buff *skb, int gre_hdr_len,
 			if (ver == 1) {
 				tunnel->parms.index = ntohl(pkt_md->u.index);
 			} else {
-				u16 md2_flags;
-				u16 dir, hwid;
-
-				md2_flags = ntohs(pkt_md->u.md2.flags);
-				dir = (md2_flags & DIR_MASK) >> DIR_OFFSET;
-				hwid = (md2_flags & HWID_MASK) >> HWID_OFFSET;
-				tunnel->parms.dir = dir;
-				tunnel->parms.hwid = hwid;
+				tunnel->parms.dir = pkt_md->u.md2.dir;
+				tunnel->parms.hwid = get_hwid(&pkt_md->u.md2);
 			}
 
 			ip6_tnl_rcv(tunnel, skb, tpi, NULL, log_ecn_error);
@@ -924,6 +918,7 @@ static netdev_tx_t ip6erspan_tunnel_xmit(struct sk_buff *skb,
 		struct ip_tunnel_info *tun_info;
 		const struct ip_tunnel_key *key;
 		struct erspan_metadata *md;
+		__be32 tun_id;
 
 		tun_info = skb_tunnel_info(skb);
 		if (unlikely(!tun_info ||
@@ -943,23 +938,18 @@ static netdev_tx_t ip6erspan_tunnel_xmit(struct sk_buff *skb,
 		if (!md)
 			goto tx_err;
 
+		tun_id = tunnel_id_to_key32(key->tun_id);
 		if (md->version == 1) {
 			erspan_build_header(skb,
-					    tunnel_id_to_key32(key->tun_id),
+					    ntohl(tun_id),
 					    ntohl(md->u.index), truncate,
 					    false);
 		} else if (md->version == 2) {
-			u16 md2_flags;
-			u16 dir, hwid;
-
-			md2_flags = ntohs(md->u.md2.flags);
-			dir = (md2_flags & DIR_MASK) >> DIR_OFFSET;
-			hwid = (md2_flags & HWID_MASK) >> HWID_OFFSET;
-
 			erspan_build_header_v2(skb,
-					       tunnel_id_to_key32(key->tun_id),
-					       dir, hwid, truncate,
-					       false);
+					       ntohl(tun_id),
+					       md->u.md2.dir,
+					       get_hwid(&md->u.md2),
+					       truncate, false);
 		}
 	} else {
 		switch (skb->protocol) {
@@ -981,11 +971,11 @@ static netdev_tx_t ip6erspan_tunnel_xmit(struct sk_buff *skb,
 		}
 
 		if (t->parms.erspan_ver == 1)
-			erspan_build_header(skb, t->parms.o_key,
+			erspan_build_header(skb, ntohl(t->parms.o_key),
 					    t->parms.index,
 					    truncate, false);
 		else
-			erspan_build_header_v2(skb, t->parms.o_key,
+			erspan_build_header_v2(skb, ntohl(t->parms.o_key),
 					       t->parms.dir,
 					       t->parms.hwid,
 					       truncate, false);
-- 
2.7.4

[PATCHv2 net-next 2/2] openvswitch: add erspan version II support

[William Tu]

The patch adds support for configuring the erspan V2 fields for
openvswitch.  For compatibility reason, the previously added
attribute 'OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS' is renamed to
'OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTSV1' and deprecated, and the newly added
attribute 'OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS' will handle both V1 and V2.

Signed-off-by: William Tu <u9012063@gmail.com>
Cc: Pravin B Shelar <pshelar@ovn.org>
---
 include/uapi/linux/openvswitch.h |  13 +++-
 net/openvswitch/flow_netlink.c   | 129 ++++++++++++++++++++++++++++++++++++---
 2 files changed, 132 insertions(+), 10 deletions(-)

diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index 4265d7f9e1f2..77c3424cc4ef 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -273,6 +273,16 @@ enum {
 
 #define OVS_VXLAN_EXT_MAX (__OVS_VXLAN_EXT_MAX - 1)
 
+enum {
+	OVS_ERSPAN_OPT_UNSPEC,
+	OVS_ERSPAN_OPT_IDX,	/* be32 index */
+	OVS_ERSPAN_OPT_VER,	/* u8 version number */
+	OVS_ERSPAN_OPT_DIR,	/* u8 direction */
+	OVS_ERSPAN_OPT_HWID,	/* u8 hardware ID */
+	__OVS_ERSPAN_OPT_MAX,
+};
+
+#define OVS_ERSPAN_OPT_MAX (__OVS_ERSPAN_OPT_MAX - 1)
 
 /* OVS_VPORT_ATTR_OPTIONS attributes for tunnels.
  */
@@ -363,7 +373,8 @@ enum ovs_tunnel_key_attr {
 	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_PAD,
-	OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS,	/* be32 ERSPAN index. */
+	OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTSV1,	/* be32 ERSPAN v1 index (deprecated). */
+	OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS,	/* Nested OVS_ERSPAN_OPT_* */
 	__OVS_TUNNEL_KEY_ATTR_MAX
 };
 
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index bce1f78b0de5..9c6b210e7893 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -335,7 +335,10 @@ size_t ovs_tun_key_attr_size(void)
 		 */
 		+ nla_total_size(2)    /* OVS_TUNNEL_KEY_ATTR_TP_SRC */
 		+ nla_total_size(2)    /* OVS_TUNNEL_KEY_ATTR_TP_DST */
-		+ nla_total_size(4);   /* OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS */
+		+ nla_total_size(4);   /* OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTSV1 */
+		/* OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS is mutually exclusive with
+		 * OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS and covered by it.
+		 */
 }
 
 static size_t ovs_nsh_key_attr_size(void)
@@ -386,6 +389,13 @@ static const struct ovs_len_tbl ovs_vxlan_ext_key_lens[OVS_VXLAN_EXT_MAX + 1] =
 	[OVS_VXLAN_EXT_GBP]	    = { .len = sizeof(u32) },
 };
 
+static const struct ovs_len_tbl ovs_erspan_opt_lens[OVS_ERSPAN_OPT_MAX + 1] = {
+	[OVS_ERSPAN_OPT_IDX]	= { .len = sizeof(u32) },
+	[OVS_ERSPAN_OPT_VER]	= { .len = sizeof(u8) },
+	[OVS_ERSPAN_OPT_DIR]	= { .len = sizeof(u8) },
+	[OVS_ERSPAN_OPT_HWID]	= { .len = sizeof(u8) },
+};
+
 static const struct ovs_len_tbl ovs_tunnel_key_lens[OVS_TUNNEL_KEY_ATTR_MAX + 1] = {
 	[OVS_TUNNEL_KEY_ATTR_ID]	    = { .len = sizeof(u64) },
 	[OVS_TUNNEL_KEY_ATTR_IPV4_SRC]	    = { .len = sizeof(u32) },
@@ -402,7 +412,9 @@ static const struct ovs_len_tbl ovs_tunnel_key_lens[OVS_TUNNEL_KEY_ATTR_MAX + 1]
 						.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) },
-	[OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS]   = { .len = sizeof(u32) },
+	[OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTSV1] = { .len = sizeof(u32) },
+	[OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS]   = { .len = OVS_ATTR_NESTED,
+						.next = ovs_erspan_opt_lens },
 };
 
 static const struct ovs_len_tbl
@@ -640,16 +652,78 @@ static int erspan_tun_opt_from_nlattr(const struct nlattr *attr,
 {
 	unsigned long opt_key_offset;
 	struct erspan_metadata opts;
+	struct nlattr *a;
+	u16 hwid, dir;
+	int rem;
 
 	BUILD_BUG_ON(sizeof(opts) > sizeof(match->key->tun_opts));
 
 	memset(&opts, 0, sizeof(opts));
-	opts.u.index = nla_get_be32(attr);
+	nla_for_each_nested(a, attr, rem) {
+		int type = nla_type(a);
+
+		if (type > OVS_ERSPAN_OPT_MAX) {
+			OVS_NLERR(log, "ERSPAN option %d out of range max %d",
+				  type, OVS_ERSPAN_OPT_MAX);
+			return -EINVAL;
+		}
+
+		if (!check_attr_len(nla_len(a),
+				    ovs_erspan_opt_lens[type].len)) {
+			OVS_NLERR(log, "ERSPAN option %d has unexpected len %d expected %d",
+				  type, nla_len(a),
+				  ovs_erspan_opt_lens[type].len);
+			return -EINVAL;
+		}
 
-	/* Index has only 20-bit */
-	if (ntohl(opts.u.index) & ~INDEX_MASK) {
-		OVS_NLERR(log, "ERSPAN index number %x too large.",
-			  ntohl(opts.u.index));
+		switch (type) {
+		case OVS_ERSPAN_OPT_IDX:
+			opts.u.index = nla_get_be32(a);
+			if (ntohl(opts.u.index) & ~INDEX_MASK) {
+				OVS_NLERR(log,
+					  "ERSPAN index number %x too large.",
+					  ntohl(opts.u.index));
+				return -EINVAL;
+			}
+			break;
+		case OVS_ERSPAN_OPT_VER:
+			opts.version = nla_get_u8(a);
+			if (opts.version != 1 && opts.version != 2) {
+				OVS_NLERR(log,
+					  "ERSPAN version %d not supported.",
+					  opts.version);
+				return -EINVAL;
+			}
+			break;
+		case OVS_ERSPAN_OPT_DIR:
+			dir = nla_get_u8(a);
+			if (dir != 0 && dir != 1) {
+				OVS_NLERR(log,
+					  "ERSPAN direction %d invalid.",
+					  dir);
+				return -EINVAL;
+			}
+			opts.u.md2.dir = dir;
+			break;
+		case OVS_ERSPAN_OPT_HWID:
+			hwid = nla_get_u8(a);
+			if (hwid & ~(HWID_MASK >> HWID_OFFSET)) {
+				OVS_NLERR(log,
+					  "ERSPAN hardware ID %x invalid.",
+					  hwid);
+				return -EINVAL;
+			}
+			set_hwid(&opts.u.md2, hwid);
+			break;
+		default:
+			OVS_NLERR(log, "Unknown ERSPAN opt attribute %d",
+				  type);
+			return -EINVAL;
+		}
+	}
+	if (rem) {
+		OVS_NLERR(log, "ERSPAN opt message has %d unknown bytes.",
+			  rem);
 		return -EINVAL;
 	}
 
@@ -768,6 +842,10 @@ static int ip_tun_from_nlattr(const struct nlattr *attr,
 			break;
 		case OVS_TUNNEL_KEY_ATTR_PAD:
 			break;
+		case OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTSV1:
+			OVS_NLERR(log, "ERSPAN attribute %d is deprecated.",
+				  type);
+			return -EINVAL;
 		case OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS:
 			if (opts_type) {
 				OVS_NLERR(log, "Multiple metadata blocks provided");
@@ -846,6 +924,39 @@ static int vxlan_opt_to_nlattr(struct sk_buff *skb,
 	return 0;
 }
 
+static int erspan_opt_to_nlattr(struct sk_buff *skb,
+				const void *tun_opts, int swkey_tun_opts_len)
+{
+	const struct erspan_metadata *opts = tun_opts;
+	struct nlattr *nla;
+
+	nla = nla_nest_start(skb, OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS);
+	if (!nla)
+		return -EMSGSIZE;
+
+	if (nla_put_u8(skb, OVS_ERSPAN_OPT_VER, opts->version) < 0)
+		return -EMSGSIZE;
+
+	if (opts->version == 1) {
+		if (nla_put_be32(skb, OVS_ERSPAN_OPT_IDX, opts->u.index) < 0)
+			return -EMSGSIZE;
+
+	} else if (opts->version == 2) {
+		if (nla_put_u8(skb, OVS_ERSPAN_OPT_DIR,
+			       opts->u.md2.dir) < 0)
+			return -EMSGSIZE;
+
+		if (nla_put_u8(skb, OVS_ERSPAN_OPT_HWID,
+			       get_hwid(&opts->u.md2)) < 0)
+			return -EMSGSIZE;
+	} else {
+		return -EINVAL;
+	}
+
+	nla_nest_end(skb, nla);
+	return 0;
+}
+
 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,
@@ -906,8 +1017,8 @@ static int __ip_tun_to_nlattr(struct sk_buff *skb,
 			 vxlan_opt_to_nlattr(skb, tun_opts, swkey_tun_opts_len))
 			return -EMSGSIZE;
 		else if (output->tun_flags & TUNNEL_ERSPAN_OPT &&
-			 nla_put_be32(skb, OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS,
-				      ((struct erspan_metadata *)tun_opts)->u.index))
+			 erspan_opt_to_nlattr(skb, tun_opts,
+					      swkey_tun_opts_len))
 			return -EMSGSIZE;
 	}
 
-- 
2.7.4

Re: [PATCHv2 net-next 2/2] openvswitch: add erspan version II support

[Jiri Benc]

On Tue,  9 Jan 2018 17:51:22 -0800, William Tu wrote:
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -363,7 +373,8 @@ enum ovs_tunnel_key_attr {
>  	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_PAD,
> -	OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS,	/* be32 ERSPAN index. */
> +	OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTSV1,	/* be32 ERSPAN v1 index (deprecated). */

You can't do this in an uapi header, the existing name must stay the
same forever. Otherwise existing programs would suddenly stop to
compile.

 Jiri

Re: [PATCHv2 net-next 2/2] openvswitch: add erspan version II support

[Jiri Benc]

On Tue,  9 Jan 2018 17:51:22 -0800, William Tu wrote:
> -	[OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS]   = { .len = sizeof(u32) },
> +	[OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTSV1] = { .len = sizeof(u32) },
> +	[OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS]   = { .len = OVS_ATTR_NESTED,
> +						.next = ovs_erspan_opt_lens },

Ouch. It's actually much worse than that, you're redefining the meaning of
the field. That's complete no-go.

> +		case OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTSV1:
> +			OVS_NLERR(log, "ERSPAN attribute %d is deprecated.",
> +				  type);
> +			return -EINVAL;

As is this.

> @@ -906,8 +1017,8 @@ static int __ip_tun_to_nlattr(struct sk_buff *skb,
>  			 vxlan_opt_to_nlattr(skb, tun_opts, swkey_tun_opts_len))
>  			return -EMSGSIZE;
>  		else if (output->tun_flags & TUNNEL_ERSPAN_OPT &&
> -			 nla_put_be32(skb, OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS,
> -				      ((struct erspan_metadata *)tun_opts)->u.index))
> +			 erspan_opt_to_nlattr(skb, tun_opts,
> +					      swkey_tun_opts_len))

And this.

The existing field must continue to work in the same way as before. It must
be accepted and *returned* by the kernel. You may add an additional field
but the existing behavior must be 100% preserved, both uABI and uAPI wise.

 Jiri

Re: [PATCHv2 net-next 2/2] openvswitch: add erspan version II support

[Jiri Benc]

On Wed, 10 Jan 2018 22:35:14 +0100, Jiri Benc wrote:
> The existing field must continue to work in the same way as before. It must
> be accepted and *returned* by the kernel. You may add an additional field
> but the existing behavior must be 100% preserved, both uABI and uAPI wise.

Another way around this is reverting ceaa001a170e in net.git and
designing the uAPI properly in net-next. I think that should be the
preferred way, as ceaa001a170e is clearly wrong since you need to redo
it after 3 months.

Not sure when Linus intends to release 4.15 and how much time you have
for this, though.

 Jiri

Re: [PATCHv2 net-next 2/2] openvswitch: add erspan version II support

[William Tu]

Hi Jiri,
Thanks a lot for the comments.

On Wed, Jan 10, 2018 at 2:02 PM, Jiri Benc <jbenc@redhat.com> wrote:
> On Wed, 10 Jan 2018 22:35:14 +0100, Jiri Benc wrote:
>> The existing field must continue to work in the same way as before. It must
>> be accepted and *returned* by the kernel. You may add an additional field
>> but the existing behavior must be 100% preserved, both uABI and uAPI wise.
>
> Another way around this is reverting ceaa001a170e in net.git and
> designing the uAPI properly in net-next. I think that should be the
> preferred way, as ceaa001a170e is clearly wrong since you need to redo
> it after 3 months.

The ceaa001a170e is designed for configuring the ERSPAN v1's fields only,
not thinking about the future needs for more fields in ERSPAN v2.
This patch tries to use the nested attr to handle both v1 and v2.

>
> Not sure when Linus intends to release 4.15 and how much time you have
> for this, though.
>
>  Jiri

I'd also prefer reverting ceaa001a170e since it's more clean but I
also hope to have this feature in 4.15.
How long does reverting take? Am I only able to submit the new patch
after the reverting is merged? Or I can submit revert and this new
patch in one series? I have little experience in reverting, can you
suggest which way is better?

Thanks
William

Re: [PATCHv2 net-next 2/2] openvswitch: add erspan version II support

[Jiri Benc]

On Thu, 11 Jan 2018 08:34:14 -0800, William Tu wrote:
> I'd also prefer reverting ceaa001a170e since it's more clean but I
> also hope to have this feature in 4.15.
> How long does reverting take? Am I only able to submit the new patch
> after the reverting is merged? Or I can submit revert and this new
> patch in one series? I have little experience in reverting, can you
> suggest which way is better?

Send the revert for net (subject will be "[PATCH net] revert:
openvswitch: Add erspan tunnel support."). Don't forget to explain why
you're proposing a revert.

After it is accepted and applied to net.git, wait until the patch
appears in net-next.git. It may take a little while. After that, send
the new patch(es) for net-next.

 Jiri

Re: [PATCHv2 net-next 2/2] openvswitch: add erspan version II support

[Pravin Shelar]

On Fri, Jan 12, 2018 at 12:27 AM, Jiri Benc <jbenc@redhat.com> wrote:
> On Thu, 11 Jan 2018 08:34:14 -0800, William Tu wrote:
>> I'd also prefer reverting ceaa001a170e since it's more clean but I
>> also hope to have this feature in 4.15.
>> How long does reverting take? Am I only able to submit the new patch
>> after the reverting is merged? Or I can submit revert and this new
>> patch in one series? I have little experience in reverting, can you
>> suggest which way is better?
>
> Send the revert for net (subject will be "[PATCH net] revert:
> openvswitch: Add erspan tunnel support."). Don't forget to explain why
> you're proposing a revert.
>
> After it is accepted and applied to net.git, wait until the patch
> appears in net-next.git. It may take a little while. After that, send
> the new patch(es) for net-next.
>

I agree, Once we have the V2 interface, this current ERSAN interface
unlikely to be used by any one, so it would be nice to get rid of the
old interface while we can.

Re: [PATCHv2 net-next 2/2] openvswitch: add erspan version II support

[William Tu]

On Fri, Jan 12, 2018 at 10:39 AM, Pravin Shelar <pshelar@ovn.org> wrote:
> On Fri, Jan 12, 2018 at 12:27 AM, Jiri Benc <jbenc@redhat.com> wrote:
>> On Thu, 11 Jan 2018 08:34:14 -0800, William Tu wrote:
>>> I'd also prefer reverting ceaa001a170e since it's more clean but I
>>> also hope to have this feature in 4.15.
>>> How long does reverting take? Am I only able to submit the new patch
>>> after the reverting is merged? Or I can submit revert and this new
>>> patch in one series? I have little experience in reverting, can you
>>> suggest which way is better?
>>
>> Send the revert for net (subject will be "[PATCH net] revert:
>> openvswitch: Add erspan tunnel support."). Don't forget to explain why
>> you're proposing a revert.
>>
>> After it is accepted and applied to net.git, wait until the patch
>> appears in net-next.git. It may take a little while. After that, send
>> the new patch(es) for net-next.
>>
>
> I agree, Once we have the V2 interface, this current ERSAN interface
> unlikely to be used by any one, so it would be nice to get rid of the
> old interface while we can.

Thanks Jiri and Pravin.
I will send out revert patch request.
William