[PATCH]SPARC32: Fixed unaligned memory copying in function

[PATCH]SPARC32: Fixed unaligned memory copying in function

[kirill]

When we are in the label cc_dword_align, registers %o0 and %o1 have the
same last 2 bits, but it's not guaranteed any one of them is zero. So we
can get unaligned memory access in label ccte. Example of parameters
which lead to this: %o0=0x7ff183e9, %o1=0x8e709e7d, %g1=3

With the parameters I had a memory corruption, when the additional 5
bytes were rewritten. This patch corrects the error.

One comment to patch. We don't care about the third bit in %o1, because
cc_end_cruft stores word or less.

Signed-off-by: Tkhai Kirill <tkhai@yandex.ru>
---

--- linux-2.6.38.5/arch/sparc/lib/checksum_32.S.orig	2011-05-06
22:54:25.000000000 +0400
+++ linux-2.6.38.5/arch/sparc/lib/checksum_32.S	2011-05-08
11:43:35.000000000 +0400
@@ -289,10 +289,16 @@ cc_end_cruft:
 
 	/* Also, handle the alignment code out of band. */
 cc_dword_align:
-	cmp	%g1, 6
-	bl,a	ccte
+	cmp	%g1, 16
+	bge,a	1f
+	 srl	%g1, 1, %o3
+2:	cmp	%o3, 0
+	be,a	ccte
 	 andcc	%g1, 0xf, %o3
-	andcc	%o0, 0x1, %g0
+	andcc	%o3, %o0, %g0	! Check %o0 only (%o1 has the same last 2 bits)
+	be,a	2b
+	 srl	%o3, 1, %o3
+1:	andcc	%o0, 0x1, %g0
 	bne	ccslow
 	 andcc	%o0, 0x2, %g0
 	be	1f

Re: [PATCH]SPARC32: Fixed unaligned memory copying in function

[David Miller]

From: kirill <tkhai@yandex.ru>
Date: Sun, 08 May 2011 13:05:58 +0400

> --- linux-2.6.38.5/arch/sparc/lib/checksum_32.S.orig	2011-05-06
> 22:54:25.000000000 +0400
> +++ linux-2.6.38.5/arch/sparc/lib/checksum_32.S	2011-05-08
> 11:43:35.000000000 +0400
> @@ -289,10 +289,16 @@ cc_end_cruft:

Your patch is corrupted, it has chopped up long lines.

Please fix this up, and send a test patch email to yourself.

Only when you can successfully apply the patch you receive in
that test email should you post your patch here to the list
again.

Because if you can't apply the patch, neither can I.

Thanks.

Re: [PATCH]SPARC32: Fixed unaligned memory copying in function __csum_partial_copy_sparc_generic

[Tkhai Kirill]

>Your patch is corrupted, it has chopped up long lines.

>Please fix this up, and send a test patch email to yourself.

It's strange... I had did it at least twice before I sent the first message. And it's possible for mine diff to apply patch from http://spinics.net/lists/sparclinux mail list without any warnings.

Ok, I'm sending patch from another mail program:

--- linux-2.6.38.5/arch/sparc/lib/checksum_32.S.orig	2011-05-06 22:54:25.000000000 +0400
+++ linux-2.6.38.5/arch/sparc/lib/checksum_32.S	2011-05-08 11:43:35.000000000 +0400
@@ -289,10 +289,16 @@ cc_end_cruft:
 
 	/* Also, handle the alignment code out of band. */
 cc_dword_align:
-	cmp	%g1, 6
-	bl,a	ccte
+	cmp	%g1, 16
+	bge,a	1f
+	 srl	%g1, 1, %o3
+2:	cmp	%o3, 0
+	be,a	ccte
 	 andcc	%g1, 0xf, %o3
-	andcc	%o0, 0x1, %g0
+	andcc	%o3, %o0, %g0	! Check %o0 only (%o1 has the same last 2 bits)
+	be,a	2b
+	 srl	%o3, 1, %o3
+1:	andcc	%o0, 0x1, %g0
 	bne	ccslow
 	 andcc	%o0, 0x2, %g0
 	be	1f

Re: [PATCH]SPARC32: Fixed unaligned memory copying in function

[David Miller]

From: Tkhai Kirill <tkhai@yandex.ru>
Date: Mon, 09 May 2011 12:49:24 +0400

>>Your patch is corrupted, it has chopped up long lines.
> 
>>Please fix this up, and send a test patch email to yourself.
> 
> It's strange... I had did it at least twice before I sent the first message. And it's possible for mine diff to apply patch from http://spinics.net/lists/sparclinux mail list without any warnings.
> 
> Ok, I'm sending patch from another mail program:

Please don't just reply with a new patch and make it part of the
discussion thread.

Instead, make a fresh new posting with the full commit log message
and patch.

Thanks.

[PATCH]SPARC32: Fixed unaligned memory copying in function __csum_partial_copy_sparc_generic

[Tkhai Kirill]

When we are in the label cc_dword_align, registers %o0 and %o1 have the same last 2 bits,
but it's not guaranteed one of they is zero. So we can get unaligned memory access
in label ccte. Example of parameters which lead to this:
%o0=0x7ff183e9, %o1=0x8e709e7d, %g1=3

With the parameters I had a memory corruption, when the additional 5 bytes were rewritten.
This patch corrects the error.

One comment to the patch. We don't care about the third bit in %o1, because cc_end_cruft
stores word or less.

Signed-off-by: Tkhai Kirill <tkhai@yandex.ru>
---

--- linux-2.6.38.5/arch/sparc/lib/checksum_32.S.orig	2011-05-06 22:54:25.000000000 +0400
+++ linux-2.6.38.5/arch/sparc/lib/checksum_32.S	2011-05-08 11:43:35.000000000 +0400
@@ -289,10 +289,16 @@ cc_end_cruft:
 
 	/* Also, handle the alignment code out of band. */
 cc_dword_align:
-	cmp	%g1, 6
-	bl,a	ccte
+	cmp	%g1, 16
+	bge,a	1f
+	 srl	%g1, 1, %o3
+2:	cmp	%o3, 0
+	be,a	ccte
 	 andcc	%g1, 0xf, %o3
-	andcc	%o0, 0x1, %g0
+	andcc	%o3, %o0, %g0	! Check %o0 only (%o1 has the same last 2 bits)
+	be,a	2b
+	 srl	%o3, 1, %o3
+1:	andcc	%o0, 0x1, %g0
 	bne	ccslow
 	 andcc	%o0, 0x2, %g0
 	be	1f

[PATCH]SPARC32: Fixed unaligned memory copying in function __csum_partial_copy_sparc_generic

[Tkhai Kirill]

Hello, Dave!

I'm sorry, there is a error in my previous letter. I added excess annul field to bge.
Final patch from me is following:

When we are in the label cc_dword_align, registers %o0 and %o1 have the same last 2 bits,
but it's not guaranteed one of they is zero. So we can get unaligned memory access
in label ccte. Example of parameters which lead to this:
%o0=0x7ff183e9, %o1=0x8e709e7d, %g1=3

With the parameters I had a memory corruption, when the additional 5 bytes were rewritten.
This patch corrects the error.

One comment to the patch. We don't care about the third bit in %o1, because cc_end_cruft
stores word or less.

Thanks. Kirill.

Signed-off-by: Tkhai Kirill <tkhai@yandex.ru>
---

--- linux-2.6.38.5/arch/sparc/lib/checksum_32.S.orig	2011-05-06 22:54:25.000000000 +0400
+++ linux-2.6.38.5/arch/sparc/lib/checksum_32.S	2011-05-08 11:43:35.000000000 +0400
@@ -289,10 +289,16 @@ cc_end_cruft:
 
 	/* Also, handle the alignment code out of band. */
 cc_dword_align:
-	cmp	%g1, 6
-	bl,a	ccte
+	cmp	%g1, 16
+	bge	1f
+	 srl	%g1, 1, %o3
+2:	cmp	%o3, 0
+	be,a	ccte
 	 andcc	%g1, 0xf, %o3
-	andcc	%o0, 0x1, %g0
+	andcc	%o3, %o0, %g0	! Check %o0 only (%o1 has the same last 2 bits)
+	be,a	2b
+	 srl	%o3, 1, %o3
+1:	andcc	%o0, 0x1, %g0
 	bne	ccslow
 	 andcc	%o0, 0x2, %g0
 	be	1f

Re: [PATCH]SPARC32: Fixed unaligned memory copying in function

[David Miller]

From: Tkhai Kirill <tkhai@yandex.ru>
Date: Tue, 10 May 2011 16:31:41 +0400

>  	/* Also, handle the alignment code out of band. */
>  cc_dword_align:
> -	cmp	%g1, 6
> -	bl,a	ccte
> +	cmp	%g1, 16
> +	bge	1f
> +	 srl	%g1, 1, %o3
> +2:	cmp	%o3, 0
> +	be,a	ccte
>  	 andcc	%g1, 0xf, %o3
> -	andcc	%o0, 0x1, %g0
> +	andcc	%o3, %o0, %g0	! Check %o0 only (%o1 has the same last 2 bits)
> +	be,a	2b
> +	 srl	%o3, 1, %o3
> +1:	andcc	%o0, 0x1, %g0

I don't understand why you're testing "andcc %o3, %o0, %g0", as %o3 has
the value computed in "srl %g1, 1, %o3" here.

What is so interesting about "ptr & (length >> 1)"?

Re: [PATCH]SPARC32: Fixed unaligned memory copying in function __csum_partial_copy_sparc_generic

[Tkhai Kirill]

11.05.11, 00:28, "David Miller" <davem@davemloft.net>:

>I don't understand why you're testing "andcc %o3, %o0, %g0", as %o3 has
>the value computed in "srl %g1, 1, %o3" here.

>What is so interesting about "ptr & (length >> 1)"?

1)Label ccte (rather, cc_end_cruft) copies data sequentially by pieces of length 2^x bytes from biggest to smallest. For example, if %g1 is 15, then the label copies 8+4+2+1\x15, and %o0 must be aligned to 8.

Let alignment a = 2^x. Pointer ptr % a = 0 when:
ptr & (a >> 1) = 0
ptr & (a >> 2) = 0
..

Or, what is the same:
ptr & (length >> 1) = 0
ptr & (length >> 2) = 0
..

I.e. we are looking for right alignment, if another case don't call ccte.

2)We check %o0 only, because %o1 has the same last two bits, and cc_end_cruft writes 4 bytes or less.

Re: [PATCH]SPARC32: Fixed unaligned memory copying in function __csum_partial_copy_sparc_generic

[Daniel Hellstrom]

Tkhai Kirill wrote:

>When we are in the label cc_dword_align, registers %o0 and %o1 have the same last 2 bits,
>but it's not guaranteed one of they is zero. So we can get unaligned memory access
>in label ccte. Example of parameters which lead to this:
>%o0=0x7ff183e9, %o1=0x8e709e7d, %g1=3
>  
>
I just wanted to add that I also got unaligned accesses in the checksum 
calculation on the SPARC32/LEON. Bad alignment is bad for performance of 
course, in my case the MNA-trap handler was erroneous storing 
incorrectly and that was the reason for me to notice it. I did not look 
at the reason for the unaligned access in the first place though.

Daniel


commit 2492218c63dca0fb4f041bdc366d243ae3426b40
Author: Daniel Hellstrom <daniel@gaisler.com>
Date:   Tue Feb 1 12:39:59 2011 -0800

    sparc32: unaligned memory access (MNA) trap handler bug

    Since commit f0e98c387e61de00646be31fab4c2fa0224e1efb ("[SPARC]: Fix
    link errors with gcc-4.3") the MNA trap handler does not emulate
    stores to unaligned addresses correctly. MNA operation from both
    kernel and user space are affected.

    A typical effect of this bug is nr_frags in skbs are overwritten
    during buffer copying/checksum-calculation, or maximally 6 bytes
    of data in the network buffer will be overwitten with garbage.

    Signed-off-by: Daniel Hellstrom <daniel@gaisler.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

Re: [PATCH]SPARC32: Fixed unaligned memory copying in function

[David Miller]

From: Daniel Hellstrom <daniel@gaisler.com>
Date: Wed, 11 May 2011 14:53:40 +0200

[ Warning, I'm testing a new email setup so if this looks strange
  or doesn't go out properly, my bad... ]

> I just wanted to add that I also got unaligned accesses in the
> checksum calculation on the SPARC32/LEON. Bad alignment is bad for
> performance of course, in my case the MNA-trap handler was erroneous
> storing incorrectly and that was the reason for me to notice it. I did
> not look at the reason for the unaligned access in the first place
> though.

I totally agree that, without question, we should fix this bug
in the 32-bit checksumming routines.  I just want to make sure
I absolutely understand the new logic before I apply the patch
:-)

>    sparc32: unaligned memory access (MNA) trap handler bug

And indeed, it's great that we've fixed this bug meanwhile too.

Thanks.

Re: [PATCH]SPARC32: Fixed unaligned memory copying in function

[David Miller]

From: Tkhai Kirill <tkhai@yandex.ru>
Date: Wed, 11 May 2011 03:21:49 +0400

> 11.05.11, 00:28, "David Miller" <davem@davemloft.net>:
> 
>>I don't understand why you're testing "andcc %o3, %o0, %g0", as %o3 has
>>the value computed in "srl %g1, 1, %o3" here.
> 
>>What is so interesting about "ptr & (length >> 1)"?
> 
> 1)Label ccte (rather, cc_end_cruft) copies data sequentially by pieces of length 2^x bytes from biggest to smallest. For example, if %g1 is 15, then the label copies 8+4+2+1\x15, and %o0 must be aligned to 8.
> 
> Let alignment a = 2^x. Pointer ptr % a = 0 when:
> ptr & (a >> 1) = 0
> ptr & (a >> 2) = 0
> ..
> 
> Or, what is the same:
> ptr & (length >> 1) = 0
> ptr & (length >> 2) = 0
> ..
> 
> I.e. we are looking for right alignment, if another case don't call ccte.
> 
> 2)We check %o0 only, because %o1 has the same last two bits, and cc_end_cruft writes 4 bytes or less.

Now I understand, thanks for explaining.

Applied.