Re: [PATCH v4 17/39] unwind_user/sframe: Add support for reading .sframe headers

[Indu Bhagat]

On 1/24/25 10:00 AM, Andrii Nakryiko wrote:
> On Tue, Jan 21, 2025 at 6:32 PM Josh Poimboeuf<jpoimboe@xxxxxxxxxx> wrote:
> > In preparation for unwinding user space stacks with sframe, add basic
> > sframe compile infrastructure and support for reading the .sframe
> > section header.
> >
> > sframe_add_section() reads the header and unconditionally returns an
> > error, so it's not very useful yet.  A subsequent patch will improve
> > that.
> >
> > Signed-off-by: Josh Poimboeuf<jpoimboe@xxxxxxxxxx>
> > ---
> >   arch/Kconfig           |   3 +
> >   include/linux/sframe.h |  36 +++++++++++
> >   kernel/unwind/Makefile |   3 +-
> >   kernel/unwind/sframe.c | 136 +++++++++++++++++++++++++++++++++++++++++
> >   kernel/unwind/sframe.h |  71 +++++++++++++++++++++
> >   5 files changed, 248 insertions(+), 1 deletion(-)
> >   create mode 100644 include/linux/sframe.h
> >   create mode 100644 kernel/unwind/sframe.c
> >   create mode 100644 kernel/unwind/sframe.h
> [...]
>
> > +
> > +extern int sframe_add_section(unsigned long sframe_start, unsigned long sframe_end,
> > +                             unsigned long text_start, unsigned long text_end);
> > +extern int sframe_remove_section(unsigned long sframe_addr);
> > +
> > +#else /* !CONFIG_HAVE_UNWIND_USER_SFRAME */
> > +
> > +static inline int sframe_add_section(unsigned long sframe_start, unsigned long sframe_end, unsigned long text_start, unsigned long text_end) { return -ENOSYS; }
> nit: very-very long, wrap it?
>
> > +static inline int sframe_remove_section(unsigned long sframe_addr) { return -ENOSYS; }
> > +
> > +#endif /* CONFIG_HAVE_UNWIND_USER_SFRAME */
> > +
> > +#endif /* _LINUX_SFRAME_H */
> [...]
>
> > +static int sframe_read_header(struct sframe_section *sec)
> > +{
> > +       unsigned long header_end, fdes_start, fdes_end, fres_start, fres_end;
> > +       struct sframe_header shdr;
> > +       unsigned int num_fdes;
> > +
> > +       if (copy_from_user(&shdr, (void __user *)sec->sframe_start, sizeof(shdr))) {
> > +               dbg("header usercopy failed\n");
> > +               return -EFAULT;
> > +       }
> > +
> > +       if (shdr.preamble.magic != SFRAME_MAGIC ||
> > +           shdr.preamble.version != SFRAME_VERSION_2 ||
> > +           !(shdr.preamble.flags & SFRAME_F_FDE_SORTED) ||
> probably more a question to Indu, but why is this sorting not
> mandatory and part of SFrame "standard"? How realistically non-sorted
> FDEs would work in practice? Ain't nobody got time to sort them just
> to unwind the stack...

It is not worthwhile for the assembler (even wasteful as it adds to 
build time for nothing) to generate an .sframe section with FDEs in 
sorted order of start PC of function.  This is because the final order 
is decided by the linker as it merges all input sections.

Thats one reason why it is already necessary that the specification 
allows SFRAME_F_FDE_SORTED not set in the section. I can also see how 
not making the sorting mandatory may also be necessary for JIT use-case..

FWIW, for non-JIT environments, non-sorted FDEs are not expected in 
linked binaries; such a thing does not seem to be useful in practice.

Hope that helps
Indu