On Thu, Jan 24, 2013 at 3:29 PM, Eric Dumazet [off-list ref] wrote:

On Thu, 2013-01-24 at 14:16 -0800, Pravin B Shelar wrote:

quoted

Signed-off-by: Pravin B Shelar <redacted>
---
Fixed according to comments from Jesse and Eric.
 - Factored a MAC layer handler out of skb_gso_segment().
 - Eliminated copy operation from gre_gso_segment().
 - Refresh header pointer after pskb_may_pull().

Seems nice !

quoted

+     if (skb_is_gso(skb)) {
+             err = skb_unclone(skb, GFP_ATOMIC);
+             if (unlikely(err))
+                     goto error;
+             skb_shinfo(skb)->gso_type |= SKB_GSO_GRE;
+             return skb;
+     } else if (skb->ip_summed == CHECKSUM_PARTIAL) {
+             /* Pages aren't locked and could change at any time.
+              * If this happens after we compute the checksum, the
+              * checksum will be wrong.  We linearize now to avoid
+              * this problem.
+              */
+             if (skb_is_nonlinear(skb)) {
+                     err = __skb_linearize(skb);
+                     if (unlikely(err))
+                             goto error;
+             }
+
+             err = skb_checksum_help(skb);
+             if (unlikely(err))
+                     goto error;
+     }
+

I really don't understand why you put chunk this in this patch.

Packet being GSO or not, the underlying problem still remains.

This must be addressed separately and at a different layer.

(in skb_checksum_help() most probably)

If the packet is GSO and we compute checksum in software,
then we also have to copy all frags that could potentially
be overwritten.

I think this patch does fix csum issue without causing any performance
regression. So this patch shld be enough to solve GRE-GSO issue. Once
you have fix, this code can be optimized even more.

quoted

+     skb->ip_summed = CHECKSUM_NONE;
+
+     return skb;
+
+error:
+     kfree_skb(skb);
+     return ERR_PTR(err);
+}
+
 static netdev_tx_t ipgre_tunnel_xmit(struct sk_buff *skb, struct net_device *dev)
 {
      struct ip_tunnel *tunnel = netdev_priv(dev);

@@ -751,10 +787,9 @@ static netdev_tx_t ipgre_tunnel_xmit(struct sk_buff *skb, struct net_device *dev
      __be32 dst;
      int    mtu;
      u8     ttl;
-
-     if (skb->ip_summed == CHECKSUM_PARTIAL &&
-         skb_checksum_help(skb))
-             goto tx_error;
+     int pkt_len;
+     struct pcpu_tstats *tstats;
+     int err;

      if (dev->type == ARPHRD_ETHER)
              IPCB(skb)->flags = 0;

@@ -852,13 +887,6 @@ static netdev_tx_t ipgre_tunnel_xmit(struct sk_buff *skb, struct net_device *dev

      if (skb->protocol == htons(ETH_P_IP)) {
              df |= (old_iph->frag_off&htons(IP_DF));
-
-             if ((old_iph->frag_off&htons(IP_DF)) &&
-                 mtu < ntohs(old_iph->tot_len)) {
-                     icmp_send(skb, ICMP_DEST_UNREACH, ICMP_FRAG_NEEDED, htonl(mtu));
-                     ip_rt_put(rt);
-                     goto tx_error;
-             }

Not clear why this chunk can be safely removed, even for non GSO
packet ?

This actually does not work specially for TAP devices since ICMP
response won't work because the tunnel endpoint is not part of that IP
network.
This was discussed in VXLAN patch thread.
(http://markmail.org/message/xmqmvdh4noljfq2n).

But I agree we shld keep it for for non tap GRE and non-gso packets.