pppoe and receive checksum offload

pppoe and receive checksum offload

[Stephen Hemminger]

Someone reported a problem with skge hardware receive checksumming and PPPOE
but it looks like a generic problem.  Since PPPOE adds additional header
bytes the hardware computed checksum will be wrong.  

Not sure if this is correct, but shouldn't pppoe be doing the following:
-----
diff -Nru a/drivers/net/pppoe.c b/drivers/net/pppoe.c
--- a/drivers/net/pppoe.c	2005-02-24 15:40:10 -08:00
+++ b/drivers/net/pppoe.c	2005-02-24 15:40:10 -08:00
@@ -339,6 +339,7 @@
 		int len = ntohs(ph->length);
 		skb_pull(skb, sizeof(struct pppoe_hdr));
 		skb_trim(skb, len);
+		skb->ip_summed = CHECKSUM_NONE;
 
 		ppp_input(&po->chan, skb);
 	} else if (sk->sk_state & PPPOX_RELAY) {

Re: pppoe and receive checksum offload

[David S. Miller]

On Thu, 24 Feb 2005 15:59:06 -0800
Stephen Hemminger <shemminger@osdl.org> wrote:

> Someone reported a problem with skge hardware receive checksumming and PPPOE
> but it looks like a generic problem.  Since PPPOE adds additional header
> bytes the hardware computed checksum will be wrong.  
> 
> Not sure if this is correct, but shouldn't pppoe be doing the following:

Changing or expanding the link level headers should only mess
up the hw checksum if you are using CHECKSUM_HW, is that what
your skge driver is using?

Re: pppoe and receive checksum offload

[Alexey Kuznetsov]

Hello!

> Changing or expanding the link level headers should only mess
> up the hw checksum if you are using CHECKSUM_HW, is that what
> your skge driver is using?

Well, even if this driver uses CHECKSUM_UNNECESSARY, the bug is still
apparently present for another devices which use CHECKSUM_HW.

Actually, the bug is in ppp_input(). It is responsible for setting correct
ip_summed before doing netif_rx(). It could even optimize subtracting
checksum of stripped ppp header from ip_summed (f.e. ipgre_rcv() do this)

But suggested fix is still correct. Unless there are another users of
ppp_input() with the same problem.

Alexey

Re: pppoe and receive checksum offload

[Stephen Hemminger]

>>Someone reported a problem with skge hardware receive checksumming and PPPOE
>>> but it looks like a generic problem.  Since PPPOE adds additional header
>>> bytes the hardware computed checksum will be wrong.  
>>> 
>>> Not sure if this is correct, but shouldn't pppoe be doing the following:
> 
> 
> Changing or expanding the link level headers should only mess
> up the hw checksum if you are using CHECKSUM_HW, is that what
> your skge driver is using?

The hardware doesn't appear to actually decode the packet, it just
has the ability to compute data sum of packet starting at an arbitrary
byte offset. This matched the description of CHECKSUM_HW so
that is what I used.

The original sk98lin attempted to receive hardware checksumming
but never actually turned it on.

Re: pppoe and receive checksum offload

[David S. Miller]

On Mon, 28 Feb 2005 09:12:52 -0800
Stephen Hemminger <shemminger@osdl.org> wrote:

> The original sk98lin attempted to receive hardware checksumming
> but never actually turned it on.

Because there is a bug in the chip wherein checksums can be
miscalculated.  I forget the details, but I do remember that
you can't enable checksumming safely because of this.

In drivers/net/sk98lin/skcsum.c it mentions this:

 * Note:
 *      There is a bug in the GENESIS ASIC which may lead to wrong checksums.

I know this comment is above the send checksum routine, but I
am pretty sure the bug applies to both directions.

Re: pppoe and receive checksum offload

[shemminger]

> On Mon, 28 Feb 2005 09:12:52 -0800
> Stephen Hemminger <shemminger@osdl.org> wrote:
>
>> The original sk98lin attempted to receive hardware checksumming
>> but never actually turned it on.
>
> Because there is a bug in the chip wherein checksums can be
> miscalculated.  I forget the details, but I do remember that
> you can't enable checksumming safely because of this.
>
> In drivers/net/sk98lin/skcsum.c it mentions this:
>
>  * Note:
>  *      There is a bug in the GENESIS ASIC which may lead to wrong
> checksums.
>
> I know this comment is above the send checksum routine, but I
> am pretty sure the bug applies to both directions.

The driver covers two types of chips (Genesis and Yukon). The new
driver only allows receive checksum on Yukon chipset. Almost all
current hardware uses Yukon and it has been pretty well tested.
The issues the sk98lin had were sloppy coding and handling of
unsigned arithmetic.

Re: pppoe and receive checksum offload

[Herbert Xu]

On Thu, Feb 24, 2005 at 11:59:06PM +0000, Stephen Hemminger wrote:
> 
> Not sure if this is correct, but shouldn't pppoe be doing the following:
> -----
> diff -Nru a/drivers/net/pppoe.c b/drivers/net/pppoe.c
> --- a/drivers/net/pppoe.c	2005-02-24 15:40:10 -08:00
> +++ b/drivers/net/pppoe.c	2005-02-24 15:40:10 -08:00
> @@ -339,6 +339,7 @@
>  		int len = ntohs(ph->length);
>  		skb_pull(skb, sizeof(struct pppoe_hdr));
>  		skb_trim(skb, len);
> +		skb->ip_summed = CHECKSUM_NONE;

This penalises CHECKSUM_HW.  We should have a generic helper
function that does the checksum folding for CHECKSUM_HW and
if otherwise set ip_summed to CHECKSUM_NONE.

In fact it looks like the current code in ip_gre is broken
in this respect as well.

Let me whip up a patch tomorrow.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: pppoe and receive checksum offload

[Herbert Xu]

On Mon, Feb 28, 2005 at 10:31:06PM +1100, herbert wrote:
> 
> In fact it looks like the current code in ip_gre is broken
> in this respect as well.

Actually ip_gre is probably right.  It seems that CHECKSUM_UNNECESSARY
should not be set for PPPOE/GRE packets at all.  So it would be a bug in
the sk* driver.

Alexey/Dave, is this interpretation of ip_summed correct?

Of course we still need to fix up CHECKSUM_HW in pppoe.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: pppoe and receive checksum offload

[Alexey Kuznetsov]

Hello!

> Actually ip_gre is probably right.  It seems that CHECKSUM_UNNECESSARY
> should not be set for PPPOE/GRE packets at all.  So it would be a bug in
> the sk* driver.
> 
> Alexey/Dave, is this interpretation of ip_summed correct?

What's about CHECKSUM_UNNECESSARY, yes, it is set only for TCP/UDP packets,
when device verifies the checksum itself. It is not the case for
PPOE frames. CHECKSUM_HW is more flxible, ipip/gre tunnels use this adjusting
skb->csum by checksum of stripped headers.

ppp_input() could do the same thing. It does not, hence suggested patch
is corrrect minimal solution.

Alexey

Re: pppoe and receive checksum offload

[Herbert Xu]

[-- Attachment #1: Type: text/plain, Size: 559 bytes --]

On Mon, Feb 28, 2005 at 05:04:01PM +0300, Alexey Kuznetsov wrote:
>
> ppp_input() could do the same thing. It does not, hence suggested patch
> is corrrect minimal solution.

Agreed.  We should use Stephen's patch for 2.6.11.

For 2.6.12, does this patch look sane? As usual, I'd love to get
some better names for the new helper functions.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: p --]
[-- Type: text/plain, Size: 4452 bytes --]

===== drivers/net/pppoe.c 1.48 vs edited =====
--- 1.48/drivers/net/pppoe.c	2005-01-21 16:02:12 +11:00
+++ edited/drivers/net/pppoe.c	2005-03-01 11:20:44 +11:00
@@ -338,7 +338,9 @@
 		struct pppoe_hdr *ph = (struct pppoe_hdr *) skb->nh.raw;
 		int len = ntohs(ph->length);
 		skb_pull(skb, sizeof(struct pppoe_hdr));
-		skb_trim(skb, len);
+		skb_postpull_rcsum(skb, ph, sizeof(*ph));
+		if (pskb_trim_rcsum(skb, len))
+			goto abort_kfree;
 
 		ppp_input(&po->chan, skb);
 	} else if (sk->sk_state & PPPOX_RELAY) {
===== include/linux/skbuff.h 1.60 vs edited =====
--- 1.60/include/linux/skbuff.h	2005-02-06 14:23:06 +11:00
+++ edited/include/linux/skbuff.h	2005-03-01 11:15:43 +11:00
@@ -1054,6 +1054,42 @@
 	return __skb_linearize(skb, gfp);
 }
 
+/**
+ *	skb_postpull_rcsum - update checksum for received skb after pull
+ *	@skb: buffer to update
+ *	@start: start of data before pull
+ *	@len: length of data pulled
+ *
+ *	After doing a pull on a received packet, you need to call this to
+ *	update the CHECKSUM_HW checksum, or set ip_summed to CHECKSUM_NONE
+ *	so that it can be recomputed from scratch.
+ */
+
+static inline void skb_postpull_rcsum(struct sk_buff *skb,
+					 const void *start, int len)
+{
+	if (skb->ip_summed == CHECKSUM_HW)
+		skb->csum = csum_sub(skb->csum, csum_partial(start, len, 0));
+}
+
+/**
+ *	pskb_trim_rcsum - trim received skb and update checksum
+ *	@skb: buffer to trim
+ *	@len: new length
+ *
+ *	This is exactly the same as pskb_trim except that it ensures the
+ *	checksum of received packets are still valid after the operation.
+ */
+
+static inline int pskb_trim_rcsum(struct sk_buff *skb, unsigned int len)
+{
+	if (len >= skb->len)
+		return 0;
+	if (skb->ip_summed == CHECKSUM_HW)
+		skb->ip_summed = CHECKSUM_NONE;
+	return __pskb_trim(skb, len);
+}
+
 static inline void *kmap_skb_frag(const skb_frag_t *frag)
 {
 #ifdef CONFIG_HIGHMEM
===== net/ipv4/ip_gre.c 1.46 vs edited =====
--- 1.46/net/ipv4/ip_gre.c	2005-01-14 15:41:05 +11:00
+++ edited/net/ipv4/ip_gre.c	2005-03-01 11:17:35 +11:00
@@ -618,10 +618,8 @@
 
 		skb->mac.raw = skb->nh.raw;
 		skb->nh.raw = __pskb_pull(skb, offset);
+		skb_postpull_rcsum(skb, skb->mac.raw, offset);
 		memset(&(IPCB(skb)->opt), 0, sizeof(struct ip_options));
-		if (skb->ip_summed == CHECKSUM_HW)
-			skb->csum = csum_sub(skb->csum,
-					     csum_partial(skb->mac.raw, skb->nh.raw-skb->mac.raw, 0));
 		skb->pkt_type = PACKET_HOST;
 #ifdef CONFIG_NET_IPGRE_BROADCAST
 		if (MULTICAST(iph->daddr)) {
===== net/ipv4/ip_input.c 1.27 vs edited =====
--- 1.27/net/ipv4/ip_input.c	2005-01-27 17:03:17 +11:00
+++ edited/net/ipv4/ip_input.c	2005-03-01 11:21:19 +11:00
@@ -410,10 +410,9 @@
 		 * is IP we can trim to the true length of the frame.
 		 * Note this now means skb->len holds ntohs(iph->tot_len).
 		 */
-		if (skb->len > len) {
-			__pskb_trim(skb, len);
-			if (skb->ip_summed == CHECKSUM_HW)
-				skb->ip_summed = CHECKSUM_NONE;
+		if (pskb_trim_rcsum(skb, len)) {
+			IP_INC_STATS_BH(IPSTATS_MIB_INDISCARDS);
+			goto drop;
 		}
 	}
 
===== net/ipv6/ip6_input.c 1.22 vs edited =====
--- 1.22/net/ipv6/ip6_input.c	2004-08-23 18:15:14 +10:00
+++ edited/net/ipv6/ip6_input.c	2005-03-01 11:24:32 +11:00
@@ -95,15 +95,11 @@
 	if (pkt_len || hdr->nexthdr != NEXTHDR_HOP) {
 		if (pkt_len + sizeof(struct ipv6hdr) > skb->len)
 			goto truncated;
-		if (pkt_len + sizeof(struct ipv6hdr) < skb->len) {
-			if (__pskb_trim(skb, pkt_len + sizeof(struct ipv6hdr))){
-				IP6_INC_STATS_BH(IPSTATS_MIB_INHDRERRORS);
-				goto drop;
-			}
-			hdr = skb->nh.ipv6h;
-			if (skb->ip_summed == CHECKSUM_HW)
-				skb->ip_summed = CHECKSUM_NONE;
+		if (pskb_trim_rcsum(skb, pkt_len + sizeof(struct ipv6hdr))) {
+			IP6_INC_STATS_BH(IPSTATS_MIB_INHDRERRORS);
+			goto drop;
 		}
+		hdr = skb->nh.ipv6h;
 	}
 
 	if (hdr->nexthdr == NEXTHDR_HOP) {
@@ -138,7 +134,6 @@
 	unsigned int nhoff;
 	int nexthdr;
 	u8 hash;
-	int cksum_sub = 0;
 
 	skb->h.raw = skb->nh.raw + sizeof(struct ipv6hdr);
 
@@ -173,11 +168,8 @@
 		if (ipprot->flags & INET6_PROTO_FINAL) {
 			struct ipv6hdr *hdr;	
 
-			if (!cksum_sub && skb->ip_summed == CHECKSUM_HW) {
-				skb->csum = csum_sub(skb->csum,
-						     csum_partial(skb->nh.raw, skb->h.raw-skb->nh.raw, 0));
-				cksum_sub++;
-			}
+			skb_postpull_rcsum(skb, skb->nh.raw,
+					   skb->h.raw - skb->nh.raw);
 			hdr = skb->nh.ipv6h;
 			if (ipv6_addr_is_multicast(&hdr->daddr) &&
 			    !ipv6_chk_mcast_addr(skb->dev, &hdr->daddr,

Re: pppoe and receive checksum offload

[David S. Miller]

On Tue, 1 Mar 2005 12:04:24 +1100
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> On Mon, Feb 28, 2005 at 05:04:01PM +0300, Alexey Kuznetsov wrote:
> >
> > ppp_input() could do the same thing. It does not, hence suggested patch
> > is corrrect minimal solution.
> 
> Agreed.  We should use Stephen's patch for 2.6.11.
> 
> For 2.6.12, does this patch look sane? As usual, I'd love to get
> some better names for the new helper functions.

I'm not so picky about the names.  Patch applied, thanks
Herbert :-)

If Stephen cares, he can try to submit his patch for 2.6.11.x
via stable@kernel.org, I have no problem with that.