[RFC][PATCH] ia64: fix csum_ipv6_magic()

[RFC][PATCH] ia64: fix csum_ipv6_magic()

[Jiri Bohac]

Hi,

I was seeing "nf_ct_icmpv6: ICMPv6 checksum failed" errors on
every ICMPv6 packet received. The packets did not pass the
netfilter checksum verification but they did pass the standard
verification later and got processed. This happenns with hardware
checksumming turned off or with adapters that do not checksum
ICMP packets (e.g. tg3). I tracked the problem down to the ia64
version of csum_ipv6_magic() introduced by 007d77d0c5.  For some
arguments, it gives differrent results than the generic version.

The following patch fixes the problem for me. IA64 experts, can
you please have a look?

Thanks!


[IA64] fix csum_ipv6_magic()

The 32-bit parameters (len and csum) of csum_ipv6_magic() are passed in 64-bit
registers in3 and in4. The high order 32 bits of the registers were never
cleared, and garbage was sometimes calculated into the checksum.

Fix this by clearing the high order 32 bits of the registers.

Signed-off-by: Jiri Bohac <jbohac@suse.cz>

diff --git a/arch/ia64/lib/ip_fast_csum.S b/arch/ia64/lib/ip_fast_csum.S
index 1f86aeb..9a8d23f 100644
--- a/arch/ia64/lib/ip_fast_csum.S
+++ b/arch/ia64/lib/ip_fast_csum.S
@@ -96,20 +96,22 @@ END(ip_fast_csum)
 GLOBAL_ENTRY(csum_ipv6_magic)
 	ld4	r20=[in0],4
 	ld4	r21=[in1],4
-	dep	r15=in3,in2,32,16
+	zxt4	in3=in3
 	;;
 	ld4	r22=[in0],4
 	ld4	r23=[in1],4
-	mux1	r15=r15,@rev
+	dep	r15=in3,in2,32,16
 	;;
 	ld4	r24=[in0],4
 	ld4	r25=[in1],4
-	shr.u	r15=r15,16
+	mux1	r15=r15,@rev
 	add	r16=r20,r21
 	add	r17=r22,r23
+	zxt4	in4=in4
 	;;
 	ld4	r26=[in0],4
 	ld4	r27=[in1],4
+	shr.u	r15=r15,16
 	add	r18=r24,r25
 	add	r8=r16,r17
 	;;
-- 
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ

RE: [RFC][PATCH] ia64: fix csum_ipv6_magic()

[Yu, Fenghua]

>I was seeing "nf_ct_icmpv6: ICMPv6 checksum failed" errors on
>every ICMPv6 packet received. The packets did not pass the
>netfilter checksum verification but they did pass the standard
>verification later and got processed. This happenns with hardware
>checksumming turned off or with adapters that do not checksum
>ICMP packets (e.g. tg3). I tracked the problem down to the ia64
>version of csum_ipv6_magic() introduced by 007d77d0c5.  For some
>arguments, it gives differrent results than the generic version.
>
>The following patch fixes the problem for me. IA64 experts, can
>you please have a look?
>
>Thanks!
>
>
>[IA64] fix csum_ipv6_magic()
>
>The 32-bit parameters (len and csum) of csum_ipv6_magic() are passed in 64-
>bit
>registers in3 and in4. The high order 32 bits of the registers were never
>cleared, and garbage was sometimes calculated into the checksum.
>
>Fix this by clearing the high order 32 bits of the registers.
>
>Signed-off-by: Jiri Bohac <jbohac@suse.cz>
>
>diff --git a/arch/ia64/lib/ip_fast_csum.S b/arch/ia64/lib/ip_fast_csum.S
>index 1f86aeb..9a8d23f 100644
>--- a/arch/ia64/lib/ip_fast_csum.S
>+++ b/arch/ia64/lib/ip_fast_csum.S
>@@ -96,20 +96,22 @@ END(ip_fast_csum)
> GLOBAL_ENTRY(csum_ipv6_magic)
> 	ld4	r20=[in0],4
> 	ld4	r21=[in1],4
>-	dep	r15=in3,in2,32,16
>+	zxt4	in3=in3
> 	;;
> 	ld4	r22=[in0],4
> 	ld4	r23=[in1],4
>-	mux1	r15=r15,@rev
>+	dep	r15=in3,in2,32,16
> 	;;
> 	ld4	r24=[in0],4
> 	ld4	r25=[in1],4
>-	shr.u	r15=r15,16
>+	mux1	r15=r15,@rev
> 	add	r16=r20,r21
> 	add	r17=r22,r23
>+	zxt4	in4=in4
> 	;;
> 	ld4	r26=[in0],4
> 	ld4	r27=[in1],4
>+	shr.u	r15=r15,16
> 	add	r18=r24,r25
> 	add	r8=r16,r17
> 	;;

Looks good to me. Do you know the caller function for the failed csum_ipv6_magic()?

Thanks.

-Fenghua

Re: [RFC][PATCH] ia64: fix csum_ipv6_magic()

[Jiri Bohac]

On Fri, Aug 28, 2009 at 02:19:19PM -0700, Yu, Fenghua wrote:
> >I was seeing "nf_ct_icmpv6: ICMPv6 checksum failed" errors on
> >every ICMPv6 packet received. The packets did not pass the
> >netfilter checksum verification but they did pass the standard
> >verification later and got processed. This happenns with hardware
> >checksumming turned off or with adapters that do not checksum
> >ICMP packets (e.g. tg3). I tracked the problem down to the ia64
> >version of csum_ipv6_magic() introduced by 007d77d0c5.  For some
> >arguments, it gives differrent results than the generic version.
> >
> >The following patch fixes the problem for me. IA64 experts, can
> >you please have a look?
> 
> Looks good to me. Do you know the caller function for the failed csum_ipv6_magic()?

I saw the problem when csum_ipv6_magic() was called from
nf_ip6_checksum(), in the CHECKSUM_NONE branch.

The the bug is triggered by having some bits set in the high
32-bits of the register used to pass the sum parameter to
csum_ipv6_magic().

For some reason, the 
	csum_sub(skb->csum, skb_checksum(skb, 0, dataoff, 0))
in the CHECKSUM_COMPLETE branch does not seem to trigger this,
while the
	csum_sub(0, skb_checksum(skb, 0, dataoff, 0))
in the CHECKSUM_NONE branch does.

I haven't looked at the disassembly to see why, though.

-- 
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ

RE: [RFC][PATCH] ia64: fix csum_ipv6_magic()

[Luck, Tony]

>--- a/arch/ia64/lib/ip_fast_csum.S
>+++ b/arch/ia64/lib/ip_fast_csum.S
>@@ -96,20 +96,22 @@ END(ip_fast_csum)
> GLOBAL_ENTRY(csum_ipv6_magic)
> 	ld4	r20=[in0],4
> 	ld4	r21=[in1],4
>-	dep	r15=in3,in2,32,16
>+	zxt4	in3=in3

I think this zxt4 instruction have a typo.  You really want to zap the
high part on "in2" here (the "len") parameter.  "in3" contains the "proto"
argument, which is only 16-bits.  But any garbage in the high part on in3
will be dropped by the "dep" instruction later which only pulls out the low
16 bits from it.

So I think you meant to type:

	zxt4	in2=in2

Does this make sense?

-Tony

Re: [RFC][PATCH] ia64: fix csum_ipv6_magic()

[Jiri Bohac]

On Tue, Sep 01, 2009 at 04:20:34PM -0700, Luck, Tony wrote:
> >--- a/arch/ia64/lib/ip_fast_csum.S
> >+++ b/arch/ia64/lib/ip_fast_csum.S
> >@@ -96,20 +96,22 @@ END(ip_fast_csum)
> > GLOBAL_ENTRY(csum_ipv6_magic)
> > 	ld4	r20=[in0],4
> > 	ld4	r21=[in1],4
> >-	dep	r15=in3,in2,32,16
> >+	zxt4	in3=in3
> 
> I think this zxt4 instruction have a typo.  You really want to zap the
> high part on "in2" here (the "len") parameter.  "in3" contains the "proto"
> argument, which is only 16-bits.  But any garbage in the high part on in3
> will be dropped by the "dep" instruction later which only pulls out the low
> 16 bits from it.
> 
> So I think you meant to type:
> 
> 	zxt4	in2=in2
> 
> Does this make sense?

Yes, exactly, you are right. Thanks for spotting it. The fixed patch
follows:


[IA64] fix csum_ipv6_magic()

The 32-bit parameters (len and csum) of csum_ipv6_magic() are passed in 64-bit
registers in3 and in4. The high order 32 bits of the registers were never
cleared, and garbage was sometimes calculated into the checksum.

Fix this by clearing the high order 32 bits of the registers.

Signed-off-by: Jiri Bohac <jbohac@suse.cz>

diff --git a/arch/ia64/lib/ip_fast_csum.S b/arch/ia64/lib/ip_fast_csum.S
index 1f86aeb..9a8d23f 100644
--- a/arch/ia64/lib/ip_fast_csum.S
+++ b/arch/ia64/lib/ip_fast_csum.S
@@ -96,20 +96,22 @@ END(ip_fast_csum)
 GLOBAL_ENTRY(csum_ipv6_magic)
 	ld4	r20=[in0],4
 	ld4	r21=[in1],4
-	dep	r15=in3,in2,32,16
+	zxt4	in2=in2
 	;;
 	ld4	r22=[in0],4
 	ld4	r23=[in1],4
-	mux1	r15=r15,@rev
+	dep	r15=in3,in2,32,16
 	;;
 	ld4	r24=[in0],4
 	ld4	r25=[in1],4
-	shr.u	r15=r15,16
+	mux1	r15=r15,@rev
 	add	r16=r20,r21
 	add	r17=r22,r23
+	zxt4	in4=in4
 	;;
 	ld4	r26=[in0],4
 	ld4	r27=[in1],4
+	shr.u	r15=r15,16
 	add	r18=r24,r25
 	add	r8=r16,r17
 	;;


-- 
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ