Re: [PATCH v3 1/8] sframe: Allow kernelspace sframe sections.

[Jens Remus <jremus@linux.ibm.com>]

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@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@de.ibm.com / jremus@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/