rseq: Optimize exit to user space

The RSEQ critical section mechanism only clears the event mask when a critical section is registered, otherwise it is stale and collects bits.

That means once a critical section is installed the first invocation of that code when TIF_NOTIFY_RESUME is set will abort the critical section, even when the TIF bit was not raised by the rseq preempt/migrate/signal helpers.

This also has a performance implication because TIF_NOTIFY_RESUME is a multiplexing TIF bit, which is utilized by quite some infrastructure. That means every invocation of __rseq_notify_resume() goes unconditionally through the heavy lifting of user space access and consistency checks even if there is no reason to do so.

Keeping the stale event mask around when exiting to user space also prevents it from being utilized by the upcoming time slice extension mechanism.

Avoid this by reading and clearing the event mask before doing the user space critical section access with preemption disabled, which ensures that the read and clear operation is CPU local atomic versus scheduling.

This is correct as after re-enabling preemption any relevant event will set the bit again and raise TIF_NOTIFY_RESUME, which makes the user space exit code take another round of TIF bit clearing.

If the event mask was non-zero, invoke the slow path. On debug kernels the slow path is invoked unconditionally and the result of the event mask evaluation is handed in.

Add a exit path check after the TIF bit loop, which validates on debug kernels that the event mask is zero before exiting to user space.

While at it reword the convoluted comment why the pt_regs pointer can be NULL under certain circumstances.

Signed-off-by Thomas Gleixner <[email protected]>

Cc Mathieu Desnoyers <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc "Paul E. McKenney" <[email protected]>

Cc Boqun Feng <[email protected]>

---
  include/linux/irq-entry-common.h |    7 ++--
  include/linux/rseq.h             |   10 +++++
  kernel/rseq.c                    |   66 ++++++++++++++++++++++++++-------------
  3 files changed, 58 insertions(+), 25 deletions(-)

include/linux/irq-entry-common.h +5 -2

@@ -2,11 +2,12 @@
  #ifndef __LINUX_IRQENTRYCOMMON_H
  #define __LINUX_IRQENTRYCOMMON_H
 
+#include <linux/context_tracking.h>
+#include <linux/kmsan.h>
+#include <linux/rseq.h>
  #include <linux/static_call_types.h>
  #include <linux/syscalls.h>
-#include <linux/context_tracking.h>
  #include <linux/tick.h>
-#include <linux/kmsan.h>
  #include <linux/unwind_deferred.h>
 
  #include <asm/entry-common.h>

@@ -226,6 +227,8 @@ static __always_inline void exit_to_user
 
  	arch_exit_to_user_mode_prepare(regs, ti_work);
 
+	rseq_exit_to_user_mode();
+
  	/* Ensure that kernel state is sane for a return to userspace */
  	kmap_assert_nomap();
  	lockdep_assert_irqs_disabled();

include/linux/rseq.h +9 -1

@@ -66,6 +66,14 @@ static inline void rseq_migrate(struct t
  	rseq_set_notify_resume(t);
  }
 
+static __always_inline void rseq_exit_to_user_mode(void)
+{
+	if (IS_ENABLED(CONFIG_DEBUG_RSEQ)) {
+		if (WARN_ON_ONCE(current->rseq && current->rseq_event_mask))
+			current->rseq_event_mask = 0;
+	}
+}
+
  /*
   * If parent process has a registered restartable sequences area, the
   * child inherits. Unregister rseq for a clone with CLONE_VM set.

@@ -118,7 +126,7 @@ static inline void rseq_fork(struct task
  static inline void rseq_execve(struct task_struct *t)
  {
  }
-
+static inline void rseq_exit_to_user_mode(void) { }
  #endif
 
  #ifdef CONFIG_DEBUG_RSEQ

kernel/rseq.c +4 -14

@@ -324,9 +324,9 @@ static bool rseq_warn_flags(const char *
  	return true;
  }
 
-static int rseq_need_restart(struct task_struct *t, u32 cs_flags)
+static int rseq_check_flags(struct task_struct *t, u32 cs_flags)
  {
-	u32 flags, event_mask;
+	u32 flags;
  	int ret;
 
  	if (rseq_warn_flags("rseq_cs", cs_flags))

@@ -339,17 +339,7 @@ static int rseq_need_restart(struct task
 
  	if (rseq_warn_flags("rseq", flags))
  		return -EINVAL;
-
-	/*
-	 * Load and clear event mask atomically with respect to
-	 * scheduler preemption and membarrier IPIs.
-	 */
-	scoped_guard(RSEQ_EVENT_GUARD) {
-		event_mask = t->rseq_event_mask;
-		t->rseq_event_mask = 0;
-	}
-
-	return !!event_mask;
+	return 0;
  }
 
  static int clear_rseq_cs(struct rseq __user *rseq)

@@ -380,7 +370,7 @@ static bool in_rseq_cs(unsigned long ip,
  	return ip - rseq_cs->start_ip < rseq_cs->post_commit_offset;
  }
 
-static int rseq_ip_fixup(struct pt_regs *regs)
+static int rseq_ip_fixup(struct pt_regs *regs, bool abort)
  {
  	unsigned long ip = instruction_pointer(regs);
  	struct task_struct *t = current;

kernel/rseq.c +37 -6

+		return 0;
  	ret = clear_rseq_cs(t->rseq);
  	if (ret)
  		return ret;

@@ -430,14 +422,44 @@ void __rseq_handle_notify_resume(struct
  		return;
 
  	/*
-	 * regs is NULL if and only if the caller is in a syscall path.  Skip
-	 * fixup and leave rseq_cs as is so that rseq_sycall() will detect and
-	 * kill a misbehaving userspace on debug kernels.
+	 * If invoked from hypervisors or IO-URING, then @regs is a NULL
+	 * pointer, so fixup cannot be done. If the syscall which led to
+	 * this invocation was invoked inside a critical section, then it
+	 * will either end up in this code again or a possible violation of
+	 * a syscall inside a critical region can only be detected by the
+	 * debug code in rseq_syscall() in a debug enabled kernel.
  	 */
  	if (regs) {
-		ret = rseq_ip_fixup(regs);
-		if (unlikely(ret < 0))
-			goto error;
+		/*
+		 * Read and clear the event mask first. If the task was not
+		 * preempted or migrated or a signal is on the way, there
+		 * is no point in doing any of the heavy lifting here on
+		 * production kernels. In that case TIF_NOTIFY_RESUME was
+		 * raised by some other functionality.
+		 *
+		 * This is correct because the read/clear operation is
+		 * guarded against scheduler preemption, which makes it CPU
+		 * local atomic. If the task is preempted right after
+		 * re-enabling preemption then TIF_NOTIFY_RESUME is set
+		 * again and this function is invoked another time _before_
+		 * the task is able to return to user mode.
+		 *
+		 * On a debug kernel, invoke the fixup code unconditionally
+		 * with the result handed in to allow the detection of
+		 * inconsistencies.
+		 */
+		u32 event_mask;
+
+		scoped_guard(RSEQ_EVENT_GUARD) {
+			event_mask = t->rseq_event_mask;
+			t->rseq_event_mask = 0;
+		}
+
+		if (IS_ENABLED(CONFIG_DEBUG_RSEQ) || event_mask) {
+			ret = rseq_ip_fixup(regs, !!event_mask);
+			if (unlikely(ret < 0))
+				goto error;
+		}
  	}
  	if (unlikely(rseq_update_cpu_node_id(t)))
  		goto error;

Scrolling over tons of pointless

{
}

lines to find the actual code is annoying at best.

Signed-off-by Thomas Gleixner <[email protected]>

Cc Mathieu Desnoyers <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc "Paul E. McKenney" <[email protected]>

Cc Boqun Feng <[email protected]>

---
  include/linux/rseq.h |   47 ++++++++++++-----------------------------------
  1 file changed, 12 insertions(+), 35 deletions(-)

include/linux/rseq.h +12 -35

@@ -101,44 +101,21 @@ static inline void rseq_execve(struct ta
  	t->rseq_event_mask = 0;
  }
 
-#else
-
-static inline void rseq_set_notify_resume(struct task_struct *t)
-{
-}
-static inline void rseq_handle_notify_resume(struct ksignal *ksig,
-					     struct pt_regs *regs)
-{
-}
-static inline void rseq_signal_deliver(struct ksignal *ksig,
-				       struct pt_regs *regs)
-{
-}
-static inline void rseq_preempt(struct task_struct *t)
-{
-}
-static inline void rseq_migrate(struct task_struct *t)
-{
-}
-static inline void rseq_fork(struct task_struct *t, unsigned long clone_flags)
-{
-}
-static inline void rseq_execve(struct task_struct *t)
-{
-}
+#else /* CONFIG_RSEQ */
+static inline void rseq_set_notify_resume(struct task_struct *t) { }
+static inline void rseq_handle_notify_resume(struct ksignal *ksig, struct pt_regs *regs) { }
+static inline void rseq_signal_deliver(struct ksignal *ksig, struct pt_regs *regs) { }
+static inline void rseq_preempt(struct task_struct *t) { }
+static inline void rseq_migrate(struct task_struct *t) { }
+static inline void rseq_fork(struct task_struct *t, unsigned long clone_flags) { }
+static inline void rseq_execve(struct task_struct *t) { }
  static inline void rseq_exit_to_user_mode(void) { }
-#endif
+#endif  /* !CONFIG_RSEQ */
 
  #ifdef CONFIG_DEBUG_RSEQ
-
  void rseq_syscall(struct pt_regs *regs);
-
-#else
-
-static inline void rseq_syscall(struct pt_regs *regs)
-{
-}
-
-#endif
+#else /* CONFIG_DEBUG_RSEQ */
+static inline void rseq_syscall(struct pt_regs *regs) { }
+#endif /* !CONFIG_DEBUG_RSEQ */
 
  #endif /* _LINUX_RSEQ_H */

rseq_syscall() is a debug function, which is invoked before the syscall exit work is handled. Name it so it's clear what it does.

Signed-off-by Thomas Gleixner <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Mathieu Desnoyers <[email protected]>

Cc "Paul E. McKenney" <[email protected]>

Cc Boqun Feng <[email protected]>

---
  include/linux/entry-common.h |    2 +-
  include/linux/rseq.h         |    4 ++--
  kernel/rseq.c                |    5 +++--
  3 files changed, 6 insertions(+), 5 deletions(-)

include/linux/entry-common.h +1 -1

@@ -162,7 +162,7 @@ static __always_inline void syscall_exit
  			local_irq_enable();
  	}
 
-	rseq_syscall(regs);
+	rseq_debug_syscall_exit(regs);
 
  	/*
  	 * Do one-time syscall specific work. If these work items are

include/linux/rseq.h +2 -2

@@ -113,9 +113,9 @@ static inline void rseq_exit_to_user_mod
  #endif  /* !CONFIG_RSEQ */
 
  #ifdef CONFIG_DEBUG_RSEQ
-void rseq_syscall(struct pt_regs *regs);
+void rseq_debug_syscall_exit(struct pt_regs *regs);
  #else /* CONFIG_DEBUG_RSEQ */
-static inline void rseq_syscall(struct pt_regs *regs) { }
+static inline void rseq_debug_syscall_exit(struct pt_regs *regs) { }
  #endif /* !CONFIG_DEBUG_RSEQ */
 
  #endif /* _LINUX_RSEQ_H */

kernel/rseq.c +3 -2

@@ -427,7 +427,8 @@ void __rseq_handle_notify_resume(struct
  	 * this invocation was invoked inside a critical section, then it
  	 * will either end up in this code again or a possible violation of
  	 * a syscall inside a critical region can only be detected by the
-	 * debug code in rseq_syscall() in a debug enabled kernel.
+	 * debug code in rseq_debug_syscall_exit() in a debug enabled
+	 * kernel.
  	 */
  	if (regs) {
  		/*

@@ -476,7 +477,7 @@ void __rseq_handle_notify_resume(struct
   * Terminate the process if a syscall is issued within a restartable
   * sequence.
   */
-void rseq_syscall(struct pt_regs *regs)
+void rseq_debug_syscall_exit(struct pt_regs *regs)
  {
  	unsigned long ip = instruction_pointer(regs);
  	struct task_struct *t = current;

Cleanup the include ordering, kernel-doc and other trivialities before making further changes.

Signed-off-by Thomas Gleixner <[email protected]>

Cc Peter Zijlstra <[email protected]>

---
 include/linux/entry-common.h     |    8 ++++----
 include/linux/irq-entry-common.h |    2 ++
 2 files changed, 6 insertions(+), 4 deletions(-)

include/linux/entry-common.h +4 -4

@@ -3,11 +3,11 @@
 #define __LINUX_ENTRYCOMMON_H
 
 #include <linux/irq-entry-common.h>
+#include <linux/livepatch.h>
 #include <linux/ptrace.h>
+#include <linux/resume_user_mode.h>
 #include <linux/seccomp.h>
 #include <linux/sched.h>
-#include <linux/livepatch.h>
-#include <linux/resume_user_mode.h>
 
 #include <asm/entry-common.h>
 #include <asm/syscall.h>

@@ -37,6 +37,7 @@
                                 SYSCALL_WORK_SYSCALL_AUDIT |           \
                                 SYSCALL_WORK_SYSCALL_USER_DISPATCH |   \
                                 ARCH_SYSCALL_WORK_ENTER)
+
 #define SYSCALL_WORK_EXIT      (SYSCALL_WORK_SYSCALL_TRACEPOINT |      \
                                 SYSCALL_WORK_SYSCALL_TRACE |           \
                                 SYSCALL_WORK_SYSCALL_AUDIT |           \

@@ -61,8 +62,7 @@
  */
 void syscall_enter_from_user_mode_prepare(struct pt_regs *regs);
 
-long syscall_trace_enter(struct pt_regs *regs, long syscall,
-                        unsigned long work);
+long syscall_trace_enter(struct pt_regs *regs, long syscall, unsigned long work);
 
 /**
  * syscall_enter_from_user_mode_work - Check and handle work before invoking

include/linux/irq-entry-common.h +2 -0

@@ -68,6 +68,7 @@ static __always_inline bool arch_in_rcu_
 
 /**
  * enter_from_user_mode - Establish state when coming from user mode
+ * @regs:      Pointer to currents pt_regs
  *
  * Syscall/interrupt entry disables interrupts, but user mode is traced as
  * interrupts enabled. Also with NO_HZ_FULL RCU might be idle.

@@ -357,6 +358,7 @@ irqentry_state_t noinstr irqentry_enter(
  * Conditional reschedule with additional sanity checks.
  */
 void raw_irqentry_exit_cond_resched(void);
+
 #ifdef CONFIG_PREEMPT_DYNAMIC
 #if defined(CONFIG_HAVE_PREEMPT_DYNAMIC_CALL)
 #define irqentry_exit_cond_resched_dynamic_enabled     raw_irqentry_exit_cond_resched

Typo in kernel-doc:
In irq-entry-common.h the doc line reads Pointer to currents pt_regs — should be Pointer to current's pt_regs (or Pointer to current->pt_regs depending on local style).

Stray blank/trailing whitespace:
In entry-common.h there is an extra blank line inserted after the ARCH_SYSCALL_WORK_ENTER) macro continuation — remove the stray blank to avoid style/noise.

include/linux/entry-common.h line 39

Quoted 4 lines

include/linux/entry-common.h +0 -0

@@ -37,6 +37,7 @@
                                 SYSCALL_WORK_SYSCALL_AUDIT |           \
                                 SYSCALL_WORK_SYSCALL_USER_DISPATCH |   \
                                 ARCH_SYSCALL_WORK_ENTER)

Review line 40

Quoted 1 line

(quoted diff) +1 -0

+

include/linux/entry-common.h line 41

Quoted 1 line

include/linux/entry-common.h +0 -0

 #define SYSCALL_WORK_EXIT      (SYSCALL_WORK_SYSCALL_TRACEPOINT |      \

kernel/rseq.c +16 -0

@@ -408,6 +408,22 @@ static int rseq_ip_fixup(struct pt_regs
 	return 0;
 }
 
+static inline bool rseq_ignore_event(bool from_irq, bool ksig)
+{
+	/*
+	 * On architectures which do not select_GENERIC_ENTRY
+	 * @from_irq is not usable.
+	 */
+	if (IS_ENABLED(CONFIG_DEBUG_RSEQ) || !IS_ENABLED(CONFIG_GENERIC_ENTRY))
+		return false;
+
+	/*
+	 * Avoid the heavy lifting when this is a return from syscall,
+	 * i.e. not from interrupt and not from signal delivery.
+	 */
+	return !from_irq && !ksig;
+}
+
 /*
  * This resume handler must always be executed between any of:
  * - preemption,

Quoted 28 lines · Thomas Gleixner

kernel/rseq.c +16 -0

@@ -408,6 +408,22 @@ static int rseq_ip_fixup(struct pt_regs
 	return 0;
 }
 
+static inline bool rseq_ignore_event(bool from_irq, bool ksig)
+{
+	/*
+	 * On architectures which do not select_GENERIC_ENTRY
+	 * @from_irq is not usable.
+	 */
+	if (IS_ENABLED(CONFIG_DEBUG_RSEQ) || !IS_ENABLED(CONFIG_GENERIC_ENTRY))
+		return false;
+
+	/*
+	 * Avoid the heavy lifting when this is a return from syscall,
+	 * i.e. not from interrupt and not from signal delivery.
+	 */
+	return !from_irq && !ksig;
+}
+
 /*
  * This resume handler must always be executed between any of:
  * - preemption,

kernel/rseq.c void __rseq_handle_notify_resume(struct · line 492

Quoted 9 lines

kernel/rseq.c +3 -0

@@ -467,6 +484,9 @@ void __rseq_handle_notify_resume(struct
 			t->rseq_event_pending = false;
 		}
 
+		if (rseq_ignore_event(from_irq, !!ksig))
+			event = false;
+
 		if (IS_ENABLED(CONFIG_DEBUG_RSEQ) || event) {
 			ret = rseq_ip_fixup(regs, event);
 			if (unlikely(ret < 0))

You now have a double check for CONFIG_DEBUG_RSEQ.

Since the value of @event is immaterial when DEBUG_RSEQ, might as well remove it from rseq_ignore_event(), right?

With the more wide spread usage of rseq in glibc, rseq is not longer a niche use case for special applications.

While working on a sane implementation of a rseq based time slice extension mechanism, I noticed several shortcomings of the current rseq code:

1) task::rseq_event_mask is a pointless bitfield despite the fact that the ABI flags it was meant to support have been deprecated and functionally disabled three years ago.

2) task::rseq_event_mask is accumulating bits unless there is a critical section discovered in the user space rseq memory. This results in pointless invocations of the rseq user space exit handler even if there had nothing changed. As a matter of correctness these bits have to be clear when exiting to user space and therefore pristine when coming back into the kernel. Aside of correctness, this also avoids pointless evaluation of the user space memory, which is a performance benefit.

3) The evaluation of critical sections does not differentiate between syscall and interrupt/exception exits. The current implementation silently fixes up critical sections which invoked a syscall unless CONFIG_DEBUG_RSEQ is enabled.

That's just wrong. If user space does that on a production kernel it can keep the pieces. The kernel is not there to proliferate mindless user space programming and letting everyone pay the performance penalty.

This series addresses these issues and on top converts parts of the user space access over to the new masked access model, which lowers the overhead of Spectre-V1 mitigations significantly on architectures which support it (x86 as of today). This is especially noticable in the access to the rseq_cs field in struct rseq, which is the first quick check to figure out whether a critical section is installed or not.

It survives the kernels rseq selftests, but I did not any performance tests vs. rseq because I have no idea how to use the gazillion of undocumented command line parameters of the benchmark. I leave that to people who are so familiar with them, that they assume everyone else is too :)

The performance gain on regular workloads is clearly measurable and the consistent event flag state allows now to build the time slice extension mechanism on top. The first POC I implemented:

   https://lore.kernel.org/lkml/87o6smb3a0.ffs@tglx/

suffered badly from the stale eventmask bits and the cleaned up version brought a whopping 25+% performance gain.

Quoted 3 lines · Thomas Gleixner

suffered badly from the stale eventmask bits and the cleaned up version brought a whopping 25+% performance gain.

Thanks for doing this work, it's been on my list to take a look at rseq as it's quite the pig currently and enabled by default (with what I assume is from a newer libc).

Quoted 5 lines · Jens Axboe

Quoted 3 lines · Thomas Gleixner

suffered badly from the stale eventmask bits and the cleaned up version brought a whopping 25+% performance gain.

Quoted 3 lines

Thanks for doing this work, it's been on my list to take a look at rseq as it's quite the pig currently and enabled by default (with what I assume is from a newer libc).

Yes, recent glibc uses it.

Could you give it a test ride to see whether this makes a difference in your environment?

Quoted 2 lines · Thomas Gleixner

Could you give it a test ride to see whether this makes a difference in your environment?

Yep, I'll give a spin.

If I recall correct, I'd see rseq at 2-3% of overall CPU time last I

tested. I'll try and do some pre/post numbers for some of the stuff I usually run.

Quoted 5 lines · Jens Axboe

Quoted 2 lines · Thomas Gleixner

Could you give it a test ride to see whether this makes a difference in your environment?

Yep, I'll give a spin.

Appreciated.

It survives the self test suite after I wasted a day to figure out why the selftests reliably segfault on a machine which has debian trixie installed. The fix is in the branch.

Quoted 3 lines

It survives the self test suite after I wasted a day to figure out why the selftests reliably segfault on a machine which has debian trixie installed. The fix is in the branch.

That's glibc 2.41 FWIW. glibc 2.36 from Debian 12 does not have this problem.

The fix unfortunately only works with a dynamically linked libc, statically linked libc fails. The fix is basically a revert of

3bcbc20942db ("selftests/rseq: Play nice with binaries statically linked

                  against glibc 2.35+")

which introduced these weak libc symbols to make static libc linking work.

I have no idea why this creates havoc, but in GDB I saw that libc manages to overwrite the TLS of the pthread at some place, but I gave up decoding it further. If no pthread is created it just works. Removing this weak muck makes it work too.

It's trivial to reproduce. All it needs is to have in the source:

__weak ptrdiff_t __rseq_offset;

w/o even being referenced and creating a pthread. Reproducer below.

* Thomas Gleixner:

Quoted 5 lines

It's trivial to reproduce. All it needs is to have in the source:

__weak ptrdiff_t __rseq_offset;

w/o even being referenced and creating a pthread. Reproducer below.

Well, that's sort of expected. You can't define glibc symbols that are not intended for interposition and expect things to work. It's kind of like writing:

int _rtld_global;

That's going to fail rather spectaculary, too. We make an exception for symbols that are not reserved (you can build in ISO C mode and define open, close, etc., at least as long as you link to glibc only). But __rseq_offset is a reserved name, so that is not applicable here.

The real change here is GCC changing from -fcommon (which made a lot of these things work in the past) to -fno-common.

Quoted 13 lines

* Thomas Gleixner:

Quoted 5 lines

It's trivial to reproduce. All it needs is to have in the source:

__weak ptrdiff_t __rseq_offset;

w/o even being referenced and creating a pthread. Reproducer below.

Well, that's sort of expected. You can't define glibc symbols that are not intended for interposition and expect things to work. It's kind of like writing:

int _rtld_global;

Quoted 7 lines

That's going to fail rather spectaculary, too. We make an exception for symbols that are not reserved (you can build in ISO C mode and define open, close, etc., at least as long as you link to glibc only). But __rseq_offset is a reserved name, so that is not applicable here.

The real change here is GCC changing from -fcommon (which made a lot of these things work in the past) to -fno-common.

Thanks for the explanation!

So the only way to make this actually work is to revert that commit and the folks who want to link that statically need to come up with:

#ifdef _BUILD_STATICALLY
extern ....

#else
        ptr = dlsym(...);

#endif

or something daft like that. A proper function interface would avoid all that nonsense, but we can't have nice things or can we?

* Thomas Gleixner:

Quoted 30 lines · Florian Weimer

Quoted 13 lines

* Thomas Gleixner:

Quoted 5 lines

It's trivial to reproduce. All it needs is to have in the source:

__weak ptrdiff_t __rseq_offset;

w/o even being referenced and creating a pthread. Reproducer below.

Well, that's sort of expected. You can't define glibc symbols that are not intended for interposition and expect things to work. It's kind of like writing:

int _rtld_global;

Quoted 7 lines

That's going to fail rather spectaculary, too. We make an exception for symbols that are not reserved (you can build in ISO C mode and define open, close, etc., at least as long as you link to glibc only). But __rseq_offset is a reserved name, so that is not applicable here.

The real change here is GCC changing from -fcommon (which made a lot of these things work in the past) to -fno-common.

Thanks for the explanation!

So the only way to make this actually work is to revert that commit and the folks who want to link that statically need to come up with:

#ifdef _BUILD_STATICALLY
extern ....

Quoted 6 lines

#else
        ptr = dlsym(...);

#endif

or something daft like that. A proper function interface would avoid all that nonsense, but we can't have nice things or can we?

I don't understand why a function would be different. Well, a function *declaration* would be implicitly extern, in a way a variable declaration is not (without -fcommon). Maybe it's just about the missing extern keyword?

You could add the extern keyword and check &__rseq_offset for NULL if you want to probe for the availability of the signal?

Or use:

#if __has_include(<sys/rseq.h>)
#include <sys/rseq.h>
/* Code that depends on glibc's rseq support goes here. */

Quoted 2 lines

You could add the extern keyword and check &__rseq_offset for NULL if you want to probe for the availability of the signal?

That will fail to link if the glibc version doesn't support rseq in any capacity, which is why I added the __weak crud.

Quoted 5 lines

Or use:

#if __has_include(<sys/rseq.h>)
#include <sys/rseq.h>
/* Code that depends on glibc's rseq support goes here. */

FWIW, the code in question doesn't depend on rseq per se, rather the problem is that attempting to register a restartable sequence fails if glibc has already "claimed" rseq.

What about something horrific like this? Or if __has_include(<sys/rseq.h>) is preferrable to checking the glibc version, go with that. The idea with checking LIBC_RSEQ_STATIC_LINK is to give folks a way to force static linking if their libc registers rseq, but doesn't satisfy the glibc version checks for whatever reason.

tools/testing/selftests/rseq/rseq.c +0 -5

index 663a9cef1952..1a88352fcff3 100644
@@ -36,17 +36,18 @@
 #include "../kselftest.h"
 #include "rseq.h"
 
-/*
- * Define weak versions to play nice with binaries that are statically linked
- * against a libc that doesn't support registering its own rseq.
- */
-__weak ptrdiff_t __rseq_offset;

* Sean Christopherson:

Quoted 4 lines · Florian Weimer

Quoted 2 lines

You could add the extern keyword and check &__rseq_offset for NULL if you want to probe for the availability of the signal?

Quoted 2 lines

That will fail to link if the glibc version doesn't support rseq in any capacity, which is why I added the __weak crud.

You need both (extern and weak) to get a weak symbol reference instead of a weak symbol definition. You still need to check &__rseq_offset, of course.

Quoted 6 lines

Quoted 5 lines

Or use:

#if __has_include(<sys/rseq.h>)
#include <sys/rseq.h>
/* Code that depends on glibc's rseq support goes here. */

Quoted 3 lines

FWIW, the code in question doesn't depend on rseq per se, rather the problem is that attempting to register a restartable sequence fails if glibc has already "claimed" rseq.

You can set GLIBC_TUNABLES=glibc.pthread.rseq=0 as an environment variable to prevent glibc from registering it. For a test that's probably okay? It won't help with other libcs that might use rseq eventually.

Quoted 3 lines · Florian Weimer

You need both (extern and weak) to get a weak symbol reference instead of a weak symbol definition. You still need to check &__rseq_offset, of course.

Ooh, you're saying add "extern" to the existing __weak symbol, not replace it. Huh, TIL weak symbol references are a thing.

This works with static and dynamic linking, with and without an rseq-aware glibc.

Thomas, does this fix the problem you were seeing?

tools/testing/selftests/rseq/rseq.c +3 -3

index 663a9cef1952..d17ded120d48 100644
@@ -40,9 +40,9 @@
  * Define weak versions to play nice with binaries that are statically linked
  * against a libc that doesn't support registering its own rseq.
  */
-__weak ptrdiff_t __rseq_offset;
-__weak unsigned int __rseq_size;
-__weak unsigned int __rseq_flags;
+extern __weak ptrdiff_t __rseq_offset;
+extern __weak unsigned int __rseq_size;
+extern __weak unsigned int __rseq_flags;
 
 static const ptrdiff_t *libc_rseq_offset_p = &__rseq_offset;
 static const unsigned int *libc_rseq_size_p = &__rseq_size;

Quoted 27 lines · Sean Christopherson

Quoted 3 lines · Florian Weimer

You need both (extern and weak) to get a weak symbol reference instead of a weak symbol definition. You still need to check &__rseq_offset, of course.

Ooh, you're saying add "extern" to the existing __weak symbol, not replace it. Huh, TIL weak symbol references are a thing.

This works with static and dynamic linking, with and without an rseq-aware glibc.

Thomas, does this fix the problem you were seeing?

tools/testing/selftests/rseq/rseq.c +3 -3

index 663a9cef1952..d17ded120d48 100644
@@ -40,9 +40,9 @@
  * Define weak versions to play nice with binaries that are statically linked
  * against a libc that doesn't support registering its own rseq.
  */
-__weak ptrdiff_t __rseq_offset;
-__weak unsigned int __rseq_size;
-__weak unsigned int __rseq_flags;
+extern __weak ptrdiff_t __rseq_offset;
+extern __weak unsigned int __rseq_size;
+extern __weak unsigned int __rseq_flags;
 

tools/testing/selftests/rseq/rseq.c void rseq_init(void) · line 212

Quoted 8 lines

tools/testing/selftests/rseq/rseq.c +1 -1

 static const ptrdiff_t *libc_rseq_offset_p = &__rseq_offset;
 static const unsigned int *libc_rseq_size_p = &__rseq_size;

@@ -209,7 +209,7 @@ void rseq_init(void)
         * libc not having registered a restartable sequence.  Try to find the
         * symbols if that's the case.
         */
-       if (!*libc_rseq_size_p) {
+       if (!libc_rseq_offset_p || !*libc_rseq_size_p) {

If I make that:

+ if (!*libc_rseq_offset_p || !*libc_rseq_size_p) {

then it makes sense and actually works. The pointer can hardly be NULL, even when statically linked, no?

Quoted 6 lines

If I make that:

+ if (!*libc_rseq_offset_p || !*libc_rseq_size_p) {

then it makes sense and actually works. The pointer can hardly be NULL, even when statically linked, no?

IIUC, it is indeed the pointers that are set to NULL/0, because for unresolved symbols, the symbol itself, not its value, is set to '0'. Which makes sense, because if there is no symbol, then it can't have a value.

Quoted 13 lines · Florian Weimer

* Thomas Gleixner:

Quoted 5 lines

It's trivial to reproduce. All it needs is to have in the source:

__weak ptrdiff_t __rseq_offset;

w/o even being referenced and creating a pthread. Reproducer below.

Well, that's sort of expected. You can't define glibc symbols that are not intended for interposition and expect things to work. It's kind of like writing:

int _rtld_global;

Quoted 7 lines

That's going to fail rather spectaculary, too. We make an exception for symbols that are not reserved (you can build in ISO C mode and define open, close, etc., at least as long as you link to glibc only). But __rseq_offset is a reserved name, so that is not applicable here.

The real change here is GCC changing from -fcommon (which made a lot of these things work in the past) to -fno-common.

Thanks for the explanation!

So the only way to make this actually work is to revert that commit and the folks who want to link that statically need to come up with:

#ifdef _BUILD_STATICALLY
extern ....

#else
       ptr = dlsym(...);

#endif

or something daft like that. A proper function interface would avoid all that nonsense, but we can't have nice things or can we?

Thanks,

tglx

Quoted 34 lines · Thomas Gleixner

Quoted 13 lines · Florian Weimer

* Thomas Gleixner:

Quoted 5 lines

It's trivial to reproduce. All it needs is to have in the source:

__weak ptrdiff_t __rseq_offset;

w/o even being referenced and creating a pthread. Reproducer below.

Well, that's sort of expected. You can't define glibc symbols that are not intended for interposition and expect things to work. It's kind of like writing:

int _rtld_global;

Quoted 7 lines

That's going to fail rather spectaculary, too. We make an exception for symbols that are not reserved (you can build in ISO C mode and define open, close, etc., at least as long as you link to glibc only). But __rseq_offset is a reserved name, so that is not applicable here.

The real change here is GCC changing from -fcommon (which made a lot of these things work in the past) to -fno-common.

Thanks for the explanation!

So the only way to make this actually work is to revert that commit and the folks who want to link that statically need to come up with:

#ifdef _BUILD_STATICALLY
extern ....

Quoted 6 lines

#else
        ptr = dlsym(...);

#endif

or something daft like that. A proper function interface would avoid all that nonsense, but we can't have nice things or can we?

Could the rseq(2) syscall itself return the already registered rseq structure address? Perhaps a new flag argument to the rseq(2) syscall to query the registered rseq address or return the address of the already registered rseq structure when it fails to register a new one.

Application can call it when the call to register a rseq structure fails.

-Prakash

Quoted 5 lines

Thanks,

tglx

Quoted 38 lines · Prakash Sangappa

Quoted 38 lines · Thomas Gleixner

Quoted 13 lines · Florian Weimer

* Thomas Gleixner:

Quoted 5 lines

It's trivial to reproduce. All it needs is to have in the source:

__weak ptrdiff_t __rseq_offset;

w/o even being referenced and creating a pthread. Reproducer below.

Well, that's sort of expected. You can't define glibc symbols that are not intended for interposition and expect things to work. It's kind of like writing:

int _rtld_global;

Quoted 7 lines

That's going to fail rather spectaculary, too. We make an exception for symbols that are not reserved (you can build in ISO C mode and define open, close, etc., at least as long as you link to glibc only). But __rseq_offset is a reserved name, so that is not applicable here.

The real change here is GCC changing from -fcommon (which made a lot of these things work in the past) to -fno-common.

Thanks for the explanation!

So the only way to make this actually work is to revert that commit and the folks who want to link that statically need to come up with:

#ifdef _BUILD_STATICALLY
extern ....

#else
       ptr = dlsym(...);

#endif

or something daft like that. A proper function interface would avoid all that nonsense, but we can't have nice things or can we?

Quoted 4 lines

Could the rseq(2) syscall itself return the already registered rseq structure address? Perhaps a new flag argument to the rseq(2) syscall to query the registered rseq address or return the address of the already registered rseq structure when it fails to register a new one. Application can call it when the call to register a rseq structure fails.

There is a ptrace(2) PTRACE_GET_RSEQ_CONFIGURATION to achieve something similar. I don't know if a dependency on ptrace would be acceptable for that use-case though.

Thanks,

Mathieu

Quoted 6 lines

-Prakash

Quoted 5 lines

Thanks,

tglx

Signature · 3 lines

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

Michael, can you please run your librseq tests against that too? They have the same segfault problem as the kernel and they lack a run script, so I couldn't be bothered to test against them. See commit 2bff3a0e5998 in that branch. I'll send out a patch with a proper change log later.

Quoted 4 lines · Thomas Gleixner

Michael, can you please run your librseq tests against that too? They have the same segfault problem as the kernel and they lack a run script, so I couldn't be bothered to test against them. See commit 2bff3a0e5998 in that branch. I'll send out a patch with a proper change log later.

I ran the librseq test suite on the new branch on a Debian Trixie amd64 system and it succeeds, here are the rseq stats before and after.

Before:

exit:             746809
signal:                3
slowp:                99
ids:                1053
cs:                    0
clear:                 0
fixup:                 0

After:

exit:          229294046
signal:               11
slowp:              4570
ids:              615950
cs:              2493682
clear:            194637
fixup:           2299044

If you want to run the librseq tests on your system, just do the regular autotools dance and then run 'make check'.

That looks about right. Can you reset the branch to

      commit 85b61b265635 ("rseq: Expose stats")

which is just adding primitive stats on top of the current mainline code, and provide numbers for that too?

That gives you 'notify: , cpuid:, fixup:' numbers, which are not 1:1 mappable to the final ones, but that should give some interesting insight.

Might be useful to put such instructions into README.md, no?

Quoted 10 lines · Thomas Gleixner

That looks about right. Can you reset the branch to

      commit 85b61b265635 ("rseq: Expose stats")

which is just adding primitive stats on top of the current mainline code, and provide numbers for that too?

That gives you 'notify: , cpuid:, fixup:' numbers, which are not 1:1 mappable to the final ones, but that should give some interesting insight.

For amd64 kernel and userspace.

Before:

notify:             12467
fixup:              12467
cpuid:              12467

After:

notify:         123669528
fixup:          123669528
cpuid:          123669528

exit:          229294046
signal:               11
slowp:              4570
ids:              615950
cs:              2493682
clear:            194637
fixup:           2299044

Quoted 8 lines · Thomas Gleixner

Quoted 5 lines · Jens Axboe

Quoted 2 lines · Thomas Gleixner

Could you give it a test ride to see whether this makes a difference in your environment?

Yep, I'll give a spin.

Appreciated.

Please do not use the git branch I had in the cover letter. I did some more analysis of this and it's even worse than I thought. Use

git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git rseq/wip

instead.

I've rewritten the whole pile by now and made it a real fast path without the TIF_NOTIFY horror show, unless the fast path, which runs _after_ the TIF work loop faults. So far that happens once for each fork() as that has to fault in the copy of the user space rseq region.

With the more wide spread usage of rseq in glibc, rseq is not longer a niche use case for special applications.

While working on a sane implementation of a rseq based time slice extension mechanism, I noticed several shortcomings of the current rseq code:

1) task::rseq_event_mask is a pointless bitfield despite the fact that the ABI flags it was meant to support have been deprecated and functionally disabled three years ago.

2) task::rseq_event_mask is accumulating bits unless there is a critical section discovered in the user space rseq memory. This results in pointless invocations of the rseq user space exit handler even if there had nothing changed. As a matter of correctness these bits have to be clear when exiting to user space and therefore pristine when coming back into the kernel. Aside of correctness, this also avoids pointless evaluation of the user space memory, which is a performance benefit.

3) The evaluation of critical sections does not differentiate between syscall and interrupt/exception exits. The current implementation silently fixes up critical sections which invoked a syscall unless CONFIG_DEBUG_RSEQ is enabled.

That's just wrong. If user space does that on a production kernel it can keep the pieces. The kernel is not there to proliferate mindless user space programming and letting everyone pay the performance penalty.

This series addresses these issues and on top converts parts of the user space access over to the new masked access model, which lowers the overhead of Spectre-V1 mitigations significantly on architectures which support it (x86 as of today). This is especially noticable in the access to the rseq_cs field in struct rseq, which is the first quick check to figure out whether a critical section is installed or not.

It survives the kernels rseq selftests, but I did not any performance tests vs. rseq because I have no idea how to use the gazillion of undocumented command line parameters of the benchmark. I leave that to people who are so familiar with them, that they assume everyone else is too :)

The performance gain on regular workloads is clearly measurable and the consistent event flag state allows now to build the time slice extension mechanism on top. The first POC I implemented:

    https://lore.kernel.org/lkml/87o6smb3a0.ffs@tglx/

suffered badly from the stale eventmask bits and the cleaned up version brought a whopping 25+% performance gain.

The series depends on the seperately posted rseq bugfix:

    https://lore.kernel.org/lkml/87o6sj6z95.ffs@tglx/

and the uaccess generic helper series:

    https://lore.kernel.org/lkml/[email protected]/

and a related futex fix in

git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git locking/urgent

For your conveniance it is also available as a conglomorate from git:

git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git rseq/perf

Thanks,

tglx

---
  drivers/hv/mshv_common.c         |    2
  include/linux/entry-common.h     |   12 +--
  include/linux/irq-entry-common.h |   31 ++++++--
  include/linux/resume_user_mode.h |   40 +++++++---
  include/linux/rseq.h             |  115 +++++++++----------------------
  include/linux/sched.h            |   10 +-
  include/uapi/linux/rseq.h        |   21 +----
  io_uring/io_uring.h              |    2
  kernel/entry/common.c            |    9 +-
  kernel/entry/kvm.c               |    2
  kernel/rseq.c                    |  142 +++++++++++++++++++++++++--------------
  kernel/sched/core.c              |    8 +-
  kernel/sched/membarrier.c        |    8 +-
  13 files changed, 213 insertions(+), 189 deletions(-)