* [RFC PATCH v2 1/3] unwind_user: Fix userspace unwind iterator 32-bit compat handling
2025-07-10 16:42 [RFC PATCH v2 0/3] Improvements to unwind user Mathieu Desnoyers
@ 2025-07-10 16:42 ` Mathieu Desnoyers
2025-07-10 16:42 ` [RFC PATCH v2 2/3] unwind: Export unwind_user symbol to GPL modules Mathieu Desnoyers
2025-07-10 16:42 ` [RFC PATCH v2 3/3] unwind deferred: Introduce unwind_user_trace_cached Mathieu Desnoyers
2 siblings, 0 replies; 4+ messages in thread
From: Mathieu Desnoyers @ 2025-07-10 16:42 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-kernel, Mathieu Desnoyers, Josh Poimboeuf,
Masami Hiramatsu, Peter Zijlstra, Ingo Molnar, Jiri Olsa,
Namhyung Kim, Thomas Gleixner, Andrii Nakryiko, Indu Bhagat,
Jose E. Marchesi, Beau Belgrave, Jens Remus, Linus Torvalds,
Andrew Morton
Refactor the stack unwind iterator to clean the overall design and fix
32-bit compat corner-cases that arise due to confusion over the semantic
of the struct unwind_user_state type field.
Observed Issue
==============
Before this refactor, the struct unwind_user_state type field identifies:
- Whether sframe is available in the current process, for at least
portions of the address space,
- Whether frame pointers should be used for stack walk,
- Whether frame pointers should be used for stack walk in a 32-bit
compat process.
It would be natural for the unwind iterator caller to expect the
"state->type" to contain the type of stack walk operation performed for
the most recent unwind iteration (unwind_user_next()), but it actually
acts as an internal cache and is unrelated to the current iteration,
which makes this field confusing.
It is not only confusing to the caller, but also to the implementation
of unwind_user_next(). Is has the following drawbacks:
- A 32-bit compat process will favor using frame pointers rather than
sframe for its next unwind iterator, which is an unexpected priority
order because sframe is more accurate than frame pointers when
available.
- Because the "UNWIND_USER_TYPE_SFRAME" state type is unaware of 32-bit
compat, handling of sframe would be broken on 32-bit compat because
compat_fp_state() would return false, thus expecting 64-bit unsigned
long types.
Cause
=====
Associating a semantic to a single enumeration field that consists of:
- many orthogonal properties, without listing all those properties
(sframe vs frame pointer, compat vs non-compat) within each of the
enumeration labels,
- an implicit priority,
leads to confusion in the unwind iterator implementation.
Solution
========
Split this type field into two fields:
- A "current_type" field, which can be queried by the iterator caller
and by architecture code to learn which method is used for the
current unwind iteration,
- An "available_types" field, which is a bitmask of available user types.
It is used internally by the iterator implementation.
Remove the "UNWIND_USER_TYPE_COMPAT_FP" label from the unwind types,
since it is now implicitly taken into account by the
unwind_compat_mode() check.
This approach will also make it easier to introduce new unwind types
in the future, such as an unwinder for JIT code.
Rename in_compat_mode to unwind_compat_mode to remove confusion between
the pre-existing in_compat_syscall() macro (which returns true for both
ia32 and x32) and unwind_compat_mode() which only returns true for ia32.
Known Drawbacks
===============
None.
[ This is based on linux-trace unwind/sframe ]
Fixes: 1abc29eeca39 ("unwind_user/sframe: Wire up unwind_user to sframe")
Fixes: ce8d69a1a578 ("unwind_user/x86: Enable compat mode frame pointer unwinding on x86")
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Steven Rostedt (Google) <rostedt@goodmis.org>
Cc: Josh Poimboeuf <jpoimboe@kernel.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Indu Bhagat <indu.bhagat@oracle.com>
Cc: "Jose E. Marchesi" <jemarch@gnu.org>
Cc: Beau Belgrave <beaub@linux.microsoft.com>
Cc: Jens Remus <jremus@linux.ibm.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
arch/x86/include/asm/unwind_user.h | 6 +-
arch/x86/kernel/stacktrace.c | 2 +-
include/asm-generic/unwind_user.h | 4 +
include/linux/unwind_user.h | 9 --
include/linux/unwind_user_types.h | 25 ++++--
kernel/unwind/user.c | 139 +++++++++++++++++------------
6 files changed, 110 insertions(+), 75 deletions(-)
diff --git a/arch/x86/include/asm/unwind_user.h b/arch/x86/include/asm/unwind_user.h
index 19634a73612d..1b32491f28a5 100644
--- a/arch/x86/include/asm/unwind_user.h
+++ b/arch/x86/include/asm/unwind_user.h
@@ -10,6 +10,8 @@
.fp_off = (s32)sizeof(long) * -2, \
.use_fp = true,
+#define unwind_compat_mode(regs) (!user_64bit_mode(regs))
+
#ifdef CONFIG_IA32_EMULATION
#define ARCH_INIT_USER_COMPAT_FP_FRAME \
@@ -18,14 +20,12 @@
.fp_off = (s32)sizeof(u32) * -2, \
.use_fp = true,
-#define in_compat_mode(regs) !user_64bit_mode(regs)
-
void arch_unwind_user_init(struct unwind_user_state *state,
struct pt_regs *regs);
static inline void arch_unwind_user_next(struct unwind_user_state *state)
{
- if (state->type != UNWIND_USER_TYPE_COMPAT_FP)
+ if (!unwind_compat_mode(task_pt_regs(current)))
return;
state->ip += state->arch.cs_base;
diff --git a/arch/x86/kernel/stacktrace.c b/arch/x86/kernel/stacktrace.c
index 8ef9d8c71df9..3e970162fc0f 100644
--- a/arch/x86/kernel/stacktrace.c
+++ b/arch/x86/kernel/stacktrace.c
@@ -137,7 +137,7 @@ void arch_unwind_user_init(struct unwind_user_state *state,
{
unsigned long cs_base, ss_base;
- if (state->type != UNWIND_USER_TYPE_COMPAT_FP)
+ if (!unwind_compat_mode(regs))
return;
cs_base = insn_get_seg_base(regs, INAT_SEG_REG_CS);
diff --git a/include/asm-generic/unwind_user.h b/include/asm-generic/unwind_user.h
index b8882b909944..f9d7603670cd 100644
--- a/include/asm-generic/unwind_user.h
+++ b/include/asm-generic/unwind_user.h
@@ -2,4 +2,8 @@
#ifndef _ASM_GENERIC_UNWIND_USER_H
#define _ASM_GENERIC_UNWIND_USER_H
+#ifndef unwind_compat_mode
+#define unwind_compat_mode(regs) false
+#endif
+
#endif /* _ASM_GENERIC_UNWIND_USER_H */
diff --git a/include/linux/unwind_user.h b/include/linux/unwind_user.h
index 8a4af0214ecb..0308adb349fc 100644
--- a/include/linux/unwind_user.h
+++ b/include/linux/unwind_user.h
@@ -5,15 +5,6 @@
#include <linux/unwind_user_types.h>
#include <asm/unwind_user.h>
-#ifndef ARCH_INIT_USER_FP_FRAME
- #define ARCH_INIT_USER_FP_FRAME
-#endif
-
-#ifndef ARCH_INIT_USER_COMPAT_FP_FRAME
- #define ARCH_INIT_USER_COMPAT_FP_FRAME
- #define in_compat_mode(regs) false
-#endif
-
/*
* If an architecture needs to initialize the state for a specific
* reason, for example, it may need to do something different
diff --git a/include/linux/unwind_user_types.h b/include/linux/unwind_user_types.h
index 4d50476e950e..41ea44c5ce5c 100644
--- a/include/linux/unwind_user_types.h
+++ b/include/linux/unwind_user_types.h
@@ -9,11 +9,23 @@
struct arch_unwind_user_state {};
#endif
+
+/*
+ * Unwind types, listed in priority order: lower numbers are attempted first if
+ * available.
+ */
+enum unwind_user_type_bits {
+ UNWIND_USER_TYPE_SFRAME_BIT = 0,
+ UNWIND_USER_TYPE_FP_BIT = 1,
+
+ _NR_UNWIND_USER_TYPE_BITS,
+};
+
enum unwind_user_type {
- UNWIND_USER_TYPE_NONE,
- UNWIND_USER_TYPE_FP,
- UNWIND_USER_TYPE_COMPAT_FP,
- UNWIND_USER_TYPE_SFRAME,
+ /* Type "none" for the start of stack walk iteration. */
+ UNWIND_USER_TYPE_NONE = 0,
+ UNWIND_USER_TYPE_SFRAME = (1U << UNWIND_USER_TYPE_SFRAME_BIT),
+ UNWIND_USER_TYPE_FP = (1U << UNWIND_USER_TYPE_FP_BIT),
};
struct unwind_stacktrace {
@@ -33,7 +45,10 @@ struct unwind_user_state {
unsigned long sp;
unsigned long fp;
struct arch_unwind_user_state arch;
- enum unwind_user_type type;
+ /* Unwind time used for the most recent unwind traversal iteration. */
+ enum unwind_user_type current_type;
+ /* Unwind types available in the current context. Bitmask of enum unwind_user_type. */
+ unsigned int available_types;
bool done;
};
diff --git a/kernel/unwind/user.c b/kernel/unwind/user.c
index 6e7ca9f1293a..18d83efc876e 100644
--- a/kernel/unwind/user.c
+++ b/kernel/unwind/user.c
@@ -13,70 +13,43 @@ static struct unwind_user_frame fp_frame = {
ARCH_INIT_USER_FP_FRAME
};
+#ifdef CONFIG_HAVE_UNWIND_USER_COMPAT_FP
static struct unwind_user_frame compat_fp_frame = {
ARCH_INIT_USER_COMPAT_FP_FRAME
};
+#endif
-static inline bool fp_state(struct unwind_user_state *state)
+static struct unwind_user_frame *get_fp_frame(struct pt_regs *regs)
{
- return IS_ENABLED(CONFIG_HAVE_UNWIND_USER_FP) &&
- state->type == UNWIND_USER_TYPE_FP;
+#ifdef CONFIG_HAVE_UNWIND_USER_COMPAT_FP
+ if (unwind_compat_mode(regs))
+ return &compat_fp_frame;
+#endif
+ return &fp_frame;
}
#define for_each_user_frame(state) \
for (unwind_user_start(state); !(state)->done; unwind_user_next(state))
-static inline bool compat_fp_state(struct unwind_user_state *state)
-{
- return IS_ENABLED(CONFIG_HAVE_UNWIND_USER_COMPAT_FP) &&
- state->type == UNWIND_USER_TYPE_COMPAT_FP;
-}
-
-static inline bool sframe_state(struct unwind_user_state *state)
-{
- return IS_ENABLED(CONFIG_HAVE_UNWIND_USER_SFRAME) &&
- state->type == UNWIND_USER_TYPE_SFRAME;
-}
-
-#define unwind_get_user_long(to, from, state) \
+#define unwind_get_user_long(to, from, regs) \
({ \
int __ret; \
- if (compat_fp_state(state)) \
+ if (unwind_compat_mode(regs)) \
__ret = get_user(to, (u32 __user *)(from)); \
else \
__ret = get_user(to, (unsigned long __user *)(from)); \
__ret; \
})
-static int unwind_user_next(struct unwind_user_state *state)
+static int unwind_user_next_common(struct unwind_user_state *state, struct unwind_user_frame *frame,
+ struct pt_regs *regs)
{
- struct unwind_user_frame *frame;
- struct unwind_user_frame _frame;
- unsigned long cfa = 0, fp, ra = 0;
+ unsigned long cfa, fp, ra = 0;
unsigned int shift;
- if (state->done)
- return -EINVAL;
-
- if (compat_fp_state(state)) {
- frame = &compat_fp_frame;
- } else if (sframe_state(state)) {
- /* sframe expects the frame to be local storage */
- frame = &_frame;
- if (sframe_find(state->ip, frame)) {
- if (!IS_ENABLED(CONFIG_HAVE_UNWIND_USER_FP))
- goto done;
- frame = &fp_frame;
- }
- } else if (fp_state(state)) {
- frame = &fp_frame;
- } else {
- goto done;
- }
-
if (frame->use_fp) {
if (state->fp < state->sp)
- goto done;
+ return -EINVAL;
cfa = state->fp;
} else {
cfa = state->sp;
@@ -87,30 +60,86 @@ static int unwind_user_next(struct unwind_user_state *state)
/* stack going in wrong direction? */
if (cfa <= state->sp)
- goto done;
+ return -EINVAL;
/* Make sure that the address is word aligned */
- shift = sizeof(long) == 4 || compat_fp_state(state) ? 2 : 3;
+ shift = (sizeof(long) == 4 || unwind_compat_mode(regs)) ? 2 : 3;
if ((cfa + frame->ra_off) & ((1 << shift) - 1))
- goto done;
+ return -EINVAL;
/* Find the Return Address (RA) */
- if (unwind_get_user_long(ra, cfa + frame->ra_off, state))
- goto done;
+ if (unwind_get_user_long(ra, cfa + frame->ra_off, regs))
+ return -EINVAL;
- if (frame->fp_off && unwind_get_user_long(fp, cfa + frame->fp_off, state))
- goto done;
+ if (frame->fp_off && unwind_get_user_long(fp, cfa + frame->fp_off, regs))
+ return -EINVAL;
state->ip = ra;
state->sp = cfa;
if (frame->fp_off)
state->fp = fp;
+ return 0;
+}
- arch_unwind_user_next(state);
+static int unwind_user_next_sframe(struct unwind_user_state *state)
+{
+ struct unwind_user_frame _frame, *frame;
+
+ /* sframe expects the frame to be local storage */
+ frame = &_frame;
+ if (sframe_find(state->ip, frame))
+ return -ENOENT;
+ return unwind_user_next_common(state, frame, task_pt_regs(current));
+}
+
+static int unwind_user_next_fp(struct unwind_user_state *state)
+{
+ struct pt_regs *regs = task_pt_regs(current);
+
+ return unwind_user_next_common(state, get_fp_frame(regs), regs);
+}
+
+static int unwind_user_next(struct unwind_user_state *state)
+{
+ unsigned long iter_mask = state->available_types;
+ unsigned int bit;
+ if (state->done)
+ return -EINVAL;
+
+ for_each_set_bit(bit, &iter_mask, _NR_UNWIND_USER_TYPE_BITS) {
+ enum unwind_user_type type = 1U << bit;
+
+ state->current_type = type;
+ switch (type) {
+ case UNWIND_USER_TYPE_SFRAME:
+ switch (unwind_user_next_sframe(state)) {
+ case 0:
+ goto end;
+ case -ENOENT:
+ continue; /* Try next method. */
+ default:
+ goto done;
+ }
+ case UNWIND_USER_TYPE_FP:
+ if (!unwind_user_next_fp(state))
+ goto end;
+ else
+ goto done;
+ case UNWIND_USER_TYPE_NONE:
+ break;
+ }
+ }
+
+ /* No successful unwind method. */
+ goto done;
+
+end:
+ arch_unwind_user_next(state);
return 0;
done:
+ state->current_type = UNWIND_USER_TYPE_NONE;
state->done = true;
return -EINVAL;
}
@@ -126,14 +155,10 @@ static int unwind_user_start(struct unwind_user_state *state)
return -EINVAL;
}
- if (IS_ENABLED(CONFIG_HAVE_UNWIND_USER_COMPAT_FP) && in_compat_mode(regs))
- state->type = UNWIND_USER_TYPE_COMPAT_FP;
- else if (current_has_sframe())
- state->type = UNWIND_USER_TYPE_SFRAME;
- else if (IS_ENABLED(CONFIG_HAVE_UNWIND_USER_FP))
- state->type = UNWIND_USER_TYPE_FP;
- else
- state->type = UNWIND_USER_TYPE_NONE;
+ if (current_has_sframe())
+ state->available_types |= UNWIND_USER_TYPE_SFRAME;
+ if (IS_ENABLED(CONFIG_HAVE_UNWIND_USER_FP))
+ state->available_types |= UNWIND_USER_TYPE_FP;
state->ip = instruction_pointer(regs);
state->sp = user_stack_pointer(regs);
--
2.43.0
^ permalink raw reply [flat|nested] 4+ messages in thread* [RFC PATCH v2 3/3] unwind deferred: Introduce unwind_user_trace_cached
2025-07-10 16:42 [RFC PATCH v2 0/3] Improvements to unwind user Mathieu Desnoyers
2025-07-10 16:42 ` [RFC PATCH v2 1/3] unwind_user: Fix userspace unwind iterator 32-bit compat handling Mathieu Desnoyers
2025-07-10 16:42 ` [RFC PATCH v2 2/3] unwind: Export unwind_user symbol to GPL modules Mathieu Desnoyers
@ 2025-07-10 16:42 ` Mathieu Desnoyers
2 siblings, 0 replies; 4+ messages in thread
From: Mathieu Desnoyers @ 2025-07-10 16:42 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-kernel, Mathieu Desnoyers, Josh Poimboeuf,
Masami Hiramatsu, Peter Zijlstra, Ingo Molnar, Jiri Olsa,
Namhyung Kim, Thomas Gleixner, Andrii Nakryiko, Indu Bhagat,
Jose E. Marchesi, Beau Belgrave, Jens Remus, Linus Torvalds,
Andrew Morton
Introduce unwind_user_trace_cached which copies the stack trace if it
was previously stored into the cache since the last reset of the cache.
The expected use-case is sampling a stack trace from a faultable context
at system call entry (coping it into the cache), and then copying the
stack trace from the cache from non-faultable context to a ring buffer.
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Steven Rostedt (Google) <rostedt@goodmis.org>
Cc: Josh Poimboeuf <jpoimboe@kernel.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Indu Bhagat <indu.bhagat@oracle.com>
Cc: "Jose E. Marchesi" <jemarch@gnu.org>
Cc: Beau Belgrave <beaub@linux.microsoft.com>
Cc: Jens Remus <jremus@linux.ibm.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
include/linux/unwind_deferred.h | 2 ++
kernel/unwind/deferred.c | 31 +++++++++++++++++++++++++++++++
2 files changed, 33 insertions(+)
diff --git a/include/linux/unwind_deferred.h b/include/linux/unwind_deferred.h
index a9d5b100d6b2..ddc985ba982a 100644
--- a/include/linux/unwind_deferred.h
+++ b/include/linux/unwind_deferred.h
@@ -34,6 +34,7 @@ void unwind_task_init(struct task_struct *task);
void unwind_task_free(struct task_struct *task);
int unwind_user_faultable(struct unwind_stacktrace *trace);
+int unwind_user_trace_cached(struct unwind_stacktrace *trace);
int unwind_deferred_init(struct unwind_work *work, unwind_callback_t func);
int unwind_deferred_request(struct unwind_work *work, u64 *cookie);
@@ -67,6 +68,7 @@ static inline void unwind_task_init(struct task_struct *task) {}
static inline void unwind_task_free(struct task_struct *task) {}
static inline int unwind_user_faultable(struct unwind_stacktrace *trace) { return -ENOSYS; }
+static inline int unwind_user_trace_cached(struct unwind_stacktrace *trace) { return -ENOSYS; }
static inline int unwind_deferred_init(struct unwind_work *work, unwind_callback_t func) { return -ENOSYS; }
static inline int unwind_deferred_request(struct unwind_work *work, u64 *timestamp) { return -ENOSYS; }
static inline void unwind_deferred_cancel(struct unwind_work *work) {}
diff --git a/kernel/unwind/deferred.c b/kernel/unwind/deferred.c
index 039e12700d49..02ab1d2afc21 100644
--- a/kernel/unwind/deferred.c
+++ b/kernel/unwind/deferred.c
@@ -145,6 +145,37 @@ int unwind_user_faultable(struct unwind_stacktrace *trace)
return 0;
}
+EXPORT_SYMBOL_GPL(unwind_user_faultable);
+
+/**
+ * unwind_user_trace_cached - Copy user stack trace from cache
+ * @trace: The descriptor that will store the user stacktrace
+ *
+ * Copy user stack trace from cache if the cache was populated by
+ * unwind_user_faultable prior to this call. The returned trace
+ * entries are only valid until the cache is reset.
+ *
+ * Return: 0 on success and negative on error
+ * On success @trace will contain the user space stacktrace
+ */
+int unwind_user_trace_cached(struct unwind_stacktrace *trace)
+{
+ struct unwind_task_info *info = ¤t->unwind_info;
+ struct unwind_cache *cache;
+
+ if (!current->mm)
+ return -EINVAL;
+
+ cache = info->cache;
+ if (!cache || !cache->nr_entries)
+ return -ENOENT;
+
+ trace->nr = cache->nr_entries;
+ trace->entries = cache->entries;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(unwind_user_trace_cached);
static void process_unwind_deferred(struct task_struct *task)
{
--
2.43.0
^ permalink raw reply [flat|nested] 4+ messages in thread