| 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: <5627A3A9.5010708@redhat.com>
Date: Wed, 21 Oct 2015 10:39:37 -0400
From: Thomas F Herbert <therbert@...hat.com>
To: Pravin Shelar <pshelar@...ira.com>,
Thomas F Herbert <thomasfherbert@...il.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/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.
>
>> 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) {
I think this should be the fix: Maybe it should be: if
((eth_type_vlan(swkey->eth.cvlan.tpid) || (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) <<<<<<<<<<<<<
OK, I see what you mean. It looks redundant because it checks for
cvlan.tci before the encap and then checks again after the encap. I got
this from the original code which checks for 8021Q || eth.tci. Then
after the encap, if it sees a zero tci, it unencaps. I guess this was to
make sure it didn't encode a zero tci. See pseudo-code above.
> 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.
https://github.com/tfherbert/ovs_8021ad/tree/8021AD
--
Thomas F Herbert Red Hat
--
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