[PATCH v3 0/2] m68k uaccess fault handling fixes

[PATCH v3 0/2] m68k uaccess fault handling fixes

[Michael Schmitz]

Version 4 of fixes for uaccess fault handling on 68030,
tested on 68030 and 68040.

Patch 1 has additional exception table entries as well as
a final NOP added - evidently, in some cases the fault PC
points more than a single instruction past the moves that
caused the fault. 

Patch 2 is unchanged from RFC v2.

These patches would still benefit from testing on 68060
and Coldfire. Looking at access_error060, I would not expect
the 68060 to behave much different from the 68040 and in
particular, addition of exception table entries to not cause
any regression. Less confident for Coldfire.

Cheers,

   Michael

[PATCH v3 1/2] m68k: Handle __generic_copy_to_user faults more carefully

[Michael Schmitz]

As mentioned by Finn Thain in his patch to improve put_user
exception handling on 040, a similar problem exists on 030
processors.

A moves instruction that crosses a page boundary from a
mapped page into an unmapped one will cause a mid-instruction
bus error exception (frame format b), with the PC pointing
(usually) two instructions past the faulting movesl instruction.

Our exception handling in __generic_copy_to_user only covers
the instruction immediately following the faulting one. As
a result, fixup_exception in send_fault_sig does not detect
this case, and cause send_fault_sig to oops.

Extend the exception table to cover one additional instruction
beyond the moves[lwb] instructions.

Tested on 68030 (Atari Falcon 030) with transfers beginning
at one to six bytes offset from the end of a mapped page,
followed by further bytes on an unmapped page (testcase
derived from stress-ng sysbadaddr stressor by Finn Thain).

Tested on 68040 (Mac Quadra) and 68030 (Mac IIci) by Finn Thain.

A similar problem is present in __clear_user(); modify the
exception table for that function in the same way (tested by
Finn Thain).

Cc: Finn Thain <fthain@linux-m68k.org>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: linux-m68k@lists.linux-m68k.org
Tested-by: Finn Thain <fthain@linux-m68k.org>
Link: https://lore.kernel.org/all/e0f23460779e6d16e2633486ac4841790ef2aca0.1713176294.git.fthain@linux-m68k.org
Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>

---

Changes from RFC v2:

Finn Thain:

- add missing extension table entries and final NOP in
  __generic_copy_to_user faults in 040 tests

Michael Schmitz:

- add yet another exception table entry in __clear_user

Changes from RFC v1:

Michael Schmitz:

- use extended exception table instead of additional NOPs
---
 arch/m68k/lib/uaccess.c | 56 ++++++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 23 deletions(-)

diff --git a/arch/m68k/lib/uaccess.c b/arch/m68k/lib/uaccess.c
index 86b6fed5151c..e3ed893047f8 100644
--- a/arch/m68k/lib/uaccess.c
+++ b/arch/m68k/lib/uaccess.c
@@ -62,35 +62,42 @@ unsigned long __generic_copy_to_user(void __user *to, const void *from,
 
 	asm volatile ("\n"
 		"	tst.l	%0\n"
-		"	jeq	4f\n"
+		"	jeq	5f\n"
 		"1:	move.l	(%1)+,%3\n"
 		"2:	"MOVES".l	%3,(%2)+\n"
 		"3:	subq.l	#1,%0\n"
-		"	jne	1b\n"
-		"4:	btst	#1,%5\n"
-		"	jeq	6f\n"
-		"	move.w	(%1)+,%3\n"
-		"5:	"MOVES".w	%3,(%2)+\n"
-		"6:	btst	#0,%5\n"
+		"4:	jne	1b\n"
+		"5:	btst	#1,%5\n"
 		"	jeq	8f\n"
-		"	move.b	(%1)+,%3\n"
-		"7:	"MOVES".b  %3,(%2)+\n"
-		"8:\n"
+		"6:	move.w	(%1)+,%3\n"
+		"7:	"MOVES".w	%3,(%2)+\n"
+		"8:	btst	#0,%5\n"
+		"9:	jeq	13f\n"
+		"10:	move.b	(%1)+,%3\n"
+		"11:	"MOVES".b	%3,(%2)+\n"
+		"12:	nop\n"
+		"13:\n"
 		"	.section .fixup,\"ax\"\n"
 		"	.even\n"
 		"20:	lsl.l	#2,%0\n"
 		"50:	add.l	%5,%0\n"
-		"	jra	8b\n"
+		"	jra	13b\n"
 		"	.previous\n"
 		"\n"
 		"	.section __ex_table,\"a\"\n"
 		"	.align	4\n"
+		"	.long	1b,20b\n"
 		"	.long	2b,20b\n"
 		"	.long	3b,20b\n"
-		"	.long	5b,50b\n"
+		"	.long	4b,20b\n"
+		"	.long	5b,20b\n"
 		"	.long	6b,50b\n"
 		"	.long	7b,50b\n"
 		"	.long	8b,50b\n"
+		"	.long	9b,50b\n"
+		"	.long	10b,50b\n"
+		"	.long	11b,50b\n"
+		"	.long	12b,50b\n"
 		"	.previous"
 		: "=d" (res), "+a" (from), "+a" (to), "=&d" (tmp)
 		: "0" (n / 4), "d" (n & 3));
@@ -109,32 +116,35 @@ unsigned long __clear_user(void __user *to, unsigned long n)
 
 	asm volatile ("\n"
 		"	tst.l	%0\n"
-		"	jeq	3f\n"
+		"	jeq	4f\n"
 		"1:	"MOVES".l	%2,(%1)+\n"
 		"2:	subq.l	#1,%0\n"
-		"	jne	1b\n"
-		"3:	btst	#1,%4\n"
-		"	jeq	5f\n"
-		"4:	"MOVES".w	%2,(%1)+\n"
-		"5:	btst	#0,%4\n"
-		"	jeq	7f\n"
-		"6:	"MOVES".b	%2,(%1)\n"
-		"7:\n"
+		"3:	jne	1b\n"
+		"4:	btst	#1,%4\n"
+		"	jeq	6f\n"
+		"5:	"MOVES".w	%2,(%1)+\n"
+		"6:	btst	#0,%4\n"
+		"7:	jeq	9f\n"
+		"8:	"MOVES".b	%2,(%1)\n"
+		"9:\n"
 		"	.section .fixup,\"ax\"\n"
 		"	.even\n"
 		"10:	lsl.l	#2,%0\n"
 		"40:	add.l	%4,%0\n"
-		"	jra	7b\n"
+		"	jra	9b\n"
 		"	.previous\n"
 		"\n"
 		"	.section __ex_table,\"a\"\n"
 		"	.align	4\n"
 		"	.long	1b,10b\n"
 		"	.long	2b,10b\n"
-		"	.long	4b,40b\n"
+		"	.long	3b,10b\n"
+		"	.long	4b,10b\n"
 		"	.long	5b,40b\n"
 		"	.long	6b,40b\n"
 		"	.long	7b,40b\n"
+		"	.long	8b,40b\n"
+		"	.long	9b,40b\n"
 		"	.previous"
 		: "=d" (res), "+a" (to)
 		: "d" (0), "0" (n / 4), "d" (n & 3));
-- 
2.17.1

[PATCH v3 2/2] m68k: improve __constant_copy_to_user_asm() fault handling

[Michael Schmitz]

A problem similar to that reported for __put_user_asm and
__generic_copy_to_user is also present in
__constant_copy_to_user_asm.

Address the problem by extending the exception table to cover
two instructions past each moves instruction, and adding a
single NOP at the very end to catch faults on the final
instruction (which is not guaranteed to be a movesb!).

Tested on 68030 (Atari Falcon 030) with a transfer beginning
at a single byte at the end of a mapped page followed by
seven more bytes on an unmapped page (testcase derived from
stress-ng sysbadaddr stressor by Finn Thain and modified to
use the llseek syscall).

Cc: Finn Thain <fthain@linux-m68k.org>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Link: https://lore.kernel.org/all/e0f23460779e6d16e2633486ac4841790ef2aca0.1713176294.git.fthain@linux-m68k.org
Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>
---
 arch/m68k/include/asm/uaccess.h | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/m68k/include/asm/uaccess.h b/arch/m68k/include/asm/uaccess.h
index 44e52d8323e5..f1f4b62d6f69 100644
--- a/arch/m68k/include/asm/uaccess.h
+++ b/arch/m68k/include/asm/uaccess.h
@@ -288,10 +288,11 @@ __constant_copy_from_user(void *to, const void __user *from, unsigned long n)
 		"21:	"MOVES"."#s2"	%3,(%1)+\n"			\
 		"22:\n"							\
 		"	.ifnc	\""#s3"\",\"\"\n"			\
-		"	move."#s3"	(%2)+,%3\n"			\
-		"31:	"MOVES"."#s3"	%3,(%1)+\n"			\
-		"32:\n"							\
+		"31:	move."#s3"	(%2)+,%3\n"			\
+		"32:	"MOVES"."#s3"	%3,(%1)+\n"			\
+		"33:\n"							\
 		"	.endif\n"					\
+		"34:	nop\n"						\
 		"4:\n"							\
 		"\n"							\
 		"	.section __ex_table,\"a\"\n"			\
@@ -303,7 +304,9 @@ __constant_copy_from_user(void *to, const void __user *from, unsigned long n)
 		"	.ifnc	\""#s3"\",\"\"\n"			\
 		"	.long	31b,5f\n"				\
 		"	.long	32b,5f\n"				\
+		"	.long	33b,5f\n"				\
 		"	.endif\n"					\
+		"	.long	34b,5f\n"				\
 		"	.previous\n"					\
 		"\n"							\
 		"	.section .fixup,\"ax\"\n"			\
-- 
2.17.1

Re: [PATCH v3 2/2] m68k: improve __constant_copy_to_user_asm() fault handling

[Finn Thain]

On Sat, 27 Apr 2024, Michael Schmitz wrote:

> A problem similar to that reported for __put_user_asm and
> __generic_copy_to_user is also present in
> __constant_copy_to_user_asm.
> 
> Address the problem by extending the exception table to cover
> two instructions past each moves instruction, and adding a
> single NOP at the very end to catch faults on the final
> instruction (which is not guaranteed to be a movesb!).
> 
> Tested on 68030 (Atari Falcon 030) with a transfer beginning
> at a single byte at the end of a mapped page followed by
> seven more bytes on an unmapped page (testcase derived from
> stress-ng sysbadaddr stressor by Finn Thain and modified to
> use the llseek syscall).
> 
> Cc: Finn Thain <fthain@linux-m68k.org>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Link: https://lore.kernel.org/all/e0f23460779e6d16e2633486ac4841790ef2aca0.1713176294.git.fthain@linux-m68k.org
> Signed-off-by: Michael Schmitz <schmitzmic@gmail.com>

Tested-by: Finn Thain <fthain@linux-m68k.org>

> ---
>  arch/m68k/include/asm/uaccess.h | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/m68k/include/asm/uaccess.h b/arch/m68k/include/asm/uaccess.h
> index 44e52d8323e5..f1f4b62d6f69 100644
> --- a/arch/m68k/include/asm/uaccess.h
> +++ b/arch/m68k/include/asm/uaccess.h
> @@ -288,10 +288,11 @@ __constant_copy_from_user(void *to, const void __user *from, unsigned long n)
>  		"21:	"MOVES"."#s2"	%3,(%1)+\n"			\
>  		"22:\n"							\
>  		"	.ifnc	\""#s3"\",\"\"\n"			\
> -		"	move."#s3"	(%2)+,%3\n"			\
> -		"31:	"MOVES"."#s3"	%3,(%1)+\n"			\
> -		"32:\n"							\
> +		"31:	move."#s3"	(%2)+,%3\n"			\
> +		"32:	"MOVES"."#s3"	%3,(%1)+\n"			\
> +		"33:\n"							\
>  		"	.endif\n"					\
> +		"34:	nop\n"						\
>  		"4:\n"							\
>  		"\n"							\
>  		"	.section __ex_table,\"a\"\n"			\
> @@ -303,7 +304,9 @@ __constant_copy_from_user(void *to, const void __user *from, unsigned long n)
>  		"	.ifnc	\""#s3"\",\"\"\n"			\
>  		"	.long	31b,5f\n"				\
>  		"	.long	32b,5f\n"				\
> +		"	.long	33b,5f\n"				\
>  		"	.endif\n"					\
> +		"	.long	34b,5f\n"				\
>  		"	.previous\n"					\
>  		"\n"							\
>  		"	.section .fixup,\"ax\"\n"			\
> 

Re: [PATCH v3 1/2] m68k: Handle __generic_copy_to_user faults more carefully

[Finn Thain]

On Sat, 27 Apr 2024, Michael Schmitz wrote:

> 
> diff --git a/arch/m68k/lib/uaccess.c b/arch/m68k/lib/uaccess.c
> index 86b6fed5151c..e3ed893047f8 100644
> --- a/arch/m68k/lib/uaccess.c
> +++ b/arch/m68k/lib/uaccess.c
> @@ -62,35 +62,42 @@ unsigned long __generic_copy_to_user(void __user *to, const void *from,
>  
>  	asm volatile ("\n"
>  		"	tst.l	%0\n"
> -		"	jeq	4f\n"
> +		"	jeq	5f\n"
>  		"1:	move.l	(%1)+,%3\n"
>  		"2:	"MOVES".l	%3,(%2)+\n"
>  		"3:	subq.l	#1,%0\n"
> -		"	jne	1b\n"
> -		"4:	btst	#1,%5\n"
> -		"	jeq	6f\n"
> -		"	move.w	(%1)+,%3\n"
> -		"5:	"MOVES".w	%3,(%2)+\n"
> -		"6:	btst	#0,%5\n"
> +		"4:	jne	1b\n"
> +		"5:	btst	#1,%5\n"
>  		"	jeq	8f\n"
> -		"	move.b	(%1)+,%3\n"
> -		"7:	"MOVES".b  %3,(%2)+\n"
> -		"8:\n"
> +		"6:	move.w	(%1)+,%3\n"
> +		"7:	"MOVES".w	%3,(%2)+\n"
> +		"8:	btst	#0,%5\n"
> +		"9:	jeq	13f\n"
> +		"10:	move.b	(%1)+,%3\n"
> +		"11:	"MOVES".b	%3,(%2)+\n"
> +		"12:	nop\n"
> +		"13:\n"
>  		"	.section .fixup,\"ax\"\n"
>  		"	.even\n"
>  		"20:	lsl.l	#2,%0\n"
>  		"50:	add.l	%5,%0\n"
> -		"	jra	8b\n"
> +		"	jra	13b\n"
>  		"	.previous\n"
>  		"\n"
>  		"	.section __ex_table,\"a\"\n"
>  		"	.align	4\n"
> +		"	.long	1b,20b\n"
>  		"	.long	2b,20b\n"
>  		"	.long	3b,20b\n"
> -		"	.long	5b,50b\n"
> +		"	.long	4b,20b\n"
> +		"	.long	5b,20b\n"
>  		"	.long	6b,50b\n"

I think a fault at 6b has to get fixed up at 20b.

>  		"	.long	7b,50b\n"
>  		"	.long	8b,50b\n"
> +		"	.long	9b,50b\n"
> +		"	.long	10b,50b\n"
> +		"	.long	11b,50b\n"
> +		"	.long	12b,50b\n"
>  		"	.previous"
>  		: "=d" (res), "+a" (from), "+a" (to), "=&d" (tmp)
>  		: "0" (n / 4), "d" (n & 3));
> @@ -109,32 +116,35 @@ unsigned long __clear_user(void __user *to, unsigned long n)
>  
>  	asm volatile ("\n"
>  		"	tst.l	%0\n"
> -		"	jeq	3f\n"
> +		"	jeq	4f\n"
>  		"1:	"MOVES".l	%2,(%1)+\n"
>  		"2:	subq.l	#1,%0\n"
> -		"	jne	1b\n"
> -		"3:	btst	#1,%4\n"
> -		"	jeq	5f\n"
> -		"4:	"MOVES".w	%2,(%1)+\n"
> -		"5:	btst	#0,%4\n"
> -		"	jeq	7f\n"
> -		"6:	"MOVES".b	%2,(%1)\n"
> -		"7:\n"
> +		"3:	jne	1b\n"
> +		"4:	btst	#1,%4\n"
> +		"	jeq	6f\n"
> +		"5:	"MOVES".w	%2,(%1)+\n"
> +		"6:	btst	#0,%4\n"
> +		"7:	jeq	9f\n"
> +		"8:	"MOVES".b	%2,(%1)\n"
> +		"9:\n"

Should we put a NOP here to avoid having the unknown next instruction 
(label 9) in the exception table? We can't actually fix up a fault there 
unless by chance it was the MOVES that caused it.

>  		"	.section .fixup,\"ax\"\n"
>  		"	.even\n"
>  		"10:	lsl.l	#2,%0\n"
>  		"40:	add.l	%4,%0\n"
> -		"	jra	7b\n"
> +		"	jra	9b\n"
>  		"	.previous\n"
>  		"\n"
>  		"	.section __ex_table,\"a\"\n"
>  		"	.align	4\n"
>  		"	.long	1b,10b\n"
>  		"	.long	2b,10b\n"
> -		"	.long	4b,40b\n"
> +		"	.long	3b,10b\n"
> +		"	.long	4b,10b\n"
>  		"	.long	5b,40b\n"
>  		"	.long	6b,40b\n"
>  		"	.long	7b,40b\n"
> +		"	.long	8b,40b\n"
> +		"	.long	9b,40b\n"
>  		"	.previous"
>  		: "=d" (res), "+a" (to)
>  		: "d" (0), "0" (n / 4), "d" (n & 3));
> 

Re: [PATCH v3 1/2] m68k: Handle __generic_copy_to_user faults more carefully

[Michael Schmitz]

Hi Finn,

thanks for your review!

Am 27.04.2024 um 21:34 schrieb Finn Thain:
>
> On Sat, 27 Apr 2024, Michael Schmitz wrote:
>
>>
>> diff --git a/arch/m68k/lib/uaccess.c b/arch/m68k/lib/uaccess.c
>> index 86b6fed5151c..e3ed893047f8 100644
>> --- a/arch/m68k/lib/uaccess.c
>> +++ b/arch/m68k/lib/uaccess.c
>> @@ -62,35 +62,42 @@ unsigned long __generic_copy_to_user(void __user *to, const void *from,
>>
>>  	asm volatile ("\n"
>>  		"	tst.l	%0\n"
>> -		"	jeq	4f\n"
>> +		"	jeq	5f\n"
>>  		"1:	move.l	(%1)+,%3\n"
>>  		"2:	"MOVES".l	%3,(%2)+\n"
>>  		"3:	subq.l	#1,%0\n"
>> -		"	jne	1b\n"
>> -		"4:	btst	#1,%5\n"
>> -		"	jeq	6f\n"
>> -		"	move.w	(%1)+,%3\n"
>> -		"5:	"MOVES".w	%3,(%2)+\n"
>> -		"6:	btst	#0,%5\n"
>> +		"4:	jne	1b\n"
>> +		"5:	btst	#1,%5\n"
>>  		"	jeq	8f\n"
>> -		"	move.b	(%1)+,%3\n"
>> -		"7:	"MOVES".b  %3,(%2)+\n"
>> -		"8:\n"
>> +		"6:	move.w	(%1)+,%3\n"
>> +		"7:	"MOVES".w	%3,(%2)+\n"
>> +		"8:	btst	#0,%5\n"
>> +		"9:	jeq	13f\n"
>> +		"10:	move.b	(%1)+,%3\n"
>> +		"11:	"MOVES".b	%3,(%2)+\n"
>> +		"12:	nop\n"
>> +		"13:\n"
>>  		"	.section .fixup,\"ax\"\n"
>>  		"	.even\n"
>>  		"20:	lsl.l	#2,%0\n"
>>  		"50:	add.l	%5,%0\n"
>> -		"	jra	8b\n"
>> +		"	jra	13b\n"
>>  		"	.previous\n"
>>  		"\n"
>>  		"	.section __ex_table,\"a\"\n"
>>  		"	.align	4\n"
>> +		"	.long	1b,20b\n"
>>  		"	.long	2b,20b\n"
>>  		"	.long	3b,20b\n"
>> -		"	.long	5b,50b\n"
>> +		"	.long	4b,20b\n"
>> +		"	.long	5b,20b\n"
>>  		"	.long	6b,50b\n"
>
> I think a fault at 6b has to get fixed up at 20b.

In my tests, the fault seen there had been caused by the movesl in the 
section above, hence the fixup at 20b.

But your comment has me thinking about a side effect of this change: in 
case the movew at label 6 were to fail, that fault was masked by our 
exception handling. We'd silently ignore a kernel bus error there.

On the other hand, we'd probably see a fault there manifest with a PC 
corresponding to the movesw or btest instruction rather than the movew 
instruction. These instructions have always been in the exception table, 
so potentially missing a kernel bus error there has always been an issue.

The only way I see that would avoid having to place this instruction 
(and the moveb later) into the exception table is to place a NOP after 
each of the movesl and movesw sections. Shouldn't have too much of a 
performance impact as long as it's not placed inside the movesl loop.

>
>>  		"	.long	7b,50b\n"
>>  		"	.long	8b,50b\n"
>> +		"	.long	9b,50b\n"
>> +		"	.long	10b,50b\n"
>> +		"	.long	11b,50b\n"
>> +		"	.long	12b,50b\n"
>>  		"	.previous"
>>  		: "=d" (res), "+a" (from), "+a" (to), "=&d" (tmp)
>>  		: "0" (n / 4), "d" (n & 3));
>> @@ -109,32 +116,35 @@ unsigned long __clear_user(void __user *to, unsigned long n)
>>
>>  	asm volatile ("\n"
>>  		"	tst.l	%0\n"
>> -		"	jeq	3f\n"
>> +		"	jeq	4f\n"
>>  		"1:	"MOVES".l	%2,(%1)+\n"
>>  		"2:	subq.l	#1,%0\n"
>> -		"	jne	1b\n"
>> -		"3:	btst	#1,%4\n"
>> -		"	jeq	5f\n"
>> -		"4:	"MOVES".w	%2,(%1)+\n"
>> -		"5:	btst	#0,%4\n"
>> -		"	jeq	7f\n"
>> -		"6:	"MOVES".b	%2,(%1)\n"
>> -		"7:\n"
>> +		"3:	jne	1b\n"
>> +		"4:	btst	#1,%4\n"
>> +		"	jeq	6f\n"
>> +		"5:	"MOVES".w	%2,(%1)+\n"
>> +		"6:	btst	#0,%4\n"
>> +		"7:	jeq	9f\n"
>> +		"8:	"MOVES".b	%2,(%1)\n"
>> +		"9:\n"
>
> Should we put a NOP here to avoid having the unknown next instruction
> (label 9) in the exception table? We can't actually fix up a fault there
> unless by chance it was the MOVES that caused it.

The movesb does not have the potential to cause a misaligned access, and 
I believe it is for that reason that a NOP there is not required (it 
probably isn't for __generic_copy_to_user either). My tests on 030 at 
least have confirmed that - selecting path length and start offset such 
that the movesb is the first instruction hitting the unmapped page has 
never produced an Ooops.

As to the unknown instructions following the final exception label: 
These functions, though they contain inline assembly code are not 
further inlined, and the instructions following the inline assembly are 
simple boilerplate register restore stack saves that ought not to fault 
(an invalid stack pointer would have faulted on function entry, if at all).

On balance, I am confident the code is correct as-is. You (and in 
particular, Geert) may argue though that the NOP approach follows the 
principle of least surprises, and can be considered safe to apply 
without further testing on 68060 and Coldfire.

Cheers,

	Michael


>
>>  		"	.section .fixup,\"ax\"\n"
>>  		"	.even\n"
>>  		"10:	lsl.l	#2,%0\n"
>>  		"40:	add.l	%4,%0\n"
>> -		"	jra	7b\n"
>> +		"	jra	9b\n"
>>  		"	.previous\n"
>>  		"\n"
>>  		"	.section __ex_table,\"a\"\n"
>>  		"	.align	4\n"
>>  		"	.long	1b,10b\n"
>>  		"	.long	2b,10b\n"
>> -		"	.long	4b,40b\n"
>> +		"	.long	3b,10b\n"
>> +		"	.long	4b,10b\n"
>>  		"	.long	5b,40b\n"
>>  		"	.long	6b,40b\n"
>>  		"	.long	7b,40b\n"
>> +		"	.long	8b,40b\n"
>> +		"	.long	9b,40b\n"
>>  		"	.previous"
>>  		: "=d" (res), "+a" (to)
>>  		: "d" (0), "0" (n / 4), "d" (n & 3));
>>

Re: [PATCH v3 1/2] m68k: Handle __generic_copy_to_user faults more carefully

[Finn Thain]

On Sun, 28 Apr 2024, Michael Schmitz wrote:

> In my tests, the fault seen there had been caused by the movesl in the 
> section above, hence the fixup at 20b.
> 

Well, that was my point. A fault at MOVE.W should get fixed up at 20b, not 
50b. So the patch is wrong, isn't it?

> >
> > Should we put a NOP here to avoid having the unknown next instruction 
> > (label 9) in the exception table? We can't actually fix up a fault 
> > there unless by chance it was the MOVES that caused it.
> 
> ... As to the unknown instructions following the final exception label: 
> These functions, though they contain inline assembly code are not 
> further inlined, and the instructions following the inline assembly are 
> simple boilerplate register restore stack saves that ought not to fault 
> (an invalid stack pointer would have faulted on function entry, if at 
> all).
> 

But that's just luck. IMHO, the asm is a foot-gun even if it has not gone 
off yet.

> On balance, I am confident the code is correct as-is. You (and in 
> particular, Geert) may argue though that the NOP approach follows the 
> principle of least surprises, and can be considered safe to apply 
> without further testing on 68060 and Coldfire.
> 

If you're saying that your patch addresses a different bug, fair enough.

All I'm saying is that, since you're adding the NOP anyway, you could make 
better use of it.

Anyway, like you, I am keen to hear from others regarding the API issue.

Re: [PATCH v3 1/2] m68k: Handle __generic_copy_to_user faults more carefully

[Michael Schmitz]

Hi Finn,

Am 28.04.2024 um 15:28 schrieb Finn Thain:
> On Sun, 28 Apr 2024, Michael Schmitz wrote:
>
>> In my tests, the fault seen there had been caused by the movesl in the
>> section above, hence the fixup at 20b.
>>
>
> Well, that was my point. A fault at MOVE.W should get fixed up at 20b, not
> 50b. So the patch is wrong, isn't it?

You are correct there - the fault may have happened while there still 
were longwords to transfer, even though the fault PC is after the loop. 
The residual from the loop must be taken into account. I'll fix that.

>>>
>>> Should we put a NOP here to avoid having the unknown next instruction
>>> (label 9) in the exception table? We can't actually fix up a fault
>>> there unless by chance it was the MOVES that caused it.
>>
>> ... As to the unknown instructions following the final exception label:
>> These functions, though they contain inline assembly code are not
>> further inlined, and the instructions following the inline assembly are
>> simple boilerplate register restore stack saves that ought not to fault
>> (an invalid stack pointer would have faulted on function entry, if at
>> all).
>>
>
> But that's just luck. IMHO, the asm is a foot-gun even if it has not gone
> off yet.

If the compiler were to inline the entire function, that could happen, 
yes. So for that reason, the NOP would be safer.

>
>> On balance, I am confident the code is correct as-is. You (and in
>> particular, Geert) may argue though that the NOP approach follows the
>> principle of least surprises, and can be considered safe to apply
>> without further testing on 68060 and Coldfire.
>>
>
> If you're saying that your patch addresses a different bug, fair enough.

Nah, I was just speculating that instead of bloating the exception table 
(and risking to miss a bad kernel address as source of the transfer), 
addition of NOPs might be considered the safer option.

>
> All I'm saying is that, since you're adding the NOP anyway, you could make
> better use of it.

I already am, in __generic_copy_to_user - there is no exception table 
entry for the jump label at the end, so no loop.

Since I have to fix patch 1 anyway, I'll add the NOP at the end of 
__clear_user and have the exception table end with that NOP.

> Anyway, like you, I am keen to hear from others regarding the API issue.

Too true ...

Cheers,

	Michael

Re: [PATCH v3 1/2] m68k: Handle __generic_copy_to_user faults more carefully

[Finn Thain]

On Sun, 28 Apr 2024, Michael Schmitz wrote:

> >
> > All I'm saying is that, since you're adding the NOP anyway, you could 
> > make better use of it.
> 
> I already am, in __generic_copy_to_user - there is no exception table 
> entry for the jump label at the end, so no loop.
> 

Right you are. I did get __generic_copy_to_user and __clear_user mixed up 
there.

> Since I have to fix patch 1 anyway, I'll add the NOP at the end of 
> __clear_user and have the exception table end with that NOP.
> 

Well, I'll leave that up to you and Geert. I'm ambivalent about that one.