why do we mangle checksums for v6 ICMP?

why do we mangle checksums for v6 ICMP?

[Al Viro]

	AFAICS, the rules are:

(1) checksum is 16-bit one's complement of the one's complement sum of
relevant 16bit words.

(2) for v4 UDP all-zeroes has special meaning - no checksum; if you get
it from (1), send all-ones instead.

(3) for v6 UDP we have the same remapping as in (2), but all-zeroes has
different meaning - not "ignore checksum" as in v4, but "reject the
packet".

(4) there is no (4).

	IOW, nobody except UDP has any business doing that 0->0xffff
replacement.  However, we have
       if (icmp6h->icmp6_cksum == 0)
               icmp6h->icmp6_cksum = -1;
and similar in net/ipv6/raw.c

AFAICS, both went in with (commit ID by linux-hist repository)
commit 6bac90985c8b65ffc25839c001aa7ef4831d2915
Author: Kazunori Miyazawa <kazunori@miyazawa.org>
Date:   Mon May 12 00:21:19 2003 -0700

    [IPV4]: Introduce ip6_append_data.

That changeset has very similar changes done in udp.c, icmp.c and raw.c;
remapping of UDP checksum used to be in the area affected by the changeset
and it looks like it got not just preserved in udp.c (as it should), but
copied to icmp.c and raw.c instances.

So... is it really needed there?

Re: why do we mangle checksums for v6 ICMP?

[Al Viro]

On Wed, Nov 08, 2006 at 10:13:32PM +0000, Al Viro wrote:
> 	AFAICS, the rules are:
> 
> (1) checksum is 16-bit one's complement of the one's complement sum of
> relevant 16bit words.
> 
> (2) for v4 UDP all-zeroes has special meaning - no checksum; if you get
> it from (1), send all-ones instead.
> 
> (3) for v6 UDP we have the same remapping as in (2), but all-zeroes has
> different meaning - not "ignore checksum" as in v4, but "reject the
> packet".
> 
> (4) there is no (4).
> 
> 	IOW, nobody except UDP has any business doing that 0->0xffff
> replacement.  However, we have
>        if (icmp6h->icmp6_cksum == 0)
>                icmp6h->icmp6_cksum = -1;
> and similar in net/ipv6/raw.c
> 
> AFAICS, both went in with (commit ID by linux-hist repository)
> commit 6bac90985c8b65ffc25839c001aa7ef4831d2915
> Author: Kazunori Miyazawa <kazunori@miyazawa.org>
> Date:   Mon May 12 00:21:19 2003 -0700
> 
>     [IPV4]: Introduce ip6_append_data.
> 
> That changeset has very similar changes done in udp.c, icmp.c and raw.c;
> remapping of UDP checksum used to be in the area affected by the changeset
> and it looks like it got not just preserved in udp.c (as it should), but
> copied to icmp.c and raw.c instances.
> 
> So... is it really needed there?

While we are at it, shouldn't ip_nat_mangle_udp_packet() do the same
remapping in
        } else
                udph->check = nf_proto_csum_update(*pskb,
                                                   htons(oldlen) ^ htons(0xFFFF),
                                                   htons(datalen),
                                                   udph->check, 1);
branch?  Note that udp_manip_pkt() does it in the same situation...

Re: why do we mangle checksums for v6 ICMP?

[Brian Haley]

Hi Al,

Al Viro wrote:
> 	AFAICS, the rules are:
> 
> (1) checksum is 16-bit one's complement of the one's complement sum of
> relevant 16bit words.
> 
> (2) for v4 UDP all-zeroes has special meaning - no checksum; if you get
> it from (1), send all-ones instead.
> 
> (3) for v6 UDP we have the same remapping as in (2), but all-zeroes has
> different meaning - not "ignore checksum" as in v4, but "reject the
> packet".
> 
> (4) there is no (4).
> 
> 	IOW, nobody except UDP has any business doing that 0->0xffff
> replacement.  However, we have
>        if (icmp6h->icmp6_cksum == 0)
>                icmp6h->icmp6_cksum = -1;

This doesn't look necessary, RFCs 4443/2463 don't mention it being 
necessary, and BSD doesn't do it either.  I'll cook-up a patch to remove 
that since I was doing some other mods in that codepath.

> and similar in net/ipv6/raw.c

Maybe here it only needs to be done if (fl->proto == IPPROTO_UDP)?

-Brian

Re: why do we mangle checksums for v6 ICMP?

[David Miller]

From: Brian Haley <brian.haley@hp.com>
Date: Thu, 09 Nov 2006 12:32:18 -0500

> Al Viro wrote:
> > 	AFAICS, the rules are:
> > 
> > (1) checksum is 16-bit one's complement of the one's complement sum of
> > relevant 16bit words.
> > 
> > (2) for v4 UDP all-zeroes has special meaning - no checksum; if you get
> > it from (1), send all-ones instead.
> > 
> > (3) for v6 UDP we have the same remapping as in (2), but all-zeroes has
> > different meaning - not "ignore checksum" as in v4, but "reject the
> > packet".
> > 
> > (4) there is no (4).
> > 
> > 	IOW, nobody except UDP has any business doing that 0->0xffff
> > replacement.  However, we have
> >        if (icmp6h->icmp6_cksum == 0)
> >                icmp6h->icmp6_cksum = -1;
> 
> This doesn't look necessary, RFCs 4443/2463 don't mention it being 
> necessary, and BSD doesn't do it either.  I'll cook-up a patch to remove 
> that since I was doing some other mods in that codepath.

This is how things look to me too.

> > and similar in net/ipv6/raw.c
> 
> Maybe here it only needs to be done if (fl->proto == IPPROTO_UDP)?

Yes, I believe that is what is needed.

[PATCH] IPv6: only modify checksum for UDP

[Brian Haley]

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

Only change upper-layer checksum from 0 to 0xFFFF for UDP (as RFC 768 
states), not for others as RFC 4443 doesn't require it.

Signed-off-by: Brian Haley <brian.haley@hp.com>

[-- Attachment #2: cksum.zero.diff --]
[-- Type: text/x-patch, Size: 721 bytes --]

diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
index 81bd45b..dbb9b1f 100644
--- a/net/ipv6/icmp.c
+++ b/net/ipv6/icmp.c
@@ -246,8 +246,6 @@ static int icmpv6_push_pending_frames(st
 					   len, fl->proto, tmp_csum);
 		icmp6h->icmp6_cksum = tmp_csum;
 	}
-	if (icmp6h->icmp6_cksum == 0)
-		icmp6h->icmp6_cksum = -1;
 	ip6_push_pending_frames(sk);
 out:
 	return err;
diff --git a/net/ipv6/raw.c b/net/ipv6/raw.c
index 6bc6655..baf7b82 100644
--- a/net/ipv6/raw.c
+++ b/net/ipv6/raw.c
@@ -536,7 +536,7 @@ static int rawv6_push_pending_frames(str
 				   &fl->fl6_dst,
 				   total_len, fl->proto, tmp_csum);
 
-	if (tmp_csum == 0)
+	if (tmp_csum == 0 && fl->proto == IPPROTO_UDP)
 		tmp_csum = -1;
 
 	csum = tmp_csum;

Re: [PATCH] IPv6: only modify checksum for UDP

[David Stevens]

Sorry, I saw this discussion a little late...

The Internet checksum is defined as a 1's-complement sum, so if  the
alternate 0 does not have a special meaning in a protocol, then by
1's-complement arithmetic, 0 == ~0.
        So, it looks to me without the remapping that a valid checksum
may also fail, if it is simply computed in a different way (or on a 
different
architecture) such that one gets 0 and one gets ~0 as un-modified answers.
        Since we're checking for equality on 2's-complement machines,
I think the easiest thing is to still re-map it. Otherwise, instead of 
testing
for 0, we have to test for both 0 and ~0 in the validity checks, right?

        Disclaimer: I've just glanced through the discussion, so maybe
I've misunderstood the point or effect of the patch...

                                                +-DLS

Re: [PATCH] IPv6: only modify checksum for UDP

[David Miller]

From: David Stevens <dlstevens@us.ibm.com>
Date: Fri, 10 Nov 2006 09:54:58 -0800

> The Internet checksum is defined as a 1's-complement sum, so if  the
> alternate 0 does not have a special meaning in a protocol, then by
> 1's-complement arithmetic, 0 == ~0.
>         So, it looks to me without the remapping that a valid checksum
> may also fail, if it is simply computed in a different way (or on a 
> different
> architecture) such that one gets 0 and one gets ~0 as un-modified answers.
>         Since we're checking for equality on 2's-complement machines,
> I think the easiest thing is to still re-map it. Otherwise, instead of 
> testing
> for 0, we have to test for both 0 and ~0 in the validity checks, right?

Puzzling :-)  Then why is the transformation only performed for
UDP in the ipv4 stack?  It seems by your logic TCP would need
to either do the "if (sum==0) sum=~0;"  thing or it would need
to accept both "0" and "~0" in the checksum checking path.

Re: [PATCH] IPv6: only modify checksum for UDP

[Al Viro]

On Mon, Nov 13, 2006 at 04:50:58PM -0800, David Miller wrote:
> From: David Stevens <dlstevens@us.ibm.com>
> Date: Fri, 10 Nov 2006 09:54:58 -0800
> 
> > The Internet checksum is defined as a 1's-complement sum, so if  the
> > alternate 0 does not have a special meaning in a protocol, then by
> > 1's-complement arithmetic, 0 == ~0.

Nope.  It isn't.

> >         So, it looks to me without the remapping that a valid checksum
> > may also fail, if it is simply computed in a different way (or on a 
> > different
> > architecture) such that one gets 0 and one gets ~0 as un-modified answers.
> >         Since we're checking for equality on 2's-complement machines,
> > I think the easiest thing is to still re-map it. Otherwise, instead of 
> > testing
> > for 0, we have to test for both 0 and ~0 in the validity checks, right?
> 
> Puzzling :-)  Then why is the transformation only performed for
> UDP in the ipv4 stack?  It seems by your logic TCP would need
> to either do the "if (sum==0) sum=~0;"  thing or it would need
> to accept both "0" and "~0" in the checksum checking path.

The check is "1's compelement sum of entire packet (including the checksum)
is 0xffff".  So unless *everything* *else* in the packet is 0, 0x0000 and
0xffff in checksum are the same.  And for very obvious reasons "everything
else is 0" won't be recognized as valid packet anyway...

The following is true:
	for any x except 0 we have x # 0xffff equal to x
	for any x we have x # 0 equal to x
	x # y is 0 if and only if both x and y are 0.

So unless the sum of all words except the checksum is 0 (i.e. all words
are except the checksum are 0), 0 and 0xffff in checksum will result in
the same total sums.

Re: [PATCH] IPv6: only modify checksum for UDP

[David Stevens]

David Miller <davem@davemloft.net> wrote on 11/13/2006 04:50:58 PM:
 
> Puzzling :-)  Then why is the transformation only performed for
> UDP in the ipv4 stack?  It seems by your logic TCP would need
> to either do the "if (sum==0) sum=~0;"  thing or it would need
> to accept both "0" and "~0" in the checksum checking path.

        That's actually what I was suggesting. In 1's-complement,
~0 == -0 which is still 0, so barring any special case (like UDP's
"0 means no checksum" rule), it should be equally valid for a
packet to have 0 or ~0 as the checksum (with otherwise identical
data)-- they are both correct, and equal to each other. That
extra 1's-complement 0 is, of course, why UDP can have the
special case of remapping 0->~0.
        Since the patch was for output-side, it doesn't matter
whether you remap 0 to ~0 or not (except for the special case),
but a receiver technically should allow either.
        In practice, it won't matter if the machines you're talking
to are remapping (or not) in the same way. But 0 and -0 (aka ~0)
are still equal. :-)
        It obviously doesn't come up much.

                                                        +-DLS

Re: [PATCH] IPv6: only modify checksum for UDP

[Al Viro]

On Mon, Nov 13, 2006 at 05:44:12PM -0800, David Stevens wrote:
>         That's actually what I was suggesting. In 1's-complement,
> ~0 == -0 which is still 0, so barring any special case (like UDP's
> "0 means no checksum" rule), it should be equally valid for a
> packet to have 0 or ~0 as the checksum (with otherwise identical
> data)-- they are both correct, and equal to each other. That
> extra 1's-complement 0 is, of course, why UDP can have the
> special case of remapping 0->~0.
>         Since the patch was for output-side, it doesn't matter
> whether you remap 0 to ~0 or not (except for the special case),
> but a receiver technically should allow either.

Could you please take a break from your Richard B. Johnson imitations
and read the fscking RFC?

Re: [PATCH] IPv6: only modify checksum for UDP

[David Miller]

From: Brian Haley <brian.haley@hp.com>
Date: Fri, 10 Nov 2006 11:24:45 -0500

> Only change upper-layer checksum from 0 to 0xFFFF for UDP (as RFC 768 
> states), not for others as RFC 4443 doesn't require it.
> 
> Signed-off-by: Brian Haley <brian.haley@hp.com>

Applied, thanks Brian.

Re: [PATCH] IPv6: only modify checksum for UDP

[Nivedita Singhvi]

David Miller wrote:
> From: Brian Haley <brian.haley@hp.com>
> Date: Fri, 10 Nov 2006 11:24:45 -0500
> 
>> Only change upper-layer checksum from 0 to 0xFFFF for UDP (as RFC 768 
>> states), not for others as RFC 4443 doesn't require it.
>>
>> Signed-off-by: Brian Haley <brian.haley@hp.com>
> 
> Applied, thanks Brian.


Brian Haley wrote:
 > Al Viro wrote:
 >> Could you fscking read what you've replied to?  Your -=1 will turn 0
 >> into 0xffff instead of correct 0xfffe.  IOW, it's broken in 1:65536
 >> cases.
 >
 > I looked again at your previous email:
 >
 >> Note that even on little-endian you want
 >>     3 -> 2
 >>     2 -> 1
 >>     1 -> 0xffff
 >>     0 -> 0xfffe
 >
 > That doesn't look right to me, but I'll take your word that there's one
 > edge case out there I don't see (even though this worked on Alpha).
 > Forget about the patch then.

Er, given all of the above, Brian, could you share your test cases
and/or other/any information on testing this?

thanks,
Nivedita

Re: [PATCH] IPv6: only modify checksum for UDP

[David Miller]

From: Nivedita Singhvi <niv@us.ibm.com>
Date: Fri, 10 Nov 2006 15:17:06 -0800

WATCH YOUR QUOTING!

> David Miller wrote:
> > From: Brian Haley <brian.haley@hp.com>
> > Date: Fri, 10 Nov 2006 11:24:45 -0500
> > 
> >> Only change upper-layer checksum from 0 to 0xFFFF for UDP (as RFC 768 
> >> states), not for others as RFC 4443 doesn't require it.
> >>
> >> Signed-off-by: Brian Haley <brian.haley@hp.com>
> > 
> > Applied, thanks Brian.

I applied Brian's FIRST PATCH, which is fine.

> Brian Haley wrote:
>  > Al Viro wrote:
>  >> Could you fscking read what you've replied to?  Your -=1 will turn 0
>  >> into 0xffff instead of correct 0xfffe.  IOW, it's broken in 1:65536
>  >> cases.
>  >
>  > I looked again at your previous email:
>  >
>  >> Note that even on little-endian you want
>  >>     3 -> 2
>  >>     2 -> 1
>  >>     1 -> 0xffff
>  >>     0 -> 0xfffe
>  >
>  > That doesn't look right to me, but I'll take your word that there's one
>  > edge case out there I don't see (even though this worked on Alpha).
>  > Forget about the patch then.
> 
> Er, given all of the above, Brian, could you share your test cases
> and/or other/any information on testing this?

This is a discussion about Brian's SECOND PATCH which needs
fixups.

Please don't quote things out of context like this!

Re: [PATCH] IPv6: only modify checksum for UDP

[Nivedita Singhvi]

David Miller wrote:
> From: Nivedita Singhvi <niv@us.ibm.com>
> Date: Fri, 10 Nov 2006 15:17:06 -0800
> 
> WATCH YOUR QUOTING!

That explains it! Arghhhh! Sorry, DaveM, a bit on autopilot
trying to put some tests together..

thanks,
Nivedita

Re: [PATCH] IPv6: only modify checksum for UDP

[Brian Haley]

> This is a discussion about Brian's SECOND PATCH which needs
> fixups.

Please forget about the second patch, optimizing this code path isn't 
worth it if the -1 trick doesn't work in all cases.

-Brian

[PATCH] IPv6: optimize echo reply checksum calculation

[Brian Haley]

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

Since the only difference between echo requests and echo replies is the 
ICMPv6 type value (which is a difference of 1), just subtracting one 
from the request checksum will result in the correct checksum for the reply.

Signed-off-by: Brian Haley <brian.haley@hp.com>

[-- Attachment #2: no.cksum.echo.diff --]
[-- Type: text/x-patch, Size: 2414 bytes --]

diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
index dbb9b1f..ee04610 100644
--- a/net/ipv6/icmp.c
+++ b/net/ipv6/icmp.c
@@ -212,7 +212,7 @@ static __inline__ int opt_unrec(struct s
 	return (*op & 0xC0) == 0x80;
 }
 
-static int icmpv6_push_pending_frames(struct sock *sk, struct flowi *fl, struct icmp6hdr *thdr, int len)
+static int icmpv6_push_pending_frames(struct sock *sk, struct flowi *fl, struct icmp6hdr *thdr, int len, int cksum_needed)
 {
 	struct sk_buff *skb;
 	struct icmp6hdr *icmp6h;
@@ -223,7 +223,9 @@ static int icmpv6_push_pending_frames(st
 
 	icmp6h = (struct icmp6hdr*) skb->h.raw;
 	memcpy(icmp6h, thdr, sizeof(struct icmp6hdr));
-	icmp6h->icmp6_cksum = 0;
+
+	if (!cksum_needed)
+		goto sendit;
 
 	if (skb_queue_len(&sk->sk_write_queue) == 1) {
 		skb->csum = csum_partial((char *)icmp6h,
@@ -246,6 +248,7 @@ static int icmpv6_push_pending_frames(st
 					   len, fl->proto, tmp_csum);
 		icmp6h->icmp6_cksum = tmp_csum;
 	}
+sendit:
 	ip6_push_pending_frames(sk);
 out:
 	return err;
@@ -451,7 +454,7 @@ void icmpv6_send(struct sk_buff *skb, in
 		ip6_flush_pending_frames(sk);
 		goto out_put;
 	}
-	err = icmpv6_push_pending_frames(sk, &fl, &tmp_hdr, len + sizeof(struct icmp6hdr));
+	err = icmpv6_push_pending_frames(sk, &fl, &tmp_hdr, len + sizeof(struct icmp6hdr), 1);
 
 	if (type >= ICMPV6_DEST_UNREACH && type <= ICMPV6_PARAMPROB)
 		ICMP6_INC_STATS_OFFSET_BH(idev, ICMP6_MIB_OUTDESTUNREACHS, type - ICMPV6_DEST_UNREACH);
@@ -489,6 +492,14 @@ static void icmpv6_echo_reply(struct sk_
 	memcpy(&tmp_hdr, icmph, sizeof(tmp_hdr));
 	tmp_hdr.icmp6_type = ICMPV6_ECHO_REPLY;
 
+	/*
+	 * The only difference between echo requests and echo replies is the
+	 * ICMPv6 type value (which is a difference of 1).  So if we subtract
+	 * one from the request checksum, it will result in the correct
+	 * checksum for the reply.
+	 */
+	tmp_hdr.icmp6_cksum--;
+
 	memset(&fl, 0, sizeof(fl));
 	fl.proto = IPPROTO_ICMPV6;
 	ipv6_addr_copy(&fl.fl6_dst, &skb->nh.ipv6h->saddr);
@@ -540,7 +551,7 @@ static void icmpv6_echo_reply(struct sk_
 		ip6_flush_pending_frames(sk);
 		goto out_put;
 	}
-	err = icmpv6_push_pending_frames(sk, &fl, &tmp_hdr, skb->len + sizeof(struct icmp6hdr));
+	err = icmpv6_push_pending_frames(sk, &fl, &tmp_hdr, skb->len + sizeof(struct icmp6hdr), 0);
 
         ICMP6_INC_STATS_BH(idev, ICMP6_MIB_OUTECHOREPLIES);
         ICMP6_INC_STATS_BH(idev, ICMP6_MIB_OUTMSGS);

Re: [PATCH] IPv6: optimize echo reply checksum calculation

[Al Viro]

On Fri, Nov 10, 2006 at 11:25:53AM -0500, Brian Haley wrote:
> Since the only difference between echo requests and echo replies is the 
> ICMPv6 type value (which is a difference of 1), just subtracting one 
> from the request checksum will result in the correct checksum for the reply.

Um, no.  That will *not* result in correct checksum.  Please, RTFRFC 1071.

Re: [PATCH] IPv6: optimize echo reply checksum calculation

[Brian Haley]

Al Viro wrote:
> On Fri, Nov 10, 2006 at 11:25:53AM -0500, Brian Haley wrote:
>> Since the only difference between echo requests and echo replies is the 
>> ICMPv6 type value (which is a difference of 1), just subtracting one 
>> from the request checksum will result in the correct checksum for the reply.
> 
> Um, no.  That will *not* result in correct checksum.  Please, RTFRFC 1071.

I verified this works for echo request/reply on my IA64 box, 
double-checked with ethereal/wireshark.  Is there something specific in 
RFC 1071 that I should be looking for?

-Brian

Re: [PATCH] IPv6: optimize echo reply checksum calculation

[Al Viro]

On Fri, Nov 10, 2006 at 12:51:19PM -0500, Brian Haley wrote:
> Al Viro wrote:
> >On Fri, Nov 10, 2006 at 11:25:53AM -0500, Brian Haley wrote:
> >>Since the only difference between echo requests and echo replies is the 
> >>ICMPv6 type value (which is a difference of 1), just subtracting one 
> >>from the request checksum will result in the correct checksum for the 
> >>reply.
> >
> >Um, no.  That will *not* result in correct checksum.  Please, RTFRFC 1071.
> 
> I verified this works for echo request/reply on my IA64 box, 
> double-checked with ethereal/wireshark.  Is there something specific in 
> RFC 1071 that I should be looking for?

Definition of checksum.

See also include/net/ip.h::ip_decrease_ttl() for similar situation.

Re: [PATCH] IPv6: optimize echo reply checksum calculation

[Al Viro]

On Fri, Nov 10, 2006 at 06:05:34PM +0000, Al Viro wrote:
> On Fri, Nov 10, 2006 at 12:51:19PM -0500, Brian Haley wrote:
> > Al Viro wrote:
> > >On Fri, Nov 10, 2006 at 11:25:53AM -0500, Brian Haley wrote:
> > >>Since the only difference between echo requests and echo replies is the 
> > >>ICMPv6 type value (which is a difference of 1), just subtracting one 
> > >>from the request checksum will result in the correct checksum for the 
> > >>reply.
> > >
> > >Um, no.  That will *not* result in correct checksum.  Please, RTFRFC 1071.
> > 
> > I verified this works for echo request/reply on my IA64 box, 
> > double-checked with ethereal/wireshark.  Is there something specific in 
> > RFC 1071 that I should be looking for?
> 
> Definition of checksum.
> 
> See also include/net/ip.h::ip_decrease_ttl() for similar situation.

Note that even on little-endian you want
	3 -> 2
	2 -> 1
	1 -> 0xffff
	0 -> 0xfffe

On big-endian you get

	0x102 -> 2
	0x101 -> 1
	0x100 -> 0xffff
	0xff  -> 0xfffe
	...
	0     -> 0xfeff

so -= 1 is broken even on ia64 and it's *always* broken on big-endian
boxen.

Re: [PATCH] IPv6: optimize echo reply checksum calculation

[Brian Haley]

Al Viro wrote:
> so -= 1 is broken even on ia64 and it's *always* broken on big-endian
> boxen.

It's not broken in ia64, I've tested that, just don't have an x86 for 
testing right now.  Can you please apply these changes and prove it's 
broken?  This little trick has been done in other UNIXes for years 
without any problems.

-Brian

Re: [PATCH] IPv6: optimize echo reply checksum calculation

[Al Viro]

On Fri, Nov 10, 2006 at 02:04:32PM -0500, Brian Haley wrote:
> Al Viro wrote:
> >so -= 1 is broken even on ia64 and it's *always* broken on big-endian
> >boxen.
> 
> It's not broken in ia64, I've tested that, just don't have an x86 for 
> testing right now.  Can you please apply these changes and prove it's 
> broken?  This little trick has been done in other UNIXes for years 
> without any problems.

Could you fscking read what you've replied to?  Your -=1 will turn 0
into 0xffff instead of correct 0xfffe.  IOW, it's broken in 1:65536
cases.

On big-endian boxen (which x86 is not, BTW), it will *always* give the
wrong result. -= htons(0x100) is better, but still fscks up in 1:256.

You _can_ adjust the checksum; however it's not that trivial.  One working
variant is
	if (sum > htons(0x100)
		sum -= htons(0x100);
	else
		sum += 0xffff - htons(0x100);
In little-endian case it turns into
	if (sum > 1)
		sum--;
	else
		sum += 0xfffe;
which is why your variant breaks rarely on itanic (or x86 - they are not
different in that respect).  You still need to handle that corner case,
though.

As for the various Unices doing the same trick, I suggest you to check
what they _really_ do and report bugs for those that have pure -- and
nothing else.

Again, the checksum is sum modulo 0xffff, *not* 0x10000.  And endianness
matters, of course, since the packet type is either LSB or MSB, depending
on it.

Re: [PATCH] IPv6: optimize echo reply checksum calculation

[Brian Haley]

Al Viro wrote:
> On Fri, Nov 10, 2006 at 02:04:32PM -0500, Brian Haley wrote:
>> Al Viro wrote:
>>> so -= 1 is broken even on ia64 and it's *always* broken on big-endian
>>> boxen.
>> It's not broken in ia64, I've tested that, just don't have an x86 for 
>> testing right now.  Can you please apply these changes and prove it's 
>> broken?  This little trick has been done in other UNIXes for years 
>> without any problems.
> 
> Could you fscking read what you've replied to?  Your -=1 will turn 0
> into 0xffff instead of correct 0xfffe.  IOW, it's broken in 1:65536
> cases.

I looked again at your previous email:

> Note that even on little-endian you want
> 	3 -> 2
> 	2 -> 1
> 	1 -> 0xffff
> 	0 -> 0xfffe

That doesn't look right to me, but I'll take your word that there's one 
edge case out there I don't see (even though this worked on Alpha). 
Forget about the patch then.

-Brian

Re: [PATCH] IPv6: optimize echo reply checksum calculation

[Al Viro]

On Fri, Nov 10, 2006 at 04:06:52PM -0500, Brian Haley wrote:
> Al Viro wrote:
> >On Fri, Nov 10, 2006 at 02:04:32PM -0500, Brian Haley wrote:
> >>Al Viro wrote:
> >>>so -= 1 is broken even on ia64 and it's *always* broken on big-endian
> >>>boxen.
> >>It's not broken in ia64, I've tested that, just don't have an x86 for 
> >>testing right now.  Can you please apply these changes and prove it's 
> >>broken?  This little trick has been done in other UNIXes for years 
> >>without any problems.
> >
> >Could you fscking read what you've replied to?  Your -=1 will turn 0
> >into 0xffff instead of correct 0xfffe.  IOW, it's broken in 1:65536
> >cases.
> 
> I looked again at your previous email:
> 
> >Note that even on little-endian you want
> >	3 -> 2
> >	2 -> 1
> >	1 -> 0xffff
> >	0 -> 0xfffe
> 
> That doesn't look right to me, but I'll take your word that there's one 
> edge case out there I don't see (even though this worked on Alpha). 

Sigh...  Here's how the checksum is defined:
	take array of bytes
	pad it to even length with 0
	split it into 16bit words
define x # y as (x + y) < 0x10000 ? x + y: x + y - 0xffff
	calculate sum = w0 # w1 # w2 # ..... # wn
	take 0xffff - sum (aka ~sum)
	store the resulting value in pair of bytes (with whatever
endianness you used all along).

Note that resulting pair of bytes does *not* depend on endianness.  See
aforementioned RFC for proof and for more fun properties of that sucker.

Re: why do we mangle checksums for v6 ICMP?

[Bill Fink]

On Thu, 09 Nov 2006, David Miller wrote:

> From: Brian Haley <brian.haley@hp.com>
> Date: Thu, 09 Nov 2006 12:32:18 -0500
> 
> > Al Viro wrote:
> > > 	AFAICS, the rules are:
> > > 
> > > (1) checksum is 16-bit one's complement of the one's complement sum of
> > > relevant 16bit words.
> > > 
> > > (2) for v4 UDP all-zeroes has special meaning - no checksum; if you get
> > > it from (1), send all-ones instead.
> > > 
> > > (3) for v6 UDP we have the same remapping as in (2), but all-zeroes has
> > > different meaning - not "ignore checksum" as in v4, but "reject the
> > > packet".
> > > 
> > > (4) there is no (4).
> > > 
> > > 	IOW, nobody except UDP has any business doing that 0->0xffff
> > > replacement.  However, we have
> > >        if (icmp6h->icmp6_cksum == 0)
> > >                icmp6h->icmp6_cksum = -1;
> > 
> > This doesn't look necessary, RFCs 4443/2463 don't mention it being 
> > necessary, and BSD doesn't do it either.  I'll cook-up a patch to remove 
> > that since I was doing some other mods in that codepath.
> 
> This is how things look to me too.
> 
> > > and similar in net/ipv6/raw.c
> > 
> > Maybe here it only needs to be done if (fl->proto == IPPROTO_UDP)?
> 
> Yes, I believe that is what is needed.

On a raw IPv6 socket, shouldn't the IP checksum just be left
unchanged, so you can test transmission of IPv6 packets with
an invalid zero IP checksum.  Or is raw not fully raw?

						-Bill

Re: why do we mangle checksums for v6 ICMP?

[David Miller]

From: Bill Fink <billfink@mindspring.com>
Date: Sat, 11 Nov 2006 13:07:24 -0500

> On a raw IPv6 socket, shouldn't the IP checksum just be left
> unchanged, so you can test transmission of IPv6 packets with
> an invalid zero IP checksum.  Or is raw not fully raw?

The code path where we do the "0 --> -1" conversion only executes when
inet->hdrincl is false.