Re: [PATCH net-next v21 3/4] openvswitch: 802.1AD Flow handling, actions, vlan parsing, netlink attributes
From: Eric Garver <hidden>
Date: 2016-09-03 17:30:12
Pravin, Thanks for the feedback. Some replies below. Thanks. Eric. On Fri, Sep 02, 2016 at 02:42:30PM -0700, pravin shelar wrote:
On Thu, Sep 1, 2016 at 1:45 PM, Eric Garver [off-list ref] wrote:quoted
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. This is based on Thomas F Herbert's original v20 patch. I made some small clean ups and bug fixes. Signed-off-by: Thomas F Herbert <redacted> Signed-off-by: Eric Garver <redacted>Thanks for working on this. This version looks pretty clone to complete.quoted
--- net/openvswitch/actions.c | 16 +-- net/openvswitch/flow.c | 64 ++++++++---- net/openvswitch/flow.h | 8 +- net/openvswitch/flow_netlink.c | 227 ++++++++++++++++++++++++++++++----------- net/openvswitch/vport.c | 7 +- 5 files changed, 235 insertions(+), 87 deletions(-)...quoted
diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c index 0ea128eeeab2..13f6ebdf379b 100644 --- a/net/openvswitch/flow.c +++ b/net/openvswitch/flow.c@@ -302,24 +302,56 @@ 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 (unlikely(skb->len < sizeof(struct qtag_prefix) + sizeof(__be16))) + if (likely(!eth_type_vlan(qp->tpid))) return 0; - if (unlikely(!pskb_may_pull(skb, sizeof(struct qtag_prefix) + - sizeof(__be16)))) + if (unlikely(skb->len < sizeof(struct vlan_head) + sizeof(__be16))) + return 0; + + if (unlikely(!pskb_may_pull(skb, sizeof(struct vlan_head) + + sizeof(__be16)))) return -ENOMEM;pskb_may_pull() can change skb->data, so you need to refresh qp pointer.
Good catch!
quoted
- 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
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c index c78a6a1476fb..fbe9e0e4792d 100644 --- a/net/openvswitch/flow_netlink.c +++ b/net/openvswitch/flow_netlink.c...quoted
+static int __parse_vlan_from_nlattrs(struct sw_flow_match *match, + u64 *key_attrs, bool inner, + const struct nlattr **a, bool is_mask, + bool log) +{ + int err; + const struct nlattr *encap; + + err = encode_vlan_from_nlattrs(match, a, is_mask, inner, log); + if (err) + return err; + + *key_attrs &= ~(1 << OVS_KEY_ATTR_ENCAP); + + /* Ensure that tci key attribute isn't + * overwritten by encapsulated customer tci. + * Ethertype is cleared because it is c_tpid. + */ + *key_attrs &= ~(1 << OVS_KEY_ATTR_VLAN); + *key_attrs &= ~(1 << OVS_KEY_ATTR_ETHERTYPE); + + encap = a[OVS_KEY_ATTR_ENCAP]; + + if (is_mask) + err = parse_flow_mask_nlattrs(encap, a, key_attrs, log); + else + err = parse_flow_nlattrs(encap, a, key_attrs, log); + + return err; +} + +static int parse_vlan_from_nlattrs(struct sw_flow_match *match, + u64 *key_attrs, bool *ie_valid, + const struct nlattr **a, bool is_mask, + bool log) +{ + int err; + + err = __parse_vlan_from_nlattrs(match, key_attrs, + false, a, is_mask, log); + if (err) + return err; + + if (!is_mask) { + if ((*key_attrs & (1 << OVS_KEY_ATTR_ETHERTYPE)) && + 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)))) { + OVS_NLERR(log, "Invalid Inner VLAN frame"); + return -EINVAL; + } + *ie_valid = true; + } + } else { + if (*key_attrs & (1 << OVS_KEY_ATTR_ENCAP)) { + if (!*ie_valid) { + OVS_NLERR(log, "Encap mask attribute is set for non-CVLAN frame."); + return -EINVAL; + } + } + } +This check can be moved to __parse_vlan_from_nlattrs(). This way there is no need to check ATTR_VLAN and ATTR_ENCAP here and in ovs_nla_get_match() function. ...
I agree there could be some refactoring done here.
quoted
@@ -1182,11 +1301,11 @@ int ovs_nla_get_match(struct net *net, struct sw_flow_match *match, bool log) { const struct nlattr *a[OVS_KEY_ATTR_MAX + 1]; - const struct nlattr *encap; struct nlattr *newmask = NULL; u64 key_attrs = 0; u64 mask_attrs = 0; bool encap_valid = false; + bool i_encap_valid = false; int err; err = parse_flow_nlattrs(nla_key, a, &key_attrs, log);@@ -1195,35 +1314,20 @@ 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))) { - __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)))) { OVS_NLERR(log, "Invalid Vlan frame."); return -EINVAL; }... ...quoted
@@ -1464,17 +1547,37 @@ 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)) { + if (swkey->eth.vlan.tci || eth_type_vlan(swkey->eth.type)) { __be16 eth_type; - eth_type = !is_mask ? htons(ETH_P_8021Q) : htons(0xffff); + eth_type = !is_mask ? output->eth.vlan.tpid : htons(0xffff); if (nla_put_be16(skb, OVS_KEY_ATTR_ETHERTYPE, eth_type) || - nla_put_be16(skb, OVS_KEY_ATTR_VLAN, output->eth.tci)) + 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 - encap = NULL; + + if (swkey->eth.cvlan.tci || eth_type_vlan(swkey->eth.type)) { + /* Customer tci is nested but uses same key attribute. + */ + eth_type = !is_mask ? output->eth.cvlan.tpid : htons(0xffff); + if (nla_put_be16(skb, OVS_KEY_ATTR_ETHERTYPE, eth_type) || + 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; + } + }there could be helper function to serialize "struct vlan_head" to avoid duplicate code above.
Good idea. I'll add a helper.
quoted
+ + if (eth_type_vlan(swkey->eth.type) && encap && in_encap) {I think check for eth.type is sufficient here. encap and in_encap should be true in this case anyways.
Yes. You're right. Call it paranoia. Would a BUG_ON(!(encap && in_encap)) be better?
quoted
+ /* There are 3 VLAN tags, we don't know anything about the rest of the + * packet, so truncate here. + */ + if (nla_put_be16(skb, OVS_KEY_ATTR_ETHERTYPE, output->eth.type))After this output->eth.type would be serialized three times, can you move the check for third vlan tag after it is serialized so as to avoid it.
Will do.
quoted
+ goto nla_put_failure; + goto unencap; + } if (swkey->eth.type == htons(ETH_P_802_2)) { /*
_______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev