Re: [PATCH net-next v2 1/2] ip_gre: allow CSUM capable devices to handle packets

["Dmitry Kravkov" <dmitry@broadcom.com>]

On Tue, 2013-02-19 at 15:03 -0800, pravin wrote:
> On Tue, Feb 19, 2013 at 11:20 AM, Dmitry Kravkov <dmitry@broadcom.com> wrote:
> >
> >> -----Original Message-----
> >> From: pravin [mailto:pravin.shelar@gmail.com]
> >> Sent: Tuesday, February 19, 2013 8:28 PM
> >> To: Dmitry Kravkov
> >> Cc: davem@davemloft.net; netdev@vger.kernel.org
> >> Subject: Re: [PATCH net-next v2 1/2] ip_gre: allow CSUM capable devices to
> >> handle packets
> >>
> >> On Mon, Feb 18, 2013 at 11:50 AM, Dmitry Kravkov <dmitry@broadcom.com>
> >> wrote:
> >> > If device is not able to handle checksumming it will
> >> > be handled in dev_xmit
> >> >
> >> > Signed-off-by: Dmitry Kravkov <dmitry@broadcom.com>
> >> > ---
> >> > Changes from v1: fixed email address
> >> >
> >> >  net/ipv4/ip_gre.c |    7 ++-----
> >> >  1 files changed, 2 insertions(+), 5 deletions(-)
> >> >
> >> > diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> >> > index a56f118..cdc31ac 100644
> >> > --- a/net/ipv4/ip_gre.c
> >> > +++ b/net/ipv4/ip_gre.c
> >> > @@ -745,12 +745,9 @@ static struct sk_buff *handle_offloads(struct sk_buff
> >> *skb)
> >> >                         goto error;
> >> >                 skb_shinfo(skb)->gso_type |= SKB_GSO_GRE;
> >> >                 return skb;
> >> > -       } else if (skb->ip_summed == CHECKSUM_PARTIAL) {
> >> > -               err = skb_checksum_help(skb);
> >> > -               if (unlikely(err))
> >> > -                       goto error;
> >> >         }
> >> > -       skb->ip_summed = CHECKSUM_NONE;
> >> > +       if (skb->ip_summed != CHECKSUM_PARTIAL)
> >> > +               skb->ip_summed = CHECKSUM_NONE;
> >> >
> >> >         return skb;
> >> >
> >> > --
> >> > 1.7.7.2
> >> >
> >> >
> >>
> >> This patch breaks GRE tunnel with GRE_CSUM. since GRE_CSUM need
> >> complete IP packet to checksum entire GRE payload.
> >
> > Testing for o_flags&GRE_CSUM does not look too hurt here, since it will be used in ipgre_tunnel_xmit() later on
> > This is the only problematic case, right?
> >
> 
> It does not work for me. I have GRE device with csum on. Ping works
> fine but Netperf is not working.
> Looking at code, I am not sure how tcp will work if inner packet TCP
> checksum is calculated after GRE_CSUM calculation.


Fixes handling when CSUM or SEQ flags are set - via skb_checksum_help()
---
Tested with each mode (separately)

 net/ipv4/ip_gre.c |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
index 5ef4da7..0de54cd 100644
--- a/net/ipv4/ip_gre.c
+++ b/net/ipv4/ip_gre.c
@@ -735,7 +735,8 @@ drop:
 	return 0;
 }
 
-static struct sk_buff *handle_offloads(struct sk_buff *skb)
+static struct sk_buff *handle_offloads(struct ip_tunnel *tunnel,
+				       struct sk_buff *skb)
 {
 	int err;
 
@@ -745,8 +746,15 @@ static struct sk_buff *handle_offloads(struct
sk_buff *skb)
 			goto error;
 		skb_shinfo(skb)->gso_type |= SKB_GSO_GRE;
 		return skb;
+	} else if (tunnel->parms.o_flags&(GRE_SEQ|GRE_CSUM) &&
+		   skb->ip_summed == CHECKSUM_PARTIAL) {
+		err = skb_checksum_help(skb);
+		if (unlikely(err))
+			goto error;
 	}
-	if (skb->ip_summed != CHECKSUM_PARTIAL)
+
+	if (skb->ip_summed != CHECKSUM_PARTIAL ||
+	    tunnel->parms.o_flags&(GRE_SEQ|GRE_CSUM))
 		skb->ip_summed = CHECKSUM_NONE;
 
 	return skb;
@@ -776,7 +784,7 @@ static netdev_tx_t ipgre_tunnel_xmit(struct sk_buff
*skb, struct net_device *dev
 	int    err;
 	int    pkt_len;
 
-	skb = handle_offloads(skb);
+	skb = handle_offloads(tunnel, skb);
 	if (IS_ERR(skb)) {
 		dev->stats.tx_dropped++;
 		return NETDEV_TX_OK;
-- 
1.7.7.2


> Thanks,
> Pravin.
>