[PATCH v12 07/14] unwind_user/deferred: Make unwind deferral requests NMI-safe

Steven Rostedt posted 14 patches 3 months, 1 week ago
There is a newer version of this series
[PATCH v12 07/14] unwind_user/deferred: Make unwind deferral requests NMI-safe
Posted by Steven Rostedt 3 months, 1 week ago
From: Steven Rostedt <rostedt@goodmis.org>

Make unwind_deferred_request() NMI-safe so tracers in NMI context can
call it and safely request a user space stacktrace when the task exits.

Note, this is only allowed for architectures that implement a safe 64 bit
cmpxchg. Which rules out some 32bit architectures and even some 64 bit
ones.  If an architecture requests a deferred stack trace from NMI context
that does not support a safe NMI 64 bit cmpxchg, it will get an -EINVAL.
For those architectures, they would need another method (perhaps an
irqwork), to request a deferred user space stack trace. That can be dealt
with later if one of theses architectures require this feature.

Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 include/linux/unwind_deferred.h       |  4 +-
 include/linux/unwind_deferred_types.h |  7 ++-
 kernel/unwind/deferred.c              | 74 ++++++++++++++++++++++-----
 3 files changed, 69 insertions(+), 16 deletions(-)

diff --git a/include/linux/unwind_deferred.h b/include/linux/unwind_deferred.h
index c6548e8d64d1..73f6cac53530 100644
--- a/include/linux/unwind_deferred.h
+++ b/include/linux/unwind_deferred.h
@@ -28,8 +28,8 @@ void unwind_deferred_cancel(struct unwind_work *work);
 
 static __always_inline void unwind_reset_info(void)
 {
-	if (unlikely(current->unwind_info.timestamp))
-		current->unwind_info.timestamp = 0;
+	if (unlikely(local64_read(&current->unwind_info.timestamp)))
+		local64_set(&current->unwind_info.timestamp, 0);
 	/*
 	 * As unwind_user_faultable() can be called directly and
 	 * depends on nr_entries being cleared on exit to user,
diff --git a/include/linux/unwind_deferred_types.h b/include/linux/unwind_deferred_types.h
index 5df264cf81ad..0d722e877473 100644
--- a/include/linux/unwind_deferred_types.h
+++ b/include/linux/unwind_deferred_types.h
@@ -2,6 +2,9 @@
 #ifndef _LINUX_UNWIND_USER_DEFERRED_TYPES_H
 #define _LINUX_UNWIND_USER_DEFERRED_TYPES_H
 
+#include <asm/local64.h>
+#include <asm/local.h>
+
 struct unwind_cache {
 	unsigned int		nr_entries;
 	unsigned long		entries[];
@@ -10,8 +13,8 @@ struct unwind_cache {
 struct unwind_task_info {
 	struct unwind_cache	*cache;
 	struct callback_head	work;
-	u64			timestamp;
-	int			pending;
+	local64_t		timestamp;
+	local_t			pending;
 };
 
 #endif /* _LINUX_UNWIND_USER_DEFERRED_TYPES_H */
diff --git a/kernel/unwind/deferred.c b/kernel/unwind/deferred.c
index d5f2c004a5b0..dd36e58c8cad 100644
--- a/kernel/unwind/deferred.c
+++ b/kernel/unwind/deferred.c
@@ -12,6 +12,35 @@
 #include <linux/slab.h>
 #include <linux/mm.h>
 
+/*
+ * For requesting a deferred user space stack trace from NMI context
+ * the architecture must support a 64bit safe cmpxchg in NMI context.
+ * For those architectures that do not have that, then it cannot ask
+ * for a deferred user space stack trace from an NMI context. If it
+ * does, then it will get -EINVAL.
+ */
+#if defined(CONFIG_ARCH_HAVE_NMI_SAFE_CMPXCHG) && \
+	!defined(CONFIG_GENERIC_ATOMIC64)
+# define CAN_USE_IN_NMI		1
+static inline u64 assign_timestamp(struct unwind_task_info *info,
+				   u64 timestamp)
+{
+	u64 old = 0;
+	if (!local64_try_cmpxchg(&info->timestamp, &old, timestamp))
+		timestamp = old;
+	return timestamp;
+}
+#else
+# define CAN_USE_IN_NMI		0
+static inline u64 assign_timestamp(struct unwind_task_info *info,
+				   u64 timestamp)
+{
+	/* For archs that do not allow NMI here */
+	local64_set(&info->timestamp, timestamp);
+	return timestamp;
+}
+#endif
+
 /* Make the cache fit in a 4K page */
 #define UNWIND_MAX_ENTRIES					\
 	((SZ_4K - sizeof(struct unwind_cache)) / sizeof(long))
@@ -31,12 +60,21 @@ static LIST_HEAD(callbacks);
  */
 static u64 get_timestamp(struct unwind_task_info *info)
 {
+	u64 timestamp;
+
 	lockdep_assert_irqs_disabled();
 
-	if (!info->timestamp)
-		info->timestamp = local_clock();
+	/*
+	 * Note, the timestamp is generated on the first request.
+	 * If it exists here, then the timestamp is earlier than
+	 * this request and it means that this request will be
+	 * valid for the stracktrace.
+	 */
+	timestamp = local64_read(&info->timestamp);
+	if (timestamp)
+		return timestamp;
 
-	return info->timestamp;
+	return assign_timestamp(info, local_clock());
 }
 
 /**
@@ -96,11 +134,11 @@ static void unwind_deferred_task_work(struct callback_head *head)
 	struct unwind_work *work;
 	u64 timestamp;
 
-	if (WARN_ON_ONCE(!info->pending))
+	if (WARN_ON_ONCE(!local_read(&info->pending)))
 		return;
 
 	/* Allow work to come in again */
-	WRITE_ONCE(info->pending, 0);
+	local_set(&info->pending, 0);
 
 	/*
 	 * From here on out, the callback must always be called, even if it's
@@ -111,7 +149,7 @@ static void unwind_deferred_task_work(struct callback_head *head)
 
 	unwind_user_faultable(&trace);
 
-	timestamp = info->timestamp;
+	timestamp = local64_read(&info->timestamp);
 
 	guard(mutex)(&callback_mutex);
 	list_for_each_entry(work, &callbacks, list) {
@@ -150,31 +188,43 @@ static void unwind_deferred_task_work(struct callback_head *head)
 int unwind_deferred_request(struct unwind_work *work, u64 *timestamp)
 {
 	struct unwind_task_info *info = &current->unwind_info;
+	long pending;
 	int ret;
 
 	*timestamp = 0;
 
-	if (WARN_ON_ONCE(in_nmi()))
-		return -EINVAL;
-
 	if ((current->flags & (PF_KTHREAD | PF_EXITING)) ||
 	    !user_mode(task_pt_regs(current)))
 		return -EINVAL;
 
+	/* NMI requires having safe 64 bit cmpxchg operations */
+	if (!CAN_USE_IN_NMI && in_nmi())
+		return -EINVAL;
+
 	guard(irqsave)();
 
 	*timestamp = get_timestamp(info);
 
 	/* callback already pending? */
-	if (info->pending)
+	pending = local_read(&info->pending);
+	if (pending)
 		return 1;
 
+	if (CAN_USE_IN_NMI) {
+		/* Claim the work unless an NMI just now swooped in to do so. */
+		if (!local_try_cmpxchg(&info->pending, &pending, 1))
+			return 1;
+	} else {
+		local_set(&info->pending, 1);
+	}
+
 	/* The work has been claimed, now schedule it. */
 	ret = task_work_add(current, &info->work, TWA_RESUME);
-	if (WARN_ON_ONCE(ret))
+	if (WARN_ON_ONCE(ret)) {
+		local_set(&info->pending, 0);
 		return ret;
+	}
 
-	info->pending = 1;
 	return 0;
 }
 
-- 
2.47.2
Re: [PATCH v12 07/14] unwind_user/deferred: Make unwind deferral requests NMI-safe
Posted by Jens Remus 3 months, 1 week ago
Hello Steve!

On 01.07.2025 02:53, Steven Rostedt wrote:
> Make unwind_deferred_request() NMI-safe so tracers in NMI context can
> call it and safely request a user space stacktrace when the task exits.

> diff --git a/include/linux/unwind_deferred_types.h b/include/linux/unwind_deferred_types.h

> @@ -2,6 +2,9 @@
>  #ifndef _LINUX_UNWIND_USER_DEFERRED_TYPES_H
>  #define _LINUX_UNWIND_USER_DEFERRED_TYPES_H
>  
> +#include <asm/local64.h>
> +#include <asm/local.h>

This creates the following circular dependency, that breaks the build on
s390 as follows, whenever local64.h is included first, so that local64_t
is not yet defined when unwind_deferred_types.h gets included down the
line:


Circular dependency:

include/linux/unwind_deferred_types.h
arch/<arch>/include/generated/asm/local64.h
include/asm-generic/local64.h
include/linux/percpu.h
include/linux/sched.h
include/linux/unwind_deferred_types.h


Compile error on s390:

  CC      net/ipv4/proc.o
In file included from ./include/linux/sched.h:49,
                 from ./include/linux/percpu.h:12,
                 from ./include/asm-generic/local64.h:5,
                 from ./arch/s390/include/generated/asm/local64.h:1,
                 from ./include/linux/u64_stats_sync.h:71,
                 from ./include/net/snmp.h:47,
                 from ./include/net/netns/mib.h:5,
                 from ./include/net/net_namespace.h:17,
                 from net/ipv4/proc.c:31:
./include/linux/unwind_deferred_types.h:17:9: error: unknown type name ‘local64_t’; did you mean ‘local_t’?
   17 |         local64_t               timestamp;
      |         ^~~~~~~~~
      |         local_t


Reason it fails on s390:

net/ipv4/proc.c
+- include/net/net_namespace.h
   +- include/net/netns/mib.h
      +- include/net/snmp.h
         +- include/linux/u64_stats_sync.h
            +- arch/s390/include/generated/asm/local64.h   <-- ISSUE: local64.h gets included before unwind_deferred_types.h
               +- include/asm-generic/local64.h
                  +- include/linux/percpu.h
                     +- include/linux/sched.h
                        +- include/linux/unwind_deferred_types.h   <-- ERROR: local64_t not yet defined


Reason it does not fail on x86:

net/ipv4/proc.c
+- include/net/net_namespace.h
   +- include/linux/workqueue.h
   |  +- include/linux/timer.h
   |     +- include/linux/ktime.h
   |        +- include/linux/jiffies.h
   |           +- include/linux/time.h
   |               +- include/linux/time32.h
   |                  +- include/linux/timex.h
   |                     +- arch/x86/include/asm/timex.h
   |                        +- arch/x86/include/asm/tsc.h
   |                           +- arch/x86/include/asm/msr.h
   |                              +- include/linux/percpu.h
   |                                 +- include/linux/sched.h
   |                                    +- include/linux/unwind_deferred_types.h   <-- OK: unwind_deferred_types.h gets included before local64.h
   |                                       +- arch/x86/include/generated/asm/local64.h
   |                                          +- include/asm-generic/local64.h
   |                                           [ +- include/linux/percpu.h ]
   +- include/net/netns/mib.h
      +- include/net/snmp.h
         +- include/linux/u64_stats_sync.h
            +- arch/x86/include/generated/asm/local64.h   <-- OK: local64.h comes after unwind_deferred_types.h
             [ +- include/asm-generic/local64.h ]
                [ +- include/linux/percpu.h ]
                   [ +- include/linux/sched.h ]
                      [ +- include/linux/unwind_deferred_types.h ]


My colleague Heiko Carstens (on Cc) suggested the following potential fix
that breaks the circular dependency, provided local[64].h does not ever
need percpu.h.  At least I verified that defconfig x86 and s390 builds
still work.

diff --git a/include/asm-generic/local.h b/include/asm-generic/local.h
index 7f97018df66f..3e7ce6a9e18e 100644
--- a/include/asm-generic/local.h
+++ b/include/asm-generic/local.h
@@ -2,7 +2,6 @@
 #ifndef _ASM_GENERIC_LOCAL_H
 #define _ASM_GENERIC_LOCAL_H
 
-#include <linux/percpu.h>
 #include <linux/atomic.h>
 #include <asm/types.h>
 
diff --git a/include/asm-generic/local64.h b/include/asm-generic/local64.h
index 14963a7a6253..1f9af89916cb 100644
--- a/include/asm-generic/local64.h
+++ b/include/asm-generic/local64.h
@@ -2,7 +2,6 @@
 #ifndef _ASM_GENERIC_LOCAL64_H
 #define _ASM_GENERIC_LOCAL64_H
 
-#include <linux/percpu.h>
 #include <asm/types.h>

> +
>  struct unwind_cache {
>  	unsigned int		nr_entries;
>  	unsigned long		entries[];
> @@ -10,8 +13,8 @@ struct unwind_cache {
>  struct unwind_task_info {
>  	struct unwind_cache	*cache;
>  	struct callback_head	work;
> -	u64			timestamp;
> -	int			pending;
> +	local64_t		timestamp;
> +	local_t			pending;
>  };
>  
>  #endif /* _LINUX_UNWIND_USER_DEFERRED_TYPES_H */

Regards,
Jens
-- 
Jens Remus
Linux on Z Development (D3303)
+49-7031-16-1128 Office
jremus@de.ibm.com

IBM

IBM Deutschland Research & Development GmbH; Vorsitzender des Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der Gesellschaft: Böblingen; Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement: https://www.ibm.com/privacy/

Re: [PATCH v12 07/14] unwind_user/deferred: Make unwind deferral requests NMI-safe
Posted by Steven Rostedt 3 months, 1 week ago
On Wed, 2 Jul 2025 17:53:05 +0200
Jens Remus <jremus@linux.ibm.com> wrote:

> > @@ -2,6 +2,9 @@
> >  #ifndef _LINUX_UNWIND_USER_DEFERRED_TYPES_H
> >  #define _LINUX_UNWIND_USER_DEFERRED_TYPES_H
> >  
> > +#include <asm/local64.h>
> > +#include <asm/local.h>  
> 
> This creates the following circular dependency, that breaks the build on
> s390 as follows, whenever local64.h is included first, so that local64_t
> is not yet defined when unwind_deferred_types.h gets included down the
> line:

As per the discussion on patch 6, this may not be an issue in the next
version. I'm looking to get rid of 64 bit cmpxchg.

-- Steve