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

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

On 4/22/2026 12:51 AM, 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 HAVE_UNWIND_USER_SFRAME, as there will be other client to
> this library than just userspace unwind.
> 
> 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>

With return -EFAULT changed to goto label in DATA_COPY() and DATA_GET():

Reviewed-by: Jens Remus <jremus@linux.ibm.com>

> ---
>  MAINTAINERS                                   |   2 +-
>  arch/Kconfig                                  |   4 +
>  .../{unwind_user_sframe.h => unwind_sframe.h} |   6 +-
>  arch/x86/include/asm/unwind_user.h            |  12 +-
>  include/linux/sframe.h                        |  48 ++--
>  include/linux/unwind_types.h                  |  46 +++
>  include/linux/unwind_user_types.h             |  41 ---
>  kernel/unwind/Makefile                        |   2 +-
>  kernel/unwind/sframe.c                        | 270 ++++++++++++------
>  kernel/unwind/user.c                          |  41 +--
>  10 files changed, 293 insertions(+), 179 deletions(-)
>  rename arch/x86/include/asm/{unwind_user_sframe.h => unwind_sframe.h} (50%)
>  create mode 100644 include/linux/unwind_types.h

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

> +enum sframe_sec_type {
> +	SFRAME_KERNEL,
> +	SFRAME_USER,
> +};

>  struct sframe_section {
> -	struct rcu_head	rcu;
> +	struct rcu_head  rcu;
>  #ifdef CONFIG_DYNAMIC_DEBUG
> -	const char	*filename;
> +	const char		*filename;
>  #endif
> -	unsigned long	sframe_start;
> -	unsigned long	sframe_end;
> -	unsigned long	text_start;
> -	unsigned long	text_end;
> -
> -	unsigned long	fdes_start;
> -	unsigned long	fres_start;
> -	unsigned long	fres_end;
> -	unsigned int	num_fdes;
> -
> -	signed char	ra_off;
> -	signed char	fp_off;
> +	enum sframe_sec_type	sec_type;
> +	unsigned long		sframe_start;
> +	unsigned long		sframe_end;
> +	unsigned long		text_start;
> +	unsigned long		text_end;
> +
> +	unsigned long		fdes_start;
> +	unsigned long		fres_start;
> +	unsigned long		fres_end;
> +	unsigned int		num_fdes;
> +
> +	signed char		ra_off;
> +	signed char		fp_off;
>  };

> diff --git a/kernel/unwind/sframe.c b/kernel/unwind/sframe.c

> +#define DATA_COPY(sec, to, from, size, label)			\
> +({								\
> +	switch (sec->sec_type) {				\
> +	case SFRAME_KERNEL:					\
> +		KERNEL_COPY(to, from, size, label);		\
> +		break;						\
> +	case SFRAME_USER:					\
> +		UNSAFE_USER_COPY(to, from, size, label);	\
> +		break;						\

I wonder whether it would be worthwhile to come up with an approach
where this would get evaluated at compile time instead at run time?
Or is this overengineering?  Of course such improvement could be
made later on, so no need to solve that now.

Options that came into my mind:
A) Introduce and pass through a "bool user" parameter, whose value is
   specified in sframe_find_user() and sframe_find_kernel().  Due to
   inlining I would expect that to get any conditions based on that
   to get evaluated at compile time.  See below.  Downside is the
   ugly additional parameter.

B) Introduce lightweight .c wrappers, e.g. sframe_kernel.c and
   sframe_user.c, that define DATA_GET() and DATA_COPY() and include
   sframe.c.  All HAVE_UNWIND_KERNEL_SFRAME code would be moved into
   sframe_kernel.c and likewise all HAVE_UNWIND_USER_SFRAME code into
   sframe_user.c.

> +	default:						\
> +		return -EFAULT;					\

		goto label;					\

Users of DATA_COPY() do expect the macro to branch to the label in case
of an error and therefore do not evaluate any return value.  The
wrapping then needs also be changed from "({ .. })" to
"do { ... } while (0)".

> +	}							\
> +})
> +
> +#define DATA_GET(sec, to, from, type, label)			\
> +({								\
> +	switch (sec->sec_type) {				\
> +	case SFRAME_KERNEL:					\
> +		KERNEL_GET(to, from, type, label);		\
> +		break;						\
> +	case SFRAME_USER:					\
> +		UNSAFE_USER_GET(to, from, type, label);		\
> +		break;						\
> +	default:						\
> +		return -EFAULT;					\

Likewise.

> +	}							\
> +})

> +#ifdef CONFIG_HAVE_UNWIND_USER_SFRAME
> +
> +int sframe_find_user(unsigned long ip, struct unwind_frame *frame)
> +{
> +	struct mm_struct *mm = current->mm;
> +	struct sframe_section *sec;
> +	int ret;
> +
> +	if (!mm)
> +		return -EINVAL;
> +
> +	guard(srcu)(&sframe_srcu);
> +
> +	sec = mtree_load(&mm->sframe_mt, ip);
> +	if (!sec)
> +		return -EINVAL;
> +
> +	if (!user_read_access_begin((void __user *)sec->sframe_start,
> +				    sec->sframe_end - sec->sframe_start))
> +		return -EFAULT;
> +
> +	ret = __sframe_find(sec, ip, frame);

In sframe_find_user() sec->sec_type must be SFRAME_USER.  Likewise in
sframe_find_kernel() it must be SFRAME_KERNEL.  So instead of
introducing sec_type, we could add a parameter
__sframe_find(..., bool user) and do:

	ret = __sframe_find(sec, ip, frame, true);

The downside is that this then requires to pass that flag through
everywhere... (see below).

> +
> +	user_read_access_end();
> +
> +	if (ret == -EFAULT) {
> +		dbg_sec("removing bad .sframe section\n");
> +		WARN_ON_ONCE(sframe_remove_section(sec->sframe_start));
> +	}
> +
> +	return ret;
> +}
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/