[PATCH net-next V9 0/2] openvswitch: Add support for 802.1AD

[PATCH net-next V9 0/2] openvswitch: Add support for 802.1AD

[Thomas F Herbert]

Add support for 802.1AD to the openvswitch kernel module.

V9 includes changes suggested by reviewers

Although the Open Flow specification specified support for 802.1AD (qinq)
as well as push and pop vlan headers,  So far Open vSwitch has only
supported a single tag header.

This patch accompanies version 7 of the patch submitted to openvswitch
dev list.

For discussion, history  and previous versions, of the kernel patch,
see the OVS dev mailing list, openvswitch.org/pipermail/dev/..

Thomas F Herbert (2):
  openvswitch: 802.1ad uapi changes
  Flow handling, actions, and parsing

 include/uapi/linux/openvswitch.h |  17 ++--
 net/openvswitch/flow.c           |  82 ++++++++++++---
 net/openvswitch/flow.h           |   3 +
 net/openvswitch/flow_netlink.c   | 212 +++++++++++++++++++++++++++++++++------
 4 files changed, 265 insertions(+), 49 deletions(-)

-- 
2.1.0

[PATCH net-next V9 1/2] openvswitch: 802.1ad uapi changes.

[Thomas F Herbert]

openvswitch: Add support for 8021.AD

Change the description of the VLAN tpid field.

Signed-off-by: Thomas F Herbert <thomasfherbert@gmail.com>
---
 include/uapi/linux/openvswitch.h | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index bbd49a0..f2ccdef 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -559,13 +559,13 @@ struct ovs_action_push_mpls {
  * @vlan_tci: Tag control identifier (TCI) to push.  The CFI bit must be set
  * (but it will not be set in the 802.1Q header that is pushed).
  *
- * The @vlan_tpid value is typically %ETH_P_8021Q.  The only acceptable TPID
- * values are those that the kernel module also parses as 802.1Q headers, to
- * prevent %OVS_ACTION_ATTR_PUSH_VLAN followed by %OVS_ACTION_ATTR_POP_VLAN
- * from having surprising results.
+ * The @vlan_tpid value is typically %ETH_P_8021Q or %ETH_P_8021AD.
+ * The only acceptable TPID values are those that the kernel module also parses
+ * as 802.1Q or 802.1AD headers, to prevent %OVS_ACTION_ATTR_PUSH_VLAN followed
+ * by %OVS_ACTION_ATTR_POP_VLAN from having surprising results.
  */
 struct ovs_action_push_vlan {
-	__be16 vlan_tpid;	/* 802.1Q TPID. */
+	__be16 vlan_tpid;	/* 802.1Q or 802.1ad TPID. */
 	__be16 vlan_tci;	/* 802.1Q TCI (VLAN ID and priority). */
 };
 
@@ -605,9 +605,10 @@ struct ovs_action_hash {
  * is copied from the value to the packet header field, rest of the bits are
  * left unchanged.  The non-masked value bits must be passed in as zeroes.
  * Masking is not supported for the %OVS_KEY_ATTR_TUNNEL attribute.
- * @OVS_ACTION_ATTR_PUSH_VLAN: Push a new outermost 802.1Q header onto the
- * packet.
- * @OVS_ACTION_ATTR_POP_VLAN: Pop the outermost 802.1Q header off the packet.
+ * @OVS_ACTION_ATTR_PUSH_VLAN: Push a new outermost 802.1Q or 802.1ad header
+ * onto the packet.
+ * @OVS_ACTION_ATTR_POP_VLAN: Pop the outermost 802.1Q or 802.1ad header
+ * from the packet.
  * @OVS_ACTION_ATTR_SAMPLE: Probabilitically executes actions, as specified in
  * the nested %OVS_SAMPLE_ATTR_* attributes.
  * @OVS_ACTION_ATTR_PUSH_MPLS: Push a new MPLS label stack entry onto the
-- 
2.1.0

[PATCH net-next V7 2/2] openvswitch: 802.1ad: Flow handling, actions, and vlan parsing

[Thomas F Herbert]

Add support for 802.1ad including the ability to push and pop double
tagged vlans.

Signed-off-by: Thomas F Herbert <thomasfherbert@gmail.com>
---
 net/openvswitch/flow.c         |  82 +++++++++++++---
 net/openvswitch/flow.h         |   3 +
 net/openvswitch/flow_netlink.c | 212 +++++++++++++++++++++++++++++++++++------
 3 files changed, 256 insertions(+), 41 deletions(-)

diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index 2dacc7b..9c73a2e 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -298,21 +298,78 @@ static bool icmp6hdr_ok(struct sk_buff *skb)
 static int parse_vlan(struct sk_buff *skb, struct sw_flow_key *key)
 {
 	struct qtag_prefix {
-		__be16 eth_type; /* ETH_P_8021Q */
+		__be16 eth_type; /* ETH_P_8021Q  or ETH_P_8021AD */
 		__be16 tci;
 	};
-	struct qtag_prefix *qp;
+	struct qtag_prefix *qp = (struct qtag_prefix *)skb->data;
 
-	if (unlikely(skb->len < sizeof(struct qtag_prefix) + sizeof(__be16)))
+	struct qinqtag_prefix {
+		__be16 eth_type; /* ETH_P_8021Q  or ETH_P_8021AD */
+		__be16 tci;
+		__be16 inner_tpid; /* ETH_P_8021Q */
+		__be16 ctci;
+	};
+
+	if (likely(skb_vlan_tag_present(skb))) {
+		key->eth.tci = htons(skb->vlan_tci);
+
+		/* Case where upstream
+		 * processing has already stripped the outer vlan tag.
+		 */
+		if (unlikely(skb->vlan_proto == htons(ETH_P_8021AD))) {
+			if (unlikely(skb->len < sizeof(struct qtag_prefix) +
+					sizeof(__be16))) {
+				key->eth.tci = 0;
+				return 0;
+			}
+
+			if (unlikely(!pskb_may_pull(skb,
+						    sizeof(struct qtag_prefix) +
+						    sizeof(__be16)))) {
+				return -ENOMEM;
+			}
+
+			if (likely(qp->eth_type == htons(ETH_P_8021Q))) {
+				key->eth.ctci = qp->tci |
+						htons(VLAN_TAG_PRESENT);
+				__skb_pull(skb, sizeof(struct qtag_prefix));
+			}
+		}
 		return 0;
+	}
 
-	if (unlikely(!pskb_may_pull(skb, sizeof(struct qtag_prefix) +
-					 sizeof(__be16))))
-		return -ENOMEM;
 
-	qp = (struct qtag_prefix *) skb->data;
-	key->eth.tci = qp->tci | htons(VLAN_TAG_PRESENT);
-	__skb_pull(skb, sizeof(struct qtag_prefix));
+	if (qp->eth_type == htons(ETH_P_8021AD)) {
+		struct qinqtag_prefix *qinqp =
+					(struct qinqtag_prefix *)skb->data;
+
+		if (unlikely(skb->len < sizeof(struct qinqtag_prefix) +
+					sizeof(__be16)))
+			return 0;
+
+		if (unlikely(!pskb_may_pull(skb, sizeof(struct qinqtag_prefix) +
+				sizeof(__be16)))) {
+			return -ENOMEM;
+		}
+		key->eth.tci = qinqp->tci | htons(VLAN_TAG_PRESENT);
+		key->eth.ctci = qinqp->ctci | htons(VLAN_TAG_PRESENT);
+
+		__skb_pull(skb, sizeof(struct qinqtag_prefix));
+
+		return 0;
+	}
+	if (qp->eth_type == htons(ETH_P_8021Q)) {
+		if (unlikely(skb->len < sizeof(struct qtag_prefix) +
+					sizeof(__be16)))
+			return -ENOMEM;
+
+		if (unlikely(!pskb_may_pull(skb, sizeof(struct qtag_prefix) +
+				sizeof(__be16))))
+			return 0;
+		key->eth.tci = qp->tci | htons(VLAN_TAG_PRESENT);
+
+		__skb_pull(skb, sizeof(struct qtag_prefix));
+	}
 
 	return 0;
 }
@@ -474,9 +531,10 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
 	 */
 
 	key->eth.tci = 0;
-	if (skb_vlan_tag_present(skb))
-		key->eth.tci = htons(skb->vlan_tci);
-	else if (eth->h_proto == htons(ETH_P_8021Q))
+	key->eth.ctci = 0;
+	if ((skb_vlan_tag_present(skb)) ||
+	    (eth->h_proto == htons(ETH_P_8021Q)) ||
+	    (eth->h_proto == htons(ETH_P_8021AD)))
 		if (unlikely(parse_vlan(skb, key)))
 			return -ENOMEM;
 
diff --git a/net/openvswitch/flow.h b/net/openvswitch/flow.h
index a076e44..fa83c61 100644
--- a/net/openvswitch/flow.h
+++ b/net/openvswitch/flow.h
@@ -134,6 +134,9 @@ struct sw_flow_key {
 		u8     src[ETH_ALEN];	/* Ethernet source address. */
 		u8     dst[ETH_ALEN];	/* Ethernet destination address. */
 		__be16 tci;		/* 0 if no VLAN, VLAN_TAG_PRESENT set otherwise. */
+		__be16 ctci;		/* 0 if no CVLAN, VLAN_TAG_PRESENT set
+					 * otherwise.
+					 */
 		__be16 type;		/* Ethernet frame type. */
 	} eth;
 	union {
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index c691b1a..062e180 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -771,6 +771,51 @@ static int metadata_from_nlattrs(struct sw_flow_match *match,  u64 *attrs,
 	return 0;
 }
 
+static int eth_type_vlan(__be16 ethertype)
+{
+	if (ethertype == htons(ETH_P_8021Q) || ethertype == htons(ETH_P_8021AD))
+		return true;
+	return false;
+}
+
+static int _ovs_vlan_from_nlattrs(struct sw_flow_match *match, u64 attrs,
+				  const struct nlattr **a, bool is_mask,
+				  bool log)
+{
+	/* This should be nested inner or "customer" tci" */
+	if (attrs & (1 << OVS_KEY_ATTR_VLAN)) {
+		__be16 ctci;
+
+		ctci = nla_get_be16(a[OVS_KEY_ATTR_VLAN]);
+		if (!(ctci & htons(VLAN_TAG_PRESENT))) {
+			if (is_mask)
+				OVS_NLERR(log, "VLAN CTCI mask does not have exact match for VLAN_TAG_PRESENT bit.");
+			else
+				OVS_NLERR(log, "VLAN CTCI does not have VLAN_TAG_PRESENT bit set.");
+
+			return -EINVAL;
+		}
+		if (is_mask)
+			SW_FLOW_KEY_PUT(match, eth.ctci, htons(0xffff),
+					is_mask);
+		else
+			SW_FLOW_KEY_PUT(match, eth.ctci, ctci, is_mask);
+	}
+	return 0;
+}
+
+static int ovs_vlan_from_nlattrs(struct sw_flow_match *match, u64 attrs,
+				 const struct nlattr **a, bool log)
+{
+	return _ovs_vlan_from_nlattrs(match, attrs, a, false, log);
+}
+
+static int ovs_vlan_mask_from_nlattrs(struct sw_flow_match *match, u64 attrs,
+				      const struct nlattr **a, bool log)
+{
+	return _ovs_vlan_from_nlattrs(match, attrs, a, true, log);
+}
+
 static int ovs_key_from_nlattrs(struct sw_flow_match *match, u64 attrs,
 				const struct nlattr **a, bool is_mask,
 				bool log)
@@ -1024,6 +1069,113 @@ static void mask_set_nlattr(struct nlattr *attr, u8 val)
 	nlattr_set(attr, val, ovs_key_lens);
 }
 
+static int _parse_vlan_from_nlattrs(const struct nlattr *nla,
+				    struct sw_flow_match *match,
+				    u64 *key_attrs,
+				    const struct nlattr **a, bool is_mask,
+				    bool log)
+{
+	int err;
+	__be16 tci;
+
+	if (!is_mask) {
+		u64 v_attrs = 0;
+
+		tci = nla_get_be16(a[OVS_KEY_ATTR_VLAN]);
+
+		if (tci & htons(VLAN_TAG_PRESENT)) {
+			if (unlikely((nla_get_be16(a[OVS_KEY_ATTR_ETHERTYPE]) ==
+				      htons(ETH_P_8021AD)))) {
+				err = parse_flow_nlattrs(nla, a, &v_attrs, log);
+				if (err)
+					return err;
+				if (v_attrs) {
+					err = ovs_vlan_from_nlattrs(match,
+								    v_attrs, a,
+								    log);
+					if (err)
+						return err;
+				}
+				/* Insure that tci key attribute isn't
+				 * overwritten by encapsulated customer tci.
+				 */
+				v_attrs &= ~(1 << OVS_KEY_ATTR_VLAN);
+				*key_attrs |= v_attrs;
+			} else {
+				err = parse_flow_nlattrs(nla, a, key_attrs,
+							 log);
+				if (err)
+					return err;
+			}
+		} else if (!tci) {
+			/* Corner case for truncated 802.1Q header. */
+			if (nla_len(nla)) {
+				OVS_NLERR(log, "Truncated 802.1Q header has non-zero encap attribute.");
+				return -EINVAL;
+			}
+		} else {
+			OVS_NLERR(log, "Encap attr is set for non-VLAN frame");
+			return  -EINVAL;
+		}
+
+	} else {
+		u64 mask_v_attrs = 0;
+
+		tci = 0;
+		if (a[OVS_KEY_ATTR_VLAN])
+			tci = nla_get_be16(a[OVS_KEY_ATTR_VLAN]);
+
+		if (!(tci & htons(VLAN_TAG_PRESENT))) {
+			OVS_NLERR(log, "VLAN tag present bit must have an exact match (tci_mask=%x).",
+				  ntohs(tci));
+			err = -EINVAL;
+			return err;
+		}
+		err = parse_flow_mask_nlattrs(nla, a, &mask_v_attrs,
+					      log);
+		if (err)
+			return err;
+
+		if (mask_v_attrs & (1ULL << OVS_KEY_ATTR_VLAN)) {
+			__be16 ctci = 0;
+
+			ctci = nla_get_be16(a[OVS_KEY_ATTR_VLAN]);
+			err = ovs_vlan_mask_from_nlattrs(match, mask_v_attrs,
+							 a, log);
+			if (err)
+				return err;
+
+			if (!(ctci & htons(VLAN_TAG_PRESENT))) {
+				OVS_NLERR(log, "VLAN ctag present bit must have an exact match (ctci_mask=%x).",
+					  ntohs(ctci));
+				err = -EINVAL;
+				return err;
+		       }
+		       mask_v_attrs &= ~(1ULL << OVS_KEY_ATTR_VLAN);
+		       *key_attrs |= mask_v_attrs;
+	       }
+	}
+	return 0;
+}
+
+static int parse_vlan_from_nlattrs(const struct nlattr *nla,
+				   struct sw_flow_match *match,
+				   u64 *key_attrs,
+				   const struct nlattr **a,
+				   bool log)
+{
+	return _parse_vlan_from_nlattrs(nla, match, key_attrs, a, false, log);
+}
+
+static int parse_vlan_mask_from_nlattrs(const struct nlattr *nla,
+					struct sw_flow_match *match,
+					u64 *key_attrs,
+					const struct nlattr **a,
+					bool log)
+{
+	return _parse_vlan_from_nlattrs(nla, match, key_attrs, a, true, log);
+}
+
 /**
  * ovs_nla_get_match - parses Netlink attributes into a flow key and
  * mask. In case the 'mask' is NULL, the flow is treated as exact match
@@ -1058,35 +1210,24 @@ int ovs_nla_get_match(struct sw_flow_match *match,
 
 	if ((key_attrs & (1 << OVS_KEY_ATTR_ETHERNET)) &&
 	    (key_attrs & (1 << OVS_KEY_ATTR_ETHERTYPE)) &&
-	    (nla_get_be16(a[OVS_KEY_ATTR_ETHERTYPE]) == htons(ETH_P_8021Q))) {
-		__be16 tci;
+	    eth_type_vlan(nla_get_be16(a[OVS_KEY_ATTR_ETHERTYPE]))) {
 
-		if (!((key_attrs & (1 << OVS_KEY_ATTR_VLAN)) &&
-		      (key_attrs & (1 << OVS_KEY_ATTR_ENCAP)))) {
+		if (!((key_attrs & (1ULL << OVS_KEY_ATTR_VLAN)) &&
+		      (key_attrs & (1ULL << OVS_KEY_ATTR_ENCAP)))) {
 			OVS_NLERR(log, "Invalid Vlan frame.");
 			return -EINVAL;
 		}
 
 		key_attrs &= ~(1 << OVS_KEY_ATTR_ETHERTYPE);
-		tci = nla_get_be16(a[OVS_KEY_ATTR_VLAN]);
 		encap = a[OVS_KEY_ATTR_ENCAP];
 		key_attrs &= ~(1 << OVS_KEY_ATTR_ENCAP);
 		encap_valid = true;
 
-		if (tci & htons(VLAN_TAG_PRESENT)) {
-			err = parse_flow_nlattrs(encap, a, &key_attrs, log);
-			if (err)
-				return err;
-		} else if (!tci) {
-			/* Corner case for truncated 802.1Q header. */
-			if (nla_len(encap)) {
-				OVS_NLERR(log, "Truncated 802.1Q header has non-zero encap attribute.");
-				return -EINVAL;
-			}
-		} else {
-			OVS_NLERR(log, "Encap attr is set for non-VLAN frame");
-			return  -EINVAL;
-		}
+		err = parse_vlan_from_nlattrs(encap, match, &key_attrs, a,
+					      log);
+		if (err)
+			return err;
+
 	}
 
 	err = ovs_key_from_nlattrs(match, key_attrs, a, false, log);
@@ -1132,7 +1273,6 @@ int ovs_nla_get_match(struct sw_flow_match *match,
 
 		if (mask_attrs & 1 << OVS_KEY_ATTR_ENCAP) {
 			__be16 eth_type = 0;
-			__be16 tci = 0;
 
 			if (!encap_valid) {
 				OVS_NLERR(log, "Encap mask attribute is set for non-VLAN frame.");
@@ -1158,15 +1298,12 @@ int ovs_nla_get_match(struct sw_flow_match *match,
 				goto free_newmask;
 			}
 
-			if (a[OVS_KEY_ATTR_VLAN])
-				tci = nla_get_be16(a[OVS_KEY_ATTR_VLAN]);
-
-			if (!(tci & htons(VLAN_TAG_PRESENT))) {
-				OVS_NLERR(log, "VLAN tag present bit must have an exact match (tci_mask=%x).",
-					  ntohs(tci));
-				err = -EINVAL;
+			err = parse_vlan_mask_from_nlattrs(encap, match,
+							   &mask_attrs,
+							   a, log);
+			if (err)
 				goto free_newmask;
-			}
+
 		}
 
 		err = ovs_key_from_nlattrs(match, mask_attrs, a, true, log);
@@ -1331,6 +1468,22 @@ static int __ovs_nla_put_key(const struct sw_flow_key *swkey,
 		encap = nla_nest_start(skb, OVS_KEY_ATTR_ENCAP);
 		if (!swkey->eth.tci)
 			goto unencap;
+	} else if (swkey->eth.ctci || swkey->eth.type == htons(ETH_P_8021AD)) {
+		__be16 eth_type;
+
+		eth_type = !is_mask ? htons(ETH_P_8021AD) : htons(0xffff);
+		if (nla_put_be16(skb, OVS_KEY_ATTR_ETHERTYPE, eth_type) ||
+		    nla_put_be16(skb, OVS_KEY_ATTR_VLAN, output->eth.tci))
+			goto nla_put_failure;
+		encap = nla_nest_start(skb, OVS_KEY_ATTR_ENCAP);
+		if (!swkey->eth.tci)
+			goto unencap;
+		/* Customer tci is nested but uses same key attribute.
+		 */
+		if (nla_put_be16(skb, OVS_KEY_ATTR_VLAN, output->eth.ctci))
+			goto nla_put_failure;
+		if (!swkey->eth.ctci)
+			goto unencap;
 	} else
 		encap = NULL;
 
@@ -2078,7 +2231,8 @@ static int __ovs_nla_copy_actions(const struct nlattr *attr,
 
 		case OVS_ACTION_ATTR_PUSH_VLAN:
 			vlan = nla_data(a);
-			if (vlan->vlan_tpid != htons(ETH_P_8021Q))
+			if ((vlan->vlan_tpid != htons(ETH_P_8021Q)) &&
+			    (vlan->vlan_tpid != htons(ETH_P_8021AD)))
 				return -EINVAL;
 			if (!(vlan->vlan_tci & htons(VLAN_TAG_PRESENT)))
 				return -EINVAL;
-- 
2.1.0

Re: [PATCH net-next V7 2/2] openvswitch: 802.1ad: Flow handling, actions, and vlan parsing

[Pravin Shelar]

On Tue, May 12, 2015 at 5:06 PM, Thomas F Herbert
<thomasfherbert@gmail.com> wrote:
> Add support for 802.1ad including the ability to push and pop double
> tagged vlans.
>
> Signed-off-by: Thomas F Herbert <thomasfherbert@gmail.com>
> ---
...
...
> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> index c691b1a..062e180 100644
> --- a/net/openvswitch/flow_netlink.c
> +++ b/net/openvswitch/flow_netlink.c
> @@ -771,6 +771,51 @@ static int metadata_from_nlattrs(struct sw_flow_match *match,  u64 *attrs,
>         return 0;
>  }
>
> +static int eth_type_vlan(__be16 ethertype)
> +{
> +       if (ethertype == htons(ETH_P_8021Q) || ethertype == htons(ETH_P_8021AD))
> +               return true;
> +       return false;
> +}
> +
You have open-coded same comparison in flow.c. May be you can define
it in header file  if_vlan.h. There are some use cases in that file
too for this function.

> +static int _ovs_vlan_from_nlattrs(struct sw_flow_match *match, u64 attrs,
> +                                 const struct nlattr **a, bool is_mask,
> +                                 bool log)
> +{
_ovs_vlan_from_nlattrs() is setting 8021AD, can you change name so
that it is clear.

> +       /* This should be nested inner or "customer" tci" */
> +       if (attrs & (1 << OVS_KEY_ATTR_VLAN)) {
> +               __be16 ctci;
> +
> +               ctci = nla_get_be16(a[OVS_KEY_ATTR_VLAN]);
> +               if (!(ctci & htons(VLAN_TAG_PRESENT))) {
> +                       if (is_mask)
> +                               OVS_NLERR(log, "VLAN CTCI mask does not have exact match for VLAN_TAG_PRESENT bit.");
> +                       else
> +                               OVS_NLERR(log, "VLAN CTCI does not have VLAN_TAG_PRESENT bit set.");
> +
> +                       return -EINVAL;
> +               }
> +               if (is_mask)
> +                       SW_FLOW_KEY_PUT(match, eth.ctci, htons(0xffff),
> +                                       is_mask);
> +               else

8021AD mask from user parameters is ignored and 0xffff is set. You
need to set default 0xffff mask for ctci and then override it with
user mask if given in the key.

> +                       SW_FLOW_KEY_PUT(match, eth.ctci, ctci, is_mask);
> +       }
> +       return 0;
> +}
> +
> +static int ovs_vlan_from_nlattrs(struct sw_flow_match *match, u64 attrs,
> +                                const struct nlattr **a, bool log)
> +{
> +       return _ovs_vlan_from_nlattrs(match, attrs, a, false, log);
> +}
> +
> +static int ovs_vlan_mask_from_nlattrs(struct sw_flow_match *match, u64 attrs,
> +                                     const struct nlattr **a, bool log)
> +{
> +       return _ovs_vlan_from_nlattrs(match, attrs, a, true, log);
> +}
> +
I do not see value in these functions. Can you directly call
_ovs_vlan_from_nlattrs().

>  static int ovs_key_from_nlattrs(struct sw_flow_match *match, u64 attrs,
>                                 const struct nlattr **a, bool is_mask,
>                                 bool log)
> @@ -1024,6 +1069,113 @@ static void mask_set_nlattr(struct nlattr *attr, u8 val)
>         nlattr_set(attr, val, ovs_key_lens);
>  }
>
> +static int _parse_vlan_from_nlattrs(const struct nlattr *nla,
> +                                   struct sw_flow_match *match,
> +                                   u64 *key_attrs,
> +                                   const struct nlattr **a, bool is_mask,
> +                                   bool log)
> +{
> +       int err;
> +       __be16 tci;
> +
> +       if (!is_mask) {
> +               u64 v_attrs = 0;
> +
> +               tci = nla_get_be16(a[OVS_KEY_ATTR_VLAN]);
> +
> +               if (tci & htons(VLAN_TAG_PRESENT)) {
> +                       if (unlikely((nla_get_be16(a[OVS_KEY_ATTR_ETHERTYPE]) ==
> +                                     htons(ETH_P_8021AD)))) {
> +                               err = parse_flow_nlattrs(nla, a, &v_attrs, log);
> +                               if (err)
> +                                       return err;
> +                               if (v_attrs) {
> +                                       err = ovs_vlan_from_nlattrs(match,
> +                                                                   v_attrs, a,
> +                                                                   log);
> +                                       if (err)
> +                                               return err;
> +                               }
> +                               /* Insure that tci key attribute isn't
> +                                * overwritten by encapsulated customer tci.
> +                                */
> +                               v_attrs &= ~(1 << OVS_KEY_ATTR_VLAN);

We also need to clear v_attrs when key has single vlan tag which is
else part of this block.

> +                               *key_attrs |= v_attrs;
> +                       } else {
> +                               err = parse_flow_nlattrs(nla, a, key_attrs,
> +                                                        log);
> +                               if (err)
> +                                       return err;
> +                       }
> +               } else if (!tci) {
> +                       /* Corner case for truncated 802.1Q header. */
> +                       if (nla_len(nla)) {
> +                               OVS_NLERR(log, "Truncated 802.1Q header has non-zero encap attribute.");
> +                               return -EINVAL;
> +                       }
> +               } else {
> +                       OVS_NLERR(log, "Encap attr is set for non-VLAN frame");
> +                       return  -EINVAL;
> +               }
> +
For double vlan tag case we need to have double encap attributes in
flow key; one for each tag. So flow key should look like:

eth_type(0x88A8),vlan(vid=10),encap(eth_type(0x08100), vlan(vid=20),
encap(eth_type(0x0800), ...))

Can you adjust vlan parsing code according ?


> +       } else {
> +               u64 mask_v_attrs = 0;
> +
> +               tci = 0;
> +               if (a[OVS_KEY_ATTR_VLAN])
> +                       tci = nla_get_be16(a[OVS_KEY_ATTR_VLAN]);
> +
> +               if (!(tci & htons(VLAN_TAG_PRESENT))) {
...
...
> +
> +static int parse_vlan_from_nlattrs(const struct nlattr *nla,
> +                                  struct sw_flow_match *match,
> +                                  u64 *key_attrs,
> +                                  const struct nlattr **a,
> +                                  bool log)
> +{
> +       return _parse_vlan_from_nlattrs(nla, match, key_attrs, a, false, log);
> +}
You can move the key parsing block from _parse_vlan_from_nlattrs()
here and move mask block in function bellow and get ride of
_parse_vlan_from_nlattrs() function.

> +
> +static int parse_vlan_mask_from_nlattrs(const struct nlattr *nla,
> +                                       struct sw_flow_match *match,
> +                                       u64 *key_attrs,
> +                                       const struct nlattr **a,
> +                                       bool log)
> +{
> +       return _parse_vlan_from_nlattrs(nla, match, key_attrs, a, true, log);
> +}
> +
>  /**

Re: [PATCH net-next V7 2/2] openvswitch: 802.1ad: Flow handling, actions, and vlan parsing

[Thomas F Herbert]

On 5/14/15 3:33 AM, Pravin Shelar wrote:
> On Tue, May 12, 2015 at 5:06 PM, Thomas F Herbert
> <thomasfherbert@gmail.com> wrote:
>> Add support for 802.1ad including the ability to push and pop double
>> tagged vlans.
>>
>> Signed-off-by: Thomas F Herbert <thomasfherbert@gmail.com>
...
>> +               if (is_mask)
>> +                       SW_FLOW_KEY_PUT(match, eth.ctci, htons(0xffff),
>> +                                       is_mask);
>> +               else
>
> 8021AD mask from user parameters is ignored and 0xffff is set. You
> need to set default 0xffff mask for ctci and then override it with
> user mask if given in the key.
Pravin, once again, thanks for your review.

I am thinking you are correct. I did it this way because it is the way 
single tagged vlans are handled in the original vlan code. Which raises 
two issues. 1. When I change this, I should also change the tci code for 
consistency and should that be a separate patch? 2. The implication is 
that so far all vlan vid mask matching has been done in user space only.
>
>> +                       SW_FLOW_KEY_PUT(match, eth.ctci, ctci, is_mask);
>> +       }
...
>> +{
>> +       return _ovs_vlan_from_nlattrs(match, attrs, a, true, log);
>> +}
>> +
> I do not see value in these functions. Can you directly call
> _ovs_vlan_from_nlattrs().
OK
>
>>   static int ovs_key_from_nlattrs(struct sw_flow_match *match, u64 attrs,
>>                                  const struct nlattr **a, bool is_mask,
>>                                  bool log)
>> @@ -1024,6 +1069,113 @@ static void mask_set_nlattr(struct nlattr *attr, u8 val)
>>          nlattr_set(attr, val, ovs_key_lens);
>>   }
>>
>> +static int _parse_vlan_from_nlattrs(const struct nlattr *nla,
>> +                                   struct sw_flow_match *match,
>> +                                   u64 *key_attrs,
>> +                                   const struct nlattr **a, bool is_mask,
>> +                                   bool log)
>> +{
>> +       int err;
>> +       __be16 tci;
>> +
>> +       if (!is_mask) {
>> +               u64 v_attrs = 0;
>> +
>> +               tci = nla_get_be16(a[OVS_KEY_ATTR_VLAN]);
>> +
>> +               if (tci & htons(VLAN_TAG_PRESENT)) {
>> +                       if (unlikely((nla_get_be16(a[OVS_KEY_ATTR_ETHERTYPE]) ==
>> +                                     htons(ETH_P_8021AD)))) {
>> +                               err = parse_flow_nlattrs(nla, a, &v_attrs, log);
>> +                               if (err)
>> +                                       return err;
>> +                               if (v_attrs) {
>> +                                       err = ovs_vlan_from_nlattrs(match,
>> +                                                                   v_attrs, a,
>> +                                                                   log);
>> +                                       if (err)
>> +                                               return err;
>> +                               }
>> +                               /* Insure that tci key attribute isn't
>> +                                * overwritten by encapsulated customer tci.
>> +                                */
>> +                               v_attrs &= ~(1 << OVS_KEY_ATTR_VLAN);
>
> We also need to clear v_attrs when key has single vlan tag which is
> else part of this block.
This code is implemented this way because I have been using only a 
single encapsulation level for double tagged vlans. I sneak the inner 
tag into the encap and and then clear it here because the flow key has 
only one attribute type for vlan and v_attrs is only used inside the 
encapsulation.
Along with this change, I can also update 
Documentation/networking/openvswitch.txt to show the double nested 
encapsulation flow key.
>
>> +                               *key_attrs |= v_attrs;
>> +                       } else {
>> +                               err = parse_flow_nlattrs(nla, a, key_attrs,
>> +                                                        log);
>> +                               if (err)
>> +                                       return err;
>> +                       }
>> +               } else if (!tci) {
>> +                       /* Corner case for truncated 802.1Q header. */
>> +                       if (nla_len(nla)) {
>> +                               OVS_NLERR(log, "Truncated 802.1Q header has non-zero encap attribute.");
>> +                               return -EINVAL;
>> +                       }
>> +               } else {
>> +                       OVS_NLERR(log, "Encap attr is set for non-VLAN frame");
>> +                       return  -EINVAL;
>> +               }
>> +
> For double vlan tag case we need to have double encap attributes in
> flow key; one for each tag. So flow key should look like:
>
> eth_type(0x88A8),vlan(vid=10),encap(eth_type(0x08100), vlan(vid=20),
> encap(eth_type(0x0800), ...))
>
> Can you adjust vlan parsing code according ?
Yes, I think you are right. I should change the code to use two levels 
of encapsulation for double tagged vlans. This also would be the best 
for consistency with future implementation of 802.1ah.
>
>
...
>> +{
>> +       return _parse_vlan_from_nlattrs(nla, match, key_attrs, a, false, log);
>> +}
> You can move the key parsing block from _parse_vlan_from_nlattrs()
> here and move mask block in function bellow and get ride of
> _parse_vlan_from_nlattrs() function.
OK. I think this will make the code more straight forward.
>
>> +
>> +static int parse_vlan_mask_from_nlattrs(const struct nlattr *nla,
>> +                                       struct sw_flow_match *match,
>> +                                       u64 *key_attrs,
>> +                                       const struct nlattr **a,
>> +                                       bool log)
>> +{
>> +       return _parse_vlan_from_nlattrs(nla, match, key_attrs, a, true, log);
>> +}
>> +
>>   /**


-- 
Thomas F. Herbert
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[PATCH net-next V7 0/2] openvswitch: Add support for 802.1AD

[Thomas F Herbert]

Add support for 802.1AD to the openvswitch kernel module.

Although the Open Flow specification specified support for 802.1AD (qinq) 
as well as push and pop vlan headers,  So far Open vSwitch has only
supported a single tag header.

This accompanies version 7 of the patch submitted to openvswitch dev list.

For discussion, history  and previous versions, of the kernel patch, 
see the OVS dev mailing list, openvswitch.org/pipermail/dev/..

Thomas F Herbert (2):
  add flow key field for ctag
  Flow handling, actions, and vlan parsing for 8021AD

 include/uapi/linux/openvswitch.h | 17 ++++----
 net/openvswitch/actions.c        |  6 ++-
 net/openvswitch/flow.c           | 83 +++++++++++++++++++++++++++++++++-------
 net/openvswitch/flow.h           |  1 +
 net/openvswitch/flow_netlink.c   | 81 ++++++++++++++++++++++++++++++++++++---
 5 files changed, 160 insertions(+), 28 deletions(-)

-- 
2.1.0

[PATCH net-next V7 2/2] openvswitch: 802.1ad: Flow handling, actions, and vlan parsing

[Thomas F Herbert]

Add support for 802.1ad including the ability to push and pop double
tagged vlans.

Signed-off-by: Thomas F Herbert <thomasfherbert@gmail.com>
---
 net/openvswitch/actions.c      |  6 ++-
 net/openvswitch/flow.c         | 83 +++++++++++++++++++++++++++++++++++-------
 net/openvswitch/flow.h         |  1 +
 net/openvswitch/flow_netlink.c | 81 ++++++++++++++++++++++++++++++++++++++---
 4 files changed, 151 insertions(+), 20 deletions(-)

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index b491c1c..0831019 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -219,7 +219,8 @@ static int pop_vlan(struct sk_buff *skb, struct sw_flow_key *key)
 	int err;
 
 	err = skb_vlan_pop(skb);
-	if (skb_vlan_tag_present(skb))
+	if (skb_vlan_tag_present(skb) &&
+	    skb->protocol != htons(ETH_P_8021Q))
 		invalidate_flow_key(key);
 	else
 		key->eth.tci = 0;
@@ -229,7 +230,8 @@ static int pop_vlan(struct sk_buff *skb, struct sw_flow_key *key)
 static int push_vlan(struct sk_buff *skb, struct sw_flow_key *key,
 		     const struct ovs_action_push_vlan *vlan)
 {
-	if (skb_vlan_tag_present(skb))
+	if (skb_vlan_tag_present(skb) &&
+	    skb->protocol != htons(ETH_P_8021Q))
 		invalidate_flow_key(key);
 	else
 		key->eth.tci = vlan->vlan_tci;
diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index 2dacc7b..6989451 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -298,21 +298,78 @@ static bool icmp6hdr_ok(struct sk_buff *skb)
 static int parse_vlan(struct sk_buff *skb, struct sw_flow_key *key)
 {
 	struct qtag_prefix {
-		__be16 eth_type; /* ETH_P_8021Q */
+		__be16 eth_type; /* ETH_P_8021Q  or ETH_P_8021AD */
 		__be16 tci;
 	};
-	struct qtag_prefix *qp;
+	struct qtag_prefix *qp = (struct qtag_prefix *) skb->data;
 
-	if (unlikely(skb->len < sizeof(struct qtag_prefix) + sizeof(__be16)))
-		return 0;
+	struct qinqtag_prefix {
+		__be16 eth_type; /* ETH_P_8021Q  or ETH_P_8021AD */
+		__be16 tci;
+		__be16 inner_tpid; /* ETH_P_8021Q */
+		__be16 ctci;
+	};
+
+	if (likely(skb_vlan_tag_present(skb))) {
+
+		key->eth.tci = htons(skb->vlan_tci);
+
+		/*
+		 * Case where upstream
+		 * processing has already stripped the outer vlan tag.
+		 */
+		if (unlikely(skb->vlan_proto == htons(ETH_P_8021AD))) {
+
+			if (unlikely(skb->len < sizeof(struct qtag_prefix) +
+					sizeof(__be16)))
+				return 0;
+
+			if (unlikely(!pskb_may_pull(skb, sizeof(struct qtag_prefix) +
+					sizeof(__be16)))) {
+				return -ENOMEM;
+			}
+
+			if (likely(qp->eth_type == htons(ETH_P_8021Q))) {
+				key->eth.ctci = qp->tci | htons(VLAN_TAG_PRESENT);
+				__skb_pull(skb, sizeof(struct qtag_prefix));
+			}
+		}
+                return 0;
+	}
 
-	if (unlikely(!pskb_may_pull(skb, sizeof(struct qtag_prefix) +
-					 sizeof(__be16))))
-		return -ENOMEM;
 
-	qp = (struct qtag_prefix *) skb->data;
-	key->eth.tci = qp->tci | htons(VLAN_TAG_PRESENT);
-	__skb_pull(skb, sizeof(struct qtag_prefix));
+        if (qp->eth_type == htons(ETH_P_8021AD)) {
+	        struct qinqtag_prefix *qinqp = (struct qinqtag_prefix *) skb->data;
+
+		if (unlikely(skb->len < sizeof(struct qinqtag_prefix) +
+					sizeof(__be16)))
+			return 0;
+
+		if (unlikely(!pskb_may_pull(skb, sizeof(struct qinqtag_prefix) +
+				sizeof(__be16)))) {
+			return -ENOMEM;
+		}
+		key->eth.tci = qinqp->tci | htons(VLAN_TAG_PRESENT);
+		key->eth.ctci = qinqp->ctci | htons(VLAN_TAG_PRESENT);
+
+		__skb_pull(skb, sizeof(struct qinqtag_prefix));
+
+                return 0;
+
+	}
+        if (qp->eth_type == htons(ETH_P_8021Q)) {
+
+		if (unlikely(skb->len < sizeof(struct qtag_prefix) +
+					sizeof(__be16)))
+			return -ENOMEM;
+
+		if (unlikely(!pskb_may_pull(skb, sizeof(struct qtag_prefix) +
+				sizeof(__be16))))
+			return 0;
+		key->eth.tci = qp->tci | htons(VLAN_TAG_PRESENT);
+
+		__skb_pull(skb, sizeof(struct qtag_prefix));
+	}
 
 	return 0;
 }
@@ -474,9 +531,9 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
 	 */
 
 	key->eth.tci = 0;
-	if (skb_vlan_tag_present(skb))
-		key->eth.tci = htons(skb->vlan_tci);
-	else if (eth->h_proto == htons(ETH_P_8021Q))
+	if ((skb_vlan_tag_present(skb)) ||
+	    (eth->h_proto == htons(ETH_P_8021Q)) ||
+	    (eth->h_proto == htons(ETH_P_8021AD)))
 		if (unlikely(parse_vlan(skb, key)))
 			return -ENOMEM;
 
diff --git a/net/openvswitch/flow.h b/net/openvswitch/flow.h
index a076e44..1057de6 100644
--- a/net/openvswitch/flow.h
+++ b/net/openvswitch/flow.h
@@ -134,6 +134,7 @@ struct sw_flow_key {
 		u8     src[ETH_ALEN];	/* Ethernet source address. */
 		u8     dst[ETH_ALEN];	/* Ethernet destination address. */
 		__be16 tci;		/* 0 if no VLAN, VLAN_TAG_PRESENT set otherwise. */
+		__be16 ctci;		/* 0 if no CVLAN, VLAN_TAG_PRESENT set otherwise. */
 		__be16 type;		/* Ethernet frame type. */
 	} eth;
 	union {
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index c691b1a..4a7ff38 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -770,6 +770,28 @@ static int metadata_from_nlattrs(struct sw_flow_match *match,  u64 *attrs,
 	}
 	return 0;
 }
+static int ovs_nested_vlan_from_nlattrs(struct sw_flow_match *match,
+					u64 attrs, const struct nlattr **a,
+					bool is_mask, bool log)
+{
+        /* This should be nested inner or "customer" tci" */
+	if (attrs & (1 << OVS_KEY_ATTR_VLAN)) {
+		__be16 ctci;
+
+		ctci = nla_get_be16(a[OVS_KEY_ATTR_VLAN]);
+		if (!(ctci & htons(VLAN_TAG_PRESENT))) {
+			if (is_mask)
+				OVS_NLERR(log, "VLAN TCI mask does not have exact match for VLAN_TAG_PRESENT bit.");
+			else
+				OVS_NLERR(log, "VLAN TCI does not have VLAN_TAG_PRESENT bit set.");
+
+			return -EINVAL;
+		}
+
+		SW_FLOW_KEY_PUT(match, eth.ctci, ctci, is_mask);
+	}
+	return 0;
+}
 
 static int ovs_key_from_nlattrs(struct sw_flow_match *match, u64 attrs,
 				const struct nlattr **a, bool is_mask,
@@ -1049,6 +1071,8 @@ int ovs_nla_get_match(struct sw_flow_match *match,
 	struct nlattr *newmask = NULL;
 	u64 key_attrs = 0;
 	u64 mask_attrs = 0;
+	u64 v_attrs = 0;
+	u64 mask_v_attrs = 0;
 	bool encap_valid = false;
 	int err;
 
@@ -1058,7 +1082,8 @@ int ovs_nla_get_match(struct sw_flow_match *match,
 
 	if ((key_attrs & (1 << OVS_KEY_ATTR_ETHERNET)) &&
 	    (key_attrs & (1 << OVS_KEY_ATTR_ETHERTYPE)) &&
-	    (nla_get_be16(a[OVS_KEY_ATTR_ETHERTYPE]) == htons(ETH_P_8021Q))) {
+	    ((nla_get_be16(a[OVS_KEY_ATTR_ETHERTYPE]) == htons(ETH_P_8021Q)) ||
+	     (nla_get_be16(a[OVS_KEY_ATTR_ETHERTYPE]) == htons(ETH_P_8021AD)))) {
 		__be16 tci;
 
 		if (!((key_attrs & (1 << OVS_KEY_ATTR_VLAN)) &&
@@ -1074,9 +1099,28 @@ int ovs_nla_get_match(struct sw_flow_match *match,
 		encap_valid = true;
 
 		if (tci & htons(VLAN_TAG_PRESENT)) {
-			err = parse_flow_nlattrs(encap, a, &key_attrs, log);
-			if (err)
-				return err;
+
+			if (unlikely((nla_get_be16(a[OVS_KEY_ATTR_ETHERTYPE]) ==
+			    htons(ETH_P_8021AD)))) {
+
+				err = parse_flow_nlattrs(encap, a, &v_attrs, log);
+				if (err)
+					return err;
+				if (v_attrs) {
+					err = ovs_nested_vlan_from_nlattrs(match, v_attrs, a, false, log);
+					if (err)
+						return err;
+				}
+				/* Insure that tci key attribute isn't overwritten by
+				 * encapsulated customer tci.
+				 */
+				v_attrs &= ~(1 << OVS_KEY_ATTR_VLAN);
+				key_attrs |= v_attrs;
+			} else {
+				err = parse_flow_nlattrs(encap, a, &key_attrs, log);
+				if (err)
+					return err;
+			}
 		} else if (!tci) {
 			/* Corner case for truncated 802.1Q header. */
 			if (nla_len(encap)) {
@@ -1133,6 +1177,7 @@ int ovs_nla_get_match(struct sw_flow_match *match,
 		if (mask_attrs & 1 << OVS_KEY_ATTR_ENCAP) {
 			__be16 eth_type = 0;
 			__be16 tci = 0;
+			__be16 ctci = 0;
 
 			if (!encap_valid) {
 				OVS_NLERR(log, "Encap mask attribute is set for non-VLAN frame.");
@@ -1167,6 +1212,22 @@ int ovs_nla_get_match(struct sw_flow_match *match,
 				err = -EINVAL;
 				goto free_newmask;
 			}
+			err = parse_flow_mask_nlattrs(encap, a, &mask_v_attrs, log);
+			if (err)
+				goto free_newmask;
+
+			if (mask_v_attrs & (1ULL << OVS_KEY_ATTR_VLAN)) {
+				ctci = nla_get_be16(a[OVS_KEY_ATTR_VLAN]);
+				if (!(ctci & htons(VLAN_TAG_PRESENT))) {
+					OVS_NLERR(log, "VLAN ctag present bit must have an exact match (ctci_mask=%x).",
+					          ntohs(ctci));
+					err = -EINVAL;
+					goto free_newmask;
+				}
+				mask_v_attrs &= ~(1ULL << OVS_KEY_ATTR_VLAN);
+				mask_attrs |= mask_v_attrs;
+			}
+
 		}
 
 		err = ovs_key_from_nlattrs(match, mask_attrs, a, true, log);
@@ -1331,6 +1392,15 @@ static int __ovs_nla_put_key(const struct sw_flow_key *swkey,
 		encap = nla_nest_start(skb, OVS_KEY_ATTR_ENCAP);
 		if (!swkey->eth.tci)
 			goto unencap;
+	} else if (swkey->eth.tci || swkey->eth.type == htons(ETH_P_8021AD)) {
+		__be16 eth_type;
+		eth_type = !is_mask ? htons(ETH_P_8021AD) : htons(0xffff);
+		if (nla_put_be16(skb, OVS_KEY_ATTR_ETHERTYPE, eth_type) ||
+		    nla_put_be16(skb, OVS_KEY_ATTR_VLAN, output->eth.tci))
+			goto nla_put_failure;
+		encap = nla_nest_start(skb, OVS_KEY_ATTR_ENCAP);
+		if (!swkey->eth.tci)
+			goto unencap;
 	} else
 		encap = NULL;
 
@@ -2078,7 +2148,8 @@ static int __ovs_nla_copy_actions(const struct nlattr *attr,
 
 		case OVS_ACTION_ATTR_PUSH_VLAN:
 			vlan = nla_data(a);
-			if (vlan->vlan_tpid != htons(ETH_P_8021Q))
+			if ((vlan->vlan_tpid != htons(ETH_P_8021Q)) &&
+			    (vlan->vlan_tpid != htons(ETH_P_8021AD)))
 				return -EINVAL;
 			if (!(vlan->vlan_tci & htons(VLAN_TAG_PRESENT)))
 				return -EINVAL;
-- 
2.1.0

Re: [PATCH net-next V7 2/2] openvswitch: 802.1ad: Flow handling, actions, and vlan parsing

[Pravin Shelar]

On Tue, Apr 28, 2015 at 3:44 PM, Thomas F Herbert
<thomasfherbert@gmail.com> wrote:
> Add support for 802.1ad including the ability to push and pop double
> tagged vlans.
>
I saw multiple checkpatch.pl errors and warning.

> Signed-off-by: Thomas F Herbert <thomasfherbert@gmail.com>
...
> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
> index 2dacc7b..6989451 100644
> --- a/net/openvswitch/flow.c
> +++ b/net/openvswitch/flow.c
> @@ -298,21 +298,78 @@ static bool icmp6hdr_ok(struct sk_buff *skb)
>  static int parse_vlan(struct sk_buff *skb, struct sw_flow_key *key)
>  {
>         struct qtag_prefix {
> -               __be16 eth_type; /* ETH_P_8021Q */
> +               __be16 eth_type; /* ETH_P_8021Q  or ETH_P_8021AD */
>                 __be16 tci;
>         };
> -       struct qtag_prefix *qp;
> +       struct qtag_prefix *qp = (struct qtag_prefix *) skb->data;
>
> -       if (unlikely(skb->len < sizeof(struct qtag_prefix) + sizeof(__be16)))
> -               return 0;
> +       struct qinqtag_prefix {
> +               __be16 eth_type; /* ETH_P_8021Q  or ETH_P_8021AD */
> +               __be16 tci;
> +               __be16 inner_tpid; /* ETH_P_8021Q */
> +               __be16 ctci;
> +       };
> +
> +       if (likely(skb_vlan_tag_present(skb))) {
> +
> +               key->eth.tci = htons(skb->vlan_tci);
> +
I think there is bug in existing OVS where it does not check
vlan_proto before populating the flow key. Can you send a separate
patch to fix this issue?.

> +               /*
> +                * Case where upstream
> +                * processing has already stripped the outer vlan tag.
> +                */
> +               if (unlikely(skb->vlan_proto == htons(ETH_P_8021AD))) {
> +
> +                       if (unlikely(skb->len < sizeof(struct qtag_prefix) +
> +                                       sizeof(__be16)))
> +                               return 0;
> +
If this returns from here then it can match on flow with tci which
expect vlan_proto to be 8021Q but with vlan_proto equal to 8021AD.

> +                       if (unlikely(!pskb_may_pull(skb, sizeof(struct qtag_prefix) +
> +                                       sizeof(__be16)))) {
> +                               return -ENOMEM;
> +                       }
> +
> +                       if (likely(qp->eth_type == htons(ETH_P_8021Q))) {
> +                               key->eth.ctci = qp->tci | htons(VLAN_TAG_PRESENT);
> +                               __skb_pull(skb, sizeof(struct qtag_prefix));
> +                       }
> +               }
> +                return 0;
> +       }
>

...

> @@ -1074,9 +1099,28 @@ int ovs_nla_get_match(struct sw_flow_match *match,
>                 encap_valid = true;
>
>                 if (tci & htons(VLAN_TAG_PRESENT)) {
> -                       err = parse_flow_nlattrs(encap, a, &key_attrs, log);
> -                       if (err)
> -                               return err;
> +
> +                       if (unlikely((nla_get_be16(a[OVS_KEY_ATTR_ETHERTYPE]) ==
> +                           htons(ETH_P_8021AD)))) {

Incorrect alignment.

> +
> +                               err = parse_flow_nlattrs(encap, a, &v_attrs, log);
> +                               if (err)
> +                                       return err;
> +                               if (v_attrs) {
v_attrs can to be defined in inner block where is actually used.

...
> @@ -1167,6 +1212,22 @@ int ovs_nla_get_match(struct sw_flow_match *match,
>                                 err = -EINVAL;
>                                 goto free_newmask;
>                         }
> +                       err = parse_flow_mask_nlattrs(encap, a, &mask_v_attrs, log);
> +                       if (err)
> +                               goto free_newmask;
> +
> +                       if (mask_v_attrs & (1ULL << OVS_KEY_ATTR_VLAN)) {
> +                               ctci = nla_get_be16(a[OVS_KEY_ATTR_VLAN]);
> +                               if (!(ctci & htons(VLAN_TAG_PRESENT))) {
> +                                       OVS_NLERR(log, "VLAN ctag present bit must have an exact match (ctci_mask=%x).",
> +                                                 ntohs(ctci));
> +                                       err = -EINVAL;
> +                                       goto free_newmask;
> +                               }
> +                               mask_v_attrs &= ~(1ULL << OVS_KEY_ATTR_VLAN);
> +                               mask_attrs |= mask_v_attrs;

mask_v_attrs can be defined in block where it is used.

> +                       }
> +
>                 }
>
>                 err = ovs_key_from_nlattrs(match, mask_attrs, a, true, log);
> @@ -1331,6 +1392,15 @@ static int __ovs_nla_put_key(const struct sw_flow_key *swkey,
>                 encap = nla_nest_start(skb, OVS_KEY_ATTR_ENCAP);
>                 if (!swkey->eth.tci)
>                         goto unencap;
> +       } else if (swkey->eth.tci || swkey->eth.type == htons(ETH_P_8021AD)) {
Checking for swkey->eth.tci does not make sense here. There is check
for it in previous if condition.

> +               __be16 eth_type;
> +               eth_type = !is_mask ? htons(ETH_P_8021AD) : htons(0xffff);
> +               if (nla_put_be16(skb, OVS_KEY_ATTR_ETHERTYPE, eth_type) ||
> +                   nla_put_be16(skb, OVS_KEY_ATTR_VLAN, output->eth.tci))

What about eth.ctci?

Re: [PATCH net-next V7 2/2] openvswitch: 802.1ad: Flow handling, actions, and vlan parsing

[Thomas F Herbert]

On 4/29/15 9:27 PM, Pravin Shelar wrote:
> On Tue, Apr 28, 2015 at 3:44 PM, Thomas F Herbert
> <thomasfherbert@gmail.com> wrote:
>> Add support for 802.1ad including the ability to push and pop double
>> tagged vlans.
>>
> I saw multiple checkpatch.pl errors and warning.
>
>> Signed-off-by: Thomas F Herbert <thomasfherbert@gmail.com>
> ...
>> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
>> index 2dacc7b..6989451 100644
>> --- a/net/openvswitch/flow.c
>> +++ b/net/openvswitch/flow.c
>> @@ -298,21 +298,78 @@ static bool icmp6hdr_ok(struct sk_buff *skb)
>>   static int parse_vlan(struct sk_buff *skb, struct sw_flow_key *key)
>>   {
>>          struct qtag_prefix {
>> -               __be16 eth_type; /* ETH_P_8021Q */
>> +               __be16 eth_type; /* ETH_P_8021Q  or ETH_P_8021AD */
>>                  __be16 tci;
>>          };
>> -       struct qtag_prefix *qp;
>> +       struct qtag_prefix *qp = (struct qtag_prefix *) skb->data;
>>
>> -       if (unlikely(skb->len < sizeof(struct qtag_prefix) + sizeof(__be16)))
>> -               return 0;
>> +       struct qinqtag_prefix {
>> +               __be16 eth_type; /* ETH_P_8021Q  or ETH_P_8021AD */
>> +               __be16 tci;
>> +               __be16 inner_tpid; /* ETH_P_8021Q */
>> +               __be16 ctci;
>> +       };
>> +
>> +       if (likely(skb_vlan_tag_present(skb))) {
>> +
>> +               key->eth.tci = htons(skb->vlan_tci);
>> +
> I think there is bug in existing OVS where it does not check
> vlan_proto before populating the flow key. Can you send a separate
> patch to fix this issue?.
Pravin,

Thanks for your review.

OK. I will find and fix this bug.
>
> Checking for swkey->eth.tci does not make sense here. There is check
> for it in previous if condition.
>
>> +               __be16 eth_type;
>> +               eth_type = !is_mask ? htons(ETH_P_8021AD) : htons(0xffff);
>> +               if (nla_put_be16(skb, OVS_KEY_ATTR_ETHERTYPE, eth_type) ||
>> +                   nla_put_be16(skb, OVS_KEY_ATTR_VLAN, output->eth.tci))
> What about eth.ctci?
OK, yes the logic seems wrong so I will fix the check for ctci above and 
fix the  checkpatch issues and resubmit to net-next.


-- 
Thomas F. Herbert

Re: [PATCH net-next V7 2/2] openvswitch: 802.1ad: Flow handling, actions, and vlan parsing

[Thomas F Herbert]

On 4/29/15 9:27 PM, Pravin Shelar wrote:
> On Tue, Apr 28, 2015 at 3:44 PM, Thomas F Herbert
> <thomasfherbert@gmail.com> wrote:
>
> +
> +       if (likely(skb_vlan_tag_present(skb))) {
> +
> +               key->eth.tci = htons(skb->vlan_tci);
> +
> I think there is bug in existing OVS where it does not check
> vlan_proto before populating the flow key. Can you send a separate
> patch to fix this issue?.
Pravin, I realized I needed to address this  comment in more detail. I 
would appreciate your and anybody
else's thoughts on the following:
1. the newer if_vlan header in the upstream kernel has a function that 
probably should be used here and
elsewhere when checking for tags, skb_vlan_tagged() which also checks 
skb->protocol field.
2.  What about the compat "stuff" in the linux datapath of openvswitch? 
Should any fix for this issue also be
applied to the compat layer which has different semantics for vlans or 
should the compat layer be updated to
be the same as the 3.19, 4.0 kernel semantics?
3.  In spite of my comment #2 above, I am not convinced that the 
generalized skb vlan functions in if_vlan.h are truly
independent of hardware acceleration. I can see cases where the vlan 
headers in the payload of the skb are not
checked. I am thinking a patch to the upstream kernel may also be necessary.
>
>> +               /*
>> +                * Case where upstream
>> +                * processing has already stripped the outer vlan tag.
>> +                */
>> +               if (unlikely(skb->vlan_proto == htons(ETH_P_8021AD))) {
>> +
>> +                       if (unlikely(skb->len < sizeof(struct qtag_prefix) +
>> +                                       sizeof(__be16)))
>> +                               return 0;
>> +
> If this returns from here then it can match on flow with tci which
> expect vlan_proto to be 8021Q but with vlan_proto equal to 8021AD.
I did not fix this. This double checking has come up before when I 
submitted an earlier revision of this patch.
The double checking done here is also used in the single tagged vlan 
code. I think that the consensus is that
Open vSwitch wants to allow incomplete headers to allow user space 
processing to decide how to
process incomplete vlan tags. For more discussion, see the following thread:
http://openvswitch.org/pipermail/dev/2014-December/049984.html

Thanks,

--TFH

-- 
Thomas F. Herbert

Re: [PATCH net-next V7 2/2] openvswitch: 802.1ad: Flow handling, actions, and vlan parsing

[Pravin Shelar]

On Tue, May 5, 2015 at 9:38 AM, Thomas F Herbert
<thomasfherbert@gmail.com> wrote:
> On 4/29/15 9:27 PM, Pravin Shelar wrote:
>>
>> On Tue, Apr 28, 2015 at 3:44 PM, Thomas F Herbert
>> <thomasfherbert@gmail.com> wrote:
>>
>> +
>> +       if (likely(skb_vlan_tag_present(skb))) {
>> +
>> +               key->eth.tci = htons(skb->vlan_tci);
>> +
>> I think there is bug in existing OVS where it does not check
>> vlan_proto before populating the flow key. Can you send a separate
>> patch to fix this issue?.
>
> Pravin, I realized I needed to address this  comment in more detail. I would
> appreciate your and anybody
> else's thoughts on the following:
> 1. the newer if_vlan header in the upstream kernel has a function that
> probably should be used here and
> elsewhere when checking for tags, skb_vlan_tagged() which also checks
> skb->protocol field.
When we populate flow key, we need to check skb->vlan_proto if it is
8021Q. This bug fix should be targeted to net tree.

> 2.  What about the compat "stuff" in the linux datapath of openvswitch?
> Should any fix for this issue also be
> applied to the compat layer which has different semantics for vlans or
> should the compat layer be updated to
> be the same as the 3.19, 4.0 kernel semantics?
I think compat layer is fine. But if you find any issue you can send fix.

> 3.  In spite of my comment #2 above, I am not convinced that the generalized
> skb vlan functions in if_vlan.h are truly
> independent of hardware acceleration. I can see cases where the vlan headers
> in the payload of the skb are not
> checked. I am thinking a patch to the upstream kernel may also be necessary.

ok.

>>
>>
>>> +               /*
>>> +                * Case where upstream
>>> +                * processing has already stripped the outer vlan tag.
>>> +                */
>>> +               if (unlikely(skb->vlan_proto == htons(ETH_P_8021AD))) {
>>> +
>>> +                       if (unlikely(skb->len < sizeof(struct
>>> qtag_prefix) +
>>> +                                       sizeof(__be16)))
>>> +                               return 0;
>>> +
>>
>> If this returns from here then it can match on flow with tci which
>> expect vlan_proto to be 8021Q but with vlan_proto equal to 8021AD.
>
> I did not fix this. This double checking has come up before when I submitted
> an earlier revision of this patch.
> The double checking done here is also used in the single tagged vlan code. I
> think that the consensus is that
> Open vSwitch wants to allow incomplete headers to allow user space
> processing to decide how to
> process incomplete vlan tags. For more discussion, see the following thread:
> http://openvswitch.org/pipermail/dev/2014-December/049984.html
>
When you return from error case you need to clear the key->eth.tci, so
that it does not match on wrong flow.

Re: [PATCH net-next V7 2/2] openvswitch: 802.1ad: Flow handling, actions, and vlan parsing

[Thomas F Herbert]

On 5/5/15 7:27 PM, Pravin Shelar wrote:
> On Tue, May 5, 2015 at 9:38 AM, Thomas F Herbert
> <thomasfherbert@gmail.com> wrote:
>> On 4/29/15 9:27 PM, Pravin Shelar wrote:
>>> On Tue, Apr 28, 2015 at 3:44 PM, Thomas F Herbert
>>> <thomasfherbert@gmail.com> wrote:
>>>
>>> +
>>> +       if (likely(skb_vlan_tag_present(skb))) {
>>> +
>>> +               key->eth.tci = htons(skb->vlan_tci);
>>> +
>>> I think there is bug in existing OVS where it does not check
>>> vlan_proto before populating the flow key. Can you send a separate
>>> patch to fix this issue?.
>> Pravin, I realized I needed to address this  comment in more detail. I would
>> appreciate your and anybody
>> else's thoughts on the following:
>> 1. the newer if_vlan header in the upstream kernel has a function that
>> probably should be used here and
>> elsewhere when checking for tags, skb_vlan_tagged() which also checks
>> skb->protocol field.
> When we populate flow key, we need to check skb->vlan_proto if it is
> 8021Q. This bug fix should be targeted to net tree.
OK
>
>> 2.  What about the compat "stuff" in the linux datapath of openvswitch?
>> Should any fix for this issue also be
>> applied to the compat layer which has different semantics for vlans or
>> should the compat layer be updated to
>> be the same as the 3.19, 4.0 kernel semantics?
> I think compat layer is fine. But if you find any issue you can send fix.
>
>> 3.  In spite of my comment #2 above, I am not convinced that the generalized
>> skb vlan functions in if_vlan.h are truly
>> independent of hardware acceleration. I can see cases where the vlan headers
>> in the payload of the skb are not
>> checked. I am thinking a patch to the upstream kernel may also be necessary.
> ok.
>
>>>
>>>> +               /*
>>>> +                * Case where upstream
>>>> +                * processing has already stripped the outer vlan tag.
>>>> +                */
>>>> +               if (unlikely(skb->vlan_proto == htons(ETH_P_8021AD))) {
>>>> +
>>>> +                       if (unlikely(skb->len < sizeof(struct
>>>> qtag_prefix) +
>>>> +                                       sizeof(__be16)))
>>>> +                               return 0;
>>>> +
>>> If this returns from here then it can match on flow with tci which
>>> expect vlan_proto to be 8021Q but with vlan_proto equal to 8021AD.
>> I did not fix this. This double checking has come up before when I submitted
>> an earlier revision of this patch.
>> The double checking done here is also used in the single tagged vlan code. I
>> think that the consensus is that
>> Open vSwitch wants to allow incomplete headers to allow user space
>> processing to decide how to
>> process incomplete vlan tags. For more discussion, see the following thread:
>> http://openvswitch.org/pipermail/dev/2014-December/049984.html
>>
> When you return from error case you need to clear the key->eth.tci, so
> that it does not match on wrong flow.
Yes, you are correct. I see the the bug now. I will fix in a V9.

--TFH

-- 
Thomas F Herbert
Principal Software Engineer
Red Hat
therbert@redhat.com
Office: 919-301-3295
Mobile: 804-741-2695