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: <CAKgT0UcGCgRt5dpG2Y0a6sBar1KF1DkHxdhMbCOYV6qj15D50A@mail.gmail.com>
Date:	Fri, 29 Apr 2016 20:46:14 -0700
From:	Alexander Duyck <alexander.duyck@...il.com>
To:	Jarno Rajahalme <jarno@....org>
Cc:	Netdev <netdev@...r.kernel.org>, Jesse Gross <jesse@...nel.org>
Subject: Re: [PATCH net 2/3] udp_offload: Set encapsulation before inner completes.

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
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