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

[Michael Schmitz <schmitzmic@gmail.com>]

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