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