Re: [PATCH net-next V18 3/3] 802.1AD: Flow handling, actions, vlan parsing and netlink attributes
From: Pravin Shelar <hidden>
Date: 2015-10-27 02:10:19
On Sun, Oct 25, 2015 at 5:11 PM, Thomas F Herbert [off-list ref] wrote:
Add support for 802.1ad including the ability to push and pop double tagged vlans. Add support for 802.1ad to netlink parsing and flow conversion. Uses double nested encap attributes to represent double tagged vlan. Inner TPID encoded along with ctci in nested attributes. Outer TPID is also encoded in the flow key. Signed-off-by: Thomas F Herbert <redacted>
This patch does not apply on current master due to conflicts related net-branch merge.
quoted hunk
--- net/openvswitch/actions.c | 6 +- net/openvswitch/flow.c | 76 ++++++++++++---- net/openvswitch/flow.h | 8 +- net/openvswitch/flow_netlink.c | 199 +++++++++++++++++++++++++++++++++++++---- net/openvswitch/vport-netdev.c | 4 +- 5 files changed, 252 insertions(+), 41 deletions(-)diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c index c8db44a..ed19e2b 100644 --- a/net/openvswitch/flow.c +++ b/net/openvswitch/flow.c@@ -302,24 +302,68 @@ static bool icmp6hdr_ok(struct sk_buff *skb) sizeof(struct icmp6hdr)); } -static int parse_vlan(struct sk_buff *skb, struct sw_flow_key *key) +/* Parse vlan tag from vlan header. + * Returns ERROR on memory error. + * Returns 0 if it encounters a non-vlan or incomplete packet. + * Returns 1 after successfully parsing vlan tag. + */ + +static int parse_vlan_tag(struct sk_buff *skb, struct vlan_head *vlan) { - struct qtag_prefix { - __be16 eth_type; /* ETH_P_8021Q */ - __be16 tci; - }; - struct qtag_prefix *qp; + struct vlan_head *qp = (struct vlan_head *)skb->data; + + if (likely(!eth_type_vlan(qp->tpid))) + return 0; - if (unlikely(skb->len < sizeof(struct qtag_prefix) + sizeof(__be16))) + if (unlikely(skb->len < sizeof(struct vlan_head) + sizeof(__be16))) return 0;
Why do we need extra sizeof(__be16) bytes here?
- if (unlikely(!pskb_may_pull(skb, sizeof(struct qtag_prefix) +
- sizeof(__be16))))
+ if (unlikely(!pskb_may_pull(skb, sizeof(struct vlan_head) +
+ 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));
+ vlan->tci = qp->tci | htons(VLAN_TAG_PRESENT);
+ vlan->tpid = qp->tpid;
+
+ __skb_pull(skb, sizeof(struct vlan_head));
+ return 1;
+}
+...
quoted hunk
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c index c92d6a2..7e90f8c 100644 --- a/net/openvswitch/flow_netlink.c +++ b/net/openvswitch/flow_netlink.c
...
+
+static int parse_vlan_from_nlattrs(const struct nlattr **nla,
+ struct sw_flow_match *match,
+ u64 *key_attrs, bool *ie_valid,
+ const struct nlattr **a, bool is_mask,
+ bool log)
+{
+ int err;
+ const struct nlattr *encap;
+ u64 v_attrs = 0;
+
+ if (!is_mask) {
+ err = __parse_vlan_from_nlattrs(nla, match, key_attrs,
+ false, a, is_mask, log);
+ if (err)
+ return err;
+
+ /* Another encap attribute here indicates
+ * the presence of a double tagged vlan.
+ */
+ encap = a[OVS_KEY_ATTR_ENCAP];
+
+ err = parse_flow_nlattrs(encap, a, &v_attrs, log);
+ if (err)
+ return err;
+
+ if ((v_attrs & (1 << OVS_KEY_ATTR_ETHERTYPE)) &&
+ eth_type_vlan(nla_get_be16(a[OVS_KEY_ATTR_ETHERTYPE]))) {
+ if (!((v_attrs & (1 << OVS_KEY_ATTR_VLAN)) &&
+ (v_attrs & (1 << OVS_KEY_ATTR_ENCAP)))) {
+ OVS_NLERR(log, "Invalid Inner VLAN frame");
+ return -EINVAL;
+ }
+ *ie_valid = true;
+ err = __parse_vlan_from_nlattrs(&encap, match, &v_attrs,
+ true, a, is_mask, log);
+ if (err)
+ return err;
+ *key_attrs |= v_attrs;
+ }
+ } else {
+ err = __parse_vlan_from_nlattrs(nla, match, key_attrs,
+ false, a, is_mask, log);
+ if (err)
+ return err;
+
+ encap = a[OVS_KEY_ATTR_ENCAP];
+
+ err = parse_flow_nlattrs(encap, a, &v_attrs, log);
+ if (err)
+ return err;
+
+ if (v_attrs & 1 << OVS_KEY_ATTR_ENCAP) {Missing parentheses ...
quoted hunk
@@ -1099,25 +1240,27 @@ int ovs_nla_get_match(struct net *net, 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))) { + eth_type_vlan(nla_get_be16(a[OVS_KEY_ATTR_ETHERTYPE]))) { __be16 tci; - 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)))) {
This is not required change.
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)) {After checks in encode_vlan_from_nlattrs() function there is no need to have checks here.
- err = parse_flow_nlattrs(encap, a, &key_attrs, log);
+ err = parse_vlan_from_nlattrs(&encap, match,
+ &key_attrs,
+ &i_encap_valid, a, false,
+ log);
if (err)
return err;
+Added white space.
quoted hunk
} else if (!tci) { /* Corner case for truncated 802.1Q header. */ if (nla_len(encap)) {@@ -1169,7 +1312,7 @@ int ovs_nla_get_match(struct net *net, struct sw_flow_match *match, goto free_newmask; /* Always match on tci. */ - SW_FLOW_KEY_PUT(match, eth.tci, htons(0xffff), true); + SW_FLOW_KEY_PUT(match, eth.vlan.tci, htons(0xffff), true);
Also need to exact match on inner tci.
quoted hunk
if (mask_attrs & 1 << OVS_KEY_ATTR_ENCAP) { __be16 eth_type = 0;@@ -1188,10 +1331,13 @@ int ovs_nla_get_match(struct net *net, struct sw_flow_match *match, if (eth_type == htons(0xffff)) {
Same as above, after checks in encode_vlan_from_nlattrs() these checks looks redundant.
mask_attrs &= ~(1 << OVS_KEY_ATTR_ETHERTYPE);
encap = a[OVS_KEY_ATTR_ENCAP];
- err = parse_flow_mask_nlattrs(encap, a,
- &mask_attrs, log);
+ err = parse_vlan_from_nlattrs(&nla_mask, match,
+ &mask_attrs,
+ &i_encap_valid,
+ a, true, log);
if (err)
goto free_newmask;
+
} else {
OVS_NLERR(log, "VLAN frames must have an exact match on the TPID (mask=%x).",
ntohs(eth_type));...
quoted hunk
@@ -1368,17 +1515,29 @@ static int __ovs_nla_put_key(const struct sw_flow_key *swkey, ether_addr_copy(eth_key->eth_src, output->eth.src); ether_addr_copy(eth_key->eth_dst, output->eth.dst); - if (swkey->eth.tci || swkey->eth.type == htons(ETH_P_8021Q)) { - __be16 eth_type; - eth_type = !is_mask ? htons(ETH_P_8021Q) : htons(0xffff); - if (nla_put_be16(skb, OVS_KEY_ATTR_ETHERTYPE, eth_type) || - nla_put_be16(skb, OVS_KEY_ATTR_VLAN, output->eth.tci)) + if (swkey->eth.vlan.tci || eth_type_vlan(swkey->eth.type)) { + if (nla_put_be16(skb, OVS_KEY_ATTR_ETHERTYPE, + output->eth.vlan.tpid) || + nla_put_be16(skb, OVS_KEY_ATTR_VLAN, output->eth.vlan.tci)) goto nla_put_failure; encap = nla_nest_start(skb, OVS_KEY_ATTR_ENCAP); - if (!swkey->eth.tci) + if (!swkey->eth.vlan.tci) goto unencap; - } else + if (swkey->eth.cvlan.tci) { + /* Customer tci is nested but uses same key attribute. + */ + if (nla_put_be16(skb, OVS_KEY_ATTR_ETHERTYPE, + output->eth.cvlan.tpid) || + nla_put_be16(skb, OVS_KEY_ATTR_VLAN, + output->eth.cvlan.tci)) + goto nla_put_failure; + in_encap = nla_nest_start(skb, OVS_KEY_ATTR_ENCAP); + if (!swkey->eth.cvlan.tci) + goto unencap;
(!swkey->eth.cvlan.tci) is never going to be true. since inside if (swkey->eth.cvlan.tci) block. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev