netdev - Re: [PATCH net-next V18 3/3] 802.1AD: Flow handling, actions, vlan parsing and netlink attributes

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 27 Oct 2015 10:22:04 -0700
From:	Pravin Shelar <pshelar@...ira.com>
To:	Thomas F Herbert <thomasfherbert@...il.com>
Cc:	netdev <netdev@...r.kernel.org>,
	Thomas F Herbert <therbert@...hat.com>,
	"dev@...nvswitch.org" <dev@...nvswitch.org>
Subject: Re: [PATCH net-next V18 3/3] 802.1AD: Flow handling, actions, vlan
 parsing and netlink attributes

On Tue, Oct 27, 2015 at 9:45 AM, Thomas F Herbert
<thomasfherbert@...il.com> wrote:
> On 10/26/15 10:10 PM, Pravin Shelar wrote:
> Thanks for the review.
>>
>> On Sun, Oct 25, 2015 at 5:11 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.
>>> Outer
>>> TPID is also encoded in the flow key.
>>>
>>> Signed-off-by: Thomas F Herbert <thomasfherbert@...il.com>
>>
>> This patch does not apply on current master due to conflicts related
>> net-branch merge.
>
> OK, I will rebase.
>
>>
>>> ---
>>>   net/openvswitch/actions.c      |   6 +-
>>>   net/openvswitch/flow.c         |  76 ++++++++++++----
>>>   net/openvswitch/flow.h         |   8 +-
>>>   net/openvswitch/flow_netlink.c | 199
>>> +++++++++++++++++++++++++++++++++++++----
>>>   net/openvswitch/vport-netdev.c |   4 +-
>>>   5 files changed, 252 insertions(+), 41 deletions(-)
>>>
>>> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
>>> index c8db44a..ed19e2b 100644
>>> --- a/net/openvswitch/flow.c
>>> +++ b/net/openvswitch/flow.c
>>> @@ -302,24 +302,68 @@ static bool icmp6hdr_ok(struct sk_buff *skb)
>>>                                    sizeof(struct icmp6hdr));
>>>   }
>>>
>>> -static int parse_vlan(struct sk_buff *skb, struct sw_flow_key *key)
>>> +/* Parse vlan tag from vlan header.
>>> + * Returns ERROR on memory error.
>>> + * Returns 0 if it encounters a non-vlan or incomplete packet.
>>> + * Returns 1 after successfully parsing vlan tag.
>>> + */
>>> +
>>> +static int parse_vlan_tag(struct sk_buff *skb, struct vlan_head *vlan)
>>>   {
>>> -       struct qtag_prefix {
>>> -               __be16 eth_type; /* ETH_P_8021Q */
>>> -               __be16 tci;
>>> -       };
>>> -       struct qtag_prefix *qp;
>>> +       struct vlan_head *qp = (struct vlan_head *)skb->data;
>>> +
>>> +       if (likely(!eth_type_vlan(qp->tpid)))
>>> +               return 0;
>>>
>>> -       if (unlikely(skb->len < sizeof(struct qtag_prefix) +
>>> sizeof(__be16)))
>>> +       if (unlikely(skb->len < sizeof(struct vlan_head) +
>>> sizeof(__be16)))
>>>                  return 0;
>>
>> Why do we need extra sizeof(__be16) bytes here?
>
> I don't have an answer to your question. I didn't write this code and have
> wondered about why the extra two bytes were reserved. I don't know why it
> should be necessarily for inner or outer vlans or the HW accelerated case or
> for the non-accelerated case. If no reviewer can state a case for it, I will
> remove it with the next version of this patch.
>
Looks like it is optimization for parsing ethertype, So lets keep it.

>>>
>>>                  } else if (!tci) {
>>>                          /* Corner case for truncated 802.1Q header. */
>>>                          if (nla_len(encap)) {
>>> @@ -1169,7 +1312,7 @@ int ovs_nla_get_match(struct net *net, struct
>>> sw_flow_match *match,
>>>                          goto free_newmask;
>>>
>>>                  /* Always match on tci. */
>>> -               SW_FLOW_KEY_PUT(match, eth.tci, htons(0xffff), true);
>>> +               SW_FLOW_KEY_PUT(match, eth.vlan.tci, htons(0xffff),
>>> true);
>>
>> Also need to exact match on inner tci.
>
> This code sets a match on tci even if no vlan is present. Is this is for the
> case where there is no explicit mask specified in the netlink encoded flow?
> If that is correct, then it does need to be done for the inner vlan too.

Yes, By default it needs to be matched. userspace can overwrite it
with different wildcard.
--
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