| 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: <55B42AB1.6080200@gmail.com>
Date: Sat, 25 Jul 2015 20:32:49 -0400
From: Thomas F Herbert <thomasfherbert@...il.com>
To: Pravin Shelar <pshelar@...ira.com>
CC: netdev <netdev@...r.kernel.org>, therbert@...hat.com,
"dev@...nvswitch.org" <dev@...nvswitch.org>
Subject: Re: [PATCH net-next 3/3] openvswitch: 802.1AD: Flow handling, actions,
and vlan parsing
On 6/30/15 12:16 AM, Pravin Shelar wrote:
> On Tue, Jun 23, 2015 at 11:26 AM, Thomas F Herbert
Pravin, I apologize because I realize now that I am finishing V12 of
this patch that I never responded to your comments in this email.
> <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.
>>
>> Signed-off-by: Thomas F Herbert <thomasfherbert@...il.com>
>> ---
>> net/openvswitch/flow.c | 84 +++++++++++++++---
>> net/openvswitch/flow.h | 5 ++
>> net/openvswitch/flow_netlink.c | 195 ++++++++++++++++++++++++++++++++++-------
>> 3 files changed, 242 insertions(+), 42 deletions(-)
>>
>> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
>> index 2dacc7b..e268865 100644
>> --- a/net/openvswitch/flow.c
>> +++ b/net/openvswitch/flow.c
>> @@ -298,21 +298,80 @@ 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);
>> +
>> + /* 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))) {
>> + key->eth.tci = 0;
>> + 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.cvlan.ctci =
>> + qp->tci | htons(VLAN_TAG_PRESENT);
>> + key->eth.cvlan.c_tpid = skb->vlan_proto;
>
> We should directly copy qp->inner_tpid here. As you have done it for
> non offloaded case below.
Thanks! It is copied but it is set to the wrong tpid. The c_tpid field
in the key should be set to the ethertype in the packet itself which is
the inner tpid, not the offloaded skb-vlan_proto which is the outer
tpid. Fixed in V12.
>
>> + __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.cvlan.ctci = qinqp->ctci | htons(VLAN_TAG_PRESENT);
>> + key->eth.cvlan.c_tpid = qinqp->inner_tpid;
>> +
>> + __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 +533,10 @@ 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))
>> + key->eth.cvlan.ctci = 0;
> cvlan.c_tpid is not initialized for all cases.
Fixed in V12
>
>> + if ((skb_vlan_tag_present(skb)) ||
>> + (eth->h_proto == htons(ETH_P_8021Q)) ||
>> + (eth->h_proto == htons(ETH_P_8021AD)))
> These are redundant check. so we can directly call this function.
I don't agree that these 3 checks are redundant. parse_vlan parses both
the offloaded and non-offloaded cases. In V12, I changed it to call
eth_type_vlan() to do the checks for the non-offloaded ethertypes.
Hmmm ... maybe I should add another function to if_vlan.h to check if a
packet is a vlan regardless of whether it is offloaded or not?
>
>> if (unlikely(parse_vlan(skb, key)))
>> return -ENOMEM;
>>
>
> ...
>>
>> +static int cust_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 CTCI mask does not have exact match for VLAN_TAG_PRESENT bit.");
>> + else
>> + OVS_NLERR(log, "VLAN CTCI does not have VLAN_TAG_PRESENT bit set.");
>> +
>> + return -EINVAL;
>> + }
>> + SW_FLOW_KEY_PUT(match, eth.cvlan.c_tpid, ctci, is_mask);
>> + SW_FLOW_KEY_PUT(match, eth.cvlan.ctci, ctci, is_mask);
>> + }
> Same value is set to tpid and tci.
Thanks! Fixed in V12.
>
>> + return 0;
>> +}
>> +
>> static int ovs_key_from_nlattrs(struct sw_flow_match *match, u64 attrs,
>> const struct nlattr **a, bool is_mask,
>> bool log)
>> @@ -1024,6 +1047,105 @@ static void mask_set_nlattr(struct nlattr *attr, u8 val)
>> nlattr_set(attr, val, ovs_key_lens);
>> }
>>
>> +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;
>> + __be16 tci;
>> + const struct nlattr *encap;
>> +
>> + if (!is_mask) {
>> + u64 v_attrs = 0;
>> +
>> + tci = nla_get_be16(a[OVS_KEY_ATTR_VLAN]);
>> +
>> + if (tci & htons(VLAN_TAG_PRESENT)) {
>> + if (unlikely((nla_get_be16(a[OVS_KEY_ATTR_ETHERTYPE]) ==
>> + htons(ETH_P_8021AD)))) {
> Since we have added tpid to flow key, we have flexibility of
> supporting generic double encapsulation. Therefore in netlink parsing
> of key, double encap should not be limited to just 8021AD. for example
> it should allow 8021Q in 8021Q header as valid key.
I agree. Although Open Flow specification doesn't support non 802.1AD
double tagged vlans, we probably should be as least restrictive as
possible here in the kernel module. I recoded this in V12 to allow a
more "generic" qinq.
>
>> + err = parse_flow_nlattrs(nla, a, &v_attrs, log);
>> + if (err)
>> + return err;
>> + if (!v_attrs)
>> + return -EINVAL;
>> +
--
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