| 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: Tue, 19 May 2015 12:55:48 -0400
From: Thomas F Herbert <thomasfherbert@...il.com>
To: Pravin Shelar <pshelar@...ira.com>
CC: netdev <netdev@...r.kernel.org>, therbert@...hat.com,
ccp@...net.net, "dev@...nvswitch.org" <dev@...nvswitch.org>
Subject: Re: [PATCH net-next V7 2/2] openvswitch: 802.1ad: Flow handling,
actions, and vlan parsing
On 5/14/15 3:33 AM, Pravin Shelar wrote:
> On Tue, May 12, 2015 at 5:06 PM, Thomas F Herbert
> <thomasfherbert@...il.com> wrote:
>> Add support for 802.1ad including the ability to push and pop double
>> tagged vlans.
>>
>> Signed-off-by: Thomas F Herbert <thomasfherbert@...il.com>
...
>> + if (is_mask)
>> + SW_FLOW_KEY_PUT(match, eth.ctci, htons(0xffff),
>> + is_mask);
>> + else
>
> 8021AD mask from user parameters is ignored and 0xffff is set. You
> need to set default 0xffff mask for ctci and then override it with
> user mask if given in the key.
Pravin, once again, thanks for your review.
I am thinking you are correct. I did it this way because it is the way
single tagged vlans are handled in the original vlan code. Which raises
two issues. 1. When I change this, I should also change the tci code for
consistency and should that be a separate patch? 2. The implication is
that so far all vlan vid mask matching has been done in user space only.
>
>> + SW_FLOW_KEY_PUT(match, eth.ctci, ctci, is_mask);
>> + }
...
>> +{
>> + return _ovs_vlan_from_nlattrs(match, attrs, a, true, log);
>> +}
>> +
> I do not see value in these functions. Can you directly call
> _ovs_vlan_from_nlattrs().
OK
>
>> static int ovs_key_from_nlattrs(struct sw_flow_match *match, u64 attrs,
>> const struct nlattr **a, bool is_mask,
>> bool log)
>> @@ -1024,6 +1069,113 @@ 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,
>> + const struct nlattr **a, bool is_mask,
>> + bool log)
>> +{
>> + int err;
>> + __be16 tci;
>> +
>> + 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)))) {
>> + err = parse_flow_nlattrs(nla, a, &v_attrs, log);
>> + if (err)
>> + return err;
>> + if (v_attrs) {
>> + err = ovs_vlan_from_nlattrs(match,
>> + v_attrs, a,
>> + log);
>> + if (err)
>> + return err;
>> + }
>> + /* Insure that tci key attribute isn't
>> + * overwritten by encapsulated customer tci.
>> + */
>> + v_attrs &= ~(1 << OVS_KEY_ATTR_VLAN);
>
> We also need to clear v_attrs when key has single vlan tag which is
> else part of this block.
This code is implemented this way because I have been using only a
single encapsulation level for double tagged vlans. I sneak the inner
tag into the encap and and then clear it here because the flow key has
only one attribute type for vlan and v_attrs is only used inside the
encapsulation.
Along with this change, I can also update
Documentation/networking/openvswitch.txt to show the double nested
encapsulation flow key.
>
>> + *key_attrs |= v_attrs;
>> + } else {
>> + err = parse_flow_nlattrs(nla, a, key_attrs,
>> + log);
>> + if (err)
>> + return err;
>> + }
>> + } else if (!tci) {
>> + /* Corner case for truncated 802.1Q header. */
>> + if (nla_len(nla)) {
>> + OVS_NLERR(log, "Truncated 802.1Q header has non-zero encap attribute.");
>> + return -EINVAL;
>> + }
>> + } else {
>> + OVS_NLERR(log, "Encap attr is set for non-VLAN frame");
>> + return -EINVAL;
>> + }
>> +
> For double vlan tag case we need to have double encap attributes in
> flow key; one for each tag. So flow key should look like:
>
> eth_type(0x88A8),vlan(vid=10),encap(eth_type(0x08100), vlan(vid=20),
> encap(eth_type(0x0800), ...))
>
> Can you adjust vlan parsing code according ?
Yes, I think you are right. I should change the code to use two levels
of encapsulation for double tagged vlans. This also would be the best
for consistency with future implementation of 802.1ah.
>
>
...
>> +{
>> + return _parse_vlan_from_nlattrs(nla, match, key_attrs, a, false, log);
>> +}
> You can move the key parsing block from _parse_vlan_from_nlattrs()
> here and move mask block in function bellow and get ride of
> _parse_vlan_from_nlattrs() function.
OK. I think this will make the code more straight forward.
>
>> +
>> +static int parse_vlan_mask_from_nlattrs(const struct nlattr *nla,
>> + struct sw_flow_match *match,
>> + u64 *key_attrs,
>> + const struct nlattr **a,
>> + bool log)
>> +{
>> + return _parse_vlan_from_nlattrs(nla, match, key_attrs, a, true, log);
>> +}
>> +
>> /**
--
Thomas F. Herbert
--
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