[PATCH net-next 0/2] openvswitch: vlan fixes

[PATCH net-next 0/2] openvswitch: vlan fixes

[Jiri Benc]

Fix a bug and inefficiency in the vlan handling code introduced by the
802.1AD set. Strictly speaking, both were present even before that set
but it made them more prominent.

Jiri Benc (2):
  openvswitch: remove nonreachable code in vlan parsing
  openvswitch: fix vlan subtraction from packet length

 net/openvswitch/flow.c  | 28 ++++++++--------------------
 net/openvswitch/vport.c | 10 +++-------
 2 files changed, 11 insertions(+), 27 deletions(-)

-- 
1.8.3.1

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

[Jiri Benc]

It can never happen that there's a vlan tag in the packet but not in
skb->vlan_tci. This is ensured in __netif_receive_skb_core and honored by
skb_vlan_push and skb_vlan_pop. The code dealing with such case is a dead
code.

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.

Fixes: 018c1dda5ff1 ("openvswitch: 802.1AD Flow handling, actions, vlan parsing, netlink attributes")
Signed-off-by: Jiri Benc <jbenc@redhat.com>
---
 net/openvswitch/flow.c | 28 ++++++++--------------------
 1 file changed, 8 insertions(+), 20 deletions(-)

diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index c8c82e109c68..d88c0a55b783 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -308,9 +308,7 @@ static bool icmp6hdr_ok(struct sk_buff *skb)
 
 /**
  * Parse vlan tag from vlan header.
- * Returns ERROR on memory error.
- * Returns 0 if it encounters a non-vlan or incomplete packet.
- * Returns 1 after successfully parsing vlan tag.
+ * Returns ERROR on memory error, 0 otherwise.
  */
 static int parse_vlan_tag(struct sk_buff *skb, struct vlan_head *key_vh)
 {
@@ -331,34 +329,24 @@ static int parse_vlan_tag(struct sk_buff *skb, struct vlan_head *key_vh)
 	key_vh->tpid = vh->tpid;
 
 	__skb_pull(skb, sizeof(struct vlan_head));
-	return 1;
+	return 0;
 }
 
 static int parse_vlan(struct sk_buff *skb, struct sw_flow_key *key)
 {
-	int res;
-
 	key->eth.vlan.tci = 0;
 	key->eth.vlan.tpid = 0;
 	key->eth.cvlan.tci = 0;
 	key->eth.cvlan.tpid = 0;
 
-	if (likely(skb_vlan_tag_present(skb))) {
-		key->eth.vlan.tci = htons(skb->vlan_tci);
-		key->eth.vlan.tpid = skb->vlan_proto;
-	} else {
-		/* Parse outer vlan tag in the non-accelerated case. */
-		res = parse_vlan_tag(skb, &key->eth.vlan);
-		if (res <= 0)
-			return res;
-	}
+	if (!skb_vlan_tag_present(skb))
+		return 0;
 
-	/* Parse inner vlan tag. */
-	res = parse_vlan_tag(skb, &key->eth.cvlan);
-	if (res <= 0)
-		return res;
+	key->eth.vlan.tci = htons(skb->vlan_tci);
+	key->eth.vlan.tpid = skb->vlan_proto;
 
-	return 0;
+	/* Parse inner vlan tag. */
+	return parse_vlan_tag(skb, &key->eth.cvlan);
 }
 
 static __be16 parse_ethertype(struct sk_buff *skb)
-- 
1.8.3.1

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

[Eric Garver]

Hi Jiri,

On Tue, Oct 04, 2016 at 02:30:01PM +0200, Jiri Benc wrote:
> It can never happen that there's a vlan tag in the packet but not in
> skb->vlan_tci. This is ensured in __netif_receive_skb_core and honored by
> skb_vlan_push and skb_vlan_pop. The code dealing with such case is a dead
> code.
>

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.

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.

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

[Jiri Benc]

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

[PATCH net-next 2/2] openvswitch: fix vlan subtraction from packet length

[Jiri Benc]

When the packet has its vlan tag in skb->vlan_tci, the length of the VLAN
header is not counted in skb->len. It doesn't make sense to subtract it.

In addition, to honor the comment below the code, the VLAN header length
should not be subtracted if there's a vlan tag in skb->vlan_tci. This leads
to the code simply subtracting the Ethernet header length.

Fixes: 018c1dda5ff1 ("openvswitch: 802.1AD Flow handling, actions, vlan parsing, netlink attributes")
Signed-off-by: Jiri Benc <jbenc@redhat.com>
---
 net/openvswitch/vport.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c
index 8f198437c724..210bafc09bd8 100644
--- a/net/openvswitch/vport.c
+++ b/net/openvswitch/vport.c
@@ -483,17 +483,13 @@ EXPORT_SYMBOL_GPL(ovs_vport_deferred_free);
 
 static unsigned int packet_length(const struct sk_buff *skb)
 {
-	unsigned int length = skb->len - ETH_HLEN;
-
-	if (skb_vlan_tagged(skb))
-		length -= VLAN_HLEN;
-
 	/* Don't subtract for multiple VLAN tags. Most (all?) drivers allow
 	 * (ETH_LEN + VLAN_HLEN) in addition to the mtu value, but almost none
 	 * account for 802.1ad. e.g. is_skb_forwardable().
+	 * Note that the first VLAN tag is always in skb->vlan_tci, thus not
+	 * accounted for in skb->len.
 	 */
-
-	return length;
+	return skb->len - ETH_HLEN;
 }
 
 void ovs_vport_send(struct vport *vport, struct sk_buff *skb)
-- 
1.8.3.1