| 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
| ||
|
Date: Fri, 23 Oct 2015 15:42:46 -0400
From: Thomas F Herbert <thomasfherbert@...il.com>
To: Pravin Shelar <pshelar@...ira.com>,
Thomas F Herbert <therbert@...hat.com>
Cc: netdev <netdev@...r.kernel.org>,
"dev@...nvswitch.org" <dev@...nvswitch.org>
Subject: Re: [PATCH net-next V17 3/3] 802.1AD: Flow handling, actions, vlan
parsing and netlink attributes
On 10/23/15 3:26 PM, Pravin Shelar wrote:
> On Wed, Oct 21, 2015 at 7:39 AM, Thomas F Herbert <therbert@...hat.com> wrote:
>>
>> On 10/20/15 4:34 PM, Pravin Shelar wrote:
>>> On Tue, Oct 20, 2015 at 7:26 AM, Thomas F Herbert
>>> <thomasfherbert@...il.com> wrote:
>>>> On 10/19/15 2:28 PM, Pravin Shelar wrote:
>>>>> On Sat, Oct 17, 2015 at 6:12 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.
>>>>>>
>>>>>> Signed-off-by: Thomas F Herbert<thomasfherbert@...il.com>
>>>>>> ---
>>>>>> net/openvswitch/actions.c | 6 +-
>>>>>> net/openvswitch/flow.c | 76 +++++++++++++-----
>>>>>> net/openvswitch/flow.h | 8 +-
>>>>>> net/openvswitch/flow_netlink.c | 172
>>>>>> +++++++++++++++++++++++++++++++++++++----
>>>>>> net/openvswitch/vport-netdev.c | 4 +-
>>>>>> 5 files changed, 227 insertions(+), 39 deletions(-)
>>>>>>
>>>>>> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
>>>>>> index 315f533..09cc1c9 100644
>>>>> ...
>>>>>
>>>>>> diff --git a/net/openvswitch/flow_netlink.c
>>>>>> b/net/openvswitch/flow_netlink.c
>>>>>> index c92d6a2..97a6d12 100644
>>>>>> --- a/net/openvswitch/flow_netlink.c
>>>>>> +++ b/net/openvswitch/flow_netlink.c
>>>>> ...
>>>>>
>>>>>> @@ -1320,6 +1443,7 @@ static int __ovs_nla_put_key(const struct
>>>>>> sw_flow_key *swkey,
>>>>>> {
>>>>>> struct ovs_key_ethernet *eth_key;
>>>>>> struct nlattr *nla, *encap;
>>>>>> + struct nlattr *in_encap = NULL;
>>>>>>
>>>>>> if (nla_put_u32(skb, OVS_KEY_ATTR_RECIRC_ID,
>>>>>> output->recirc_id))
>>>>>> goto nla_put_failure;
>>>>>> @@ -1368,17 +1492,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;
>>>>>> + }
>>>>>> + } else {
>>>>>> encap = NULL;
>>>>>> + }
>>>>> After the vlan parsing changes, we need to encode cvlan in outer
>>>>> netlink attribute and then encode regular vlan.
>>>> I don't understand this. cvlan is inner vlan, why would it be encoded in
>>>> outer vlan without the 2nd layer of encapsulation?
>>>>
>>> Lets start with double tagged packet.
>>> packet: eth, vlan 10, inner vlan 20, ip.
>>> flow key would be:
>>> flow_key: vlan = 10, cvlan = 20
>>> That would get serialize over netlink like:
>>>
>>> eth_type(0x8100),vlan(vid=10,pcp=0),encap(eth_type(0x8100),vlan(vid=20,pcp=0),
>>> encap(eth_type(0x0800),ipv4(frag=no))...
>>> So far looks good.
>>>
>>> Now userspace sends back same key and installs flow in kernel
>>> datapath. But ovs_nla_get_match() parses netink key and sets vlan in
>>> reverse order. After parsing netlink in ovs_nla_get_match() vlans
>>> flow-key would look like:
>>> flow_key: vlan = 20, cvlan = 10. This is not what we started with.
>>>
>>> Now I think rather than fixing __ovs_nla_put_key (), we need to fix
>>> ovs_nla_get_match() to keep vlan order.
>>> I also noticed that eth.vlan.tpid is never initialized from netlink
>>> attributes.
>> Yes, you are right. The code in master never encoded the tpid in the key and
>> I didn't add it when I modified the swkey for the addition of the vlan
>> struct.
>>
>> Yes, you are right the code is wrong. The intention was to leave the old
>> code which left the outer tci in the key for later processing. In the code
>> in master, after checking proper vlan the function ovs_key_from_nlattrs()
>> encoded the tci.
>>
>> Now I see that I must refactor the code in ovs_nla_get_match() and
>> parse_vlan_from_nlattrs to explicitly encode the outer vlan including tpid
>> before the inner vlan.
>>> Call to parse_flow_nlattrs in parse_vlan_from_nlattrs() has no effect
>>> so it should be removed
>> parse_flow_nlattrs() is called for each level of encapsulation. It sets
>> v_attrs with the keys found in the flow key. This is to insure that I am
>> looking only at the keys for the current level of encapsulation. Then it is
>> called a final time to parse out the l3 attributes for encoding.
> ovs_nla_get_match() calls parse_flow_nlattrs() and then again in
> parse_vlan_from_nlattrs(), second parse_flow_nlattr() can potentially
> overwrite vlan attributes. Therefore it should called after the outer
> vlan attributes are read.
Thanks! Yes, I agree. I am fixing this with the refactoring I am doing now.
--
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