| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <562FAA0E.4090908@gmail.com>
Date: Tue, 27 Oct 2015 12:45:02 -0400
From: Thomas F Herbert <thomasfherbert@...il.com>
To: Pravin Shelar <pshelar@...ira.com>
Cc: netdev <netdev@...r.kernel.org>,
Thomas F Herbert <therbert@...hat.com>,
"dev@...nvswitch.org" <dev@...nvswitch.org>
Subject: Re: [PATCH net-next V18 3/3] 802.1AD: Flow handling, actions, vlan
parsing and netlink attributes
On 10/26/15 10:10 PM, Pravin Shelar wrote:
Thanks for the review.
> On Sun, Oct 25, 2015 at 5:11 PM, Thomas F Herbert
> <thomasfherbert@...il.com> 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 <thomasfherbert@...il.com>
> This patch does not apply on current master due to conflicts related
> net-branch merge.
OK, I will rebase.
>
>> ---
>> 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?
I don't have an answer to your question. I didn't write this code and
have wondered about why the extra two bytes were reserved. I don't know
why it should be necessarily for inner or outer vlans or the HW
accelerated case or for the non-accelerated case. If no reviewer can
state a case for it, I will remove it with the next version of this patch.
>
>> - 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;
>> +}
>> +
> ...
>
>> 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
Yes, thanks for spotting this.
>
> ...
>> @@ -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.
Yes, we already agreed that forcing to a 64 bit constant was not
necessary and should be removed for consistency. Sorry but it crept back
into this version via cut and paste when refactoring. Thanks for
spotting this and I will fix.
>
>> 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.
Yes, some of this code is redundant and should be removed.
>
>> - 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.
Fixed in next version.
>> } 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.
This code sets a match on tci even if no vlan is present. Is this is for
the case where there is no explicit mask specified in the netlink
encoded flow? If that is correct, then it does need to be done for the
inner vlan too.
>
>> 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.
I agree. This patch makes this the extra check redundant with new code
in encode_vlan_from_nlattrs() and can be removed.
>
>> 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));
> ...
>> @@ -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.
Yes, it will be removed.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists