__copy_tofrom_user fails on unaligned read faults

For performance reasons, __copy_tofrom_user copies
data in 16-bytes chunks.  If a read fault occurs in the
middle of one of these chunks, the data which has been
successfully read (up to 15 bytes within the chunk) is
discarded.

Valid data is similarly not copied properly while copying
words and the faulting read address is not word aligned.

Is this a known issue?

While this a rare corner case, I propose that we retry
with byte copies after an unaligned read fault.

I've appended a patch against linuxppc_2_4_devel

-Dale Farnsworth

--- linux/arch/ppc/lib/string.S.orig	2002-11-21 09:26:41.000000000 -0700
+++ linux/arch/ppc/lib/string.S	2002-11-21 11:47:29.000000000 -0700

@@ -512,7 +512,12 @@
 	li	r3,0
 	b	99f
 /* read fault, initial word copy */
-102:	li	r4,0
+102:	andi.	r0,r4,3
+	beq	85f
+/* read fault was not word aligned, retry one byte at a time */
+	li	r3,2
+	b	86f
+85:	li	r4,0
 	b	91f
 /* write fault, initial word copy */
 103:	li	r4,1

@@ -539,15 +544,33 @@
 #endif

 /* read fault in cacheline loop */
-104:	li	r4,0
+104:	addi	r0,r4,4
+	andi.	r0,r0,15
+	beq	87f
+/* read fault was not 16-byte aligned, retry one byte at a time */
+/* number of bytes remaining is r5 + (ctr << r3) */
+86:	mfctr	r0
+	slw	r3,r0,r3
+	add	r3,r3,r5
+	mtctr	r3
+	b	40b
+
+87:	li	r4,0
 	b	92f
+
 /* fault on dcbz (effectively a write fault) */
 /* or write fault in cacheline loop */
 105:	li	r4,1
 92:	li	r3,LG_CACHELINE_BYTES
 	b	99f
 /* read fault in final word loop */
-108:	li	r4,0
+108:	andi.	r0,r4,3
+	beq	88f
+/* read fault was not word aligned, retry one byte at a time */
+	andi.	r5,r5,3
+	li	r3,2
+	b	86b
+88:	li	r4,0
 	b	93f
 /* write fault in final word loop */
 109:	li	r4,1

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Dale Farnsworth wrote:

For performance reasons, __copy_tofrom_user copies
data in 16-bytes chunks.  If a read fault occurs in the
middle of one of these chunks,

How do you get a read fault in the middle of a 16-byte
aligned data copy?


	-- Dan


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

On Thu, Nov 21, 2002 at 05:07:32PM -0500, Dan Malek wrote:

Dale Farnsworth wrote:

quoted

For performance reasons, __copy_tofrom_user copies
data in 16-bytes chunks.  If a read fault occurs in the
middle of one of these chunks,

How do you get a read fault in the middle of a 16-byte
aligned data copy?

The 16-byte copy is aligned on the destination address.
It is unaligned with respect to the source address.

-Dale

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Dale Farnsworth wrote:

The 16-byte copy is aligned on the destination address.
It is unaligned with respect to the source address.

I see, so do the semantics of a copy_to/from allow this behavior?
If you still return an error, how does the caller know how many
bytes were transferred?  I always thought if you handled a SEG/BUS
fault exception the contents of the buffer were undefined.


	-- Dan


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

On Thu, Nov 21, 2002 at 05:27:53PM -0500, Dan Malek wrote:

Dale Farnsworth wrote:

quoted

The 16-byte copy is aligned on the destination address.
It is unaligned with respect to the source address.

I see, so do the semantics of a copy_to/from allow this behavior?
If you still return an error, how does the caller know how many
bytes were transferred?  I always thought if you handled a SEG/BUS
fault exception the contents of the buffer were undefined.

	-- Dan

copy_from_user is supposed to transfer as much data as is
valid and then to return the number of bytes not tranferred.
That's how it works on x86.  On ppc it can be as much as 15
bytes short.

I initially saw the problem with the mount system call.  Here's
a partial strace:

# strace mount -t jffs2 /dev/mtdblock0 /xx
execve("/bin/mount", ["mount", "-t", "jffs2", "/dev/mtdblock0", "/xx"], [/* 7 vars */]) = 0
<deleted lots of stuff>
stat("/dev/mtdblock0", {st_mode=S_IFBLK|0644, st_rdev=makedev(31, 0), ...}) = 0
brk(0x1005e000)                         = 0x1005e000
brk(0x1005f000)                         = 0x1005f000
mount("/dev/mtdblock0", "/xx", "jffs2", 0xc0ed0000, 0x1005eff8) = -1 EFAULT (Bad address)

Note that the fifth argument to mount is an address 8 bytes from the end
of user data space.  There is a null byte at that address, since no mount
options are being passed.

In the kernel, sys_mount() allocates a page for the options and
does copy_from_user(new_page, 0x1005eff8, PAGE_SIZE).  copy_from_user
should copy 8 bytes and return (PAGE_SIZE-8).  Instead, on ppc it reads
8 bytes, faults, writes no bytes, and returns PAGE_SIZE, which causes the
EFAULT to be erroneously reported.

-Dale

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Dale Farnsworth writes:

copy_from_user is supposed to transfer as much data as is
valid and then to return the number of bytes not tranferred.
That's how it works on x86.  On ppc it can be as much as 15
bytes short.

I initially saw the problem with the mount system call.  Here's
a partial strace:

I've always disliked the way that the mount system call does that.
However, we just have to deal with it, I guess.

IMO you are right in thinking that we need to try to copy bytes one at
a time after we get a read fault.  I think I would do the extra byte
copy loop inline after the 99: label instead of jumping back the way
you do.  Could you try this patch, please?

Paul.

diff -urN linuxppc_2_4_devel/arch/ppc/lib/string.S pmac/arch/ppc/lib/string.S

--- linuxppc_2_4_devel/arch/ppc/lib/string.S	2002-08-13 21:52:53.000000000 +1000
+++ pmac/arch/ppc/lib/string.S	2002-11-23 12:01:31.000000000 +1100

@@ -567,10 +567,19 @@
  */
 99:	mfctr	r0
 	slw	r3,r0,r3
-	add	r3,r3,r5
+	add.	r3,r3,r5
+	beq	120f			/* shouldn't happen */
 	cmpwi	0,r4,0
 	bne	120f
-/* for read fault, clear out the destination: r3 bytes starting at 4(r6) */
+/* for a read fault, first try to continue the copy one byte at a time */
+	mtctr	r3
+130:	lbz	r0,4(r4)
+131:	stb	r0,4(r6)
+	addi	r4,r4,1
+	addi	r6,r6,1
+	bdnz	130b
+/* then clear out the destination: r3 bytes starting at 4(r6) */
+132:	mfctr	r3
 	srwi.	r0,r3,2
 	li	r9,0
 	mtctr	r0

@@ -591,6 +600,8 @@
 	.long	31b,109b
 	.long	40b,110b
 	.long	41b,111b
+	.long	130b,132b
+	.long	131b,120b
 	.long	112b,120b
 	.long	114b,120b
 	.text

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

On Sat, Nov 23, 2002 at 12:03:36PM +1100, Paul Mackerras wrote:

IMO you are right in thinking that we need to try to copy bytes one at
a time after we get a read fault.  I think I would do the extra byte
copy loop inline after the 99: label instead of jumping back the way
you do.

By the time we get to 99:, r4 has already been zeroed.  We need to
insert the byte copy at 104:.  Now, to figure out how to do that without
duplicating code...  I won't be able to spend time on it until Monday.

Note that we also need to handle an unaligned fault in the initial
and final word copy loops.

-Dale

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

On Sat, Nov 23, 2002 at 12:03:36PM +1100, Paul Mackerras wrote:

Could you try this patch, please?

Your patch worked fine after I moved the read/write flag from r4 to r9.
Here's the resultant patch.  I'm happy with it.

-Dale


diff -u linuxppc_2_4_devel/arch/ppc/lib/string.S hxeb100/arch/ppc/lib/string.S

--- linuxppc_2_4_devel/arch/ppc/lib/string.S	2002-11-25 10:38:45.000000000 -0700
+++ hxeb100/arch/ppc/lib/string.S	2002-11-25 09:32:00.000000000 -0700

@@ -504,18 +504,18 @@
 	blr

 /* read fault, initial single-byte copy */
-100:	li	r4,0
+100:	li	r9,0
 	b	90f
 /* write fault, initial single-byte copy */
-101:	li	r4,1
+101:	li	r9,1
 90:	subf	r5,r8,r5
 	li	r3,0
 	b	99f
 /* read fault, initial word copy */
-102:	li	r4,0
+102:	li	r9,0
 	b	91f
 /* write fault, initial word copy */
-103:	li	r4,1
+103:	li	r9,1
 91:	li	r3,2
 	b	99f

@@ -539,38 +539,47 @@
 #endif

 /* read fault in cacheline loop */
-104:	li	r4,0
+104:	li	r9,0
 	b	92f
 /* fault on dcbz (effectively a write fault) */
 /* or write fault in cacheline loop */
-105:	li	r4,1
+105:	li	r9,1
 92:	li	r3,LG_CACHELINE_BYTES
 	b	99f
 /* read fault in final word loop */
-108:	li	r4,0
+108:	li	r9,0
 	b	93f
 /* write fault in final word loop */
-109:	li	r4,1
+109:	li	r9,1
 93:	andi.	r5,r5,3
 	li	r3,2
 	b	99f
 /* read fault in final byte loop */
-110:	li	r4,0
+110:	li	r9,0
 	b	94f
 /* write fault in final byte loop */
-111:	li	r4,1
+111:	li	r9,1
 94:	li	r5,0
 	li	r3,0
 /*
  * At this stage the number of bytes not copied is
- * r5 + (ctr << r3), and r4 is 0 for read or 1 for write.
+ * r5 + (ctr << r3), and r9 is 0 for read or 1 for write.
  */
 99:	mfctr	r0
 	slw	r3,r0,r3
-	add	r3,r3,r5
-	cmpwi	0,r4,0
+	add.	r3,r3,r5
+	beq	120f			/* shouldn't happen */
+	cmpwi	0,r9,0
 	bne	120f
-/* for read fault, clear out the destination: r3 bytes starting at 4(r6) */
+/* for a read fault, first try to continue the copy one byte at a time */
+	mtctr	r3
+130:	lbz	r0,4(r4)
+131:	stb	r0,4(r6)
+	addi	r4,r4,1
+	addi	r6,r6,1
+	bdnz	130b
+/* then clear out the destination: r3 bytes starting at 4(r6) */
+132:	mfctr	r3
 	srwi.	r0,r3,2
 	li	r9,0
 	mtctr	r0

@@ -591,6 +600,8 @@
 	.long	31b,109b
 	.long	40b,110b
 	.long	41b,111b
+	.long	130b,132b
+	.long	131b,120b
 	.long	112b,120b
 	.long	114b,120b
 	.text

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/