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 <[email protected]> > 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) [email protected] / [email protected] 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/
