Re: [RFC PATCH 1/1] sched/rseq: Consider rseq abort in page fault handler

From: Dmitry Vyukov
Date: Tue Feb 20 2024 - 08:56:06 EST

Next message: Bartosz Golaszewski: "[PATCH] gpio: sim: delimit the fwnode name with a ":" when generating labels"
Previous message: Jagadeesh Kona: "[PATCH V2 5/6] clk: qcom: camcc-sm8650: Add SM8650 camera clock controller driver"
Next in thread: Dmitry Vyukov: "Re: [RFC PATCH 1/1] sched/rseq: Consider rseq abort in page fault handler"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, 15 Feb 2024 at 20:14, Mathieu Desnoyers
<mathieu.desnoyers@xxxxxxxxxxxx> wrote:
>
> Consider rseq abort before emitting the SIGSEGV or SIGBUS signals from
> the page fault handler.
>
> This allows using membarrier MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ
> to abort rseq critical sections which include memory accesses to
> memory which mapping can be munmap'd or mprotect'd after the
> membarrier "rseq fence" without causing SIGSEGV or SIGBUS when the page
> fault handler triggered by a faulting memory access within a rseq
> critical section is preempted before handling the page fault.
>
> The problematic scenario is:
>
> CPU 0 CPU 1
> ------------------------------------------------------------------
> old_p = P
> P = NULL
> - rseq c.s. begins
> - x = P
> - if (x != NULL)
> - v = *x
> - page fault
> - preempted
> membarrier(MEMBARRIER_CMD_PRIVATE_EXPEDITED_RSEQ)
> munmap(old_p) (or mprotect(old_p))
> - handle page fault
> - force_sig_fault(SIGSEGV)
> - rseq resume notifier
> - move IP to abort IP
> -> SIGSEGV handler runs.
>
> This is solved by postponing the force_sig_fault() to return to
> user-space when the page fault handler detects that rseq events will
> cause the thread to call the rseq resume notifier before going back to
> user-space. This allows the rseq resume notifier to load the userspace
> memory pointed by rseq->rseq_cs to compare the IP with the rseq c.s.
> range before either moving the IP to the abort handler or calling
> force_sig_fault() with the parameters previously saved by the page fault
> handler.
>
> Add a new AT_RSEQ_FEATURE_FLAGS getauxval(3) to allow user-space to
> query whether the kernel implements this behavior (flag:
> RSEQ_FEATURE_PAGE_FAULT_ABORT).
>
> Untested implementation submitted for early feedback.
>
> Only x86 is implemented in this PoC.
>
> Link: https://lore.kernel.org/lkml/CACT4Y+bXfekygoyhO7pCctjnL15=E=Zs31BUGXU0dk8d4rc1Cw@xxxxxxxxxxxxxx/
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx>
> Cc: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
> Cc: Peter Oskolkov <posk@xxxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: "Paul E. McKenney" <paulmck@xxxxxxxxxx>
> Cc: Boqun Feng <boqun.feng@xxxxxxxxx>
> Cc: Chris Kennelly <ckennelly@xxxxxxxxxx>
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Cc: Andy Lutomirski <luto@xxxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Borislav Petkov <bp@xxxxxxxxx>
> Cc: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> Cc: "H. Peter Anvin" <hpa@xxxxxxxxx>
> Cc: linux-mm@xxxxxxxxx

Hi Mathieu,

Thanks for the quick fix.
I can try to test this, but I can't apply this.
What's the base commit for the patch?

On top of latest upstream head v6.8-rc5:

$ patch -p1 < /tmp/patch
patching file arch/x86/mm/fault.c
patching file fs/binfmt_elf.c
patching file include/linux/sched.h
Hunk #1 succeeded at 745 (offset 2 lines).
Hunk #2 succeeded at 1329 (offset 3 lines).
Hunk #3 succeeded at 2143 with fuzz 2 (offset -197 lines).
Hunk #4 FAILED at 2402.
Hunk #5 FAILED at 2417.
2 out of 5 hunks FAILED -- saving rejects to file include/linux/sched.h.rej
patching file include/linux/sched/signal.h
Hunk #1 succeeded at 784 (offset 3 lines).
patching file include/uapi/linux/auxvec.h
patching file include/uapi/linux/rseq.h
patching file kernel/rseq.c
Hunk #2 succeeded at 299 with fuzz 1.

> ---
> arch/x86/mm/fault.c | 4 ++--
> fs/binfmt_elf.c | 1 +
> include/linux/sched.h | 16 ++++++++++++++++
> include/linux/sched/signal.h | 24 ++++++++++++++++++++++++
> include/uapi/linux/auxvec.h | 1 +
> include/uapi/linux/rseq.h | 7 +++++++
> kernel/rseq.c | 36 +++++++++++++++++++++++++++++++-----
> 7 files changed, 82 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index 679b09cfe241..42ac39680cb6 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -854,7 +854,7 @@ __bad_area_nosemaphore(struct pt_regs *regs, unsigned long error_code,
> if (si_code == SEGV_PKUERR)
> force_sig_pkuerr((void __user *)address, pkey);
> else
> - force_sig_fault(SIGSEGV, si_code, (void __user *)address);
> + rseq_lazy_force_sig_fault(SIGSEGV, si_code, (void __user *)address);
>
> local_irq_disable();
> }
> @@ -973,7 +973,7 @@ do_sigbus(struct pt_regs *regs, unsigned long error_code, unsigned long address,
> return;
> }
> #endif
> - force_sig_fault(SIGBUS, BUS_ADRERR, (void __user *)address);
> + rseq_lazy_force_sig_fault(SIGBUS, BUS_ADRERR, (void __user *)address);
> }
>
> static int spurious_kernel_fault_check(unsigned long error_code, pte_t *pte)
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 5397b552fbeb..8fece0911c7d 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -273,6 +273,7 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec,
> #ifdef CONFIG_RSEQ
> NEW_AUX_ENT(AT_RSEQ_FEATURE_SIZE, offsetof(struct rseq, end));
> NEW_AUX_ENT(AT_RSEQ_ALIGN, __alignof__(struct rseq));
> + NEW_AUX_ENT(AT_RSEQ_FEATURE_FLAGS, RSEQ_FEATURE_FLAGS);
> #endif
> #undef NEW_AUX_ENT
> /* AT_NULL is zero; clear the rest too */
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 292c31697248..39aa585ba2a3 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -743,6 +743,15 @@ struct kmap_ctrl {
> #endif
> };
>
> +#ifdef CONFIG_RSEQ
> +struct rseq_lazy_sig {
> + bool pending;
> + int sig;
> + int code;
> + void __user *addr;
> +};
> +#endif
> +
> struct task_struct {
> #ifdef CONFIG_THREAD_INFO_IN_TASK
> /*
> @@ -1317,6 +1326,7 @@ struct task_struct {
> * with respect to preemption.
> */
> unsigned long rseq_event_mask;
> + struct rseq_lazy_sig rseq_lazy_sig;
> #endif
>
> #ifdef CONFIG_SCHED_MM_CID
> @@ -2330,6 +2340,8 @@ unsigned long sched_cpu_util(int cpu);
>
> #ifdef CONFIG_RSEQ
>
> +#define RSEQ_FEATURE_FLAGS RSEQ_FEATURE_PAGE_FAULT_ABORT
> +
> /*
> * Map the event mask on the user-space ABI enum rseq_cs_flags
> * for direct mask checks.
> @@ -2390,6 +2402,8 @@ static inline void rseq_migrate(struct task_struct *t)
> */
> static inline void rseq_fork(struct task_struct *t, unsigned long clone_flags)
> {
> + WARN_ON_ONCE(current->rseq_lazy_sig.pending);
> +
> if (clone_flags & CLONE_VM) {
> t->rseq = NULL;
> t->rseq_len = 0;
> @@ -2405,6 +2419,8 @@ static inline void rseq_fork(struct task_struct *t, unsigned long clone_flags)
>
> static inline void rseq_execve(struct task_struct *t)
> {
> + WARN_ON_ONCE(current->rseq_lazy_sig.pending);
> +
> t->rseq = NULL;
> t->rseq_len = 0;
> t->rseq_sig = 0;
> diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
> index 3499c1a8b929..0d75dfde2f9b 100644
> --- a/include/linux/sched/signal.h
> +++ b/include/linux/sched/signal.h
> @@ -781,4 +781,28 @@ static inline unsigned long rlimit_max(unsigned int limit)
> return task_rlimit_max(current, limit);
> }
>
> +#ifdef CONFIG_RSEQ
> +
> +static inline int rseq_lazy_force_sig_fault(int sig, int code, void __user *addr)
> +{
> + struct task_struct *t = current;
> +
> + if (!t->rseq_event_mask)
> + return force_sig_fault(sig, code, addr);
> + t->rseq_lazy_sig.pending = true;
> + t->rseq_lazy_sig.sig = sig;
> + t->rseq_lazy_sig.code = code;
> + t->rseq_lazy_sig.addr = addr;
> + return 0;
> +}
> +
> +#else
> +
> +static inline int rseq_lazy_force_sig_fault(int sig, int code, void __user *addr)
> +{
> + return force_sig_fault(sig, code, addr);
> +}
> +
> +#endif
> +
> #endif /* _LINUX_SCHED_SIGNAL_H */
> diff --git a/include/uapi/linux/auxvec.h b/include/uapi/linux/auxvec.h
> index 6991c4b8ab18..5044f367a219 100644
> --- a/include/uapi/linux/auxvec.h
> +++ b/include/uapi/linux/auxvec.h
> @@ -32,6 +32,7 @@
> #define AT_HWCAP2 26 /* extension of AT_HWCAP */
> #define AT_RSEQ_FEATURE_SIZE 27 /* rseq supported feature size */
> #define AT_RSEQ_ALIGN 28 /* rseq allocation alignment */
> +#define AT_RSEQ_FEATURE_FLAGS 29 /* rseq feature flags */
>
> #define AT_EXECFN 31 /* filename of program */
>
> diff --git a/include/uapi/linux/rseq.h b/include/uapi/linux/rseq.h
> index c233aae5eac9..0fdb192e3cd3 100644
> --- a/include/uapi/linux/rseq.h
> +++ b/include/uapi/linux/rseq.h
> @@ -37,6 +37,13 @@ enum rseq_cs_flags {
> (1U << RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE_BIT),
> };
>
> +/*
> + * rseq feature flags. Query with getauxval(AT_RSEQ_FEATURE_FLAGS).
> + */
> +enum rseq_feature_flags {
> + RSEQ_FEATURE_PAGE_FAULT_ABORT = (1U << 0),
> +};
> +
> /*
> * struct rseq_cs is aligned on 4 * 8 bytes to ensure it is always
> * contained within a single cache-line. It is usually declared as
> diff --git a/kernel/rseq.c b/kernel/rseq.c
> index 9de6e35fe679..f686a97abb45 100644
> --- a/kernel/rseq.c
> +++ b/kernel/rseq.c
> @@ -271,6 +271,25 @@ static bool in_rseq_cs(unsigned long ip, struct rseq_cs *rseq_cs)
> return ip - rseq_cs->start_ip < rseq_cs->post_commit_offset;
> }
>
> +static void rseq_clear_lazy_sig_fault(struct task_struct *t)
> +{
> + if (!t->rseq_lazy_sig.pending)
> + return;
> + t->rseq_lazy_sig.pending = false;
> + t->rseq_lazy_sig.sig = 0;
> + t->rseq_lazy_sig.code = 0;
> + t->rseq_lazy_sig.addr = NULL;
> +}
> +
> +static void rseq_force_lazy_sig_fault(struct task_struct *t)
> +{
> + if (!t->rseq_lazy_sig.pending)
> + return;
> + force_sig_fault(t->rseq_lazy_sig.sig, t->rseq_lazy_sig.code,
> + t->rseq_lazy_sig.addr);
> + rseq_clear_lazy_sig_fault(t);
> +}
> +
> static int rseq_ip_fixup(struct pt_regs *regs)
> {
> unsigned long ip = instruction_pointer(regs);
> @@ -280,25 +299,32 @@ static int rseq_ip_fixup(struct pt_regs *regs)
>
> ret = rseq_get_rseq_cs(t, &rseq_cs);
> if (ret)
> - return ret;
> + goto nofixup;
>
> /*
> * Handle potentially not being within a critical section.
> * If not nested over a rseq critical section, restart is useless.
> * Clear the rseq_cs pointer and return.
> */
> - if (!in_rseq_cs(ip, &rseq_cs))
> - return clear_rseq_cs(t);
> + if (!in_rseq_cs(ip, &rseq_cs)) {
> + ret = clear_rseq_cs(t);
> + goto nofixup;
> + }
> ret = rseq_need_restart(t, rseq_cs.flags);
> if (ret <= 0)
> - return ret;
> + goto nofixup;
> ret = clear_rseq_cs(t);
> if (ret)
> - return ret;
> + goto nofixup;
> + rseq_clear_lazy_sig_fault(t);
> trace_rseq_ip_fixup(ip, rseq_cs.start_ip, rseq_cs.post_commit_offset,
> rseq_cs.abort_ip);
> instruction_pointer_set(regs, (unsigned long)rseq_cs.abort_ip);
> return 0;
> +
> +nofixup:
> + rseq_force_lazy_sig_fault(t);
> + return ret;
> }
>
> /*
> --
> 2.39.2
>

Next message: Bartosz Golaszewski: "[PATCH] gpio: sim: delimit the fwnode name with a ":" when generating labels"
Previous message: Jagadeesh Kona: "[PATCH V2 5/6] clk: qcom: camcc-sm8650: Add SM8650 camera clock controller driver"
Next in thread: Dmitry Vyukov: "Re: [RFC PATCH 1/1] sched/rseq: Consider rseq abort in page fault handler"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]