[PATCH net-next v2 0/3] openvswitch: make vlan handling consistent

[PATCH net-next v2 0/3] openvswitch: make vlan handling consistent

[Jiri Benc]

Always keep the first vlan tag "accelerated", i.e. in skb->vlan_tci. This
simplifies things considerably.

v2: added the first patch

Jiri Benc (3):
  openvswitch: normalize vlan rx path
  openvswitch: remove unreachable code in vlan parsing
  openvswitch: fix vlan subtraction from packet length

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

-- 
1.8.3.1

[PATCH net-next v2 1/3] openvswitch: normalize vlan rx path

[Jiri Benc]

Similarly to how the core networking stack behaves, let the first vlan tag
be always stored in skb->vlan_tci. This is already ensured in
__netif_receive_skb_core for packets that were received from the kernel and
honored by skb_vlan_push and skb_vlan_pop. The only remaining place are
packets received from the user space. Just do the same things with vlan
frames as __netif_receive_skb_core does.

Signed-off-by: Jiri Benc <jbenc@redhat.com>
---
 net/openvswitch/datapath.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 4d67ea856067..c47b3da8ecf2 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -594,6 +594,16 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)
 	else
 		packet->protocol = htons(ETH_P_802_2);
 
+	if (eth_type_vlan(packet->protocol)) {
+		__skb_pull(packet, ETH_HLEN);
+		skb_reset_network_header(packet);
+		skb_reset_mac_len(packet);
+		packet = skb_vlan_untag(packet);
+		if (unlikely(!packet))
+			goto err;
+		skb_push(packet, ETH_HLEN);
+	}
+
 	/* Set packet's mru */
 	if (a[OVS_PACKET_ATTR_MRU]) {
 		mru = nla_get_u16(a[OVS_PACKET_ATTR_MRU]);
-- 
1.8.3.1

[PATCH net-next v2 2/3] openvswitch: remove unreachable code in vlan parsing

[Jiri Benc]

Now when the first vlan tag is always in skb->vlan_tci, drop code that
assumed it might not be the case.

This patch also removes the wrong likely() statement around
skb_vlan_tag_present introduced by 018c1dda5ff1 ("openvswitch: 802.1AD Flow
handling, actions, vlan parsing, netlink attributes"). This code is called
whenever flow key is being extracted from the packet, the packet may be as
likely vlan tagged as not.

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

[PATCH net-next v2 3/3] 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.

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

Re: [PATCH net-next v2 1/3] openvswitch: normalize vlan rx path

[Eyal Birger]

Hi,

On Wed, Oct 5, 2016 at 4:07 PM, Jiri Benc <jbenc@redhat.com> wrote:
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index 4d67ea856067..c47b3da8ecf2 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -594,6 +594,16 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)
>         else
>                 packet->protocol = htons(ETH_P_802_2);
>
> +       if (eth_type_vlan(packet->protocol)) {
> +               __skb_pull(packet, ETH_HLEN);
> +               skb_reset_network_header(packet);
> +               skb_reset_mac_len(packet);
> +               packet = skb_vlan_untag(packet);
> +               if (unlikely(!packet))
> +                       goto err;

I think at this point, 'eth' may point to a freed packet.
Maybe replace the 'eth' variable with a 'proto' variable caching the
value of eth_hdr(packet)->h_proto?

> +               skb_push(packet, ETH_HLEN);
> +       }
> +
>         /* Set packet's mru */
>         if (a[OVS_PACKET_ATTR_MRU]) {
>                 mru = nla_get_u16(a[OVS_PACKET_ATTR_MRU]);
> --
> 1.8.3.1
>

Eyal.

Re: [PATCH net-next v2 1/3] openvswitch: normalize vlan rx path

[Jiri Benc]

On Wed, 5 Oct 2016 17:18:08 +0300, Eyal Birger wrote:
> I think at this point, 'eth' may point to a freed packet.

It may but how does that matter? eth is not used beyond that point.

 Jiri

Re: [PATCH net-next v2 1/3] openvswitch: normalize vlan rx path

[Eyal Birger]

On Wed, Oct 5, 2016 at 8:23 PM, Jiri Benc <jbenc@redhat.com> wrote:
> On Wed, 5 Oct 2016 17:18:08 +0300, Eyal Birger wrote:
>> I think at this point, 'eth' may point to a freed packet.
>
> It may but how does that matter? eth is not used beyond that point.

Definitely a nit. For sure not critical.

Just seemed less future safe to keep a pointer to an old packet lying around.

Eyal.

Re: [PATCH net-next v2 1/3] openvswitch: normalize vlan rx path

[Eric Garver]

On Wed, Oct 05, 2016 at 08:31:52PM +0300, Eyal Birger wrote:
> On Wed, Oct 5, 2016 at 8:23 PM, Jiri Benc <jbenc@redhat.com> wrote:
> > On Wed, 5 Oct 2016 17:18:08 +0300, Eyal Birger wrote:
> >> I think at this point, 'eth' may point to a freed packet.
> >
> > It may but how does that matter? eth is not used beyond that point.
> 
> Definitely a nit. For sure not critical.
> 
> Just seemed less future safe to keep a pointer to an old packet lying around.

I agree. Alternatively refresh the eth pointer.

Re: [PATCH net-next v2 1/3] openvswitch: normalize vlan rx path

[Jiri Benc]

On Wed, 5 Oct 2016 14:44:26 -0400, Eric Garver wrote:
> On Wed, Oct 05, 2016 at 08:31:52PM +0300, Eyal Birger wrote:
> > Just seemed less future safe to keep a pointer to an old packet lying around.
> 
> I agree. Alternatively refresh the eth pointer.

Sorry guys, that just doesn't make sense. Everyone should know that
reloading of skb pointer means the former pointers to its data may
become invalid. Please point me to any place in the kernel where we
reload the data pointer "just because" even when not used.

 Jiri

Re: [PATCH net-next v2 1/3] openvswitch: normalize vlan rx path

[Eric Garver]

On Wed, Oct 05, 2016 at 09:07:09PM +0200, Jiri Benc wrote:
> On Wed, 5 Oct 2016 14:44:26 -0400, Eric Garver wrote:
> > On Wed, Oct 05, 2016 at 08:31:52PM +0300, Eyal Birger wrote:
> > > Just seemed less future safe to keep a pointer to an old packet lying around.
> > 
> > I agree. Alternatively refresh the eth pointer.
> 
> Sorry guys, that just doesn't make sense. Everyone should know that
> reloading of skb pointer means the former pointers to its data may
> become invalid. Please point me to any place in the kernel where we
> reload the data pointer "just because" even when not used.
> 

How about this incremental change?


diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 7ef02752d4ba..0dd36f353c53 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -562,7 +562,6 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)
 	struct sw_flow *flow;
 	struct sw_flow_actions *sf_acts;
 	struct datapath *dp;
-	struct ethhdr *eth;
 	struct vport *input_vport;
 	u16 mru = 0;
 	int len;
@@ -584,14 +583,12 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)
 	nla_memcpy(__skb_put(packet, len), a[OVS_PACKET_ATTR_PACKET], len);
 
 	skb_reset_mac_header(packet);
-	eth = eth_hdr(packet);
 
 	/* Normally, setting the skb 'protocol' field would be handled by a
 	 * call to eth_type_trans(), but it assumes there's a sending
 	 * device, which we may not have. */
-	if (eth_proto_is_802_3(eth->h_proto))
-		packet->protocol = eth->h_proto;
-	else
+	packet->protocol = eth_hdr(packet)->h_proto;
+	if (!eth_proto_is_802_3(packet->protocol))
 		packet->protocol = htons(ETH_P_802_2);
 
 	if (eth_type_vlan(packet->protocol)) {

Re: [PATCH net-next v2 1/3] openvswitch: normalize vlan rx path

[Jiri Benc]

On Wed, 5 Oct 2016 15:21:32 -0400, Eric Garver wrote:
> How about this incremental change?

Feel free to submit it as a standalone patch. It has nothing to do with
this patchset. In this particular regard, the code is identical before
and after the patchset and is in no way altered by this patch.

 Jiri

Re: [PATCH net-next v2 2/3] openvswitch: remove unreachable code in vlan parsing

[Pravin Shelar]

On Wed, Oct 5, 2016 at 6:07 AM, Jiri Benc <jbenc@redhat.com> wrote:
> Now when the first vlan tag is always in skb->vlan_tci, drop code that
> assumed it might not be the case.
>
User can turn off TX vlan offload for OVS internal device that would
allow vlan tagged packet with vlan header on the skb-data. This case
will cause issue here.

We could handle this case by not allowing this configuration.

> This patch also removes the wrong likely() statement around
> skb_vlan_tag_present introduced by 018c1dda5ff1 ("openvswitch: 802.1AD Flow
> handling, actions, vlan parsing, netlink attributes"). This code is called
> whenever flow key is being extracted from the packet, the packet may be as
> likely vlan tagged as not.
>
> Signed-off-by: Jiri Benc <jbenc@redhat.com>

Re: [PATCH net-next v2 2/3] openvswitch: remove unreachable code in vlan parsing

[Jiri Benc]

On Wed, 5 Oct 2016 22:22:13 -0700, Pravin Shelar wrote:
> User can turn off TX vlan offload for OVS internal device that would
> allow vlan tagged packet with vlan header on the skb-data. This case
> will cause issue here.

Good catch. This is the feedback I hoped for, not the bikesheding about
a value of unused variable :-)

> We could handle this case by not allowing this configuration.

I'm not sure how clean this is but let's try and see if anyone objects.
I'll send v3.

I also noticed we don't set NETIF_F_HW_VLAN_STAG_TX on internal ports.
I'll fix it, too.

Thanks!

 Jiri