openvswitch: Add support for 802.1AD
Subsystems (3)
v8 cover openvswitch: Add support for 802.1AD
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(-)
Signature · 6 lines
2.1.0 _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
A message that is not in the archive (sent off-list or not yet received): 1430841087-20764-1-git-send-email-thomasfherbert-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
PATCH v8 1/2 openvswitch: 802.1ad uapi changes.
openvswitch: Add support for 802.1AD
Change the description of the VLAN tpid field.
--- include/uapi/linux/openvswitch.h | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)
include/uapi/linux/openvswitch.h +9 -8
@@ -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
Signature · 6 lines
2.1.0 _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
PATCH v8 2/2 openvswitch: 802.1ad: Flow handling, actions, and parsing
In reply to a message not in the archiveAdd support for 802.1ad including the ability to push and pop double tagged vlans.
--- 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(-)
net/openvswitch/actions.c +4 -2
@@ -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;
net/openvswitch/flow.c +71 -12
101 lines not on this page · raw message
net/openvswitch/flow.h +1 -0
@@ -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 {
net/openvswitch/flow_netlink.c +89 -5
142 lines not on this page · raw message
Signature · 6 lines
2.1.0 _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH net-next V8 2/2] openvswitch: 802.1ad: Flow handling, actions, and parsing
Quoted 10 lines · Thomas F Herbert
Add support for 802.1ad including the ability to push and pop double tagged vlans.
Signed-off-by Thomas F Herbert <[email protected]>--- 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.
net/openvswitch/actions.c static int pop_vlan(struct sk_buff *skb, struct sw_flow_key *key) · line 224
Quoted 13 lines
net/openvswitch/actions.c +2
-1@@ -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.
net/openvswitch/actions.c static int pop_vlan(struct sk_buff *skb, struct sw_flow_key *key) · line 235
Quoted 10 lines
net/openvswitch/actions.c +2
-1else 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.
Quoted 28 lines
net/openvswitch/actions.c +0
-0else key->eth.tci = vlan->vlan_tci;net/openvswitch/flow.c +12
-3@@ -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);
net/openvswitch/flow.c static bool icmp6hdr_ok(struct sk_buff *skb) · line 325
Quoted 10 lines
net/openvswitch/flow.c +10
-0+ +/* 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.
Quoted 2 lines
net/openvswitch/flow.c +2
-0+ if (unlikely(!pskb_may_pull(skb, + sizeof(struct qtag_prefix) +
....
....
net/openvswitch/flow.c int ovs_nla_get_match(struct sw_flow_match *match, · line 1077
Quoted 11 lines
net/openvswitch/flow.c +3
-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;
Move local variable declaration to block where it is actually used.
Quoted 38 lines
net/openvswitch/flow.c +21
-4int 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 );
net/openvswitch/flow.c int ovs_nla_get_match(struct sw_flow_match *match, · line 1131
Quoted 10 lines
net/openvswitch/flow.c +7
-0+ 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.
Quoted 19 lines
net/openvswitch/flow.c +8
-0@@ -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 ]);
net/openvswitch/flow.c int ovs_nla_get_match(struct sw_flow_match *match, · line 1236
Quoted 10 lines
net/openvswitch/flow.c +10
-0+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.
net/openvswitch/flow.c int ovs_nla_get_match(struct sw_flow_match *match, · line 1239
Quoted 3 lines
net/openvswitch/flow.c +0
-0} 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
Quoted 9 lines · Pravin Shelar
Quoted 5 lines · Thomas F Herbert
Add support for 802.1ad including the ability to push and pop double tagged vlans.
Signed-off-by Thomas F Herbert <[email protected]>---
....
checkpatch.pl complains about blank lines.
Pravin, Thanks for your review.
Fixed in V9
Quoted 7 lines
Quoted 3 lines
....
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.
Quoted 3 lines
Quoted 2 lines
....
Same goes here, we need to invalidate flow key here.
See above.
Quoted 15 lines
Quoted 8 lines
else key->eth.tci = vlan->vlan_tci;net/openvswitch/flow.c +0
-0net/openvswitch/flow.c static bool icmp6hdr_ok(struct sk_buff *skb) · line 325
Quoted 6 lines
net/openvswitch/flow.c +4
-0@@ -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
Quoted 3 lines
Move local variable declaration to block where it is actually used.
Done. Fixed in V9
Quoted 3 lines
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.
Quoted 5 lines
I do not see
key->eth.ctcimask set according to userspace mask attribute. You could useovs_nested_vlan_from_nlattrs().
Fixed in V9.
Quoted 1 line
Signature · 1 line
Thomas F. Herbert