[PATCH net-next v2 0/3] openvswitch: mpls fix and clean up

[PATCH net-next v2 0/3] openvswitch: mpls fix and clean up

[Jiri Benc]

Convert to the new mpls skb layout the last remaining place in openvswitch,
forgotten on the mpls GSO rework. The GSO rework also allows for some
cleanup in the third patch.

Jiri Benc (3):
  openvswitch: mpls: set network header correctly on key extract
  mpls: move mpls_hdr to a common location
  openvswitch: use mpls_hdr

 include/net/mpls.h        | 15 ++++++---------
 net/mpls/internal.h       | 10 +---------
 net/openvswitch/actions.c | 24 ++++++++++++------------
 net/openvswitch/flow.c    | 11 +++--------
 4 files changed, 22 insertions(+), 38 deletions(-)

-- 
1.8.3.1

[PATCH net-next v2 1/3] openvswitch: mpls: set network header correctly on key extract

[Jiri Benc]

After the 48d2ab609b6b ("net: mpls: Fixups for GSO"), MPLS handling in
openvswitch was changed to have network header pointing to the start of the
MPLS headers and inner_network_header pointing after the MPLS headers.

However, key_extract was missed by the mentioned commit, causing incorrect
headers to be set when a MPLS packet just enters the bridge or after it is
recirculated.

Fixes: 48d2ab609b6b ("net: mpls: Fixups for GSO")
Signed-off-by: Jiri Benc <jbenc@redhat.com>
Acked-by: Pravin B Shelar <pshelar@ovn.org>
---
v2: no changes
---
 net/openvswitch/flow.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index 634cc10d6dee..c8c82e109c68 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -633,12 +633,7 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
 	} else if (eth_p_mpls(key->eth.type)) {
 		size_t stack_len = MPLS_HLEN;
 
-		/* In the presence of an MPLS label stack the end of the L2
-		 * header and the beginning of the L3 header differ.
-		 *
-		 * Advance network_header to the beginning of the L3
-		 * header. mac_len corresponds to the end of the L2 header.
-		 */
+		skb_set_inner_network_header(skb, skb->mac_len);
 		while (1) {
 			__be32 lse;
 
@@ -646,12 +641,12 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
 			if (unlikely(error))
 				return 0;
 
-			memcpy(&lse, skb_network_header(skb), MPLS_HLEN);
+			memcpy(&lse, skb_inner_network_header(skb), MPLS_HLEN);
 
 			if (stack_len == MPLS_HLEN)
 				memcpy(&key->mpls.top_lse, &lse, MPLS_HLEN);
 
-			skb_set_network_header(skb, skb->mac_len + stack_len);
+			skb_set_inner_network_header(skb, skb->mac_len + stack_len);
 			if (lse & htonl(MPLS_LS_S_MASK))
 				break;
 
-- 
1.8.3.1

[PATCH net-next v2 2/3] mpls: move mpls_hdr to a common location

[Jiri Benc]

This will be also used by openvswitch.

Signed-off-by: Jiri Benc <jbenc@redhat.com>
---
v2: new in v2
---
 include/net/mpls.h  |  9 +++++++++
 net/mpls/internal.h | 10 +---------
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/include/net/mpls.h b/include/net/mpls.h
index 5b3b5addfb08..3ebbc0bb57ff 100644
--- a/include/net/mpls.h
+++ b/include/net/mpls.h
@@ -19,12 +19,21 @@
 
 #define MPLS_HLEN 4
 
+struct mpls_shim_hdr {
+	__be32 label_stack_entry;
+};
+
 static inline bool eth_p_mpls(__be16 eth_type)
 {
 	return eth_type == htons(ETH_P_MPLS_UC) ||
 		eth_type == htons(ETH_P_MPLS_MC);
 }
 
+static inline struct mpls_shim_hdr *mpls_hdr(const struct sk_buff *skb)
+{
+	return (struct mpls_shim_hdr *)skb_network_header(skb);
+}
+
 /*
  * For non-MPLS skbs this will correspond to the network header.
  * For MPLS skbs it will be before the network_header as the MPLS
diff --git a/net/mpls/internal.h b/net/mpls/internal.h
index 732a5c17e986..bdfef6c3271a 100644
--- a/net/mpls/internal.h
+++ b/net/mpls/internal.h
@@ -1,9 +1,6 @@
 #ifndef MPLS_INTERNAL_H
 #define MPLS_INTERNAL_H
-
-struct mpls_shim_hdr {
-	__be32 label_stack_entry;
-};
+#include <net/mpls.h>
 
 struct mpls_entry_decoded {
 	u32 label;
@@ -93,11 +90,6 @@ struct mpls_route { /* next hop label forwarding entry */
 
 #define endfor_nexthops(rt) }
 
-static inline struct mpls_shim_hdr *mpls_hdr(const struct sk_buff *skb)
-{
-	return (struct mpls_shim_hdr *)skb_network_header(skb);
-}
-
 static inline struct mpls_shim_hdr mpls_entry_encode(u32 label, unsigned ttl, unsigned tc, bool bos)
 {
 	struct mpls_shim_hdr result;
-- 
1.8.3.1

[PATCH net-next v2 3/3] openvswitch: use mpls_hdr

[Jiri Benc]

skb_mpls_header is equivalent to mpls_hdr now. Use the existing helper
instead.

Signed-off-by: Jiri Benc <jbenc@redhat.com>
---
v2: using the existing mpls_hdr instead of skb_network_header
---
 include/net/mpls.h        | 12 ------------
 net/openvswitch/actions.c | 24 ++++++++++++------------
 2 files changed, 12 insertions(+), 24 deletions(-)

diff --git a/include/net/mpls.h b/include/net/mpls.h
index 3ebbc0bb57ff..1dbc669b770e 100644
--- a/include/net/mpls.h
+++ b/include/net/mpls.h
@@ -33,16 +33,4 @@ static inline struct mpls_shim_hdr *mpls_hdr(const struct sk_buff *skb)
 {
 	return (struct mpls_shim_hdr *)skb_network_header(skb);
 }
-
-/*
- * For non-MPLS skbs this will correspond to the network header.
- * For MPLS skbs it will be before the network_header as the MPLS
- * label stack lies between the end of the mac header and the network
- * header. That is, for MPLS skbs the end of the mac header
- * is the top of the MPLS label stack.
- */
-static inline unsigned char *skb_mpls_header(struct sk_buff *skb)
-{
-	return skb_mac_header(skb) + skb->mac_len;
-}
 #endif
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 863e992dfbc0..4e03f64709bc 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -160,7 +160,7 @@ static void update_ethertype(struct sk_buff *skb, struct ethhdr *hdr,
 static int push_mpls(struct sk_buff *skb, struct sw_flow_key *key,
 		     const struct ovs_action_push_mpls *mpls)
 {
-	__be32 *new_mpls_lse;
+	struct mpls_shim_hdr *new_mpls_lse;
 
 	/* Networking stack do not allow simultaneous Tunnel and MPLS GSO. */
 	if (skb->encapsulation)
@@ -180,8 +180,8 @@ static int push_mpls(struct sk_buff *skb, struct sw_flow_key *key,
 	skb_reset_mac_header(skb);
 	skb_set_network_header(skb, skb->mac_len);
 
-	new_mpls_lse = (__be32 *)skb_mpls_header(skb);
-	*new_mpls_lse = mpls->mpls_lse;
+	new_mpls_lse = mpls_hdr(skb);
+	new_mpls_lse->label_stack_entry = mpls->mpls_lse;
 
 	skb_postpush_rcsum(skb, new_mpls_lse, MPLS_HLEN);
 
@@ -202,7 +202,7 @@ static int pop_mpls(struct sk_buff *skb, struct sw_flow_key *key,
 	if (unlikely(err))
 		return err;
 
-	skb_postpull_rcsum(skb, skb_mpls_header(skb), MPLS_HLEN);
+	skb_postpull_rcsum(skb, mpls_hdr(skb), MPLS_HLEN);
 
 	memmove(skb_mac_header(skb) + MPLS_HLEN, skb_mac_header(skb),
 		skb->mac_len);
@@ -211,10 +211,10 @@ static int pop_mpls(struct sk_buff *skb, struct sw_flow_key *key,
 	skb_reset_mac_header(skb);
 	skb_set_network_header(skb, skb->mac_len);
 
-	/* skb_mpls_header() is used to locate the ethertype
-	 * field correctly in the presence of VLAN tags.
+	/* mpls_hdr() is used to locate the ethertype field correctly in the
+	 * presence of VLAN tags.
 	 */
-	hdr = (struct ethhdr *)(skb_mpls_header(skb) - ETH_HLEN);
+	hdr = (struct ethhdr *)((void *)mpls_hdr(skb) - ETH_HLEN);
 	update_ethertype(skb, hdr, ethertype);
 	if (eth_p_mpls(skb->protocol))
 		skb->protocol = ethertype;
@@ -226,7 +226,7 @@ static int pop_mpls(struct sk_buff *skb, struct sw_flow_key *key,
 static int set_mpls(struct sk_buff *skb, struct sw_flow_key *flow_key,
 		    const __be32 *mpls_lse, const __be32 *mask)
 {
-	__be32 *stack;
+	struct mpls_shim_hdr *stack;
 	__be32 lse;
 	int err;
 
@@ -234,16 +234,16 @@ static int set_mpls(struct sk_buff *skb, struct sw_flow_key *flow_key,
 	if (unlikely(err))
 		return err;
 
-	stack = (__be32 *)skb_mpls_header(skb);
-	lse = OVS_MASKED(*stack, *mpls_lse, *mask);
+	stack = mpls_hdr(skb);
+	lse = OVS_MASKED(stack->label_stack_entry, *mpls_lse, *mask);
 	if (skb->ip_summed == CHECKSUM_COMPLETE) {
-		__be32 diff[] = { ~(*stack), lse };
+		__be32 diff[] = { ~(stack->label_stack_entry), lse };
 
 		skb->csum = ~csum_partial((char *)diff, sizeof(diff),
 					  ~skb->csum);
 	}
 
-	*stack = lse;
+	stack->label_stack_entry = lse;
 	flow_key->mpls.top_lse = lse;
 	return 0;
 }
-- 
1.8.3.1

Re: [PATCH net-next v2 2/3] mpls: move mpls_hdr to a common location

[David Ahern]

On 9/30/16 11:08 AM, Jiri Benc wrote:
> This will be also used by openvswitch.
> 
> Signed-off-by: Jiri Benc <jbenc@redhat.com>
> ---
> v2: new in v2
> ---
>  include/net/mpls.h  |  9 +++++++++
>  net/mpls/internal.h | 10 +---------
>  2 files changed, 10 insertions(+), 9 deletions(-)
> 

Acked-by: David Ahern <dsa@cumulusnetworks.com>

Re: [PATCH net-next v2 2/3] mpls: move mpls_hdr to a common location

[pravin shelar]

On Fri, Sep 30, 2016 at 10:08 AM, Jiri Benc <jbenc@redhat.com> wrote:
> This will be also used by openvswitch.
>
> Signed-off-by: Jiri Benc <jbenc@redhat.com>
> ---
> v2: new in v2
> ---
>  include/net/mpls.h  |  9 +++++++++
>  net/mpls/internal.h | 10 +---------
>  2 files changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/include/net/mpls.h b/include/net/mpls.h
> index 5b3b5addfb08..3ebbc0bb57ff 100644
> --- a/include/net/mpls.h
> +++ b/include/net/mpls.h
> @@ -19,12 +19,21 @@
>
>  #define MPLS_HLEN 4
>
> +struct mpls_shim_hdr {
> +       __be32 label_stack_entry;
> +};
> +
There is already struct mpls_label defined in uapi mpls header file.
Is there reason for not using it here?

Re: [PATCH net-next v2 2/3] mpls: move mpls_hdr to a common location

[Jiri Benc]

On Fri, 30 Sep 2016 10:43:42 -0700, pravin shelar wrote:
> There is already struct mpls_label defined in uapi mpls header file.
> Is there reason for not using it here?

I think this should be unified as well. But it's for a different patch
not related to this patchset. This patch just moves the existing
mpls_hdr and mpls_shim_hdr definition to a new place.

In general, I think the mpls code deserves a round of overall cleanups.
It should not block this patchset, though. It can be considered as a
step towards that goal.

 Jiri

Re: [PATCH net-next v2 2/3] mpls: move mpls_hdr to a common location

[pravin shelar]

On Fri, Sep 30, 2016 at 10:51 AM, Jiri Benc <jbenc@redhat.com> wrote:
> On Fri, 30 Sep 2016 10:43:42 -0700, pravin shelar wrote:
>> There is already struct mpls_label defined in uapi mpls header file.
>> Is there reason for not using it here?
>
> I think this should be unified as well. But it's for a different patch
> not related to this patchset. This patch just moves the existing
> mpls_hdr and mpls_shim_hdr definition to a new place.
>
> In general, I think the mpls code deserves a round of overall cleanups.
> It should not block this patchset, though. It can be considered as a
> step towards that goal.
>

Alright, I do not oppose this patch going in as it is.

Acked-by: Pravin B Shelar <pshelar@ovn.org>

Re: [PATCH net-next v2 3/3] openvswitch: use mpls_hdr

[pravin shelar]

On Fri, Sep 30, 2016 at 10:08 AM, Jiri Benc <jbenc@redhat.com> wrote:
> skb_mpls_header is equivalent to mpls_hdr now. Use the existing helper
> instead.
>
> Signed-off-by: Jiri Benc <jbenc@redhat.com>
Acked-by: Pravin B Shelar <pshelar@ovn.org>

Re: [PATCH net-next v2 0/3] openvswitch: mpls fix and clean up

[David Miller]

From: Jiri Benc <jbenc@redhat.com>
Date: Fri, 30 Sep 2016 19:08:04 +0200

> Convert to the new mpls skb layout the last remaining place in openvswitch,
> forgotten on the mpls GSO rework. The GSO rework also allows for some
> cleanup in the third patch.

Series applied, thanks.