| Message ID | 20240730073822.27732-1-qiaozhe@iscas.ac.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC] riscv/mm: Add handling for VM_FAULT_SIGSEGV in mm_fault_error() | expand |
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(); > } >
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(); >> } >>
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(); }
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(-)