Hi,
there is a small bug that causes issues with this patch, see below.
On Thu, 2024-10-10 at 23:12 +0200, Johannes Berg wrote:
> [SNIP]
> diff --git a/arch/um/kernel/trap.c b/arch/um/kernel/trap.c
> index cdaee3e94273..378e6e1815b9 100644
> --- a/arch/um/kernel/trap.c
> +++ b/arch/um/kernel/trap.c
> @@ -16,6 +16,7 @@
> #include <kern_util.h>
> #include <os.h>
> #include <skas.h>
> +#include <arch.h>
>
> /*
> * Note this is constrained to return 0, -EFAULT, -EACCES, -ENOMEM by
> @@ -180,7 +181,8 @@ void fatal_sigsegv(void)
> * 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 segv_handler(int sig, struct siginfo *unused_si, struct uml_pt_regs
> *regs,
> + void *mc)
> {
> struct faultinfo * fi = UPT_FAULTINFO(regs);
>
> @@ -189,7 +191,7 @@ void segv_handler(int sig, struct siginfo *unused_si,
> struct uml_pt_regs *regs)
> bad_segv(*fi, UPT_IP(regs));
> return;
> }
> - segv(*fi, UPT_IP(regs), UPT_IS_USER(regs), regs);
> + segv(*fi, UPT_IP(regs), UPT_IS_USER(regs), regs, mc);
> }
>
> /*
> @@ -199,7 +201,7 @@ void segv_handler(int sig, struct siginfo *unused_si,
> struct uml_pt_regs *regs)
> * give us bad data!
> */
> unsigned long segv(struct faultinfo fi, unsigned long ip, int is_user,
> - struct uml_pt_regs *regs)
> + struct uml_pt_regs *regs, void *mc)
> {
> int si_code;
> int err;
> @@ -223,6 +225,19 @@ unsigned long segv(struct faultinfo fi, unsigned long
> ip, int is_user,
> goto out;
> }
> else if (current->mm == NULL) {
> + if (current->pagefault_disabled) {
> + if (!mc) {
> + show_regs(container_of(regs, struct pt_regs,
> regs));
> + panic("Segfault with pagefaults disabled but no
> mcontext");
> + }
> + if (!current->thread.segv_continue) {
> + show_regs(container_of(regs, struct pt_regs,
> regs));
> + panic("Segfault without recovery target");
> + }
> + mc_set_rip(mc, current->thread.segv_continue);
> + current->thread.segv_continue = NULL;
> + return 0;
This needs to be "goto out". Otherwise segv_regs is still set, which
will result in a crash later on if the stacktrace code reads an
incorrect stack pointer from there.
Benjamin
> + }
> show_regs(container_of(regs, struct pt_regs, regs));
> panic("Segfault with no mm");
> }
> @@ -274,7 +289,8 @@ unsigned long segv(struct faultinfo fi, unsigned long ip,
> int is_user,
> return 0;
> }
>
> -void relay_signal(int sig, struct siginfo *si, struct uml_pt_regs *regs)
> +void relay_signal(int sig, struct siginfo *si, struct uml_pt_regs *regs,
> + void *mc)
> {
> int code, err;
> if (!UPT_IS_USER(regs)) {
> @@ -302,7 +318,8 @@ void relay_signal(int sig, struct siginfo *si, struct
> uml_pt_regs *regs)
> }
> }
>
> -void winch(int sig, struct siginfo *unused_si, struct uml_pt_regs *regs)
> +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 1978eaa557e9..2aa02657ee78 100644
> --- a/arch/um/os-Linux/signal.c
> +++ b/arch/um/os-Linux/signal.c
> @@ -21,7 +21,7 @@
> #include <sys/ucontext.h>
> #include <timetravel.h>
>
> -void (*sig_info[NSIG])(int, struct siginfo *, struct uml_pt_regs *) = {
> +void (*sig_info[NSIG])(int, struct siginfo *, struct uml_pt_regs *, void
> *mc) = {
> [SIGTRAP] = relay_signal,
> [SIGFPE] = relay_signal,
> [SIGILL] = relay_signal,
> @@ -47,7 +47,7 @@ static void sig_handler_common(int sig, struct siginfo *si,
> mcontext_t *mc)
> if ((sig != SIGIO) && (sig != SIGWINCH))
> unblock_signals_trace();
>
> - (*sig_info[sig])(sig, si, &r);
> + (*sig_info[sig])(sig, si, &r, mc);
>
> errno = save_errno;
> }
> diff --git a/arch/um/os-Linux/skas/process.c b/arch/um/os-Linux/skas/process.c
> index 2286486bb0f3..1447be7a4bf2 100644
> --- a/arch/um/os-Linux/skas/process.c
> +++ b/arch/um/os-Linux/skas/process.c
> @@ -166,7 +166,7 @@ static void get_skas_faultinfo(int pid, struct faultinfo
> *fi)
> static void handle_segv(int pid, struct uml_pt_regs *regs)
> {
> get_skas_faultinfo(pid, ®s->faultinfo);
> - segv(regs->faultinfo, 0, 1, NULL);
> + segv(regs->faultinfo, 0, 1, NULL, NULL);
> }
>
> static void handle_trap(int pid, struct uml_pt_regs *regs)
> @@ -489,7 +489,7 @@ void userspace(struct uml_pt_regs *regs)
> get_skas_faultinfo(pid,
> ®s->faultinfo);
> (*sig_info[SIGSEGV])(SIGSEGV, (struct
> siginfo *)&si,
> - regs);
> + regs, NULL);
> }
> else handle_segv(pid, regs);
> break;
> @@ -497,7 +497,7 @@ void userspace(struct uml_pt_regs *regs)
> handle_trap(pid, regs);
> break;
> case SIGTRAP:
> - relay_signal(SIGTRAP, (struct siginfo *)&si,
> regs);
> + relay_signal(SIGTRAP, (struct siginfo *)&si,
> regs, NULL);
> break;
> case SIGALRM:
> break;
> @@ -507,7 +507,7 @@ void userspace(struct uml_pt_regs *regs)
> case SIGFPE:
> case SIGWINCH:
> block_signals_trace();
> - (*sig_info[sig])(sig, (struct siginfo *)&si,
> regs);
> + (*sig_info[sig])(sig, (struct siginfo *)&si,
> regs, NULL);
> unblock_signals_trace();
> break;
> default:
> diff --git a/arch/x86/um/os-Linux/mcontext.c b/arch/x86/um/os-Linux/mcontext.c
> index e80ab7d28117..d2f3a595b4ef 100644
> --- a/arch/x86/um/os-Linux/mcontext.c
> +++ b/arch/x86/um/os-Linux/mcontext.c
> @@ -4,6 +4,7 @@
> #include <asm/ptrace.h>
> #include <sysdep/ptrace.h>
> #include <sysdep/mcontext.h>
> +#include <arch.h>
>
> void get_regs_from_mc(struct uml_pt_regs *regs, mcontext_t *mc)
> {
> @@ -31,3 +32,14 @@ void get_regs_from_mc(struct uml_pt_regs *regs, mcontext_t
> *mc)
> regs->gp[CS / sizeof(unsigned long)] |= 3;
> #endif
> }
> +
> +void mc_set_rip(void *_mc, void *target)
> +{
> + mcontext_t *mc = _mc;
> +
> +#ifdef __i386__
> + mc->gregs[REG_EIP] = (unsigned long)target;
> +#else
> + mc->gregs[REG_RIP] = (unsigned long)target;
> +#endif
> +}
> diff --git a/arch/x86/um/shared/sysdep/faultinfo_32.h
> b/arch/x86/um/shared/sysdep/faultinfo_32.h
> index b6f2437ec29c..ab5c8e47049c 100644
> --- a/arch/x86/um/shared/sysdep/faultinfo_32.h
> +++ b/arch/x86/um/shared/sysdep/faultinfo_32.h
> @@ -29,4 +29,16 @@ struct faultinfo {
>
> #define PTRACE_FULL_FAULTINFO 0
>
> +#define ___backtrack_faulted(_faulted)
> \
> + asm volatile ( \
> + "mov $0, %0\n" \
> + "movl $__get_kernel_nofault_faulted_%=,%1\n" \
> + "jmp _end_%=\n" \
> + "__get_kernel_nofault_faulted_%=:\n" \
> + "mov $1, %0;" \
> + "_end_%=:" \
> + : "=r" (_faulted), \
> + "=m" (current->thread.segv_continue) :: \
> + )
> +
> #endif
> diff --git a/arch/x86/um/shared/sysdep/faultinfo_64.h
> b/arch/x86/um/shared/sysdep/faultinfo_64.h
> index ee88f88974ea..26fb4835d3e9 100644
> --- a/arch/x86/um/shared/sysdep/faultinfo_64.h
> +++ b/arch/x86/um/shared/sysdep/faultinfo_64.h
> @@ -29,4 +29,16 @@ struct faultinfo {
>
> #define PTRACE_FULL_FAULTINFO 1
>
> +#define ___backtrack_faulted(_faulted)
> \
> + asm volatile ( \
> + "mov $0, %0\n" \
> + "movq $__get_kernel_nofault_faulted_%=,%1\n" \
> + "jmp _end_%=\n" \
> + "__get_kernel_nofault_faulted_%=:\n" \
> + "mov $1, %0;" \
> + "_end_%=:" \
> + : "=r" (_faulted), \
> + "=m" (current->thread.segv_continue) :: \
> + )
> +
> #endif
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
