[PATCH net 2/3] udp_offload: Set encapsulation before inner completes.

[PATCH net 2/3] udp_offload: Set encapsulation before inner completes.

[Jarno Rajahalme]

UDP tunnel segmentation code relies on the inner offsets being set for
an UDP tunnel GSO packet.  The inner *_complete() functions will set
the inner offsets only if the encapsulation is set before calling
them, but udp_gro_complete() set it only after the inner *_complete()
functions were done.

Also, remove the setting of the inner_mac_header in udp_gro_complete()
as it was wrongly set to the beginning of the UDP tunnel header rather
than the inner MAC header.

Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
---
 net/ipv4/udp_offload.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index 0ed2daf..e330c0e 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -399,6 +399,11 @@ int udp_gro_complete(struct sk_buff *skb, int nhoff)
 
 	uh->len = newlen;
 
+	/* Set encapsulation before calling into inner gro_complete() functions
+	 * to make them set up the inner offsets.
+	 */
+	skb->encapsulation = 1;
+
 	rcu_read_lock();
 
 	uo_priv = rcu_dereference(udp_offload_base);
@@ -421,9 +426,6 @@ int udp_gro_complete(struct sk_buff *skb, int nhoff)
 	if (skb->remcsum_offload)
 		skb_shinfo(skb)->gso_type |= SKB_GSO_TUNNEL_REMCSUM;
 
-	skb->encapsulation = 1;
-	skb_set_inner_mac_header(skb, nhoff + sizeof(struct udphdr));
-
 	return err;
 }
 
-- 
2.7.4

Re: [PATCH net 2/3] udp_offload: Set encapsulation before inner completes.

[Dan Carpenter]

These patches look reasonable.  But can you update the changelogs for 2
and 3 to say the user visible effects of these bugs.

Resend it to the people from ./scripts/get_maintainer.pl -f net/ipv4/udp_offload.c

regards,
dan carpenter

Re: [PATCH net 2/3] udp_offload: Set encapsulation before inner completes.

[Jarno Rajahalme]

> On Apr 30, 2016, at 12:32 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> These patches look reasonable.  But can you update the changelogs for 2
> and 3 to say the user visible effects of these bugs.
> 
> Resend it to the people from ./scripts/get_maintainer.pl -f net/ipv4/udp_offload.c
> 
> regards,
> dan carpenter
> 

Thanks, I send a v2 with a better commit message to the email addresses returned by the get_maintainer script.

   Jarno

[PATCH net 1/3] udp_tunnel: Remove redundant udp_tunnel_gro_complete().

[Jarno Rajahalme]

The setting of the UDP tunnel GSO type is already performed by
udp[46]_gro_complete().

Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
---
 drivers/net/geneve.c     | 2 --
 drivers/net/vxlan.c      | 2 --
 include/net/udp_tunnel.h | 9 ---------
 net/ipv4/fou.c           | 2 --
 4 files changed, 15 deletions(-)

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index bc16889..98f1224 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -504,8 +504,6 @@ static int geneve_gro_complete(struct sk_buff *skb, int nhoff,
 	int gh_len;
 	int err = -ENOSYS;
 
-	udp_tunnel_gro_complete(skb, nhoff);
-
 	gh = (struct genevehdr *)(skb->data + nhoff);
 	gh_len = geneve_hlen(gh);
 	type = gh->proto_type;
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index 1c0fa36..dd2d032 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -616,8 +616,6 @@ out:
 static int vxlan_gro_complete(struct sk_buff *skb, int nhoff,
 			      struct udp_offload *uoff)
 {
-	udp_tunnel_gro_complete(skb, nhoff);
-
 	return eth_gro_complete(skb, nhoff + sizeof(struct vxlanhdr));
 }
 
diff --git a/include/net/udp_tunnel.h b/include/net/udp_tunnel.h
index b831140..a114024 100644
--- a/include/net/udp_tunnel.h
+++ b/include/net/udp_tunnel.h
@@ -106,15 +106,6 @@ static inline struct sk_buff *udp_tunnel_handle_offloads(struct sk_buff *skb,
 	return iptunnel_handle_offloads(skb, type);
 }
 
-static inline void udp_tunnel_gro_complete(struct sk_buff *skb, int nhoff)
-{
-	struct udphdr *uh;
-
-	uh = (struct udphdr *)(skb->data + nhoff - sizeof(struct udphdr));
-	skb_shinfo(skb)->gso_type |= uh->check ?
-				SKB_GSO_UDP_TUNNEL_CSUM : SKB_GSO_UDP_TUNNEL;
-}
-
 static inline void udp_tunnel_encap_enable(struct socket *sock)
 {
 #if IS_ENABLED(CONFIG_IPV6)
diff --git a/net/ipv4/fou.c b/net/ipv4/fou.c
index a39068b..305d9ac 100644
--- a/net/ipv4/fou.c
+++ b/net/ipv4/fou.c
@@ -228,8 +228,6 @@ static int fou_gro_complete(struct sk_buff *skb, int nhoff,
 	int err = -ENOSYS;
 	const struct net_offload **offloads;
 
-	udp_tunnel_gro_complete(skb, nhoff);
-
 	rcu_read_lock();
 	offloads = NAPI_GRO_CB(skb)->is_ipv6 ? inet6_offloads : inet_offloads;
 	ops = rcu_dereference(offloads[proto]);
-- 
2.7.4

[PATCH net 2/3] udp_offload: Set encapsulation before inner completes.

[Jarno Rajahalme]

UDP tunnel segmentation code relies on the inner offsets being set for
an UDP tunnel GSO packet.  The inner *_complete() functions will set
the inner offsets only if the encapsulation is set before calling
them, but udp_gro_complete() set it only after the inner *_complete()
functions were done.

Also, remove the setting of the inner_mac_header in udp_gro_complete()
as it was wrongly set to the beginning of the UDP tunnel header rather
than the inner MAC header.

Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
---
 net/ipv4/udp_offload.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index 0ed2daf..e330c0e 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -399,6 +399,11 @@ int udp_gro_complete(struct sk_buff *skb, int nhoff)
 
 	uh->len = newlen;
 
+	/* Set encapsulation before calling into inner gro_complete() functions
+	 * to make them set up the inner offsets.
+	 */
+	skb->encapsulation = 1;
+
 	rcu_read_lock();
 
 	uo_priv = rcu_dereference(udp_offload_base);
@@ -421,9 +426,6 @@ int udp_gro_complete(struct sk_buff *skb, int nhoff)
 	if (skb->remcsum_offload)
 		skb_shinfo(skb)->gso_type |= SKB_GSO_TUNNEL_REMCSUM;
 
-	skb->encapsulation = 1;
-	skb_set_inner_mac_header(skb, nhoff + sizeof(struct udphdr));
-
 	return err;
 }
 
-- 
2.7.4

Re: [PATCH net 2/3] udp_offload: Set encapsulation before inner completes.

[Tom Herbert]

On Fri, Apr 29, 2016 at 3:28 PM, Jarno Rajahalme <jarno@ovn.org> wrote:
> UDP tunnel segmentation code relies on the inner offsets being set for
> an UDP tunnel GSO packet.  The inner *_complete() functions will set
> the inner offsets only if the encapsulation is set before calling
> them, but udp_gro_complete() set it only after the inner *_complete()
> functions were done.
>
> Also, remove the setting of the inner_mac_header in udp_gro_complete()
> as it was wrongly set to the beginning of the UDP tunnel header rather
> than the inner MAC header.
>
How did you test this? Do you have a test case for the problem?

The general problem is that skb->encapsulation is the indicator that
the inner offsets are valid, but there are many instances where we set
skb->encapsulation independently of setting the inner headers. It
would be nice if setting the flag and setting the inner headers were
somehow unified.

Thanks,
Tom

> Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
> ---
>  net/ipv4/udp_offload.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index 0ed2daf..e330c0e 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -399,6 +399,11 @@ int udp_gro_complete(struct sk_buff *skb, int nhoff)
>
>         uh->len = newlen;
>
> +       /* Set encapsulation before calling into inner gro_complete() functions
> +        * to make them set up the inner offsets.
> +        */
> +       skb->encapsulation = 1;
> +
>         rcu_read_lock();
>
>         uo_priv = rcu_dereference(udp_offload_base);
> @@ -421,9 +426,6 @@ int udp_gro_complete(struct sk_buff *skb, int nhoff)
>         if (skb->remcsum_offload)
>                 skb_shinfo(skb)->gso_type |= SKB_GSO_TUNNEL_REMCSUM;
>
> -       skb->encapsulation = 1;
> -       skb_set_inner_mac_header(skb, nhoff + sizeof(struct udphdr));
> -
>         return err;
>  }
>
> --
> 2.7.4
>

Re: [PATCH net 2/3] udp_offload: Set encapsulation before inner completes.

[Jarno Rajahalme]

> On Apr 29, 2016, at 6:42 PM, Tom Herbert <tom@herbertland.com> wrote:
> 
> On Fri, Apr 29, 2016 at 3:28 PM, Jarno Rajahalme <jarno@ovn.org> wrote:
>> UDP tunnel segmentation code relies on the inner offsets being set for
>> an UDP tunnel GSO packet.  The inner *_complete() functions will set
>> the inner offsets only if the encapsulation is set before calling
>> them, but udp_gro_complete() set it only after the inner *_complete()
>> functions were done.
>> 
>> Also, remove the setting of the inner_mac_header in udp_gro_complete()
>> as it was wrongly set to the beginning of the UDP tunnel header rather
>> than the inner MAC header.
>> 
> How did you test this? Do you have a test case for the problem?
> 

I faced this problem when developing UDP tunnel offloads for virtio_net. Basically the GRO of encapsulated packets on the rx side should produce the same inner offsets that the original encapsulation did on the tx side. __skb_udp_tunnel_segment() has this line:

        int tnl_hlen = skb_inner_mac_header(skb) - skb_transport_header(skb);

which is intended to capture both the UDP header and the tunneling protocol header (e.g., UDP + variable length GENEVE header). So, if the inner MAC header is incorrectly set the segmentation would not work as expected.

> The general problem is that skb->encapsulation is the indicator that
> the inner offsets are valid, but there are many instances where we set
> skb->encapsulation independently of setting the inner headers. It
> would be nice if setting the flag and setting the inner headers were
> somehow unified.
> 

Agree, it was rather tedious to trace the call flow to figure out what is going on.

  Jarno

> Thanks,
> Tom
> 
>> Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
>> ---
>> net/ipv4/udp_offload.c | 8 +++++---
>> 1 file changed, 5 insertions(+), 3 deletions(-)
>> 
>> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
>> index 0ed2daf..e330c0e 100644
>> --- a/net/ipv4/udp_offload.c
>> +++ b/net/ipv4/udp_offload.c
>> @@ -399,6 +399,11 @@ int udp_gro_complete(struct sk_buff *skb, int nhoff)
>> 
>>        uh->len = newlen;
>> 
>> +       /* Set encapsulation before calling into inner gro_complete() functions
>> +        * to make them set up the inner offsets.
>> +        */
>> +       skb->encapsulation = 1;
>> +
>>        rcu_read_lock();
>> 
>>        uo_priv = rcu_dereference(udp_offload_base);
>> @@ -421,9 +426,6 @@ int udp_gro_complete(struct sk_buff *skb, int nhoff)
>>        if (skb->remcsum_offload)
>>                skb_shinfo(skb)->gso_type |= SKB_GSO_TUNNEL_REMCSUM;
>> 
>> -       skb->encapsulation = 1;
>> -       skb_set_inner_mac_header(skb, nhoff + sizeof(struct udphdr));
>> -
>>        return err;
>> }
>> 
>> --
>> 2.7.4
>> 

Re: [PATCH net 2/3] udp_offload: Set encapsulation before inner completes.

[Alexander Duyck]

On Fri, Apr 29, 2016 at 3:28 PM, Jarno Rajahalme <jarno@ovn.org> wrote:
> UDP tunnel segmentation code relies on the inner offsets being set for
> an UDP tunnel GSO packet.  The inner *_complete() functions will set
> the inner offsets only if the encapsulation is set before calling
> them, but udp_gro_complete() set it only after the inner *_complete()
> functions were done.
>
> Also, remove the setting of the inner_mac_header in udp_gro_complete()
> as it was wrongly set to the beginning of the UDP tunnel header rather
> than the inner MAC header.
>
> Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
> ---
>  net/ipv4/udp_offload.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index 0ed2daf..e330c0e 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -399,6 +399,11 @@ int udp_gro_complete(struct sk_buff *skb, int nhoff)
>
>         uh->len = newlen;
>
> +       /* Set encapsulation before calling into inner gro_complete() functions
> +        * to make them set up the inner offsets.
> +        */
> +       skb->encapsulation = 1;
> +
>         rcu_read_lock();
>
>         uo_priv = rcu_dereference(udp_offload_base);
> @@ -421,9 +426,6 @@ int udp_gro_complete(struct sk_buff *skb, int nhoff)
>         if (skb->remcsum_offload)
>                 skb_shinfo(skb)->gso_type |= SKB_GSO_TUNNEL_REMCSUM;
>
> -       skb->encapsulation = 1;
> -       skb_set_inner_mac_header(skb, nhoff + sizeof(struct udphdr));
> -
>         return err;
>  }
>
> --
> 2.7.4
>

So looking over this I think this patch is just a band-aid and it
isn't really fixing much of anything.  For example I cannot see where
the inner transport offset is ever set.  From what I can tell it never
is.

What we probably need to do is the same thing we currently do in the
transmit path itself.  We need to record all the headers on the way up
so that we end up with the network and transport headers matching the
inner headers, then when we complete the inner-most tunnel we set the
inner headers and set skb->encapsulation.  Then on the way down we
start overwriting the outer header offsets when skb->encapsulation is
set.  That way we don't have to worry about screwing things up for
headers like GRE/GUE because the GRE can write the inner header
offsets, set skb->encapsulation, and then we only overwrite the outer
headers all the way down because with skb->encapsulation set we know
we are only dealing with outer headers because the inner header values
have been written.

- Alex

Re: [PATCH net 2/3] udp_offload: Set encapsulation before inner completes.

[Jarno Rajahalme]

> On Apr 29, 2016, at 8:46 PM, Alexander Duyck <alexander.duyck@gmail.com> wrote:
> 
> On Fri, Apr 29, 2016 at 3:28 PM, Jarno Rajahalme <jarno@ovn.org> wrote:
>> UDP tunnel segmentation code relies on the inner offsets being set for
>> an UDP tunnel GSO packet.  The inner *_complete() functions will set
>> the inner offsets only if the encapsulation is set before calling
>> them, but udp_gro_complete() set it only after the inner *_complete()
>> functions were done.
>> 
>> Also, remove the setting of the inner_mac_header in udp_gro_complete()
>> as it was wrongly set to the beginning of the UDP tunnel header rather
>> than the inner MAC header.
>> 
>> Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
>> ---
>> net/ipv4/udp_offload.c | 8 +++++---
>> 1 file changed, 5 insertions(+), 3 deletions(-)
>> 
>> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
>> index 0ed2daf..e330c0e 100644
>> --- a/net/ipv4/udp_offload.c
>> +++ b/net/ipv4/udp_offload.c
>> @@ -399,6 +399,11 @@ int udp_gro_complete(struct sk_buff *skb, int nhoff)
>> 
>>        uh->len = newlen;
>> 
>> +       /* Set encapsulation before calling into inner gro_complete() functions
>> +        * to make them set up the inner offsets.
>> +        */
>> +       skb->encapsulation = 1;
>> +
>>        rcu_read_lock();
>> 
>>        uo_priv = rcu_dereference(udp_offload_base);
>> @@ -421,9 +426,6 @@ int udp_gro_complete(struct sk_buff *skb, int nhoff)
>>        if (skb->remcsum_offload)
>>                skb_shinfo(skb)->gso_type |= SKB_GSO_TUNNEL_REMCSUM;
>> 
>> -       skb->encapsulation = 1;
>> -       skb_set_inner_mac_header(skb, nhoff + sizeof(struct udphdr));
>> -
>>        return err;
>> }
>> 
>> --
>> 2.7.4
>> 
> 
> So looking over this I think this patch is just a band-aid and it
> isn't really fixing much of anything.  For example I cannot see where

It fixes the immediate bug/problem that segmentation of GRO UDP tunnel packet fails due to incorrectly set inner MAC offset and/or missing inner network offset.

> the inner transport offset is ever set.  From what I can tell it never
> is.
> 

inner transport offset is set in validate_xmit_skb(), but you are right, it is not set by the GRO complete code path. AFAIK no-one uses it before reaching validate_xmit_skb(), though. The documentation in (net-next) Documentation/networking/segmentation-offloads.txt could be fixed by removing the inner transport header from UDP/GRE tunnel case, as it is not used by either the GRE or UDP tunnel code.

  Jarno

> What we probably need to do is the same thing we currently do in the
> transmit path itself.  We need to record all the headers on the way up
> so that we end up with the network and transport headers matching the
> inner headers, then when we complete the inner-most tunnel we set the
> inner headers and set skb->encapsulation.  Then on the way down we
> start overwriting the outer header offsets when skb->encapsulation is
> set.  That way we don't have to worry about screwing things up for
> headers like GRE/GUE because the GRE can write the inner header
> offsets, set skb->encapsulation, and then we only overwrite the outer
> headers all the way down because with skb->encapsulation set we know
> we are only dealing with outer headers because the inner header values
> have been written.
> 
> - Alex

Re: [PATCH net 2/3] udp_offload: Set encapsulation before inner completes.

[Alexander Duyck]

On Mon, May 2, 2016 at 2:21 PM, Jarno Rajahalme <jarno@ovn.org> wrote:
>
>> On Apr 29, 2016, at 8:46 PM, Alexander Duyck <alexander.duyck@gmail.com> wrote:
>>
>> On Fri, Apr 29, 2016 at 3:28 PM, Jarno Rajahalme <jarno@ovn.org> wrote:
>>> UDP tunnel segmentation code relies on the inner offsets being set for
>>> an UDP tunnel GSO packet.  The inner *_complete() functions will set
>>> the inner offsets only if the encapsulation is set before calling
>>> them, but udp_gro_complete() set it only after the inner *_complete()
>>> functions were done.
>>>
>>> Also, remove the setting of the inner_mac_header in udp_gro_complete()
>>> as it was wrongly set to the beginning of the UDP tunnel header rather
>>> than the inner MAC header.
>>>
>>> Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
>>> ---
>>> net/ipv4/udp_offload.c | 8 +++++---
>>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
>>> index 0ed2daf..e330c0e 100644
>>> --- a/net/ipv4/udp_offload.c
>>> +++ b/net/ipv4/udp_offload.c
>>> @@ -399,6 +399,11 @@ int udp_gro_complete(struct sk_buff *skb, int nhoff)
>>>
>>>        uh->len = newlen;
>>>
>>> +       /* Set encapsulation before calling into inner gro_complete() functions
>>> +        * to make them set up the inner offsets.
>>> +        */
>>> +       skb->encapsulation = 1;
>>> +
>>>        rcu_read_lock();
>>>
>>>        uo_priv = rcu_dereference(udp_offload_base);
>>> @@ -421,9 +426,6 @@ int udp_gro_complete(struct sk_buff *skb, int nhoff)
>>>        if (skb->remcsum_offload)
>>>                skb_shinfo(skb)->gso_type |= SKB_GSO_TUNNEL_REMCSUM;
>>>
>>> -       skb->encapsulation = 1;
>>> -       skb_set_inner_mac_header(skb, nhoff + sizeof(struct udphdr));
>>> -
>>>        return err;
>>> }
>>>
>>> --
>>> 2.7.4
>>>
>>
>> So looking over this I think this patch is just a band-aid and it
>> isn't really fixing much of anything.  For example I cannot see where
>
> It fixes the immediate bug/problem that segmentation of GRO UDP tunnel packet fails due to incorrectly set inner MAC offset and/or missing inner network offset.

The problem is I think this will introduce other issues as there are
now situations where the inner MAC offset never gets set.  For example
did you try testing with GUE carrying either an IPIP or SIT tunnel?

>From what I can tell it looks like you are trying to fix VXLAN and
similar offloads, and Tom is focused on GUE.  What you will probably
need to do is add some code to ipip_gro_complete and sit_gro_complete
so that if skb->encapsulation is set before you call the function it
will set the inner MAC offset to the same offset as the inner network
header.  Otherwise you run the risk of screwing up segmentation
because the UDP tunnel segment code will use the inner mac header
offset when computing the tunnel header length and get it completely
wrong.

>> the inner transport offset is ever set.  From what I can tell it never
>> is.
>>
>
> inner transport offset is set in validate_xmit_skb(), but you are right, it is not set by the GRO complete code path. AFAIK no-one uses it before reaching validate_xmit_skb(), though. The documentation in (net-next) Documentation/networking/segmentation-offloads.txt could be fixed by removing the inner transport header from UDP/GRE tunnel case, as it is not used by either the GRE or UDP tunnel code.

Actually that is an interesting point.  Right now we need to keep the
inner transport header around because that is getting set.  If we were
to enforce the use of csum_start as the inner transport header offset,
then we could assume that transport_header is always the outer-most
transport header.  We might be able to drop a bit of space from the
skbuff by getting rid of it.  If nothing else it means one less header
offset we would have to copy.

- Alex

Re: [PATCH net 2/3] udp_offload: Set encapsulation before inner completes.

[Tom Herbert]

On Mon, May 2, 2016 at 3:54 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Mon, May 2, 2016 at 2:21 PM, Jarno Rajahalme <jarno@ovn.org> wrote:
>>
>>> On Apr 29, 2016, at 8:46 PM, Alexander Duyck <alexander.duyck@gmail.com> wrote:
>>>
>>> On Fri, Apr 29, 2016 at 3:28 PM, Jarno Rajahalme <jarno@ovn.org> wrote:
>>>> UDP tunnel segmentation code relies on the inner offsets being set for
>>>> an UDP tunnel GSO packet.  The inner *_complete() functions will set
>>>> the inner offsets only if the encapsulation is set before calling
>>>> them, but udp_gro_complete() set it only after the inner *_complete()
>>>> functions were done.
>>>>
>>>> Also, remove the setting of the inner_mac_header in udp_gro_complete()
>>>> as it was wrongly set to the beginning of the UDP tunnel header rather
>>>> than the inner MAC header.
>>>>
>>>> Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
>>>> ---
>>>> net/ipv4/udp_offload.c | 8 +++++---
>>>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
>>>> index 0ed2daf..e330c0e 100644
>>>> --- a/net/ipv4/udp_offload.c
>>>> +++ b/net/ipv4/udp_offload.c
>>>> @@ -399,6 +399,11 @@ int udp_gro_complete(struct sk_buff *skb, int nhoff)
>>>>
>>>>        uh->len = newlen;
>>>>
>>>> +       /* Set encapsulation before calling into inner gro_complete() functions
>>>> +        * to make them set up the inner offsets.
>>>> +        */
>>>> +       skb->encapsulation = 1;
>>>> +
>>>>        rcu_read_lock();
>>>>
>>>>        uo_priv = rcu_dereference(udp_offload_base);
>>>> @@ -421,9 +426,6 @@ int udp_gro_complete(struct sk_buff *skb, int nhoff)
>>>>        if (skb->remcsum_offload)
>>>>                skb_shinfo(skb)->gso_type |= SKB_GSO_TUNNEL_REMCSUM;
>>>>
>>>> -       skb->encapsulation = 1;
>>>> -       skb_set_inner_mac_header(skb, nhoff + sizeof(struct udphdr));
>>>> -
>>>>        return err;
>>>> }
>>>>
>>>> --
>>>> 2.7.4
>>>>
>>>
>>> So looking over this I think this patch is just a band-aid and it
>>> isn't really fixing much of anything.  For example I cannot see where
>>
>> It fixes the immediate bug/problem that segmentation of GRO UDP tunnel packet fails due to incorrectly set inner MAC offset and/or missing inner network offset.
>
> The problem is I think this will introduce other issues as there are
> now situations where the inner MAC offset never gets set.  For example
> did you try testing with GUE carrying either an IPIP or SIT tunnel?
>
> From what I can tell it looks like you are trying to fix VXLAN and
> similar offloads, and Tom is focused on GUE.  What you will probably
> need to do is add some code to ipip_gro_complete and sit_gro_complete
> so that if skb->encapsulation is set before you call the function it
> will set the inner MAC offset to the same offset as the inner network
> header.  Otherwise you run the risk of screwing up segmentation
> because the UDP tunnel segment code will use the inner mac header
> offset when computing the tunnel header length and get it completely
> wrong.
>
>>> the inner transport offset is ever set.  From what I can tell it never
>>> is.
>>>
>>
>> inner transport offset is set in validate_xmit_skb(), but you are right, it is not set by the GRO complete code path. AFAIK no-one uses it before reaching validate_xmit_skb(), though. The documentation in (net-next) Documentation/networking/segmentation-offloads.txt could be fixed by removing the inner transport header from UDP/GRE tunnel case, as it is not used by either the GRE or UDP tunnel code.
>
> Actually that is an interesting point.  Right now we need to keep the
> inner transport header around because that is getting set.  If we were
> to enforce the use of csum_start as the inner transport header offset,
> then we could assume that transport_header is always the outer-most
> transport header.  We might be able to drop a bit of space from the
> skbuff by getting rid of it.  If nothing else it means one less header
> offset we would have to copy.
>
Or can we just eliminate the inner headers altogether? IIRC the only
sticking point for GSO needing this was ipid, all the inner headers
are just copied per packet assuming segment lengths are the same. Is
this possible yet?

Tom

> - Alex

Re: [PATCH net 2/3] udp_offload: Set encapsulation before inner completes.

[Alexander Duyck]

On Mon, May 2, 2016 at 4:18 PM, Tom Herbert <tom@herbertland.com> wrote:
> On Mon, May 2, 2016 at 3:54 PM, Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
>> On Mon, May 2, 2016 at 2:21 PM, Jarno Rajahalme <jarno@ovn.org> wrote:
>>>
>>>> On Apr 29, 2016, at 8:46 PM, Alexander Duyck <alexander.duyck@gmail.com> wrote:
>>>>
>>>> On Fri, Apr 29, 2016 at 3:28 PM, Jarno Rajahalme <jarno@ovn.org> wrote:
>>>>> UDP tunnel segmentation code relies on the inner offsets being set for
>>>>> an UDP tunnel GSO packet.  The inner *_complete() functions will set
>>>>> the inner offsets only if the encapsulation is set before calling
>>>>> them, but udp_gro_complete() set it only after the inner *_complete()
>>>>> functions were done.
>>>>>
>>>>> Also, remove the setting of the inner_mac_header in udp_gro_complete()
>>>>> as it was wrongly set to the beginning of the UDP tunnel header rather
>>>>> than the inner MAC header.
>>>>>
>>>>> Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
>>>>> ---
>>>>> net/ipv4/udp_offload.c | 8 +++++---
>>>>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
>>>>> index 0ed2daf..e330c0e 100644
>>>>> --- a/net/ipv4/udp_offload.c
>>>>> +++ b/net/ipv4/udp_offload.c
>>>>> @@ -399,6 +399,11 @@ int udp_gro_complete(struct sk_buff *skb, int nhoff)
>>>>>
>>>>>        uh->len = newlen;
>>>>>
>>>>> +       /* Set encapsulation before calling into inner gro_complete() functions
>>>>> +        * to make them set up the inner offsets.
>>>>> +        */
>>>>> +       skb->encapsulation = 1;
>>>>> +
>>>>>        rcu_read_lock();
>>>>>
>>>>>        uo_priv = rcu_dereference(udp_offload_base);
>>>>> @@ -421,9 +426,6 @@ int udp_gro_complete(struct sk_buff *skb, int nhoff)
>>>>>        if (skb->remcsum_offload)
>>>>>                skb_shinfo(skb)->gso_type |= SKB_GSO_TUNNEL_REMCSUM;
>>>>>
>>>>> -       skb->encapsulation = 1;
>>>>> -       skb_set_inner_mac_header(skb, nhoff + sizeof(struct udphdr));
>>>>> -
>>>>>        return err;
>>>>> }
>>>>>
>>>>> --
>>>>> 2.7.4
>>>>>
>>>>
>>>> So looking over this I think this patch is just a band-aid and it
>>>> isn't really fixing much of anything.  For example I cannot see where
>>>
>>> It fixes the immediate bug/problem that segmentation of GRO UDP tunnel packet fails due to incorrectly set inner MAC offset and/or missing inner network offset.
>>
>> The problem is I think this will introduce other issues as there are
>> now situations where the inner MAC offset never gets set.  For example
>> did you try testing with GUE carrying either an IPIP or SIT tunnel?
>>
>> From what I can tell it looks like you are trying to fix VXLAN and
>> similar offloads, and Tom is focused on GUE.  What you will probably
>> need to do is add some code to ipip_gro_complete and sit_gro_complete
>> so that if skb->encapsulation is set before you call the function it
>> will set the inner MAC offset to the same offset as the inner network
>> header.  Otherwise you run the risk of screwing up segmentation
>> because the UDP tunnel segment code will use the inner mac header
>> offset when computing the tunnel header length and get it completely
>> wrong.
>>
>>>> the inner transport offset is ever set.  From what I can tell it never
>>>> is.
>>>>
>>>
>>> inner transport offset is set in validate_xmit_skb(), but you are right, it is not set by the GRO complete code path. AFAIK no-one uses it before reaching validate_xmit_skb(), though. The documentation in (net-next) Documentation/networking/segmentation-offloads.txt could be fixed by removing the inner transport header from UDP/GRE tunnel case, as it is not used by either the GRE or UDP tunnel code.
>>
>> Actually that is an interesting point.  Right now we need to keep the
>> inner transport header around because that is getting set.  If we were
>> to enforce the use of csum_start as the inner transport header offset,
>> then we could assume that transport_header is always the outer-most
>> transport header.  We might be able to drop a bit of space from the
>> skbuff by getting rid of it.  If nothing else it means one less header
>> offset we would have to copy.
>>
> Or can we just eliminate the inner headers altogether? IIRC the only
> sticking point for GSO needing this was ipid, all the inner headers
> are just copied per packet assuming segment lengths are the same. Is
> this possible yet?

Actually the big two we need are the inner MAC offset and the inner
network offset.  We end up needing the MAC header in order to be able
to UDP checksum offloads because that is how we are currently
measuring the length of the header itself.  As you said we end up
needing the inner network header offset in order to handle the inner
IP ID field and checksum.

I was doing a bit of digging and found that the whole reason for
csum_start is essentially for the same use cases as the
inner_transport_header.  It was a means of storing off the pointer to
the transport header in the event that you have a packet that goes
through some receive processing that we then have to transmit.  On top
of that the only two spots where we actually set it are in
skb_reset_inner_headers which should only be called if we are
performing an offload of some sort on the inner headers, and
validate_xmit_skb when we are supposed to be performing a checksum and
in that case we just overwrite the inner_transport_header offset
anyway.

- Alex