[PATCH net v2] openvswitch: Fix pop_vlan action for double tagged frames

[PATCH net v2] openvswitch: Fix pop_vlan action for double tagged frames

[Eric Garver]

skb_vlan_pop() expects skb->protocol to be a valid TPID for double
tagged frames, but skb->protocol is set to the ethertype by
key_extract(). So temporarily set it to the TPID when doing a pop_vlan.

Fixes: 5108bbaddc37 ("openvswitch: add processing of L3 packets")
Signed-off-by: Eric Garver <e@erig.me>
---
 net/openvswitch/actions.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 30a5df27116e..c484e0941047 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -280,6 +280,13 @@ static int set_mpls(struct sk_buff *skb, struct sw_flow_key *flow_key,
 static int pop_vlan(struct sk_buff *skb, struct sw_flow_key *key)
 {
 	int err;
+	__be16 proto = skb->protocol;
+
+	/* skb->protocol is set to the inner most parsed ethertype. To satisfy
+	 * skb_vlan_pop() for multi-tagged frames we must set it to the tpid.
+	 */
+	if (is_flow_key_valid(key) && key->eth.vlan.tci && key->eth.cvlan.tci)
+		skb->protocol = key->eth.cvlan.tpid;
 
 	err = skb_vlan_pop(skb);
 	if (skb_vlan_tag_present(skb)) {
@@ -288,6 +295,9 @@ static int pop_vlan(struct sk_buff *skb, struct sw_flow_key *key)
 		key->eth.vlan.tci = 0;
 		key->eth.vlan.tpid = 0;
 	}
+
+	skb->protocol = proto;
+
 	return err;
 }
 
-- 
2.12.0

Re: [PATCH net v2] openvswitch: Fix pop_vlan action for double tagged frames

[Jiri Benc]

On Wed, 20 Dec 2017 10:39:32 -0500, Eric Garver wrote:
> +	if (is_flow_key_valid(key) && key->eth.vlan.tci && key->eth.cvlan.tci)

Maybe (key->eth.vlan.tci & htons(VLAN_TAG_PRESENT)) for consistency
with the rest of the code? But it's just nitpicking.

The real problem here is when a double tagged packet leaves the ovs
bridge, it won't have the skb->protocol that the kernel expects: it
will be ethertype of the payload, while my understanding is it should
be the inner tpid, right?

This patch fixes that nicely for the pop vlan case. But what about
other cases? It seems to me that we need to add the logic to
key_extract.

Thanks!

 Jiri

Re: [PATCH net v2] openvswitch: Fix pop_vlan action for double tagged frames

[Eric Garver]

On Wed, Dec 20, 2017 at 06:41:17PM +0100, Jiri Benc wrote:
> On Wed, 20 Dec 2017 10:39:32 -0500, Eric Garver wrote:
> > +	if (is_flow_key_valid(key) && key->eth.vlan.tci && key->eth.cvlan.tci)
> 
> Maybe (key->eth.vlan.tci & htons(VLAN_TAG_PRESENT)) for consistency
> with the rest of the code? But it's just nitpicking.
> 
> The real problem here is when a double tagged packet leaves the ovs
> bridge, it won't have the skb->protocol that the kernel expects: it
> will be ethertype of the payload, while my understanding is it should
> be the inner tpid, right?

Right.

> 
> This patch fixes that nicely for the pop vlan case. But what about
> other cases? It seems to me that we need to add the logic to
> key_extract.
> 

The part I was missing is; before encap into the L3 tunnel all the VLAN
tags must be explicitly popped.

Setting skb->protocol to the TPID for double tagged frames means the pop
operations shift the payload ethertype into skb->protocol.

I'll send a v3 that does this is key_extract().

Re: [PATCH net v2] openvswitch: Fix pop_vlan action for double tagged frames

[Jiri Benc]

On Wed, 20 Dec 2017 10:39:32 -0500, Eric Garver wrote:
> +	if (is_flow_key_valid(key) && key->eth.vlan.tci && key->eth.cvlan.tci)

Maybe (key->eth.vlan.tci & htons(VLAN_TAG_PRESENT)) for consistency
with the rest of the code? But it's just nitpicking.

The real problem here is when a double tagged packet leaves the ovs
bridge, it won't have the skb->protocol that the kernel expects: it
will be ethertype of the payload, while my understanding is it should
be the inner tpid, right?

This patch fixes that nicely for the pop vlan case. But what about
other cases? It seems to me that we need to add the logic to
key_extract.

Thanks!

 Jiri