* [PATCH net-next V7 0/2] openvswitch: Add support for 802.1AD @ 2015-04-28 22:44 Thomas F Herbert 2015-04-28 22:44 ` [PATCH net-next V7 1/2] openvswitch: 802.1ad uapi changes Thomas F Herbert 2015-04-28 22:44 ` [PATCH net-next V7 2/2] openvswitch: 802.1ad: Flow handling, actions, and vlan parsing Thomas F Herbert 0 siblings, 2 replies; 10+ messages in thread From: Thomas F Herbert @ 2015-04-28 22:44 UTC (permalink / raw) To: netdev; +Cc: therbert, ccp, 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH net-next V7 1/2] openvswitch: 802.1ad uapi changes. 2015-04-28 22:44 [PATCH net-next V7 0/2] openvswitch: Add support for 802.1AD Thomas F Herbert @ 2015-04-28 22:44 ` Thomas F Herbert 2015-04-28 22:44 ` [PATCH net-next V7 2/2] openvswitch: 802.1ad: Flow handling, actions, and vlan parsing Thomas F Herbert 1 sibling, 0 replies; 10+ messages in thread From: Thomas F Herbert @ 2015-04-28 22:44 UTC (permalink / raw) To: netdev; +Cc: therbert, ccp, 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net-next V7 2/2] openvswitch: 802.1ad: Flow handling, actions, and vlan parsing 2015-04-28 22:44 [PATCH net-next V7 0/2] openvswitch: Add support for 802.1AD Thomas F Herbert 2015-04-28 22:44 ` [PATCH net-next V7 1/2] openvswitch: 802.1ad uapi changes Thomas F Herbert @ 2015-04-28 22:44 ` Thomas F Herbert 2015-04-30 1:27 ` Pravin Shelar 1 sibling, 1 reply; 10+ messages in thread From: Thomas F Herbert @ 2015-04-28 22:44 UTC (permalink / raw) To: netdev; +Cc: therbert, ccp, 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net-next V7 2/2] openvswitch: 802.1ad: Flow handling, actions, and vlan parsing 2015-04-28 22:44 ` [PATCH net-next V7 2/2] openvswitch: 802.1ad: Flow handling, actions, and vlan parsing Thomas F Herbert @ 2015-04-30 1:27 ` Pravin Shelar 2015-04-30 16:26 ` Thomas F Herbert 2015-05-05 16:38 ` Thomas F Herbert 0 siblings, 2 replies; 10+ messages in thread From: Pravin Shelar @ 2015-04-30 1:27 UTC (permalink / raw) To: Thomas F Herbert; +Cc: netdev, therbert, ccp 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? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next V7 2/2] openvswitch: 802.1ad: Flow handling, actions, and vlan parsing 2015-04-30 1:27 ` Pravin Shelar @ 2015-04-30 16:26 ` Thomas F Herbert 2015-05-05 16:38 ` Thomas F Herbert 1 sibling, 0 replies; 10+ messages in thread From: Thomas F Herbert @ 2015-04-30 16:26 UTC (permalink / raw) To: Pravin Shelar; +Cc: netdev, therbert, ccp 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next V7 2/2] openvswitch: 802.1ad: Flow handling, actions, and vlan parsing 2015-04-30 1:27 ` Pravin Shelar 2015-04-30 16:26 ` Thomas F Herbert @ 2015-05-05 16:38 ` Thomas F Herbert 2015-05-05 23:27 ` Pravin Shelar 1 sibling, 1 reply; 10+ messages in thread From: Thomas F Herbert @ 2015-05-05 16:38 UTC (permalink / raw) To: Pravin Shelar; +Cc: netdev, therbert, ccp, Flavio Leitner 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next V7 2/2] openvswitch: 802.1ad: Flow handling, actions, and vlan parsing 2015-05-05 16:38 ` Thomas F Herbert @ 2015-05-05 23:27 ` Pravin Shelar 2015-05-06 15:32 ` Thomas F Herbert 0 siblings, 1 reply; 10+ messages in thread From: Pravin Shelar @ 2015-05-05 23:27 UTC (permalink / raw) To: Thomas F Herbert; +Cc: netdev, therbert, ccp, Flavio Leitner 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next V7 2/2] openvswitch: 802.1ad: Flow handling, actions, and vlan parsing 2015-05-05 23:27 ` Pravin Shelar @ 2015-05-06 15:32 ` Thomas F Herbert 0 siblings, 0 replies; 10+ messages in thread From: Thomas F Herbert @ 2015-05-06 15:32 UTC (permalink / raw) To: Pravin Shelar, Thomas F Herbert; +Cc: netdev, ccp, Flavio Leitner 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH net-next V7 0/2] openvswitch: Add support for 802.1AD @ 2015-04-26 18:33 Thomas F Herbert 2015-04-27 3:03 ` David Miller 0 siblings, 1 reply; 10+ messages in thread From: Thomas F Herbert @ 2015-04-26 18:33 UTC (permalink / raw) To: netdev-u79uwXL29TY76Z2rM5mHXA Cc: dev-yBygre7rU0TnMu66kgdUjQ, 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 _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next V7 0/2] openvswitch: Add support for 802.1AD 2015-04-26 18:33 [PATCH net-next V7 0/2] openvswitch: Add support for 802.1AD Thomas F Herbert @ 2015-04-27 3:03 ` David Miller 0 siblings, 0 replies; 10+ messages in thread From: David Miller @ 2015-04-27 3:03 UTC (permalink / raw) To: thomasfherbert; +Cc: netdev, dev net-next is not open at this time, please resubmit this when the net-next tree is open for submissions again, thanks. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-05-06 15:32 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-04-28 22:44 [PATCH net-next V7 0/2] openvswitch: Add support for 802.1AD Thomas F Herbert 2015-04-28 22:44 ` [PATCH net-next V7 1/2] openvswitch: 802.1ad uapi changes Thomas F Herbert 2015-04-28 22:44 ` [PATCH net-next V7 2/2] openvswitch: 802.1ad: Flow handling, actions, and vlan parsing Thomas F Herbert 2015-04-30 1:27 ` Pravin Shelar 2015-04-30 16:26 ` Thomas F Herbert 2015-05-05 16:38 ` Thomas F Herbert 2015-05-05 23:27 ` Pravin Shelar 2015-05-06 15:32 ` Thomas F Herbert -- strict thread matches above, loose matches on Subject: below -- 2015-04-26 18:33 [PATCH net-next V7 0/2] openvswitch: Add support for 802.1AD Thomas F Herbert 2015-04-27 3:03 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).