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

Jens Remus
2026-04-22T14:09:29Z

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/