| 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: <CALnjE+rjNNwvuRRa2hktwHukTvnTx=AC4PegiAvwOT9UGtZKFw@mail.gmail.com>
Date: Tue, 20 Oct 2015 13:34:46 -0700
From: Pravin Shelar <pshelar@...ira.com>
To: Thomas F Herbert <thomasfherbert@...il.com>
Cc: netdev <netdev@...r.kernel.org>, therbert@...hat.com,
"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 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.
Call to parse_flow_nlattrs in parse_vlan_from_nlattrs() has no effect
so it should be removed.
> One think I see I should have done is check for eth_type_vlan() on the inner
> tag as well.
>>
>> Currently we are
>> reversing netlink encoding while sending back the vlan attributes.
>> cvlan and vlan is independent,
>
> Are you talking about a corner case where the incoming packet has an inner
> vlan but no outer vlN?
>>
nope, this is usual case.
>> therefore we need to check
>> swkey->eth.cvlan.tc outside of swkey->eth.vlan.tci block. Also
>> redundant check for swkey->eth.cvlan.tci should be removed
>
> I must be missing something because I don't understand this either. First my
> patch encodes the outer vlan and then sets the encap bit and then encodes
> the inner vlan. The encoding shows up correctly in user space.
>>
Once ovs_nla_get_match() is fixed, there is no need to change this
function to fix that ordering.
I was talking about following redundant check:
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;
}
>>
>> Can you also post changes for userspace vswitchd so that I can try next
>> patch.
>
> I posted a patch for user space (V14) to ovs dev on October 2nd. I haven't
> made any changes since then.
> http://openvswitch.org/pipermail/dev/2015-October/060874.html
>
That patch does not apply on OVS master. You could also send me link
to ovs userpsace github repo with these patches applied so that I can
try kernel datapath changes.
--
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