Re: [patch V4 27/36] rseq: Implement fast path for exit to user

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

On 2025-09-08 17:32, Thomas Gleixner wrote:
> Implement the actual logic for handling RSEQ updates in a fast path after
> handling the TIF work and at the point where the task is actually returning
> to user space.
> 
> This is the right point to do that because at this point the CPU and the MM
> CID are stable and cannot longer change due to yet another reschedule.
> That happens when the task is handling it via TIF_NOTIFY_RESUME in
> resume_user_mode_work(), which is invoked from the exit to user mode work
> loop.
> 
> The function is invoked after the TIF work is handled and runs with
> interrupts disabled, which means it cannot resolve page faults. It
> therefore disables page faults and in case the access to the user space
> memory faults, it:
> 
>    - notes the fail in the event struct
>    - raises TIF_NOTIFY_RESUME
>    - returns false to the caller
> 
> The caller has to go back to the TIF work, which runs with interrupts
> enabled and therefore can resolve the page faults. This happens mostly on
> fork() when the memory is marked COW. That will be optimized by setting the
> failure flag and raising TIF_NOTIFY_RESUME right on fork to avoid the
> otherwise unavoidable round trip.
> 
> If the user memory inspection finds invalid data, the function returns
> false as well and sets the fatal flag in the event struct along with
> TIF_NOTIFY_RESUME. The slow path notify handler has to evaluate that flag
> and terminate the task with SIGSEGV as documented.
> 
> The initial decision to invoke any of this is based on two flags in the
> event struct: @has_rseq and @sched_switch. The decision is in pseudo ASM:
> 
>        load	tsk::event::has_rseq
>        and	tsk::event::sched_switch

I suspect that my earlier comment about has_rseq and AND applies here
as well. We could probably remove the has_rseq load and the AND
altogether and just load sched_switch for the conditional branch ?


>        jnz	inspect_user_space
>        mov	$0, tsk::event::events
>        ...
>        leave
> 
> So for the common case where the task was not scheduled out, this really
> boils down to four instructions before going out if the compiler is not
> completely stupid (and yes, some of them are).

Or three.

> 
> If the condition is true, then it checks, whether CPU ID or MM CID have
> changed. If so, then the CPU/MM IDs have to be updated and are thereby
> cached for the next round. The update unconditionally retrieves the user
> space critical section address to spare another user*begin/end() pair.  If
> that's not zero and tsk::event::user_irq is set, then the critical section
> is analyzed and acted upon. If either zero or the entry came via syscall
> the critical section analysis is skipped.
> 
> If the comparison is false then the critical section has to be analyzed
> because the event flag is then only true when entry from user was by
> interrupt.
> 
> This is provided without the actual hookup to let reviewers focus on the
> implementation details. The hookup happens in the next step.
> 
> Note: As with quite some other optimizations this depends on the generic
> entry infrastructure and is not enabled to be sucked into random
> architecture implementations.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> 
> ---
>   include/linux/rseq_entry.h |  133 +++++++++++++++++++++++++++++++++++++++++++--
>   include/linux/rseq_types.h |    3 +
>   kernel/rseq.c              |    2
>   3 files changed, 133 insertions(+), 5 deletions(-)
> 
> --- a/include/linux/rseq_entry.h
> +++ b/include/linux/rseq_entry.h
> @@ -10,6 +10,7 @@ struct rseq_stats {
>   	unsigned long	exit;
>   	unsigned long	signal;
>   	unsigned long	slowpath;
> +	unsigned long	fastpath;
>   	unsigned long	ids;
>   	unsigned long	cs;
>   	unsigned long	clear;
> @@ -202,8 +203,8 @@ bool rseq_debug_update_user_cs(struct ta
>   
>   /*
>    * 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.
> + * debugging is enabled, but don't do that on the first exit to user
> + * space. In that case cpu_cid is ~0. See fork/execve.

see earlier comment about using either ~0 or ~0ULL for consistency.

>    */
>   bool rseq_debug_validate_ids(struct task_struct *t)
>   {
> @@ -254,12 +255,13 @@ rseq_update_user_cs(struct task_struct *
>   {
>   	struct rseq_cs __user *ucs = (struct rseq_cs __user *)(unsigned long)csaddr;
>   	unsigned long ip = instruction_pointer(regs);
> +	unsigned long tasksize = TASK_SIZE;
>   	u64 start_ip, abort_ip, offset;
>   	u32 usig, __user *uc_sig;
>   
>   	rseq_stat_inc(rseq_stats.cs);
>   
> -	if (unlikely(csaddr >= TASK_SIZE)) {
> +	if (unlikely(csaddr >= tasksize)) {
>   		t->rseq.event.fatal = true;
>   		return false;
>   	}
> @@ -298,7 +300,7 @@ rseq_update_user_cs(struct task_struct *
>   	 * in TLS::rseq::rseq_cs. An RSEQ abort would then evade ROP
>   	 * protection.
>   	 */
> -	if (abort_ip >= TASK_SIZE || abort_ip < sizeof(*uc_sig))
> +	if (unlikely(abort_ip >= tasksize || abort_ip < sizeof(*uc_sig)))
>   		goto die;
>   
>   	/* The address is guaranteed to be >= 0 and < TASK_SIZE */
> @@ -412,6 +414,124 @@ static rseq_inline bool rseq_update_usr(
>   	return rseq_update_user_cs(t, regs, csaddr);
>   }
>   
> +/*
> + * If you want to use this then convert your architecture to the generic
> + * entry code. I'm tired of building workarounds for people who can't be
> + * bothered to make the maintainence of generic infrastructure less

maintenance

> + * burdensome. Just sucking everything into the architecture code and
> + * thereby making others chase the horrible hacks and keep them working is
> + * neither acceptable nor sustainable.
> + */
> +#ifdef CONFIG_GENERIC_ENTRY
> +
> +/*
> + * This is inlined into the exit path because:
> + *
> + * 1) It's a one time comparison in the fast path when there is no event to
> + *    handle
> + *
> + * 2) The access to the user space rseq memory (TLS) is unlikely to fault
> + *    so the straight inline operation is:
> + *
> + *	- Four 32-bit stores only if CPU ID/ MM CID need to be updated
> + *	- One 64-bit load to retrieve the critical section address
> + *
> + * 3) In the unlikely case that the critical section address is != NULL:
> + *
> + *     - One 64-bit load to retrieve the start IP
> + *     - One 64-bit load to retrieve the offset for calculating the end
> + *     - One 64-bit load to retrieve the abort IP
> + *     - One store to clear the critical section address
> + *
> + * The non-debug case implements only the minimal required checking and
> + * protection against a rogue abort IP in kernel space, which would be
> + * exploitable at least on x86. Any fallout from invalid critical section
> + * descriptors is a user space problem. The debug case provides the full
> + * set of checks and terminates the task if a condition is not met.
> + *
> + * In case of a fault or an invalid value, this sets TIF_NOTIFY_RESUME and
> + * tells the caller to loop back into exit_to_user_mode_loop(). The rseq
> + * slow path there will handle the fail.
> + */
> +static __always_inline bool __rseq_exit_to_user_mode_restart(struct pt_regs *regs)
> +{
> +	struct task_struct *t = current;
> +
> +	/*
> +	 * If the task did not go through schedule or got the flag enforced
> +	 * by the rseq syscall or execve, then nothing to do here.
> +	 *
> +	 * CPU ID and MM CID can only change when going through a context
> +	 * switch.
> +	 *
> +	 * This can only be done when rseq_event::has_rseq is true.
> +	 * rseq_sched_switch_event() sets rseq_event::sched unconditionally
> +	 * true to avoid a load of rseq_event::has_rseq in the context
> +	 * switch path.

Based on the patch just before this one, I don't see that the
implementation of rseq_sched_switch_event() matches this comment.
It does check ev->has_rseq.

So I guess we need to pick one here:

Either we skip a has_rseq check in the scheduler and always set the
sched_switch event, in which case we need the sched_switch & has_rseq
check on return to userspace,

or we check has_rseq in the scheduler, and we don't need to load it
on return to userspace.

I suspect that this series went from one approach to the other without
entirely updating the relevant return to userspace code.

> +	 *
> +	 * This check uses a '&' and not a '&&' to force the compiler to do
> +	 * an actual AND operation instead of two seperate conditionals.

separate

> +	 *
> +	 * A sane compiler requires four instructions for the nothing to do
> +	 * case including clearing the events, but your milage might vary.

mileage

> +	 */
> +	if (likely(!(t->rseq.event.sched_switch & t->rseq.event.has_rseq)))

Same comment as earlier about checking has_rseq which may be redundant.

Thanks,

Mathieu

> +		goto done;
> +
> +	rseq_stat_inc(rseq_stats.fastpath);
> +
> +	pagefault_disable();
> +
> +	if (likely(!t->rseq.event.ids_changed)) {
> +		/*
> +		 * If IDs have not changed rseq_event::user_irq must be true
> +		 * See rseq_sched_switch_event().
> +		 */
> +		u64 csaddr;
> +
> +		if (unlikely(get_user_masked_u64(&csaddr, &t->rseq.usrptr->rseq_cs)))
> +			goto fail;
> +
> +		if (static_branch_unlikely(&rseq_debug_enabled) || unlikely(csaddr)) {
> +			if (unlikely(!rseq_update_user_cs(t, regs, csaddr)))
> +				goto fail;
> +		}
> +	} else {
> +		struct rseq_ids ids = {
> +			.cpu_id = task_cpu(t),
> +			.mm_cid = task_mm_cid(t),
> +		};
> +		u32 node_id = cpu_to_node(ids.cpu_id);
> +
> +		if (unlikely(!rseq_update_usr(t, regs, &ids, node_id)))
> +			goto fail;
> +	}
> +
> +	pagefault_enable();
> +
> +done:
> +	/* Clear state so next entry starts from a clean slate */
> +	t->rseq.event.events = 0;
> +	return false;
> +
> +fail:
> +	pagefault_enable();
> +	/* Force it into the slow path. Don't clear the state! */
> +	t->rseq.event.slowpath = true;
> +	set_tsk_thread_flag(t, TIF_NOTIFY_RESUME);
> +	return true;
> +}
> +
> +static __always_inline bool rseq_exit_to_user_mode_restart(struct pt_regs *regs)
> +{
> +	if (unlikely(__rseq_exit_to_user_mode_restart(regs)))
> +		return true;
> +
> +	return false;
> +}
> +
> +#endif /* CONFIG_GENERIC_ENTRY */
> +
>   static __always_inline void rseq_exit_to_user_mode(void)
>   {
>   	struct rseq_event *ev = &current->rseq.event;
> @@ -436,9 +556,12 @@ static inline void rseq_debug_syscall_re
>   	if (static_branch_unlikely(&rseq_debug_enabled))
>   		__rseq_debug_syscall_return(regs);
>   }
> -
>   #else /* CONFIG_RSEQ */
>   static inline void rseq_note_user_irq_entry(void) { }
> +static inline bool rseq_exit_to_user_mode_restart(struct pt_regs *regs)
> +{
> +	return false;
> +}
>   static inline void rseq_exit_to_user_mode(void) { }
>   static inline void rseq_debug_syscall_return(struct pt_regs *regs) { }
>   #endif /* !CONFIG_RSEQ */
> --- a/include/linux/rseq_types.h
> +++ b/include/linux/rseq_types.h
> @@ -18,6 +18,8 @@ struct rseq;
>    * @has_rseq:		True if the task has a rseq pointer installed
>    * @error:		Compound error code for the slow path to analyze
>    * @fatal:		User space data corrupted or invalid
> + * @slowpath:		Indicator that slow path processing via TIF_NOTIFY_RESUME
> + *			is required
>    *
>    * @sched_switch and @ids_changed must be adjacent and the combo must be
>    * 16bit aligned to allow a single store, when both are set at the same
> @@ -42,6 +44,7 @@ struct rseq_event {
>   				u16		error;
>   				struct {
>   					u8	fatal;
> +					u8	slowpath;
>   				};
>   			};
>   		};
> --- a/kernel/rseq.c
> +++ b/kernel/rseq.c
> @@ -133,6 +133,7 @@ static int rseq_stats_show(struct seq_fi
>   		stats.exit	+= data_race(per_cpu(rseq_stats.exit, cpu));
>   		stats.signal	+= data_race(per_cpu(rseq_stats.signal, cpu));
>   		stats.slowpath	+= data_race(per_cpu(rseq_stats.slowpath, cpu));
> +		stats.fastpath	+= data_race(per_cpu(rseq_stats.fastpath, cpu));
>   		stats.ids	+= data_race(per_cpu(rseq_stats.ids, cpu));
>   		stats.cs	+= data_race(per_cpu(rseq_stats.cs, cpu));
>   		stats.clear	+= data_race(per_cpu(rseq_stats.clear, cpu));
> @@ -142,6 +143,7 @@ static int rseq_stats_show(struct seq_fi
>   	seq_printf(m, "exit:   %16lu\n", stats.exit);
>   	seq_printf(m, "signal: %16lu\n", stats.signal);
>   	seq_printf(m, "slowp:  %16lu\n", stats.slowpath);
> +	seq_printf(m, "fastp:  %16lu\n", stats.fastpath);
>   	seq_printf(m, "ids:    %16lu\n", stats.ids);
>   	seq_printf(m, "cs:     %16lu\n", stats.cs);
>   	seq_printf(m, "clear:  %16lu\n", stats.clear);
> 


-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com