[PATCH] Bug in pskb_trim_rcsum()

[PATCH] Bug in pskb_trim_rcsum()

[Wei Yongjun]

  Since network device can auto calculate and verify the checksum of a
packet, for example: some e1000 interface. Different device will set
different value of skb->ip_summed.
  a) If device do nothing to checksum, skb->ip_summed would be set to
CHECKSUM_NONE.
  b) If device can only calculate a checksum, and the checksum is
correct, skb->ip_summed would be set to CHECKSUM_HW.
  c) If device can verify the checksum, and the checksum is correct,
skb->ip_summed would be set to CHECKSUM_UNNECESSARY.
  So if I want to trim a skb, I think I must do a checksum even if the
skb->ip_summed is CHECKSUM_UNNECESSARY.

Following is the comment about CHECKSUM_UNNECESSARY in
include/linux/skbuff.h:
 *	UNNECESSARY: device parsed packet and wouldbe verified checksum.
 *		skb->csum is undefined.
 *	      It is bad option, but, unfortunately, many of vendors do this.
 *	      Apparently with secret goal to sell you new device, when you
 *	      will add new protocol to your host. F.e. IPv6. 8)

Signed-off-by: Wei Yongjun <weiyj@soft.fujitsu.com>


--- a/include/linux/skbuff.h	2006-07-17 10:14:23.175070472 -0400
+++ b/include/linux/skbuff.h	2006-07-17 10:18:31.762279472 -0400
@@ -1208,8 +1208,8 @@ static inline int pskb_trim_rcsum(struct
 {
 	if (likely(len >= skb->len))
 		return 0;
-	if (skb->ip_summed == CHECKSUM_HW)
-		skb->ip_summed = CHECKSUM_NONE;
+
+	skb->ip_summed = CHECKSUM_NONE;
 	return __pskb_trim(skb, len);
 }
 

Re: [PATCH] Bug in pskb_trim_rcsum()

[Herbert Xu]

Wei Yongjun <yjwei@nanjing-fnst.com> wrote:
>
>  So if I want to trim a skb, I think I must do a checksum even if the
> skb->ip_summed is CHECKSUM_UNNECESSARY.

Nope.  CHECKSUM_UNNECESSARY means that the hardware has already
verified the checksum to be correct for the given protocol.
Trimming a packet does not affect it.

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: [PATCH] Bug in pskb_trim_rcsum()

[Wei Yongjun]

On Monday 17 July 2006 08:17, Herbert Xu wrote:
> Wei Yongjun <yjwei@nanjing-fnst.com> wrote:
> >  So if I want to trim a skb, I think I must do a checksum even if
the
> > skb->ip_summed is CHECKSUM_UNNECESSARY.
>
> Nope.  CHECKSUM_UNNECESSARY means that the hardware has already
> verified the checksum to be correct for the given protocol.
> Trimming a packet does not affect it.

Yes, you are right. hardware has already verified the checksum to be
correct.
But I think hardware do this:
  1) calculate a checksum for the given protocol.(as the same as the
hardware which does not support verified the checksum)
  2) verified the checksum.(compare protocol checksum of packet with
calculated checksum)
If that is true, Trimming a packet also affect CHECKSUM_UNNECESSARY.

And in my test, UDP under IPv4 maybe do that.
My UDP packet is:

packet1:
 ___________________________________
| Source Port     | Dest Port       |
|_________________|_________________|
| Length = 16     | Checksum(*1)    |
|_________________|_________________|
|             payload24             |
|___________________________________|

correct is :
 ___________________________________
| Source Port     | Dest Port       |
|_________________|_________________|
| Length = 32     | Checksum        |
|_________________|_________________|
|             payload24             |
|___________________________________|

  Checksum(*1)  is the checksum of payload24, not only the first 8 byte
of payload24.

  When I send packet1 to echo-udp(target used e1000 inferface), echo-
reply can be reply. When I send packet1 to echo-udp(target used e100
inferface), echo-reply can not be reply.

  In e1000, when packet1 with checksum(*1) ,  skb->ip_summed is set to
CHECKSUM_UNNECESSARY. And when other, skb->ip_summed is set to
CHECKSUM_HW. So I think my guess is correct. Patch is necessary.


Regards

Re: [PATCH] Bug in pskb_trim_rcsum()

[Herbert Xu]

On Tue, Jul 18, 2006 at 09:09:34AM +0800, Wei Yongjun wrote:
>
> And in my test, UDP under IPv4 maybe do that.
> My UDP packet is:
> 
> packet1:
>  ___________________________________
> | Source Port               | Dest Port                   |
> |_________________|_________________|
> | Length = 16               | Checksum(*1)          |
> |_________________|_________________|
> |                          payload24                          |
> |__________________________________|

The whole point of CHECKSUM_UNNECESSARY is that the hardware parses
the protocol header for us.  So in this case it must calculate the
checksum for only the first 8 bytes of the payload.

If it does this incorrectly, then it doesn't support RX checksums at
all.

Which NIC is doing this BTW?

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: [PATCH] Bug in pskb_trim_rcsum()

[Alexey Kuznetsov]

Hello!

> The whole point of CHECKSUM_UNNECESSARY is that the hardware parses
> the protocol header for us.  So in this case it must calculate the
> checksum for only the first 8 bytes of the payload.

I remember this place, I stalled there for a minute ages ago.

He is right in his doubts, the place is confusing, device do it wrong
(but they are innocent, it is just a flaw of UDP, people did not know
what to do with redundant two octets in header and placed some useless
thing there :-)) and if we want to be paranoid about checksum,
we should really clear CHECKSUM_UNNECESSARY tag and to recheck.

I preferred optimistic approach: if the checksum comes out correct,
we do not really care, how device calculated it. Probably, it calculated
checksum over wrong data, but got a good checksum. So what? It is
not a crypto digest yet. And if device found wrong checksum, we will
recalculate it anyway.


I would like to add that CHECKSUM_UNNECESSARY can be used, when
checksum is really wrong (on loopback), that's why it is not cleared,
when trimming. CHECKSUM_HW can always fall back to CHECKSUM_NONE,
but CHECKSUM_UNNECESSARY cannot. Probably, this was bad idea, but
it still means that if some generic function starts to clear it,
all the code using it should be reverified.

Alexey

Re: [PATCH] Bug in pskb_trim_rcsum()

[Herbert Xu]

On Tue, Jul 18, 2006 at 04:54:39PM +0400, Alexey Kuznetsov wrote:
>
> I preferred optimistic approach: if the checksum comes out correct,
> we do not really care, how device calculated it. Probably, it calculated
> checksum over wrong data, but got a good checksum. So what? It is
> not a crypto digest yet. And if device found wrong checksum, we will
> recalculate it anyway.

Agreed.

> I would like to add that CHECKSUM_UNNECESSARY can be used, when
> checksum is really wrong (on loopback), that's why it is not cleared,
> when trimming. CHECKSUM_HW can always fall back to CHECKSUM_NONE,
> but CHECKSUM_UNNECESSARY cannot. Probably, this was bad idea, but
> it still means that if some generic function starts to clear it,
> all the code using it should be reverified.

Actually, I plan to differentiate between RX CHECKSUM_HW and TX
CHECKSUM_HW.  Now that we have things like Xen it is possible for
RX packets to have patial checksums too.

When this is done loopback can send TX CHECKSUM_HW packets instead
of CHECKSUM_UNNECESSARY (I'm currently calling this CHECKSUM_PARTIAL).

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: [PATCH] Bug in pskb_trim_rcsum()

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Tue, 18 Jul 2006 23:04:04 +1000

> Actually, I plan to differentiate between RX CHECKSUM_HW and TX
> CHECKSUM_HW.  Now that we have things like Xen it is possible for
> RX packets to have patial checksums too.
> 
> When this is done loopback can send TX CHECKSUM_HW packets instead
> of CHECKSUM_UNNECESSARY (I'm currently calling this CHECKSUM_PARTIAL).

This sounds like a good idea.

Undoing CHECKSUM_UNNECESSARY when trimming is definitely going to
break loopback the way things are done currently.