> On Apr 29, 2016, at 8:46 PM, Alexander Duyck <alexander.du...@gmail.com> > wrote: > > On Fri, Apr 29, 2016 at 3:28 PM, Jarno Rajahalme <ja...@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 <ja...@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
