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

On Wed, Apr 22, 2026 at 7:08=E2=80=AFAM Jens Remus <[email protected]> w=
rote:
>
> 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 sfram=
e
> > 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 =3D> 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 =3D> 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 =3D current->mm;
> > +     struct sframe_section *sec;
> > +     int ret;
> > +
> > +     if (!mm)
> > +             return -EINVAL;
> > +
> > +     guard(srcu)(&sframe_srcu);
> > +
> > +     sec =3D 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 =3D __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 =3D __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 =3D=3D -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 Aufsichtsra=
ts: Wolfgang Wendt; Gesch=C3=A4ftsf=C3=BChrung: David Faller; Sitz der Gese=
llschaft: Ehningen; Registergericht: Amtsgericht Stuttgart, HRB 243294
> IBM Data Privacy Statement: https://www.ibm.com/privacy/
>