Re: [patch V4 23/36] rseq: Provide and use rseq_set_ids()

Mathieu Desnoyers
2025-09-11T13:41:02Z

On 2025-09-08 17:32, Thomas Gleixner wrote:
> Provide a new and straight forward implementation to set the IDs (CPU ID,
> Node ID and MM CID), which can be later inlined into the fast path.
>
> It does all operations in one user_rw_masked_begin() section and retrieves
> also the critical section member (rseq::cs_rseq) from user space to avoid
> another user..begin/end() pair. This is in preparation for optimizing the
> fast path to avoid extra work when not required.
>
> Use it to replace the whole related zoo in rseq.c
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
> V3: Fixed the node ID comparison in the debug path - Mathieu
> ---
> fs/binfmt_elf.c | 2
> include/linux/rseq_entry.h | 101 +++++++++++++++++++++
> include/linux/sched.h | 10 --
> kernel/rseq.c | 208 ++++++---------------------------------------
> 4 files changed, 134 insertions(+), 187 deletions(-)
>
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -46,7 +46,7 @@
> #include <linux/cred.h>
> #include <linux/dax.h>
> #include <linux/uaccess.h>
> -#include <linux/rseq.h>
> +#include <uapi/linux/rseq.h>
> #include <asm/param.h>
> #include <asm/page.h>
>
> --- a/include/linux/rseq_entry.h
> +++ b/include/linux/rseq_entry.h
> @@ -38,6 +38,8 @@ DECLARE_PER_CPU(struct rseq_stats, rseq_
> #include <linux/rseq.h>
> #include <linux/uaccess.h>
>
> +#include <uapi/linux/rseq.h>
> +
> #include <linux/tracepoint-defs.h>
>
> #ifdef CONFIG_TRACEPOINTS
> @@ -75,6 +77,7 @@ DECLARE_STATIC_KEY_MAYBE(CONFIG_RSEQ_DEB
> #endif
>
> bool rseq_debug_update_user_cs(struct task_struct *t, struct pt_regs *regs, unsigned long csaddr);
> +bool rseq_debug_validate_ids(struct task_struct *t);
>
> static __always_inline void rseq_note_user_irq_entry(void)
> {
> @@ -196,6 +199,50 @@ bool rseq_debug_update_user_cs(struct ta
> user_access_end();
> return false;
> }
> +
> +/*
> + * On debug kernels validate that user space did not mess with it if
> + * DEBUG_RSEQ is enabled, but don't on the first exit to user space. In
> + * that case cpu_cid is ~0. See fork/execve.
> + */
> +bool rseq_debug_validate_ids(struct task_struct *t)
> +{
> + struct rseq __user *rseq = t->rseq.usrptr;

Why not check on NULL rseq user pointer rather than skip using
the ~0ULL cpu_cid ?

> + u32 cpu_id, uval, node_id;
> +
> + if (t->rseq.ids.cpu_cid == ~0)

Here we are using ~0 and where cpu_cid is set in rseq_set() ~0ULL is
used. Now I understand that because of sign-extend and type promotion
this happens to provide all bits set on a 64-bit, but can we please just
pick one, e.g. ~0ULL ?

Also why does rseq_fork() set it to ~0ULL rather than keep the value
from the parent when forking a new process ?

Whatever was present in the parent process in the rseq area should
still be in the child, including the rseq registration.

Or am missing something ?

> + return true;
> +
> + /*
> + * Look it up outside of the user access section as cpu_to_node()
> + * can end up in debug code.
> + */
> + node_id = cpu_to_node(t->rseq.ids.cpu_id);
> +
> + if (!user_read_masked_begin(rseq))
> + return false;
> +
> + unsafe_get_user(cpu_id, &rseq->cpu_id_start, efault);
> + if (cpu_id != t->rseq.ids.cpu_id)
> + goto die;
> + unsafe_get_user(uval, &rseq->cpu_id, efault);
> + if (uval != cpu_id)
> + goto die;
> + unsafe_get_user(uval, &rseq->node_id, efault);
> + if (uval != node_id)
> + goto die;
> + unsafe_get_user(uval, &rseq->mm_cid, efault);
> + if (uval != t->rseq.ids.mm_cid)
> + goto die;
> + user_access_end();
> + return true;
> +die:
> + t->rseq.event.fatal = true;
> +efault:
> + user_access_end();
> + return false;
> +}
> +
> #endif /* RSEQ_BUILD_SLOW_PATH */
>
> /*
> @@ -281,6 +328,60 @@ rseq_update_user_cs(struct task_struct *
> user_access_end();
> return false;
> }
> +
> +/*
> + * Updates CPU ID, Node ID and MM CID and reads the critical section
> + * address, when @csaddr != NULL. This allows to put the ID update and the
> + * read under the same uaccess region to spare a seperate begin/end.

separate

> + *
> + * As this is either invoked from a C wrapper with @csaddr = NULL or from
> + * the fast path code with a valid pointer, a clever compiler should be
> + * able to optimize the read out. Spares a duplicate implementation.
> + *
> + * Returns true, if the operation was successful, false otherwise.
> + *
> + * In the failure case task::rseq_event::fatal is set when invalid data
> + * was found on debug kernels. It's clear when the failure was an unresolved page
> + * fault.
> + *
> + * If inlined into the exit to user path with interrupts disabled, the
> + * caller has to protect against page faults with pagefault_disable().
> + *
> + * In preemptible task context this would be counterproductive as the page
> + * faults could not be fully resolved. As a consequence unresolved page
> + * faults in task context are fatal too.
> + */
> +static rseq_inline
> +bool rseq_set_ids_get_csaddr(struct task_struct *t, struct rseq_ids *ids,
> + u32 node_id, u64 *csaddr)
> +{
> + struct rseq __user *rseq = t->rseq.usrptr;
> +
> + if (static_branch_unlikely(&rseq_debug_enabled)) {
> + if (!rseq_debug_validate_ids(t))
> + return false;
> + }
> +
> + if (!user_rw_masked_begin(rseq))
> + return false;
> +
> + unsafe_put_user(ids->cpu_id, &rseq->cpu_id_start, efault);
> + unsafe_put_user(ids->cpu_id, &rseq->cpu_id, efault);
> + unsafe_put_user(node_id, &rseq->node_id, efault);
> + unsafe_put_user(ids->mm_cid, &rseq->mm_cid, efault);
> + if (csaddr)
> + unsafe_get_user(*csaddr, &rseq->rseq_cs, efault);
> + user_access_end();
> +
> + /* Cache the new values */
> + t->rseq.ids.cpu_cid = ids->cpu_cid;
> + rseq_stat_inc(rseq_stats.ids);
> + rseq_trace_update(t, ids);
> + return true;
> +efault:
> + user_access_end();
> + return false;
> +}
>
> static __always_inline void rseq_exit_to_user_mode(void)
> {
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -42,7 +42,6 @@
> #include <linux/posix-timers_types.h>
> #include <linux/restart_block.h>
> #include <linux/rseq_types.h>
> -#include <uapi/linux/rseq.h>
> #include <linux/seqlock_types.h>
> #include <linux/kcsan.h>
> #include <linux/rv.h>
> @@ -1402,15 +1401,6 @@ struct task_struct {
> #endif /* CONFIG_NUMA_BALANCING */
>
> struct rseq_data rseq;
> -#ifdef CONFIG_DEBUG_RSEQ
> - /*
> - * This is a place holder to save a copy of the rseq fields for
> - * validation of read-only fields. The struct rseq has a
> - * variable-length array at the end, so it cannot be used
> - * directly. Reserve a size large enough for the known fields.
> - */
> - char rseq_fields[sizeof(struct rseq)];
> -#endif
>
> #ifdef CONFIG_SCHED_MM_CID
> int mm_cid; /* Current cid in mm */
> --- a/kernel/rseq.c
> +++ b/kernel/rseq.c
> @@ -88,13 +88,6 @@
> # define RSEQ_EVENT_GUARD preempt
> #endif
>
> -/* The original rseq structure size (including padding) is 32 bytes. */
> -#define ORIG_RSEQ_SIZE 32
> -
> -#define RSEQ_CS_NO_RESTART_FLAGS (RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT | \
> - RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL | \
> - RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE)
> -
> DEFINE_STATIC_KEY_MAYBE(CONFIG_RSEQ_DEBUG_DEFAULT_ENABLE, rseq_debug_enabled);
>
> static inline void rseq_control_debug(bool on)
> @@ -227,159 +220,9 @@ static int __init rseq_debugfs_init(void
> __initcall(rseq_debugfs_init);
> #endif /* CONFIG_DEBUG_FS */
>
> -#ifdef CONFIG_DEBUG_RSEQ
> -static struct rseq *rseq_kernel_fields(struct task_struct *t)
> -{
> - return (struct rseq *) t->rseq_fields;
> -}
> -
> -static int rseq_validate_ro_fields(struct task_struct *t)
> -{
> - static DEFINE_RATELIMIT_STATE(_rs,
> - DEFAULT_RATELIMIT_INTERVAL,
> - DEFAULT_RATELIMIT_BURST);
> - u32 cpu_id_start, cpu_id, node_id, mm_cid;
> - struct rseq __user *rseq = t->rseq.usrptr;
> -
> - /*
> - * Validate fields which are required to be read-only by
> - * user-space.
> - */
> - if (!user_read_access_begin(rseq, t->rseq.len))
> - goto efault;
> - unsafe_get_user(cpu_id_start, &rseq->cpu_id_start, efault_end);
> - unsafe_get_user(cpu_id, &rseq->cpu_id, efault_end);
> - unsafe_get_user(node_id, &rseq->node_id, efault_end);
> - unsafe_get_user(mm_cid, &rseq->mm_cid, efault_end);
> - user_read_access_end();
> -
> - if ((cpu_id_start != rseq_kernel_fields(t)->cpu_id_start ||
> - cpu_id != rseq_kernel_fields(t)->cpu_id ||
> - node_id != rseq_kernel_fields(t)->node_id ||
> - mm_cid != rseq_kernel_fields(t)->mm_cid) && __ratelimit(&_rs)) {
> -
> - pr_warn("Detected rseq corruption for pid: %d, name: %s\n"
> - "\tcpu_id_start: %u ?= %u\n"
> - "\tcpu_id: %u ?= %u\n"
> - "\tnode_id: %u ?= %u\n"
> - "\tmm_cid: %u ?= %u\n",
> - t->pid, t->comm,
> - cpu_id_start, rseq_kernel_fields(t)->cpu_id_start,
> - cpu_id, rseq_kernel_fields(t)->cpu_id,
> - node_id, rseq_kernel_fields(t)->node_id,
> - mm_cid, rseq_kernel_fields(t)->mm_cid);
> - }
> -
> - /* For now, only print a console warning on mismatch. */
> - return 0;
> -
> -efault_end:
> - user_read_access_end();
> -efault:
> - return -EFAULT;
> -}
> -
> -/*
> - * Update an rseq field and its in-kernel copy in lock-step to keep a coherent
> - * state.
> - */
> -#define rseq_unsafe_put_user(t, value, field, error_label) \
> - do { \
> - unsafe_put_user(value, &t->rseq.usrptr->field, error_label); \
> - rseq_kernel_fields(t)->field = value; \
> - } while (0)
> -
> -#else
> -static int rseq_validate_ro_fields(struct task_struct *t)
> -{
> - return 0;
> -}
> -
> -#define rseq_unsafe_put_user(t, value, field, error_label) \
> - unsafe_put_user(value, &t->rseq.usrptr->field, error_label)
> -#endif
> -
> -static int rseq_update_cpu_node_id(struct task_struct *t)
> -{
> - struct rseq __user *rseq = t->rseq.usrptr;
> - u32 cpu_id = raw_smp_processor_id();
> - u32 node_id = cpu_to_node(cpu_id);
> - u32 mm_cid = task_mm_cid(t);
> -
> - rseq_stat_inc(rseq_stats.ids);
> -
> - /* Validate read-only rseq fields on debug kernels */
> - if (rseq_validate_ro_fields(t))
> - goto efault;
> - WARN_ON_ONCE((int) mm_cid < 0);
> -
> - if (!user_write_access_begin(rseq, t->rseq.len))
> - goto efault;
> -
> - rseq_unsafe_put_user(t, cpu_id, cpu_id_start, efault_end);
> - rseq_unsafe_put_user(t, cpu_id, cpu_id, efault_end);
> - rseq_unsafe_put_user(t, node_id, node_id, efault_end);
> - rseq_unsafe_put_user(t, mm_cid, mm_cid, efault_end);
> -
> - /* Cache the user space values */
> - t->rseq.ids.cpu_id = cpu_id;
> - t->rseq.ids.mm_cid = mm_cid;
> -
> - /*
> - * Additional feature fields added after ORIG_RSEQ_SIZE
> - * need to be conditionally updated only if
> - * t->rseq_len != ORIG_RSEQ_SIZE.
> - */
> - user_write_access_end();
> - trace_rseq_update(t);
> - return 0;
> -
> -efault_end:
> - user_write_access_end();
> -efault:
> - return -EFAULT;
> -}
> -
> -static int rseq_reset_rseq_cpu_node_id(struct task_struct *t)
> +static bool rseq_set_ids(struct task_struct *t, struct rseq_ids *ids, u32 node_id)
> {
> - struct rseq __user *rseq = t->rseq.usrptr;
> - u32 cpu_id_start = 0, cpu_id = RSEQ_CPU_ID_UNINITIALIZED, node_id = 0,
> - mm_cid = 0;
> -
> - /*
> - * Validate read-only rseq fields.
> - */
> - if (rseq_validate_ro_fields(t))
> - goto efault;
> -
> - if (!user_write_access_begin(rseq, t->rseq.len))
> - goto efault;
> -
> - /*
> - * Reset all fields to their initial state.
> - *
> - * All fields have an initial state of 0 except cpu_id which is set to
> - * RSEQ_CPU_ID_UNINITIALIZED, so that any user coming in after
> - * unregistration can figure out that rseq needs to be registered
> - * again.
> - */
> - rseq_unsafe_put_user(t, cpu_id_start, cpu_id_start, efault_end);
> - rseq_unsafe_put_user(t, cpu_id, cpu_id, efault_end);
> - rseq_unsafe_put_user(t, node_id, node_id, efault_end);
> - rseq_unsafe_put_user(t, mm_cid, mm_cid, efault_end);
> -
> - /*
> - * Additional feature fields added after ORIG_RSEQ_SIZE
> - * need to be conditionally reset only if
> - * t->rseq_len != ORIG_RSEQ_SIZE.
> - */
> - user_write_access_end();
> - return 0;
> -
> -efault_end:
> - user_write_access_end();
> -efault:
> - return -EFAULT;
> + return rseq_set_ids_get_csaddr(t, ids, node_id, NULL);
> }
>
> static bool rseq_handle_cs(struct task_struct *t, struct pt_regs *regs)
> @@ -407,6 +250,8 @@ static bool rseq_handle_cs(struct task_s
> void __rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs)
> {
> struct task_struct *t = current;
> + struct rseq_ids ids;
> + u32 node_id;
> bool event;
> int sig;
>
> @@ -453,6 +298,8 @@ void __rseq_handle_notify_resume(struct
> scoped_guard(RSEQ_EVENT_GUARD) {
> event = t->rseq.event.sched_switch;
> t->rseq.event.sched_switch = false;
> + ids.cpu_id = task_cpu(t);
> + ids.mm_cid = task_mm_cid(t);
> }
>
> if (!IS_ENABLED(CONFIG_DEBUG_RSEQ) && !event)
> @@ -461,7 +308,8 @@ void __rseq_handle_notify_resume(struct
> if (!rseq_handle_cs(t, regs))
> goto error;
>
> - if (unlikely(rseq_update_cpu_node_id(t)))
> + node_id = cpu_to_node(ids.cpu_id);
> + if (!rseq_set_ids(t, &ids, node_id))
> goto error;
> return;
>
> @@ -502,13 +350,33 @@ void rseq_syscall(struct pt_regs *regs)
> }
> #endif
>
> +static bool rseq_reset_ids(void)
> +{
> + struct rseq_ids ids = {
> + .cpu_id = RSEQ_CPU_ID_UNINITIALIZED,
> + .mm_cid = 0,
> + };
> +
> + /*
> + * If this fails, terminate it because this leaves the kernel in
> + * stupid state as exit to user space will try to fixup the ids
> + * again.
> + */
> + if (rseq_set_ids(current, &ids, 0))
> + return true;
> +
> + force_sig(SIGSEGV);
> + return false;
> +}
> +
> +/* The original rseq structure size (including padding) is 32 bytes. */
> +#define ORIG_RSEQ_SIZE 32
> +
> /*
> * sys_rseq - setup restartable sequences for caller thread.
> */
> SYSCALL_DEFINE4(rseq, struct rseq __user *, rseq, u32, rseq_len, int, flags, u32, sig)
> {
> - int ret;
> -
> if (flags & RSEQ_FLAG_UNREGISTER) {
> if (flags & ~RSEQ_FLAG_UNREGISTER)
> return -EINVAL;
> @@ -519,9 +387,8 @@ SYSCALL_DEFINE4(rseq, struct rseq __user
> return -EINVAL;
> if (current->rseq.sig != sig)
> return -EPERM;
> - ret = rseq_reset_rseq_cpu_node_id(current);
> - if (ret)
> - return ret;
> + if (!rseq_reset_ids())
> + return -EFAULT;

AFAIU, this is only done to have the debug checks, stats, and
tracing side-effects, because the content of cpu_cid will be set
to ~0ULL by the following rseq_reset, am I correct ?

It's kind of weird to do some reset work to have it immediately
overwritten.

> rseq_reset(current);
> return 0;
> }
> @@ -571,17 +438,6 @@ SYSCALL_DEFINE4(rseq, struct rseq __user
> if (put_user_masked_u64(0UL, &rseq->rseq_cs))
> return -EFAULT;
>
> -#ifdef CONFIG_DEBUG_RSEQ
> - /*
> - * Initialize the in-kernel rseq fields copy for validation of
> - * read-only fields.
> - */
> - if (get_user(rseq_kernel_fields(current)->cpu_id_start, &rseq->cpu_id_start) ||
> - get_user(rseq_kernel_fields(current)->cpu_id, &rseq->cpu_id) ||
> - get_user(rseq_kernel_fields(current)->node_id, &rseq->node_id) ||
> - get_user(rseq_kernel_fields(current)->mm_cid, &rseq->mm_cid))
> - return -EFAULT;
> -#endif

OK I think I get why you are using cpu_cid != ~0ULL rather than the rseq
user pointer in the debug code. After rseq registration, now that you
remove those get_user, the kernel vs userspace fields are out of sync,
so you need a way to figure this out on return to userspace from
sys_rseq register.

Thanks,

Mathieu

> /*
> * Activate the registration by setting the rseq area address, length
> * and signature in the task struct.
>

--
Mathieu Desnoyers
EfficiOS Inc.