Message-ID: <4e0ce353fe0a24b767226ecb9a8cca9382e2f7e4.camel@sipsolutions.net>
Date: Sat, 12 Jul 2025 09:58:38 +0200
From: Benjamin Berg <benjamin@...solutions.net>
To: Hajime Tazaki <thehajime@...il.com>
Cc: linux-um@...ts.infradead.org, ricarkol@...gle.com,
 Liam.Howlett@...cle.com, 	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v10 09/13] x86/um: nommu: signal handling

Hi,

On Sat, 2025-07-12 at 10:16 +0900, Hajime Tazaki wrote:
> 
> Hello,
> 
> > Honestly, I think we need a test case to be able to move forward. The
> > test needs to trigger an exception (FPE, segfault, whatever) and then
> > handle the signal. In the signal handler, verify the register state in
> > the mcontext is expected (RIP, RSP, FP regs), then update it to not
> > raise an exception again and return. The test should obviously exit
> > cleanly afterwards.
> 
> I agree to have a test case.
> 
> I played with your RFC patch ([RFC 0/2] Experimental kunit test for
> signal context handling), which I guess the similar one which you gave
> me in the past, with minor modification for nommu mode, and looks like
> that test passed.

That test triggers the signal emission using a self-kill (i.e. SIGSYS
and then the syscall entry point). The problems that I believe exist
will only happen if the kernel is entered for other reasons. I was
primarily thinking about exceptions (e.g. SIGFPE), but I suppose it
could even be scheduling right now (SIGALRM).

Benjamin


> 
> 
> (none):/#  /root/test-fp-save-restore 
> TAP version 13
> 1..1
> # pre-signal:  50 / 100, 11223344 55667788 99aabbcc ddeeff00
> # sighandler: extended_size: 2700, xstate_size: 2696
> # post-signal: 51200 / 100, 11233345 55677789 99abbbcd ddefff01 (should change: 1, changed: 1)
> ok 1 mcontext
> # Totals: pass:1 fail:0 xfail:0 xpass:0 skip:0 error:0
> 
> 
> I couldn't invoke this test via `kunit.py run` (which I should
> investigate more), but this can be a good start to have the test case
> which you proposed.
> 
> I will follow up the highlevel discussion on how syscall/signal
> entry/exit code is implemented in nommu, which I think I've been
> explained several times but why not :)
> 
> -- Hajime
> 
> On Fri, 11 Jul 2025 19:05:13 +0900,
> Benjamin Berg wrote:
> > 
> > On Fri, 2025-07-11 at 11:39 +0200, Benjamin Berg wrote:
> > > [SNIP]
> > > 
> > > That said, I would also still like to see a higher level discussion on
> > > how userspace registers are saved and restored. We have two separate
> > > cases--interrupts/exceptions (host signals) and the syscall path--and
> > > both need to be well defined. My hope is still that both of these can
> > > use the same register save/restore mechanism.
> > 
> > Now syscalls are also just signals. The crucial difference is that for 
> > syscalls you are allowed to clobber R11 and RCX. Your current syscall
> > entry code uses that fact, but that does not work for other signals.
> > 
> > Benjamin
> > 
> > > 
> > > Benjamin
> > > 
> > > > 
> > > > ---
> > > >  arch/um/include/shared/kern_util.h      |   4 +
> > > >  arch/um/nommu/Makefile                  |   2 +-
> > > >  arch/um/nommu/os-Linux/signal.c         |   8 +
> > > >  arch/um/nommu/trap.c                    | 201
> > > > ++++++++++++++++++++++++
> > > >  arch/um/os-Linux/signal.c               |   3 +-
> > > >  arch/x86/um/nommu/do_syscall_64.c       |   6 +
> > > >  arch/x86/um/nommu/entry_64.S            |  14 ++
> > > >  arch/x86/um/nommu/os-Linux/mcontext.c   |   5 +
> > > >  arch/x86/um/shared/sysdep/mcontext.h    |   1 +
> > > >  arch/x86/um/shared/sysdep/ptrace.h      |   2 +-
> > > >  arch/x86/um/shared/sysdep/syscalls_64.h |   1 +
> > > >  11 files changed, 244 insertions(+), 3 deletions(-)
> > > >  create mode 100644 arch/um/nommu/trap.c
> > > > 
> > > > diff --git a/arch/um/include/shared/kern_util.h
> > > > b/arch/um/include/shared/kern_util.h
> > > > index ec8ba1f13c58..7f55402b6385 100644
> > > > --- a/arch/um/include/shared/kern_util.h
> > > > +++ b/arch/um/include/shared/kern_util.h
> > > > @@ -73,4 +73,8 @@ void um_idle_sleep(void);
> > > >  
> > > >  void kasan_map_memory(void *start, size_t len);
> > > >  
> > > > +#ifndef CONFIG_MMU
> > > > +extern void nommu_relay_signal(void *ptr);
> > > > +#endif
> > > > +
> > > >  #endif
> > > > diff --git a/arch/um/nommu/Makefile b/arch/um/nommu/Makefile
> > > > index baab7c2f57c2..096221590cfd 100644
> > > > --- a/arch/um/nommu/Makefile
> > > > +++ b/arch/um/nommu/Makefile
> > > > @@ -1,3 +1,3 @@
> > > >  # SPDX-License-Identifier: GPL-2.0
> > > >  
> > > > -obj-y := os-Linux/
> > > > +obj-y := trap.o os-Linux/
> > > > diff --git a/arch/um/nommu/os-Linux/signal.c b/arch/um/nommu/os-
> > > > Linux/signal.c
> > > > index 19043b9652e2..27b6b37744b7 100644
> > > > --- a/arch/um/nommu/os-Linux/signal.c
> > > > +++ b/arch/um/nommu/os-Linux/signal.c
> > > > @@ -5,6 +5,7 @@
> > > >  #include <os.h>
> > > >  #include <sysdep/mcontext.h>
> > > >  #include <sys/ucontext.h>
> > > > +#include <as-layout.h>
> > > >  
> > > >  void sigsys_handler(int sig, struct siginfo *si,
> > > >  		    struct uml_pt_regs *regs, void *ptr)
> > > > @@ -14,3 +15,10 @@ void sigsys_handler(int sig, struct siginfo *si,
> > > >  	/* hook syscall via SIGSYS */
> > > >  	set_mc_sigsys_hook(mc);
> > > >  }
> > > > +
> > > > +void nommu_relay_signal(void *ptr)
> > > > +{
> > > > +	mcontext_t *mc = (mcontext_t *) ptr;
> > > > +
> > > > +	set_mc_return_address(mc);
> > > > +}
> > > > diff --git a/arch/um/nommu/trap.c b/arch/um/nommu/trap.c
> > > > new file mode 100644
> > > > index 000000000000..430297517455
> > > > --- /dev/null
> > > > +++ b/arch/um/nommu/trap.c
> > > > @@ -0,0 +1,201 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +
> > > > +#include <linux/mm.h>
> > > > +#include <linux/sched/signal.h>
> > > > +#include <linux/hardirq.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/uaccess.h>
> > > > +#include <linux/sched/debug.h>
> > > > +#include <asm/current.h>
> > > > +#include <asm/tlbflush.h>
> > > > +#include <arch.h>
> > > > +#include <as-layout.h>
> > > > +#include <kern_util.h>
> > > > +#include <os.h>
> > > > +#include <skas.h>
> > > > +
> > > > +/*
> > > > + * Note this is constrained to return 0, -EFAULT, -EACCES, -ENOMEM
> > > > by
> > > > + * segv().
> > > > + */
> > > > +int handle_page_fault(unsigned long address, unsigned long ip,
> > > > +		      int is_write, int is_user, int *code_out)
> > > > +{
> > > > +	/* !MMU has no pagefault */
> > > > +	return -EFAULT;
> > > > +}
> > > > +
> > > > +static void show_segv_info(struct uml_pt_regs *regs)
> > > > +{
> > > > +	struct task_struct *tsk = current;
> > > > +	struct faultinfo *fi = UPT_FAULTINFO(regs);
> > > > +
> > > > +	if (!unhandled_signal(tsk, SIGSEGV))
> > > > +		return;
> > > > +
> > > > +	pr_warn_ratelimited("%s%s[%d]: segfault at %lx ip %p sp %p
> > > > error %x",
> > > > +			    task_pid_nr(tsk) > 1 ? KERN_INFO :
> > > > KERN_EMERG,
> > > > +			    tsk->comm, task_pid_nr(tsk),
> > > > FAULT_ADDRESS(*fi),
> > > > +			    (void *)UPT_IP(regs), (void
> > > > *)UPT_SP(regs),
> > > > +			    fi->error_code);
> > > > +}
> > > > +
> > > > +static void bad_segv(struct faultinfo fi, unsigned long ip)
> > > > +{
> > > > +	current->thread.arch.faultinfo = fi;
> > > > +	force_sig_fault(SIGSEGV, SEGV_ACCERR, (void __user *)
> > > > FAULT_ADDRESS(fi));
> > > > +}
> > > > +
> > > > +void fatal_sigsegv(void)
> > > > +{
> > > > +	force_fatal_sig(SIGSEGV);
> > > > +	do_signal(&current->thread.regs);
> > > > +	/*
> > > > +	 * This is to tell gcc that we're not returning -
> > > > do_signal
> > > > +	 * can, in general, return, but in this case, it's not,
> > > > since
> > > > +	 * we just got a fatal SIGSEGV queued.
> > > > +	 */
> > > > +	os_dump_core();
> > > > +}
> > > > +
> > > > +/**
> > > > + * segv_handler() - the SIGSEGV handler
> > > > + * @sig:	the signal number
> > > > + * @unused_si:	the signal info struct; unused in this handler
> > > > + * @regs:	the ptrace register information
> > > > + *
> > > > + * The handler first extracts the faultinfo from the UML ptrace
> > > > regs struct.
> > > > + * If the userfault did not happen in an UML userspace process,
> > > > bad_segv is called.
> > > > + * Otherwise the signal did happen in a cloned userspace process,
> > > > handle it.
> > > > + */
> > > > +void segv_handler(int sig, struct siginfo *unused_si, struct
> > > > uml_pt_regs *regs,
> > > > +		  void *mc)
> > > > +{
> > > > +	struct faultinfo *fi = UPT_FAULTINFO(regs);
> > > > +
> > > > +	/* !MMU specific part; detection of userspace */
> > > > +	/* mark is_user=1 when the IP is from userspace code. */
> > > > +	if (UPT_IP(regs) > uml_reserved && UPT_IP(regs) <
> > > > high_physmem)
> > > > +		regs->is_user = 1;
> > > > +
> > > > +	if (UPT_IS_USER(regs) && !SEGV_IS_FIXABLE(fi)) {
> > > > +		show_segv_info(regs);
> > > > +		bad_segv(*fi, UPT_IP(regs));
> > > > +		return;
> > > > +	}
> > > > +	segv(*fi, UPT_IP(regs), UPT_IS_USER(regs), regs, mc);
> > > > +
> > > > +	/* !MMU specific part; detection of userspace */
> > > > +	relay_signal(sig, unused_si, regs, mc);
> > > > +}
> > > > +
> > > > +/*
> > > > + * We give a *copy* of the faultinfo in the regs to segv.
> > > > + * This must be done, since nesting SEGVs could overwrite
> > > > + * the info in the regs. A pointer to the info then would
> > > > + * give us bad data!
> > > > + */
> > > > +unsigned long segv(struct faultinfo fi, unsigned long ip, int
> > > > is_user,
> > > > +		   struct uml_pt_regs *regs, void *mc)
> > > > +{
> > > > +	int si_code;
> > > > +	int err;
> > > > +	int is_write = FAULT_WRITE(fi);
> > > > +	unsigned long address = FAULT_ADDRESS(fi);
> > > > +
> > > > +	if (!is_user && regs)
> > > > +		current->thread.segv_regs = container_of(regs,
> > > > struct pt_regs, regs);
> > > > +
> > > > +	if (current->mm == NULL) {
> > > > +		show_regs(container_of(regs, struct pt_regs,
> > > > regs));
> > > > +		panic("Segfault with no mm");
> > > > +	} else if (!is_user && address > PAGE_SIZE && address <
> > > > TASK_SIZE) {
> > > > +		show_regs(container_of(regs, struct pt_regs,
> > > > regs));
> > > > +		panic("Kernel tried to access user memory at addr
> > > > 0x%lx, ip 0x%lx",
> > > > +		       address, ip);
> > > > +	}
> > > > +
> > > > +	if (SEGV_IS_FIXABLE(&fi))
> > > > +		err = handle_page_fault(address, ip, is_write,
> > > > is_user,
> > > > +					&si_code);
> > > > +	else {
> > > > +		err = -EFAULT;
> > > > +		/*
> > > > +		 * A thread accessed NULL, we get a fault, but CR2
> > > > is invalid.
> > > > +		 * This code is used in __do_copy_from_user() of
> > > > TT mode.
> > > > +		 * XXX tt mode is gone, so maybe this isn't needed
> > > > any more
> > > > +		 */
> > > > +		address = 0;
> > > > +	}
> > > > +
> > > > +	if (!err)
> > > > +		goto out;
> > > > +	else if (!is_user && arch_fixup(ip, regs))
> > > > +		goto out;
> > > > +
> > > > +	if (!is_user) {
> > > > +		show_regs(container_of(regs, struct pt_regs,
> > > > regs));
> > > > +		panic("Kernel mode fault at addr 0x%lx, ip 0x%lx",
> > > > +		      address, ip);
> > > > +	}
> > > > +
> > > > +	show_segv_info(regs);
> > > > +
> > > > +	if (err == -EACCES) {
> > > > +		current->thread.arch.faultinfo = fi;
> > > > +		force_sig_fault(SIGBUS, BUS_ADRERR, (void __user
> > > > *)address);
> > > > +	} else {
> > > > +		WARN_ON_ONCE(err != -EFAULT);
> > > > +		current->thread.arch.faultinfo = fi;
> > > > +		force_sig_fault(SIGSEGV, si_code, (void __user *)
> > > > address);
> > > > +	}
> > > > +
> > > > +out:
> > > > +	if (regs)
> > > > +		current->thread.segv_regs = NULL;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +void relay_signal(int sig, struct siginfo *si, struct uml_pt_regs
> > > > *regs,
> > > > +		  void *mc)
> > > > +{
> > > > +	int code, err;
> > > > +
> > > > +	/* !MMU specific part; detection of userspace */
> > > > +	/* mark is_user=1 when the IP is from userspace code. */
> > > > +	if (UPT_IP(regs) > uml_reserved && UPT_IP(regs) <
> > > > high_physmem)
> > > > +		regs->is_user = 1;
> > > > +
> > > > +	if (!UPT_IS_USER(regs)) {
> > > > +		if (sig == SIGBUS)
> > > > +			pr_err("Bus error - the host /dev/shm or
> > > > /tmp mount likely just ran out of space\n");
> > > > +		panic("Kernel mode signal %d", sig);
> > > > +	}
> > > > +	/* if is_user==1, set return to userspace sig handler to
> > > > relay signal */
> > > > +	nommu_relay_signal(mc);
> > > > +
> > > > +	arch_examine_signal(sig, regs);
> > > > +
> > > > +	/* Is the signal layout for the signal known?
> > > > +	 * Signal data must be scrubbed to prevent information
> > > > leaks.
> > > > +	 */
> > > > +	code = si->si_code;
> > > > +	err = si->si_errno;
> > > > +	if ((err == 0) && (siginfo_layout(sig, code) ==
> > > > SIL_FAULT)) {
> > > > +		struct faultinfo *fi = UPT_FAULTINFO(regs);
> > > > +
> > > > +		current->thread.arch.faultinfo = *fi;
> > > > +		force_sig_fault(sig, code, (void __user
> > > > *)FAULT_ADDRESS(*fi));
> > > > +	} else {
> > > > +		pr_err("Attempted to relay unknown signal %d
> > > > (si_code = %d) with errno %d\n",
> > > > +		       sig, code, err);
> > > > +		force_sig(sig);
> > > > +	}
> > > > +}
> > > > +
> > > > +void winch(int sig, struct siginfo *unused_si, struct uml_pt_regs
> > > > *regs,
> > > > +	   void *mc)
> > > > +{
> > > > +	do_IRQ(WINCH_IRQ, regs);
> > > > +}
> > > > diff --git a/arch/um/os-Linux/signal.c b/arch/um/os-Linux/signal.c
> > > > index 53e276e81b37..67dcd88b45b1 100644
> > > > --- a/arch/um/os-Linux/signal.c
> > > > +++ b/arch/um/os-Linux/signal.c
> > > > @@ -40,9 +40,10 @@ static void sig_handler_common(int sig, struct
> > > > siginfo *si, mcontext_t *mc)
> > > >  	int save_errno = errno;
> > > >  
> > > >  	r.is_user = 0;
> > > > +	if (mc)
> > > > +		get_regs_from_mc(&r, mc);
> > > >  	if (sig == SIGSEGV) {
> > > >  		/* For segfaults, we want the data from the
> > > > sigcontext. */
> > > > -		get_regs_from_mc(&r, mc);
> > > >  		GET_FAULTINFO_FROM_MC(r.faultinfo, mc);
> > > >  	}
> > > >  
> > > > diff --git a/arch/x86/um/nommu/do_syscall_64.c
> > > > b/arch/x86/um/nommu/do_syscall_64.c
> > > > index 74d5bcc4508d..d77e69e097c1 100644
> > > > --- a/arch/x86/um/nommu/do_syscall_64.c
> > > > +++ b/arch/x86/um/nommu/do_syscall_64.c
> > > > @@ -44,6 +44,9 @@ __visible void do_syscall_64(struct pt_regs
> > > > *regs)
> > > >  	/* set fs register to the original host one */
> > > >  	os_x86_arch_prctl(0, ARCH_SET_FS, (void *)host_fs);
> > > >  
> > > > +	/* save fp registers */
> > > > +	asm volatile("fxsaveq %0" : "=m"(*(struct _xstate *)regs-
> > > > > regs.fp));
> > > > +
> > > >  	if (likely(syscall < NR_syscalls)) {
> > > >  		PT_REGS_SET_SYSCALL_RETURN(regs,
> > > >  				EXECUTE_SYSCALL(syscall, regs));
> > > > @@ -54,6 +57,9 @@ __visible void do_syscall_64(struct pt_regs
> > > > *regs)
> > > >  	/* handle tasks and signals at the end */
> > > >  	interrupt_end();
> > > >  
> > > > +	/* restore fp registers */
> > > > +	asm volatile("fxrstorq %0" : : "m"((current-
> > > > > thread.regs.regs.fp)));
> > > > +
> > > >  	/* restore back fs register to userspace configured one */
> > > >  	os_x86_arch_prctl(0, ARCH_SET_FS,
> > > >  		      (void *)(current-
> > > > > thread.regs.regs.gp[FS_BASE
> > > > diff --git a/arch/x86/um/nommu/entry_64.S
> > > > b/arch/x86/um/nommu/entry_64.S
> > > > index 950447dfa66b..e038bc7b53ac 100644
> > > > --- a/arch/x86/um/nommu/entry_64.S
> > > > +++ b/arch/x86/um/nommu/entry_64.S
> > > > @@ -111,3 +111,17 @@ ENTRY(userspace)
> > > >  	jmp	*%r11
> > > >  
> > > >  END(userspace)
> > > > +
> > > > +/*
> > > > + * this routine prepares the stack to return via host-generated
> > > > + * signals (e.g., SEGV, FPE) via do_signal() from interrupt_end().
> > > > + */
> > > > +ENTRY(__prep_sigreturn)
> > > > +	/*
> > > > +	 * Switch to current top of stack, so "current->" points
> > > > +	 * to the right task.
> > > > +	 */
> > > > +	movq	current_top_of_stack, %rsp
> > > > +
> > > > +	jmp	userspace
> > > > +END(__prep_sigreturn)
> > > > diff --git a/arch/x86/um/nommu/os-Linux/mcontext.c
> > > > b/arch/x86/um/nommu/os-Linux/mcontext.c
> > > > index c4ef877d5ea0..87fb2a35e7ff 100644
> > > > --- a/arch/x86/um/nommu/os-Linux/mcontext.c
> > > > +++ b/arch/x86/um/nommu/os-Linux/mcontext.c
> > > > @@ -6,6 +6,11 @@
> > > >  #include <sysdep/mcontext.h>
> > > >  #include <sysdep/syscalls.h>
> > > >  
> > > > +void set_mc_return_address(mcontext_t *mc)
> > > > +{
> > > > +	mc->gregs[REG_RIP] = (unsigned long) __prep_sigreturn;
> > > > +}
> > > > +
> > > >  void set_mc_sigsys_hook(mcontext_t *mc)
> > > >  {
> > > >  	mc->gregs[REG_RCX] = mc->gregs[REG_RIP];
> > > > diff --git a/arch/x86/um/shared/sysdep/mcontext.h
> > > > b/arch/x86/um/shared/sysdep/mcontext.h
> > > > index 9a0d6087f357..de4041b758f3 100644
> > > > --- a/arch/x86/um/shared/sysdep/mcontext.h
> > > > +++ b/arch/x86/um/shared/sysdep/mcontext.h
> > > > @@ -19,6 +19,7 @@ extern int set_stub_state(struct uml_pt_regs
> > > > *regs, struct stub_data *data,
> > > >  
> > > >  #ifndef CONFIG_MMU
> > > >  extern void set_mc_sigsys_hook(mcontext_t *mc);
> > > > +extern void set_mc_return_address(mcontext_t *mc);
> > > >  #endif
> > > >  
> > > >  #ifdef __i386__
> > > > diff --git a/arch/x86/um/shared/sysdep/ptrace.h
> > > > b/arch/x86/um/shared/sysdep/ptrace.h
> > > > index 8f7476ff6e95..7d553d9f05be 100644
> > > > --- a/arch/x86/um/shared/sysdep/ptrace.h
> > > > +++ b/arch/x86/um/shared/sysdep/ptrace.h
> > > > @@ -65,7 +65,7 @@ struct uml_pt_regs {
> > > >  	int is_user;
> > > >  
> > > >  	/* Dynamically sized FP registers (holds an XSTATE) */
> > > > -	unsigned long fp[];
> > > > +	unsigned long fp[] __attribute__((aligned(16)));
> > > >  };
> > > >  
> > > >  #define EMPTY_UML_PT_REGS { }
> > > > diff --git a/arch/x86/um/shared/sysdep/syscalls_64.h
> > > > b/arch/x86/um/shared/sysdep/syscalls_64.h
> > > > index ffd80ee3b9dc..bd152422cdfb 100644
> > > > --- a/arch/x86/um/shared/sysdep/syscalls_64.h
> > > > +++ b/arch/x86/um/shared/sysdep/syscalls_64.h
> > > > @@ -29,6 +29,7 @@ extern syscall_handler_t sys_arch_prctl;
> > > >  extern void do_syscall_64(struct pt_regs *regs);
> > > >  extern long __kernel_vsyscall(int64_t a0, int64_t a1, int64_t a2,
> > > > int64_t a3,
> > > >  			      int64_t a4, int64_t a5, int64_t a6);
> > > > +extern void __prep_sigreturn(void);
> > > >  #endif
> > > >  
> > > >  #endif
> > > 
> > 
> 

Powered by blists - more mailing lists