Re: [PATCH 2/2] v2 GRE: Add segmentation offload for GRE
From: Eric Dumazet <hidden>
Date: 2013-01-24 23:30:03
On Thu, 2013-01-24 at 14:16 -0800, Pravin B Shelar wrote:
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 !
+ 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.
quoted hunk ↗ jump to hunk
+ 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 ?