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

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

[Thomas F Herbert]

Add support for 802.1AD to the openvswitch kernel module (Version 8.)

Please replace version 7 submitted on April 25, with this patch.
Version 8 has changes based on the review by Pravin Shelar of V7 and a fix
to flow.c to initialize the ctci in the flow key.

There was one recommended change from the review that was not made in this 
patch about a double check for vlan headers. The rationale for not making
that change will be explained separately.

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 addresses adds 802.1ad to the
Linux kernel openvswitch data path.

This patch is version 8 of the patch andccompanies user space version 7 of
the patch previously submitted separately 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   | 94 +++++++++++++++++++++++++++++++++++++---
 5 files changed, 174 insertions(+), 27 deletions(-)

-- 
2.1.0

_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

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

[Thomas F Herbert]

openvswitch: Add support for 802.1AD

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

_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[PATCH net-next V8 2/2] openvswitch: 802.1ad: Flow handling, actions, and 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 | 94 +++++++++++++++++++++++++++++++++++++++---
 4 files changed, 165 insertions(+), 19 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..0430466 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -298,21 +298,79 @@ 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)))
+				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 +532,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..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..ec504c4 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -771,6 +771,29 @@ 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 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;
+		}
+
+		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,
 				bool log)
@@ -1049,6 +1072,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 +1083,9 @@ 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 +1101,31 @@ 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 +1182,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 +1217,23 @@ 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 +1398,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 +2161,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

_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH net-next V8 2/2] openvswitch: 802.1ad: Flow handling, actions, and parsing

[Pravin Shelar]

On Tue, May 5, 2015 at 8:51 AM, 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>
> ---
>  net/openvswitch/actions.c      |  6 ++-
>  net/openvswitch/flow.c         | 83 +++++++++++++++++++++++++++++++------
>  net/openvswitch/flow.h         |  1 +
>  net/openvswitch/flow_netlink.c | 94 +++++++++++++++++++++++++++++++++++++++---
>  4 files changed, 165 insertions(+), 19 deletions(-)

checkpatch.pl complains about blank lines.

>
> 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);
I think we need to invalidate the flow key even when protocol is
8021Q. This is because packet is changed from double vlan
encapsulation to single. In this case vlan 8021Q TCI is mapped to
different key struct member.

>         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);
Same goes here, we need to invalidate flow key here.

>         else
>                 key->eth.tci = vlan->vlan_tci;
> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
> index 2dacc7b..0430466 100644
> --- a/net/openvswitch/flow.c
> +++ b/net/openvswitch/flow.c
> @@ -298,21 +298,79 @@ 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)))
> +                               return 0;
> +
In case of error we need to clear key->eth.tci.

> +                       if (unlikely(!pskb_may_pull(skb,
> +                                                   sizeof(struct qtag_prefix) +
....
....
> +
>  static int ovs_key_from_nlattrs(struct sw_flow_match *match, u64 attrs,
>                                 const struct nlattr **a, bool is_mask,
>                                 bool log)
> @@ -1049,6 +1072,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;
Move local variable declaration to block where it is actually used.

>         int err;
>
> @@ -1058,7 +1083,9 @@ 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 +1101,31 @@ 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)) {
Can you move this vlan key parsing to separate function.


> @@ -1133,6 +1182,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 +1217,23 @@ 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;
> +                       }
> +
we need to always initialize ctci mask to 0xff. Same as we do it for tci.

>                 }
>
>                 err = ovs_key_from_nlattrs(match, mask_attrs, a, true, log);

I do not see key->eth.ctci mask set according to userspace mask attribute.
You could use ovs_nested_vlan_from_nlattrs().

Re: [ovs-dev] [PATCH net-next V8 2/2] openvswitch: 802.1ad: Flow handling, actions, and parsing

[Thomas F Herbert]

On 5/6/15 2:36 PM, Pravin Shelar wrote:
> On Tue, May 5, 2015 at 8:51 AM, 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>
>> ---
 >....
> checkpatch.pl complains about blank lines.
Pravin, Thanks for your review.
Fixed in V9
>
>>
>>....
> I think we need to invalidate the flow key even when protocol is
> 8021Q. This is because packet is changed from double vlan
> encapsulation to single. In this case vlan 8021Q TCI is mapped to
> different key struct member.
Yes, good catch and thanks. this extra check in the pop and push actions 
wasn't necessary. The original push and pop code in net-next was already 
qinq-ready.
>
>>....
> Same goes here, we need to invalidate flow key here.
See above.
>
>>          else
>>                  key->eth.tci = vlan->vlan_tci;
>> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
>> index 2dacc7b..0430466 100644
>> --- a/net/openvswitch/flow.c
>> +++ b/net/openvswitch/flow.c
>> @@ -298,21 +298,79 @@ static bool icmp6hdr_ok(struct sk_buff *skb)
>>   static int parse_vlan(struct sk_buff *skb, struct sw_flow_key *key)
>>...
>> +                       if (unlikely(skb->len < sizeof(struct qtag_prefix) +
>> +                                       sizeof(__be16)))
>> +                               return 0;
>> +
> In case of error we need to clear key->eth.tci.
Done. Fixed in V9
>
>
> Move local variable declaration to block where it is actually used.
Done. Fixed in V9
>

> Can you move this vlan key parsing to separate function.
Good idea. In V9, Vlan to key parsing is now consolidated in a separate 
function.
>
>

> I do not see key->eth.ctci mask set according to userspace mask attribute.
> You could use ovs_nested_vlan_from_nlattrs().
Fixed in V9.
>


-- 
Thomas F. Herbert