[PATCH v3 1/8] sframe: Allow kernelspace sframe sections.
Jens Remus
jremus at linux.ibm.com
Tue Apr 14 05:09:42 PDT 2026
Hello Dylan!
On 4/6/2026 8:49 PM, Dylan Hatch wrote:
> Generalize the sframe lookup code to support kernelspace sections. This
> is done by defining a SFRAME_LOOKUP option that can be activated
> separate from UNWIND_USER_SFRAME, as there will be other clients to this
> library than just userspace unwind.
Nit: s/UNWIND_USER_SFRAME/HAVE_UNWIND_USER_SFRAME/
This actually uses the following two new Kconfig options (with
SFRAME_UNWINDER technically being introduced in the next patch):
SFRAME_LOOKUP
SFRAME_UNWINDER
IIUC SFRAME_UNWINDER is the kernel counterpart to the existing
HAVE_UNWIND_USER_SFRAME. Would it therefore make sense to align the
naming as follows?
HAVE_UNWIND_KERNEL_SFRAME (instead of SFRAME_UNWINDER)
HAVE_UNWIND_USER_SFRAME
> Sframe section location is now tracked in a separate sec_type field to
> determine whether user-access functions are necessary to read the sframe
> data. Relevant type delarations are moved and renamed to reflect the
> non-user sframe support.
>
> Signed-off-by: Dylan Hatch <dylanbhatch at google.com>
> diff --git a/arch/Kconfig b/arch/Kconfig
> @@ -486,6 +486,9 @@ config AS_SFRAME3
> def_bool $(as-instr,.cfi_startproc\n.cfi_endproc,-Wa$(comma)--gsframe-3)
> select AS_SFRAME
>
> +config SFRAME_LOOKUP
> + bool
> +
> config UNWIND_USER
> bool
>
> @@ -496,6 +499,7 @@ config HAVE_UNWIND_USER_FP
> config HAVE_UNWIND_USER_SFRAME
> bool
> select UNWIND_USER
> + select SFRAME_LOOKUP
>
> config SFRAME_VALIDATION
> bool "Enable .sframe section debugging"
IIUC SFRAME_LOOKUP only exists to pull in the common (kernel and user)
sframe lookup code if SFRAME_UNWINDER and/or UNWIND_USER_SFRAME are
enabled. Given there is currently no other use case than kernel/user
stacktrace unwinding, would it make sense to rename it as follows to
group all of the related options with the UNWIND prefix?
UNWIND_SFRAME[_LOOKUP]
> diff --git a/include/linux/sframe.h b/include/linux/sframe.h
> @@ -4,36 +4,85 @@
>
> #include <linux/mm_types.h>
> #include <linux/srcu.h>
> -#include <linux/unwind_user_types.h>
>
> -#ifdef CONFIG_HAVE_UNWIND_USER_SFRAME
> +#define UNWIND_RULE_DEREF BIT(31)
> +
> +enum unwind_cfa_rule {
> + UNWIND_CFA_RULE_SP_OFFSET, /* CFA = SP + offset */
> + UNWIND_CFA_RULE_FP_OFFSET, /* CFA = FP + offset */
> + UNWIND_CFA_RULE_REG_OFFSET, /* CFA = reg + offset */
> + /* DEREF variants */
> + UNWIND_CFA_RULE_REG_OFFSET_DEREF = /* CFA = *(reg + offset) */
> + UNWIND_CFA_RULE_REG_OFFSET | UNWIND_RULE_DEREF,
> +};
> +
> +struct unwind_cfa_rule_data {
> + enum unwind_cfa_rule rule;
> + s32 offset;
> + unsigned int regnum;
> +};
> +
> +enum unwind_rule {
> + UNWIND_RULE_RETAIN, /* entity = entity */
> + UNWIND_RULE_CFA_OFFSET, /* entity = CFA + offset */
> + UNWIND_RULE_REG_OFFSET, /* entity = register + offset */
> + /* DEREF variants */
> + UNWIND_RULE_CFA_OFFSET_DEREF = /* entity = *(CFA + offset) */
> + UNWIND_RULE_CFA_OFFSET | UNWIND_RULE_DEREF,
> + UNWIND_RULE_REG_OFFSET_DEREF = /* entity = *(register + offset) */
> + UNWIND_RULE_REG_OFFSET | UNWIND_RULE_DEREF,
> +};
> +
> +struct unwind_rule_data {
> + enum unwind_rule rule;
> + s32 offset;
> + unsigned int regnum;
> +};
> +
> +struct unwind_frame {
> + struct unwind_cfa_rule_data cfa;
> + struct unwind_rule_data ra;
> + struct unwind_rule_data fp;
> + bool outermost;
> +};
You are moving (and renaming to generalize for kernel and user unwind
use) the above definitions from include/linux/unwind_user_types.h to
include/linux/sframe.h. Given the definitions are used in
kernel/unwind/user.c for FP and SFRAME unwinding this seems wrong to
me. The definitions should better be moved (and renamed as you did)
into a new include/linux/unwind_types.h (or the like).
> diff --git a/include/linux/unwind_user_types.h b/include/linux/unwind_user_types.h
> @@ -27,47 +27,6 @@ struct unwind_stacktrace {
> unsigned long *entries;
> };
>
> -#define UNWIND_USER_RULE_DEREF BIT(31)
> -
> -enum unwind_user_cfa_rule {
> - UNWIND_USER_CFA_RULE_SP_OFFSET, /* CFA = SP + offset */
> - UNWIND_USER_CFA_RULE_FP_OFFSET, /* CFA = FP + offset */
> - UNWIND_USER_CFA_RULE_REG_OFFSET, /* CFA = reg + offset */
> - /* DEREF variants */
> - UNWIND_USER_CFA_RULE_REG_OFFSET_DEREF = /* CFA = *(reg + offset) */
> - UNWIND_USER_CFA_RULE_REG_OFFSET | UNWIND_USER_RULE_DEREF,
> -};
> -
> -struct unwind_user_cfa_rule_data {
> - enum unwind_user_cfa_rule rule;
> - s32 offset;
> - unsigned int regnum;
> -};
> -
> -enum unwind_user_rule {
> - UNWIND_USER_RULE_RETAIN, /* entity = entity */
> - UNWIND_USER_RULE_CFA_OFFSET, /* entity = CFA + offset */
> - UNWIND_USER_RULE_REG_OFFSET, /* entity = register + offset */
> - /* DEREF variants */
> - UNWIND_USER_RULE_CFA_OFFSET_DEREF = /* entity = *(CFA + offset) */
> - UNWIND_USER_RULE_CFA_OFFSET | UNWIND_USER_RULE_DEREF,
> - UNWIND_USER_RULE_REG_OFFSET_DEREF = /* entity = *(register + offset) */
> - UNWIND_USER_RULE_REG_OFFSET | UNWIND_USER_RULE_DEREF,
> -};
> -
> -struct unwind_user_rule_data {
> - enum unwind_user_rule rule;
> - s32 offset;
> - unsigned int regnum;
> -};
> -
> -struct unwind_user_frame {
> - struct unwind_user_cfa_rule_data cfa;
> - struct unwind_user_rule_data ra;
> - struct unwind_user_rule_data fp;
> - bool outermost;
> -};
> -
> struct unwind_user_state {
> unsigned long ip;
> unsigned long sp;
> diff --git a/kernel/unwind/Makefile b/kernel/unwind/Makefile
> @@ -1,2 +1,2 @@
> obj-$(CONFIG_UNWIND_USER) += user.o deferred.o
> - obj-$(CONFIG_HAVE_UNWIND_USER_SFRAME) += sframe.o
> + obj-$(CONFIG_SFRAME_LOOKUP) += sframe.o
> diff --git a/kernel/unwind/sframe.c b/kernel/unwind/sframe.c
> @@ -44,8 +43,6 @@ struct sframe_fre_internal {
> unsigned char dw_size;
> };
>
> -DEFINE_STATIC_SRCU(sframe_srcu);
> -
> static __always_inline unsigned char fre_type_to_size(unsigned char fre_type)
> {
> if (fre_type > 2)
> @@ -60,6 +57,78 @@ static __always_inline unsigned char dataword_size_enum_to_size(unsigned char da
> return 1 << dataword_size;
> }
>
> +#ifdef CONFIG_HAVE_UNWIND_USER_SFRAME
> +
> +DEFINE_STATIC_SRCU(sframe_srcu);
> +
> +#define UNSAFE_USER_COPY(to, from, size, label) \
> + unsafe_copy_from_user(to, (void __user *)from, size, label)
> +
> +#define UNSAFE_USER_GET(to, from, type, label) \
> + unsafe_get_user(to, (type __user *)from, label)
> +
> +#else /* !CONFIG_HAVE_UNWIND_USER_SFRAME */
> +
> +#define UNSAFE_USER_COPY(to, from, size, label) do { \
> + (void)to; (void)from; (void)size; \
> + goto label; \
> +} while (0)
> +
> +#define UNSAFE_USER_GET(to, from, type, label) do { \
> + (void)to; (void)from; \
> + goto label; \
> +} while (0)
> +
> +#endif /* !CONFIG_HAVE_UNWIND_USER_SFRAME */
> +
> +#ifdef CONFIG_SFRAME_UNWINDER
> +
> +#define KERNEL_COPY(to, from, size) memcpy(to, (void *)from, size)
> +#define KERNEL_GET(to, from, type) ({ (to) = *(type *)(from); })
> +
> +#else /* !CONFIG_SFRAME_UNWINDER */
> +
> +#define KERNEL_COPY(to, from, size) do { \
> + (void)(to); (void)(from); (void)size; \
> + return -EFAULT; \
> +} while (0)
> +
> +#define KERNEL_GET(to, from, type) do { \
> + (void)(to); (void)(from); \
> + return -EFAULT; \
> +} while (0)
The error return value in above dummy implementations is never used
(see DATA_COPY() and DATA_GET() below). Maybe better define the KERNEL
flavors with the same interface as the UNSAFE_USER ones (with error
label) and have the dummy implementations goto that label?
> +
> +
Nit: Two instead of one empty line.
> +#endif /* !CONFIG_SFRAME_UNWINDER */
> +
> +#define DATA_COPY(sec, to, from, size, label) \
> +({ \
> + switch (sec->sec_type) { \
> + case SFRAME_KERNEL: \
> + KERNEL_COPY(to, from, size); \
> + break; \
> + case SFRAME_USER: \
> + UNSAFE_USER_COPY(to, from, size, label); \
> + break; \
> + default: \
> + return -EFAULT; \
> + } \
> +})
> +
> +#define DATA_GET(sec, to, from, type, label) \
> +({ \
> + switch (sec->sec_type) { \
> + case SFRAME_KERNEL: \
> + KERNEL_GET(to, from, type); \
> + break; \
> + case SFRAME_USER: \
> + UNSAFE_USER_GET(to, from, type, label); \
> + break; \
> + default: \
> + return -EFAULT; \
> + } \
> +})
> +
> static __always_inline int __read_fde(struct sframe_section *sec,
> unsigned int fde_num,
> struct sframe_fde_internal *fde)
Regards,
Jens
--
Jens Remus
Linux on Z Development (D3303)
jremus at de.ibm.com / jremus at linux.ibm.com
IBM Deutschland Research & Development GmbH; Vorsitzender des Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der Gesellschaft: Ehningen; Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement: https://www.ibm.com/privacy/
More information about the linux-arm-kernel
mailing list