netdev - Re: [PATCH net-next 1/2] openvswitch: remove nonreachable code in vlan parsing

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: <20161005145349.51509e00@griffin>
Date:   Wed, 5 Oct 2016 14:53:49 +0200
From:   Jiri Benc <jbenc@...hat.com>
To:     Eric Garver <e@...g.me>
Cc:     netdev@...r.kernel.org, pravin shelar <pshelar@....org>
Subject: Re: [PATCH net-next 1/2] openvswitch: remove nonreachable code in
 vlan parsing

On Tue, 4 Oct 2016 20:06:18 -0400, Eric Garver wrote:
> This code is also called for packets passed back down from userspace
> (after the flow key miss and upcall). So it does happen that we have a
> skb without skb->vlan_tci set.

That sucks. The vlan handling should be really consistent, Jiri Pirko
and others put great effort to unify it in the stack, we should make
use of that effort.

I'll respin this patchset and add a patch to make the vlan handling
consistent.

> See the chain:
> ovs_packet_cmd_execute()
>   ovs_flow_key_extract_userspace()
>     key_extract()
>       parse_vlan()
> 
> > Moreover, the likely() statement around skb_vlan_tag_present is bogus. This
> > code is called whenever flow key is being extracted from the packet. The
> > packet may be as likely vlan tagged as not.
> 
> I guess the unlikely scenario is the one I mention above.

But it's wrong. It's also called via

ovs_vport_receive()
  ovs_flow_key_extract()
    key_extract()
      parse_vlan()

and in that path, both cases (vlan vs. non-vlan) are equally possible.

 Jiri