On Tue, Jul 23, 2024 at 04:47:50PM +0200, Andrew Zaborowski wrote: > Uncorrected memory errors for user pages are signaled to processes > using SIGBUS or, if the error happens in a syscall, an error retval > from the syscall. The SIGBUS is documented in > Documentation/mm/hwpoison.rst#failure-recovery-modes > > But there are corner cases where we cannot or don't want to return a > plain error from the syscall. Subsequent commits covers two such cases: > execve and rseq. Current code, in both places, will kill the task with a > SIGSEGV on error. While not explicitly stated, it can be argued that it > should be a SIGBUS, for consistency and for the benefit of the userspace > signal handlers. Even if the process cannot handle the signal, perhaps > the parent process can. This was the case in the scenario that > motivated this patch. > > In both cases, the architecture's exception handler (MCE handler on x86) > will queue a call to memory_failure. This doesn't work because the > syscall-specific code sees the -EFAULT and terminates the task before > the queued work runs. > > To fix this: 1. let pending work run in the error cases in both places. > > And 2. on MCE, ensure memory_failure() is passed MF_ACTION_REQUIRED so > that the SIGBUS is queued. Normally when the MCE is in a syscall, > a fixup of return IP and a call to kill_me_never() are what we want. > But in this case it's necessary to queue kill_me_maybe() which will set > MF_ACTION_REQUIRED which is checked by memory_failure(). > > To do this the syscall code will set current->kill_on_efault, a new > task_struct flag. Check that flag in > arch/x86/kernel/cpu/mce/core.c:do_machine_check() > > Note: the flag is not x86 specific even if only x86 handling is being > added here. The definition could be guarded by #ifdef > CONFIG_MEMORY_FAILURE, but it would then need set/clear utilities. > > Signed-off-by: Andrew Zaborowski <andrew.zaborowski@xxxxxxxxx> > --- > Resending through an SMTP server that won't add the company footer. > > This is a v2 of > https://lore.kernel.org/linux-mm/20240501015340.3014724-1-andrew.zaborowski@xxxxxxxxx/ > In the v1 the existing flag current->in_execve was being reused instead > of adding a new one. Kees Cook commented in > https://lore.kernel.org/linux-mm/202405010915.465AF19@keescook/ that > current->in_execve is going away. Lacking a better idea and seeing > that execve() and rseq() would benefit from using a common mechanism, I > decided to add this new flag. > > Perhaps with a better name current->kill_on_efault could replace > brpm->point_of_no_return to offset the pain of having this extra flag. > --- > arch/x86/kernel/cpu/mce/core.c | 18 +++++++++++++++++- Since this touches arch/x86/, can an x86 maintainer review this? I can carry this via the execve tree... -Kees > include/linux/sched.h | 2 ++ > 2 files changed, 19 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c > index ad0623b65..13f2ace3d 100644 > --- a/arch/x86/kernel/cpu/mce/core.c > +++ b/arch/x86/kernel/cpu/mce/core.c > @@ -1611,7 +1611,7 @@ noinstr void do_machine_check(struct pt_regs *regs) > if (p) > SetPageHWPoison(p); > } > - } else { > + } else if (!current->kill_on_efault) { > /* > * Handle an MCE which has happened in kernel space but from > * which the kernel can recover: ex_has_fault_handler() has > @@ -1628,6 +1628,22 @@ noinstr void do_machine_check(struct pt_regs *regs) > > if (m.kflags & MCE_IN_KERNEL_COPYIN) > queue_task_work(&m, msg, kill_me_never); > + } else { > + /* > + * Even with recovery code extra handling is required when > + * we're not returning to userspace after error (e.g. in > + * execve() beyond the point of no return) to ensure that > + * a SIGBUS is delivered. > + */ > + if (m.kflags & MCE_IN_KERNEL_RECOV) { > + if (!fixup_exception(regs, X86_TRAP_MC, 0, 0)) > + mce_panic("Failed kernel mode recovery", &m, msg); > + } > + > + if (!mce_usable_address(&m)) > + queue_task_work(&m, msg, kill_me_now); > + else > + queue_task_work(&m, msg, kill_me_maybe); > } > > out: > diff --git a/include/linux/sched.h b/include/linux/sched.h > index 61591ac6e..0cde1ba11 100644 > --- a/include/linux/sched.h > +++ b/include/linux/sched.h > @@ -975,6 +975,8 @@ struct task_struct { > /* delay due to memory thrashing */ > unsigned in_thrashing:1; > #endif > + /* Kill task on user memory access error */ > + unsigned kill_on_efault:1; > > unsigned long atomic_flags; /* Flags requiring atomic access. */ > > -- > 2.43.0 > -- Kees Cook
