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

[Michael Schmitz <schmitzmic@gmail.com>]

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