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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CALx6S35QFms9KUE1K4c5odN7QCuvpBaeeL=yjfMBFxqFfcTSWg@mail.gmail.com>
Date:	Mon, 2 May 2016 16:18:31 -0700
From:	Tom Herbert <tom@...bertland.com>
To:	Alexander Duyck <alexander.duyck@...il.com>
Cc:	Jarno Rajahalme <jarno@....org>, Netdev <netdev@...r.kernel.org>,
	Jesse Gross <jesse@...nel.org>
Subject: Re: [PATCH net 2/3] udp_offload: Set encapsulation before inner completes.

On Mon, May 2, 2016 at 3:54 PM, Alexander Duyck
<alexander.duyck@...il.com> wrote:
> On Mon, May 2, 2016 at 2:21 PM, Jarno Rajahalme <jarno@....org> wrote:
>>
>>> On Apr 29, 2016, at 8:46 PM, Alexander Duyck <alexander.duyck@...il.com> wrote:
>>>
>>> On Fri, Apr 29, 2016 at 3:28 PM, Jarno Rajahalme <jarno@....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@....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