[PATCH net-next] openvswitch: correctly fragment packet with mpls headers

[PATCH net-next] openvswitch: correctly fragment packet with mpls headers

[Jiri Benc]

If mpls headers were pushed to a defragmented packet, the refragmentation no
longer works correctly after 48d2ab609b6b ("net: mpls: Fixups for GSO"). The
network header has to be shifted after the mpls headers for the
fragmentation and restored afterwards.

Fixes: 48d2ab609b6b ("net: mpls: Fixups for GSO")
Signed-off-by: Jiri Benc <jbenc@redhat.com>
---
 net/openvswitch/actions.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 4e03f64709bc..370b2ba3df4c 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -62,7 +62,8 @@ struct ovs_frag_data {
 	struct vport *vport;
 	struct ovs_skb_cb cb;
 	__be16 inner_protocol;
-	__u16 vlan_tci;
+	u16 network_offset;	/* valid only if inner_protocol is set */
+	u16 vlan_tci;
 	__be16 vlan_proto;
 	unsigned int l2_len;
 	u8 l2_data[MAX_L2_LEN];
@@ -656,7 +657,6 @@ static int ovs_vport_output(struct net *net, struct sock *sk, struct sk_buff *sk
 
 	__skb_dst_copy(skb, data->dst);
 	*OVS_CB(skb) = data->cb;
-	skb->inner_protocol = data->inner_protocol;
 	skb->vlan_tci = data->vlan_tci;
 	skb->vlan_proto = data->vlan_proto;
 
@@ -666,6 +666,13 @@ static int ovs_vport_output(struct net *net, struct sock *sk, struct sk_buff *sk
 	skb_postpush_rcsum(skb, skb->data, data->l2_len);
 	skb_reset_mac_header(skb);
 
+	if (data->inner_protocol) {
+		skb->inner_protocol = data->inner_protocol;
+		skb->inner_network_header = skb->network_header;
+		skb_set_network_header(skb, data->network_offset);
+	}
+	skb_reset_mac_len(skb);
+
 	ovs_vport_send(vport, skb);
 	return 0;
 }
@@ -684,7 +691,8 @@ static struct dst_ops ovs_dst_ops = {
 /* prepare_frag() is called once per (larger-than-MTU) frame; its inverse is
  * ovs_vport_output(), which is called once per fragmented packet.
  */
-static void prepare_frag(struct vport *vport, struct sk_buff *skb)
+static void prepare_frag(struct vport *vport, struct sk_buff *skb,
+			 u16 orig_network_offset)
 {
 	unsigned int hlen = skb_network_offset(skb);
 	struct ovs_frag_data *data;
@@ -694,6 +702,7 @@ static void prepare_frag(struct vport *vport, struct sk_buff *skb)
 	data->vport = vport;
 	data->cb = *OVS_CB(skb);
 	data->inner_protocol = skb->inner_protocol;
+	data->network_offset = orig_network_offset;
 	data->vlan_tci = skb->vlan_tci;
 	data->vlan_proto = skb->vlan_proto;
 	data->l2_len = hlen;
@@ -706,6 +715,13 @@ static void prepare_frag(struct vport *vport, struct sk_buff *skb)
 static void ovs_fragment(struct net *net, struct vport *vport,
 			 struct sk_buff *skb, u16 mru, __be16 ethertype)
 {
+	u16 orig_network_offset = 0;
+
+	if (skb->inner_protocol) {
+		orig_network_offset = skb_network_offset(skb);
+		skb->network_header = skb->inner_network_header;
+	}
+
 	if (skb_network_offset(skb) > MAX_L2_LEN) {
 		OVS_NLERR(1, "L2 header too long to fragment");
 		goto err;
@@ -715,7 +731,7 @@ static void ovs_fragment(struct net *net, struct vport *vport,
 		struct dst_entry ovs_dst;
 		unsigned long orig_dst;
 
-		prepare_frag(vport, skb);
+		prepare_frag(vport, skb, orig_network_offset);
 		dst_init(&ovs_dst, &ovs_dst_ops, NULL, 1,
 			 DST_OBSOLETE_NONE, DST_NOCOUNT);
 		ovs_dst.dev = vport->dev;
@@ -735,7 +751,7 @@ static void ovs_fragment(struct net *net, struct vport *vport,
 			goto err;
 		}
 
-		prepare_frag(vport, skb);
+		prepare_frag(vport, skb, orig_network_offset);
 		memset(&ovs_rt, 0, sizeof(ovs_rt));
 		dst_init(&ovs_rt.dst, &ovs_dst_ops, NULL, 1,
 			 DST_OBSOLETE_NONE, DST_NOCOUNT);
-- 
1.8.3.1

Re: [PATCH net-next] openvswitch: correctly fragment packet with mpls headers

[Pravin Shelar]

On Mon, Oct 3, 2016 at 9:33 AM, Jiri Benc <jbenc@redhat.com> wrote:
> If mpls headers were pushed to a defragmented packet, the refragmentation no
> longer works correctly after 48d2ab609b6b ("net: mpls: Fixups for GSO"). The
> network header has to be shifted after the mpls headers for the
> fragmentation and restored afterwards.
>
> Fixes: 48d2ab609b6b ("net: mpls: Fixups for GSO")
> Signed-off-by: Jiri Benc <jbenc@redhat.com>
> ---
>  net/openvswitch/actions.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 4e03f64709bc..370b2ba3df4c 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -62,7 +62,8 @@ struct ovs_frag_data {
>         struct vport *vport;
>         struct ovs_skb_cb cb;
>         __be16 inner_protocol;
> -       __u16 vlan_tci;
> +       u16 network_offset;     /* valid only if inner_protocol is set */
> +       u16 vlan_tci;
>         __be16 vlan_proto;
>         unsigned int l2_len;
>         u8 l2_data[MAX_L2_LEN];
> @@ -656,7 +657,6 @@ static int ovs_vport_output(struct net *net, struct sock *sk, struct sk_buff *sk
>
>         __skb_dst_copy(skb, data->dst);
>         *OVS_CB(skb) = data->cb;
> -       skb->inner_protocol = data->inner_protocol;
>         skb->vlan_tci = data->vlan_tci;
>         skb->vlan_proto = data->vlan_proto;
>
> @@ -666,6 +666,13 @@ static int ovs_vport_output(struct net *net, struct sock *sk, struct sk_buff *sk
>         skb_postpush_rcsum(skb, skb->data, data->l2_len);
>         skb_reset_mac_header(skb);
>
> +       if (data->inner_protocol) {
> +               skb->inner_protocol = data->inner_protocol;
> +               skb->inner_network_header = skb->network_header;
> +               skb_set_network_header(skb, data->network_offset);
> +       }
> +       skb_reset_mac_len(skb);
> +
>         ovs_vport_send(vport, skb);
>         return 0;
>  }
> @@ -684,7 +691,8 @@ static struct dst_ops ovs_dst_ops = {
>  /* prepare_frag() is called once per (larger-than-MTU) frame; its inverse is
>   * ovs_vport_output(), which is called once per fragmented packet.
>   */
> -static void prepare_frag(struct vport *vport, struct sk_buff *skb)
> +static void prepare_frag(struct vport *vport, struct sk_buff *skb,
> +                        u16 orig_network_offset)
>  {
>         unsigned int hlen = skb_network_offset(skb);
>         struct ovs_frag_data *data;
> @@ -694,6 +702,7 @@ static void prepare_frag(struct vport *vport, struct sk_buff *skb)
>         data->vport = vport;
>         data->cb = *OVS_CB(skb);
>         data->inner_protocol = skb->inner_protocol;
> +       data->network_offset = orig_network_offset;
>         data->vlan_tci = skb->vlan_tci;
>         data->vlan_proto = skb->vlan_proto;
>         data->l2_len = hlen;
> @@ -706,6 +715,13 @@ static void prepare_frag(struct vport *vport, struct sk_buff *skb)
>  static void ovs_fragment(struct net *net, struct vport *vport,
>                          struct sk_buff *skb, u16 mru, __be16 ethertype)
>  {
> +       u16 orig_network_offset = 0;
> +
> +       if (skb->inner_protocol) {
> +               orig_network_offset = skb_network_offset(skb);
> +               skb->network_header = skb->inner_network_header;
> +       }
> +
This is not correct way to detect MPLS packet. inner_protocol can be
set by any tunnel device for using tunnel offloads. So this would
break the fragmentation for encapsulated packets. How about using
eth_p_mpls() as done in do-output()?

Re: [PATCH net-next] openvswitch: correctly fragment packet with mpls headers

[Jiri Benc]

On Mon, 3 Oct 2016 11:04:46 -0700, Pravin Shelar wrote:
> This is not correct way to detect MPLS packet. inner_protocol can be
> set by any tunnel device for using tunnel offloads. So this would
> break the fragmentation for encapsulated packets.

You're right, stupid me.

> How about using eth_p_mpls() as done in do-output()?

Will look into it.

 Jiri

Re: [PATCH net-next] openvswitch: correctly fragment packet with mpls headers

[Jiri Benc]

On Tue, 4 Oct 2016 10:24:58 +0200, Jiri Benc wrote:
> On Mon, 3 Oct 2016 11:04:46 -0700, Pravin Shelar wrote:
> > This is not correct way to detect MPLS packet. inner_protocol can be
> > set by any tunnel device for using tunnel offloads. So this would
> > break the fragmentation for encapsulated packets.
> 
> You're right, stupid me.

Actually, too little caffeine in the morning. I actually did consider
this and I believe my patch is correct. It doesn't matter what the
encapsulation is, we want to fragment the *inner* packet. And this is
exactly what this patch does.

Besides, the only case is MPLS anyway. I'm not aware of any code path
that could lead us to here, set inner protocol and not be MPLS. But
even if it is, it should work, provided the encapsulation header is
identical for all fragments and smaller than MAX_L2_LEN.

 Jiri

Re: [PATCH net-next] openvswitch: correctly fragment packet with mpls headers

[Pravin Shelar]

On Tue, Oct 4, 2016 at 2:28 AM, Jiri Benc <jbenc@redhat.com> wrote:
> On Tue, 4 Oct 2016 10:24:58 +0200, Jiri Benc wrote:
>> On Mon, 3 Oct 2016 11:04:46 -0700, Pravin Shelar wrote:
>> > This is not correct way to detect MPLS packet. inner_protocol can be
>> > set by any tunnel device for using tunnel offloads. So this would
>> > break the fragmentation for encapsulated packets.
>>
>> You're right, stupid me.
>
> Actually, too little caffeine in the morning. I actually did consider
> this and I believe my patch is correct. It doesn't matter what the
> encapsulation is, we want to fragment the *inner* packet. And this is
> exactly what this patch does.
>
> Besides, the only case is MPLS anyway. I'm not aware of any code path
> that could lead us to here, set inner protocol and not be MPLS. But
> even if it is, it should work, provided the encapsulation header is
> identical for all fragments and smaller than MAX_L2_LEN.
>

This code can be executed on encapsulated geneve or vxlan packets. So
in that case encapsulation header would not be same for all fragments.

Re: [PATCH net-next] openvswitch: correctly fragment packet with mpls headers

[Jiri Benc]

On Tue, 4 Oct 2016 09:53:25 -0700, Pravin Shelar wrote:
> This code can be executed on encapsulated geneve or vxlan packets.

How? The encapsulation header is in the form of metadata_dst at this
point and not present in the packet itself. Am I missing something?

If this patch is wrong, then the current push_mpls is wrong, too, it
does the same assumption.

 Jiri

Re: [PATCH net-next] openvswitch: correctly fragment packet with mpls headers

[Pravin Shelar]

On Tue, Oct 4, 2016 at 9:59 AM, Jiri Benc <jbenc@redhat.com> wrote:
> On Tue, 4 Oct 2016 09:53:25 -0700, Pravin Shelar wrote:
>> This code can be executed on encapsulated geneve or vxlan packets.
>
> How? The encapsulation header is in the form of metadata_dst at this
> point and not present in the packet itself. Am I missing something?
>

We could have encapsulated packet defragmented in physical bridge.
that mean the packet is entering OVS after egressing tunnel device.
That use case would break due to this patch.

> If this patch is wrong, then the current push_mpls is wrong, too, it
> does the same assumption.
>
I am not sure what you mean, can you explain?

Re: [PATCH net-next] openvswitch: correctly fragment packet with mpls headers

[Jiri Benc]

On Tue, 4 Oct 2016 19:03:46 -0700, Pravin Shelar wrote:
> We could have encapsulated packet defragmented in physical bridge.
> that mean the packet is entering OVS after egressing tunnel device.
> That use case would break due to this patch.

Okay, thanks for explanation. I missed this use case and it would
indeed break. And we can't clear existing inner headers when the frame
enters the bridge as it would break GSO.

Seems checking for the MPLS ethertype is indeed the only safe solution.

> > If this patch is wrong, then the current push_mpls is wrong, too, it
> > does the same assumption.
> >
> I am not sure what you mean, can you explain?

push_mpls() uses inner_proto as an indication whether this is the first
MPLS label or not. But it checks skb->encapsulation at the start, thus
it's safe (I expected this check to be done at the config time, not
runtime, and looked in the wrong place for it.)

I'll respin the patch.

Thanks for the patience with me,

 Jiri