Re: [PATCH RFC] riscv/mm: Add handling for VM_FAULT_SIGSEGV in mm_fault_error()

[Alexandre Ghiti]

Hi Zhe,

On 30/07/2024 09:38, Zhe Qiao wrote:
> Hello everyone, recently while learning about the Riscv architecture's
> handling of page fault exceptions in the Linux kernel, I found that
> there is no handling of VM_CAULT_SIGSEGV in mm_fault_erroneous (), but
> rather a BUG() is executed directly. Therefore, I simultaneously analyzed
> the processing procedures of arm64, powerpc, and sh architectures for
> this area and found that all three architectures have processing for
> VM_CAULT_SIGSEGV. Therefore, I added relevant processing methods for
> VM_CAULT_SIGSEGV on the riscv architecture.
>
> As a beginner, I am not sure if this processing method is correct and
> would like to hear the opinions of my seniors.


FWIW, we correctly handle "normal" segfaults (ie no VMA or permission 
access faults).

What we don't handle is handle_mm_fault() returning VM_FAULT_SIGSEGV, 
which can happen in different situations. For example, the BPF arena 
stuff implements a vma fault handler which can return VM_FAULT_SIGSEGV 
(see 
https://elixir.bootlin.com/linux/v6.10/source/kernel/bpf/arena.c#L273). 
In that case, we would BUG() instead of correctly killing the process.

So I agree that we need this patch. Can you just rephrase the commit 
log? I would say something similar (without the link) to what I have 
just said above. And can you also add the following Fixes tag?

Fixes: 07037db5d479 ("RISC-V: Paging and MMU")

Thanks,

Alex


> Signed-off-by: Zhe Qiao <qiaozhe@xxxxxxxxxxx>
> ---
>   arch/riscv/mm/fault.c | 17 +++++++++--------
>   1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/arch/riscv/mm/fault.c b/arch/riscv/mm/fault.c
> index 5224f3733802..868163b904e2 100644
> --- a/arch/riscv/mm/fault.c
> +++ b/arch/riscv/mm/fault.c
> @@ -61,26 +61,27 @@ static inline void no_context(struct pt_regs *regs, unsigned long addr)
>   
>   static inline void mm_fault_error(struct pt_regs *regs, unsigned long addr, vm_fault_t fault)
>   {
> +	if (!user_mode(regs)) {
> +		no_context(regs, addr);
> +		return;
> +	}
> +
>   	if (fault & VM_FAULT_OOM) {
>   		/*
>   		 * We ran out of memory, call the OOM killer, and return the userspace
>   		 * (which will retry the fault, or kill us if we got oom-killed).
>   		 */
> -		if (!user_mode(regs)) {
> -			no_context(regs, addr);
> -			return;
> -		}
>   		pagefault_out_of_memory();
>   		return;
>   	} else if (fault & (VM_FAULT_SIGBUS | VM_FAULT_HWPOISON | VM_FAULT_HWPOISON_LARGE)) {
>   		/* Kernel mode? Handle exceptions or die */
> -		if (!user_mode(regs)) {
> -			no_context(regs, addr);
> -			return;
> -		}
>   		do_trap(regs, SIGBUS, BUS_ADRERR, addr);
>   		return;
> +	} else if (fault & VM_FAULT_SIGSEGV) {
> +		do_trap(regs, SIGSEGV, SEGV_MAPERR, addr);
> +		return;
>   	}
> +
>   	BUG();
>   }
>