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

[PATCH net-next 0/2] 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 nice
cleanup in the second patch.

Jiri Benc (2):
  openvswitch: mpls: set network header correctly on key extract
  openvswitch: remove skb_mpls_header

 include/net/mpls.h        | 11 -----------
 net/openvswitch/actions.c | 10 +++++-----
 net/openvswitch/flow.c    | 11 +++--------
 3 files changed, 8 insertions(+), 24 deletions(-)

-- 
1.8.3.1

[PATCH net-next 1/2] 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>
---
 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 2/2] openvswitch: remove skb_mpls_header

[Jiri Benc]

skb_mpls_header is equivalent to skb_network_header now. There's no reason
to keep it.

Signed-off-by: Jiri Benc <jbenc@redhat.com>
---
 include/net/mpls.h        | 11 -----------
 net/openvswitch/actions.c | 10 +++++-----
 2 files changed, 5 insertions(+), 16 deletions(-)

diff --git a/include/net/mpls.h b/include/net/mpls.h
index 5b3b5addfb08..fde22d0b0ec1 100644
--- a/include/net/mpls.h
+++ b/include/net/mpls.h
@@ -25,15 +25,4 @@ static inline bool eth_p_mpls(__be16 eth_type)
 		eth_type == htons(ETH_P_MPLS_MC);
 }
 
-/*
- * 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..bf4211f5c974 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -180,7 +180,7 @@ 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 = (__be32 *)skb_network_header(skb);
 	*new_mpls_lse = 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, skb_network_header(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
+	/* skb_network_header() 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 *)(skb_network_header(skb) - ETH_HLEN);
 	update_ethertype(skb, hdr, ethertype);
 	if (eth_p_mpls(skb->protocol))
 		skb->protocol = ethertype;
@@ -234,7 +234,7 @@ 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);
+	stack = (__be32 *)skb_network_header(skb);
 	lse = OVS_MASKED(*stack, *mpls_lse, *mask);
 	if (skb->ip_summed == CHECKSUM_COMPLETE) {
 		__be32 diff[] = { ~(*stack), lse };
-- 
1.8.3.1

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

[pravin shelar]

On Thu, Sep 29, 2016 at 12:19 PM, Jiri Benc <jbenc@redhat.com> wrote:
> 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>

Re: [PATCH net-next 2/2] openvswitch: remove skb_mpls_header

[pravin shelar]

On Thu, Sep 29, 2016 at 12:19 PM, Jiri Benc <jbenc@redhat.com> wrote:
> skb_mpls_header is equivalent to skb_network_header now. There's no reason
> to keep it.
>
> Signed-off-by: Jiri Benc <jbenc@redhat.com>
> ---
>  include/net/mpls.h        | 11 -----------
>  net/openvswitch/actions.c | 10 +++++-----
>  2 files changed, 5 insertions(+), 16 deletions(-)
>
> diff --git a/include/net/mpls.h b/include/net/mpls.h
> index 5b3b5addfb08..fde22d0b0ec1 100644
> --- a/include/net/mpls.h
> +++ b/include/net/mpls.h
> @@ -25,15 +25,4 @@ static inline bool eth_p_mpls(__be16 eth_type)
>                 eth_type == htons(ETH_P_MPLS_MC);
>  }
>
> -/*
> - * 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;
> -}

I think we should keep this API, so that it is clear that MPLS header
mapped skb network header.

Re: [PATCH net-next 2/2] openvswitch: remove skb_mpls_header

[Jiri Benc]

On Thu, 29 Sep 2016 16:03:19 -0700, pravin shelar wrote:
> > --- a/include/net/mpls.h
> > +++ b/include/net/mpls.h
> > @@ -25,15 +25,4 @@ static inline bool eth_p_mpls(__be16 eth_type)
> >                 eth_type == htons(ETH_P_MPLS_MC);
> >  }
> >
> > -/*
> > - * 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;
> > -}
> 
> I think we should keep this API, so that it is clear that MPLS header
> mapped skb network header.

I was pondering this but I don't think it really gains us anything.
Wrappers like ip_hdr() are useful as they do type conversion; it's much
nicer to write
	ip_hdr(skb)->daddr
than 
	(struct iphdr *)skb_network_header(skb)->daddr.

But we don't really have a good type to return from skb_mpls_header, it
boils down to be 100% equivalent to skb_network_header. In fact, you
could just do:
#define skb_mpls_header skb_network_header

In the code, I don't think there's much benefit from calling the
wrapper, meaning of skb_network_header itself is clear enough. It *is*
the network header, after all. In the whole openvswitch code, MPLS is
treated as L3 header - no dissection is performed after the MPLS
headers, the network header and mac_len is set as expected now, etc.

 Jiri

Re: [PATCH net-next 2/2] openvswitch: remove skb_mpls_header

[pravin shelar]

On Fri, Sep 30, 2016 at 1:06 AM, Jiri Benc <jbenc@redhat.com> wrote:
> On Thu, 29 Sep 2016 16:03:19 -0700, pravin shelar wrote:
>> > --- a/include/net/mpls.h
>> > +++ b/include/net/mpls.h
>> > @@ -25,15 +25,4 @@ static inline bool eth_p_mpls(__be16 eth_type)
>> >                 eth_type == htons(ETH_P_MPLS_MC);
>> >  }
>> >
>> > -/*
>> > - * 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;
>> > -}
>>
>> I think we should keep this API, so that it is clear that MPLS header
>> mapped skb network header.
>
> I was pondering this but I don't think it really gains us anything.
> Wrappers like ip_hdr() are useful as they do type conversion; it's much
> nicer to write
>         ip_hdr(skb)->daddr
> than
>         (struct iphdr *)skb_network_header(skb)->daddr.
>
> But we don't really have a good type to return from skb_mpls_header, it
> boils down to be 100% equivalent to skb_network_header. In fact, you
> could just do:
> #define skb_mpls_header skb_network_header
>
I am fine with this too.

> In the code, I don't think there's much benefit from calling the
> wrapper, meaning of skb_network_header itself is clear enough. It *is*
> the network header, after all. In the whole openvswitch code, MPLS is
> treated as L3 header - no dissection is performed after the MPLS
> headers, the network header and mac_len is set as expected now, etc.
>

It makes it easier to locate which modules are using this API. That
also helps if in future we decide to change MPLS header mapping again.