netdev - Re: [PATCH net-next 7/8] net: ip_tunnel: remove 'csum_help' argument to iptunnel_handle

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAKgT0UeZwSm4oKoydXWU3iWmjL5m6H=K=hJ2wAkp_GugbZJwjA@mail.gmail.com>
Date:	Fri, 8 Jan 2016 18:05:16 -0800
From:	Alexander Duyck <alexander.duyck@...il.com>
To:	Tom Herbert <tom@...bertland.com>
Cc:	Edward Cree <ecree@...arflare.com>,
	David Miller <davem@...emloft.net>,
	Netdev <netdev@...r.kernel.org>, linux-net-drivers@...arflare.com
Subject: Re: [PATCH net-next 7/8] net: ip_tunnel: remove 'csum_help' argument
 to iptunnel_handle_offloads

On Fri, Jan 8, 2016 at 4:44 PM, Tom Herbert <tom@...bertland.com> wrote:
> On Fri, Jan 8, 2016 at 4:35 PM, Alexander Duyck
> <alexander.duyck@...il.com> wrote:
>> On Fri, Jan 8, 2016 at 11:47 AM, Edward Cree <ecree@...arflare.com> wrote:
>>> All users now pass false, so we can remove it, and remove the code that
>>>  was conditional upon it.
>>>
>>> Signed-off-by: Edward Cree <ecree@...arflare.com>
>>> ---
>>>  drivers/net/vxlan.c             |  4 ++--
>>>  include/net/ip_tunnels.h        |  3 +--
>>>  include/net/udp_tunnel.h        |  3 +--
>>>  net/ipv4/fou.c                  |  4 ++--
>>>  net/ipv4/ip_gre.c               |  3 +--
>>>  net/ipv4/ip_tunnel_core.c       | 15 +--------------
>>>  net/ipv4/ipip.c                 |  2 +-
>>>  net/ipv6/sit.c                  |  4 ++--
>>>  net/netfilter/ipvs/ip_vs_xmit.c |  6 ++----
>>>  9 files changed, 13 insertions(+), 31 deletions(-)
>>>
>>
>>
>>> diff --git a/net/ipv4/ip_tunnel_core.c b/net/ipv4/ip_tunnel_core.c
>>> index 1db8418..f98bd53 100644
>>> --- a/net/ipv4/ip_tunnel_core.c
>>> +++ b/net/ipv4/ip_tunnel_core.c
>>> @@ -147,7 +147,6 @@ struct metadata_dst *iptunnel_metadata_reply(struct metadata_dst *md,
>>>  EXPORT_SYMBOL_GPL(iptunnel_metadata_reply);
>>>
>>>  struct sk_buff *iptunnel_handle_offloads(struct sk_buff *skb,
>>> -                                        bool csum_help,
>>>                                          int gso_type_mask)
>>>  {
>>>         int err;
>>> @@ -165,19 +164,7 @@ struct sk_buff *iptunnel_handle_offloads(struct sk_buff *skb,
>>>                 return skb;
>>>         }
>>>
>>> -       /* If packet is not gso and we are resolving any partial checksum,
>>> -        * clear encapsulation flag. This allows setting CHECKSUM_PARTIAL
>>> -        * on the outer header without confusing devices that implement
>>> -        * NETIF_F_IP_CSUM with encapsulation.
>>> -        */
>>> -       if (csum_help)
>>> -               skb->encapsulation = 0;
>>> -
>>> -       if (skb->ip_summed == CHECKSUM_PARTIAL && csum_help) {
>>> -               err = skb_checksum_help(skb);
>>> -               if (unlikely(err))
>>> -                       goto error;
>>> -       } else if (skb->ip_summed != CHECKSUM_PARTIAL)
>>> +       if (skb->ip_summed != CHECKSUM_PARTIAL)
>>>                 skb->ip_summed = CHECKSUM_NONE;
>>>
>>
>> So this patch is a bit broken here.  We should be clearing
>> skb->encapsulation if CHECKSUM_PARTIAL is not set.  That way we don't
>> incorrectly pull in the inner headers when computing the outer
>> checksum.
>>
> In the original code this is done in csum_help argument is true in
> iptunnel_handle_offloads. These patches essentially imply that
> csum_help would always be false so we shouldn't need to clear
> skb->encapsulation any more?

Actually it is causing me to throw warnings on an ixgbe NIC because it
implies that we want to offload the inner checksum, but we are setting
things up to offload the outer checksum.  For example if the inner is
just an ARP or ICMP frame I don't need to checksum the inner headers,
but skb->encapsulation is still set so the inner headers are being
evaluated instead of the outer ones in the Tx checksum routine in the
driver.

If we clear skb->encapsulation if we don't have CHECKSUM_PARTIAL set
then we don't have the issue.  Really the addition of the line
clearing skb->encapsulation should probably be added to the first
patch so that we don't leave skb->encapsulation set when we aren't
requesting offloads.

- Alex