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. 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) [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/
