On Wed, Apr 22, 2026 at 7:08 AM Jens Remus <[email protected]> wrote:
>
> 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 <[email protected]>
>
> With return -EFAULT changed to goto label in DATA_COPY() and DATA_GET():
>
> Reviewed-by: Jens Remus <[email protected]>
>
> > ---
> > 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.
I had a similar thought when I was writing this patch, but I ended up
deciding to avoid premature optimization before getting feedback. I'd
definitely be interested in improving upon this later on.
>
> 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.
(A) definitely sounds simpler to implement. For (B) it seems uncommon
for .c files to include one another. Style-wise, is this something
that is typically allowed (e.g. by checkpatch.pl)?
>
> > + 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)
> [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/
>
