On Wed, Nov 30, 2016 at 6:30 AM, Jiri Benc <jb...@redhat.com> wrote: > On Tue, 29 Nov 2016 15:30:53 -0800, Jarno Rajahalme wrote: >> Do not always set skb->protocol to be the ethertype of the L3 header. >> For a packet with non-accelerated VLAN tags skb->protocol needs to be >> the ethertype of the outermost non-accelerated VLAN ethertype. > > Well, the current handling of skb->protocol matches what used to be the > handling of the kernel net stack before Jiri Pirko cleaned up the vlan > code. > > I'm not opposed to changing this but I'm afraid it needs much deeper > review. Because with this in place, no core kernel functions that > depend on skb->protocol may be called from within openvswitch. > Can you give specific example where it does not work?
>> @@ -361,6 +362,11 @@ static int parse_vlan(struct sk_buff *skb, struct >> sw_flow_key *key) >> if (res <= 0) >> return res; >> >> + /* If the outer vlan tag was accelerated, skb->protocol should >> + * refelect the inner vlan type. */ >> + if (!eth_type_vlan(skb->protocol)) >> + skb->protocol = key->eth.cvlan.tpid; > > This should not depend on the current value in skb->protocol which > could be arbitrary at this point (from the point of view of how this > patch understands the skb->protocol values). It's easy to fix, though - > just add a local bool variable tracking whether the skb->protocol has > been set. > skb-protocol value is set by the caller, so it should not be arbitrary. is it missing in any case?