SPARC32: Fixed unaligned memory copying in function

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.

---

--- 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

From: kirill <[email protected]>
Date: Sun, 08 May 2011 13:05:58 +0400

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:

@@ -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

From: Tkhai Kirill <[email protected]>
Date: Mon, 09 May 2011 12:49:24 +0400

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.

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.

---

@@ -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

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.

---

@@ -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

From: Tkhai Kirill <[email protected]>
Date: Tue, 10 May 2011 16:31:41 +0400

11.05.11, 00:28, "David Miller" <[email protected]>:

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, 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.

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 <[email protected]>
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.

From: Daniel Hellstrom <[email protected]>
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 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 :-)

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

Thanks.

From: Tkhai Kirill <[email protected]>
Date: Wed, 11 May 2011 03:21:49 +0400

Now I understand, thanks for explaining.

Applied.