Re: [PATCH] riscv: misaligned: disable pagefault before accessing user memory

["Clément Léger" <cleger@rivosinc.com>]

On 07/01/2025 03:33, Maciej W. Rozycki wrote:
> On Mon, 6 Jan 2025, Clément Léger wrote:
> 
>>> I'm wondering why the irqs are disabled here? What prevents us from
>>> enabling them? IIUC, if for some reason the user page is not in memory,
>>> we will fail the misaligned access and kill the process, so such
>>> failures could be completely random right?
>>
>> Hi Alex,
>>
>> This code is called from interrupt context as a result from an
>> exception. AFAIK, interrupts are disabled during the whole exception
>> handling and re-enabled when returning to user mode in
>> do_notify_resume(). So that's why it triggers this warning.
> 
>  This feels outright wrong.  We have analogous emulation code in Alpha and 
> MIPS ports and we stay away from quick sands by enabling interrupts in the 
> relevant exception handlers as soon as it is safe to do so; in particular 
> when emulating user-mode unaligned accesses.  What causes the RISC-V port 
> to be different here?

Hi Maciej,

I probably did not clarify that enough. The current path for misaligned
accesses does not re-enable irqs (whatever the mode we enter from). Some
other path does (page_fault) as it's indeed safe.

> 
>  See e.g. the MIPS exception dispatcher in arch/mips/kernel/genex.S where 
> BUILD_HANDLER invocations explicitly enable interrupts for most of the 
> handlers (via `sti' -> `__build_clear_sti').  Then unaligned emulation is 
> run with the interrupt enable setting preserved from the context the fault 
> happened in (via `ade' -> `__build_clear_ade'), clearly because it can in 
> principle trigger from kernel code run with interrupts disabled.  But such 
> code is not supposed to access user memory in the first place and traps 
> from the user mode will have had interrupts enabled.
> 
>  I think the RISC-V port has to do the same, i.e. run the handler with the 
> supervisor interrupt enable bit set according to whether interrupts were 
> enabled or not for the mode the fault has triggered in.

It seems like as you say, it would actually be safe to re-enable
interrupts in the misaligned load/store path as well as
do_trap()/do_trap_break() which might also benefit from this improvement.

After reviewing, the code, there is also a bunch of irqentry_nmi_enter()
that seems a bit weird, maybe they could be replaced with irqentry_enter().

Thanks for the review,

Clément

> 
>   Maciej