netdev - Re: [PATCH v2 net-next 2/2] openvswitch: Fix skb->protocol for vlan frames.

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] [day] [month] [year] [list]

Message-Id: <8D71D8D7-9C09-4833-AF25-3110E7927186@ovn.org>
Date:   Tue, 29 Nov 2016 15:32:50 -0800
From:   Jarno Rajahalme <jarno@....org>
To:     Pravin Shelar <pshelar@....org>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Jiri Benc <jbenc@...hat.com>
Subject: Re: [PATCH v2 net-next 2/2] openvswitch: Fix skb->protocol for vlan frames.


> On Nov 28, 2016, at 11:21 PM, Pravin Shelar <pshelar@....org> wrote:
> 
> On Mon, Nov 28, 2016 at 6:41 PM, Jarno Rajahalme <jarno@....org> wrote:
>> Do not set skb->protocol to be the ethertype of the L3 header, unless
>> the packet only has the L3 header.  For a non-hardware offloaded VLAN
>> frame skb->protocol needs to be one of the VLAN ethertypes.
>> 
>> Any VLAN offloading is undone on the OVS netlink interface.  Also any
>> VLAN tags added by userspace are non-offloaded.
>> 
>> Incorrect skb->protocol value on a full-size non-offloaded VLAN skb
>> causes packet drop due to failing MTU check, as the VLAN header should
>> not be counted in when considering MTU in ovs_vport_send().
>> 
> I think we should move to is_skb_forwardable() type of packet length
> check in vport-send and get rid of skb-protocol checks altogether.
> 

I added a new patch to do this, thanks for the suggestion.

>> Fixes: 5108bbaddc ("openvswitch: add processing of L3 packets")
>> Signed-off-by: Jarno Rajahalme <jarno@....org>
>> ---
>> v2: Set skb->protocol when an ETH_P_TEB frame is received via ARPHRD_NONE
>>    interface.
>> 
>> net/openvswitch/datapath.c |  1 -
>> net/openvswitch/flow.c     | 30 ++++++++++++++++++++++--------
>> 2 files changed, 22 insertions(+), 9 deletions(-)
> ...
> ...
>> @@ -531,15 +538,22 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
>>                if (unlikely(parse_vlan(skb, key)))
>>                        return -ENOMEM;
>> 
>> -               skb->protocol = parse_ethertype(skb);
>> -               if (unlikely(skb->protocol == htons(0)))
>> +               key->eth.type = parse_ethertype(skb);
>> +               if (unlikely(key->eth.type == htons(0)))
>>                        return -ENOMEM;
>> 
>> +               if (skb->protocol == htons(ETH_P_TEB)) {
>> +                       if (key->eth.vlan.tci & htons(VLAN_TAG_PRESENT)
>> +                           && !skb_vlan_tag_present(skb))
>> +                               skb->protocol = key->eth.vlan.tpid;
>> +                       else
>> +                               skb->protocol = key->eth.type;
>> +               }
>> +
> 
> I am not sure if this work in case of nested vlans.
> Can we move skb-protocol assignment to parse_vlan() to avoid checking
> for non-accelerated vlan case again here?

I did this for the v3 that I sent out a moment ago.

Thanks for the review,

  Jarno