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 [off-list ref] wrote:

On Fri, Jan 8, 2016 at 4:35 PM, Alexander Duyck
[off-list ref] wrote:

quoted

On Fri, Jan 8, 2016 at 11:47 AM, Edward Cree [off-list ref] wrote:

quoted

All users now pass false, so we can remove it, and remove the code that
 was conditional upon it.

Signed-off-by: Edward Cree <redacted>
---
 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(-)

quoted

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