[v1] riscv/mm: Add handling for VM_FAULT_SIGSEGV in mm_fault_error()

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

Posted by Zhe Qiao 1 year, 4 months ago

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.

Signed-off-by: Zhe Qiao <qiaozhe@iscas.ac.cn>
---
 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();
 }
 
-- 
2.43.0

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

Posted by Alexandre Ghiti 1 year, 4 months ago

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@iscas.ac.cn>
> ---
>   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();
>   }
>

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

Posted by Zhe Qiao 1 year, 4 months ago

Hi everyone，

On 2024/7/30 20:43, Alexandre Ghiti wrote:
> 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 


 Thinks for your reply. I have submitted a new patch. Connection address: https://lists.infradead.org/pipermail/linux-riscv/2024-July/058216.html


Thanks，


Zhe


>
>
>>
>> Signed-off-by: Zhe Qiao <qiaozhe@iscas.ac.cn>
>> ---
>>   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();
>>   }
>>