From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <netdev-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:51062 "EHLO mx1.redhat.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1751171AbdLSUmn (ORCPT <rfc822;netdev@vger.kernel.org>);
        Tue, 19 Dec 2017 15:42:43 -0500
Date: Tue, 19 Dec 2017 15:42:47 -0500
From: Eric Garver <e@erig.me>
To: Jiri Benc <jbenc@redhat.com>
Cc: netdev@vger.kernel.org, ovs-dev@openvswitch.org
Subject: Re: [PATCH net] openvswitch: Fix pop_vlan action for double tagged
 frames
Message-ID: <20171219204247.GD25853@dev-rhel7>
References: <20171219185753.23012-1-e@erig.me>
 <20171219203929.12c1cb93@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20171219203929.12c1cb93@redhat.com>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

On Tue, Dec 19, 2017 at 08:39:29PM +0100, Jiri Benc wrote:
> On Tue, 19 Dec 2017 13:57:53 -0500, Eric Garver wrote:
> > --- a/net/openvswitch/flow.c
> > +++ b/net/openvswitch/flow.c
> > @@ -559,8 +559,9 @@ static int parse_nsh(struct sk_buff *skb, struct sw_flow_key *key)
> >   *      of a correct length, otherwise the same as skb->network_header.
> >   *      For other key->eth.type values it is left untouched.
> >   *
> > - *    - skb->protocol: the type of the data starting at skb->network_header.
> > - *      Equals to key->eth.type.
> > + *    - skb->protocol: For Ethernet, the ethertype or VLAN TPID.
> > + *      For non-Ethernet, the type of the data starting at skb->network_header
> > + *      (also equal to key->eth.type).
> >   */
> >  static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
> >  {
> > @@ -579,6 +580,7 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
> >  			return -EINVAL;
> >  
> >  		skb_reset_network_header(skb);
> > +		key->eth.type = skb->protocol;
> >  	} else {
> >  		eth = eth_hdr(skb);
> >  		ether_addr_copy(key->eth.src, eth->h_source);
> > @@ -592,15 +594,14 @@ 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;
> >  
> >  		skb_reset_network_header(skb);
> >  		__skb_push(skb, skb->data - skb_mac_header(skb));
> >  	}
> >  	skb_reset_mac_len(skb);
> > -	key->eth.type = skb->protocol;
> >  
> >  	/* Network layer. */
> >  	if (key->eth.type == htons(ETH_P_IP)) {
> 
> Unfortunately, this does not work. key_extract must set skb->protocol
> even for Ethernet frames that come from a mixed L2/L3 tunnel. Such
> packets will have key->mac_proto set to MAC_PROTO_ETHERNET and
> skb->protocol set to ETH_P_TEB (see key_extract_mac_proto). In
> key_extract, skb->protocol has to be correctly set to the dissected
> value.

AFAICS, it's always overridden to ETH_P_TEB on output by
ovs_vport_send() and that's the sole reason it works today.

For dissecting, the L2 case is currently setting skb->protocol to the
real ethertype (e.g. 0x800) not ETH_P_TEB. For RX from tunnel case it'll
indeed be ETH_P_TEB.

> 
> Which means that we have to check for the existence of inner vlan tag
> (by checking key->eth.cvlan.tci or, perhaps better, by returning it
> from parse_vlan) and set skb->protocol accordingly.
> 
>  Jiri