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

[Indu Bhagat]

On 1/24/25 11:21 AM, Josh Poimboeuf wrote:
> On Fri, Jan 24, 2025 at 10:00:52AM -0800, Andrii Nakryiko wrote:
> > On Tue, Jan 21, 2025 at 6:32 PM Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
> > > +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?
>
> That was intentional as it's just an empty stub, but yeah, maybe 160
> chars is a bit much.
>
> > > +       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...
>
> No idea...
>
> > > +       if (!shdr.num_fdes || !shdr.num_fres) {
> >
> > given SFRAME_F_FRAME_POINTER in the header, is it really that
> > nonsensical and illegal to have zero FDEs/FREs? Maybe we should allow
> > that?
>
> It would seem a bit silly to create an empty .sframe section just to set
> that SFRAME_F_FRAME_POINTER bit.  Regardless, there's nothing the kernel
> can do with that.

Yes, in theory, it is allowed (as per the specification) to have an 
SFrame section with zero number of FDEs/FREs.  But since such a section 
will not be useful, I share the opinion that it makes sense to disallow 
it in the current unwinding contexts, for now (JIT usecase may change 
things later).

SFRAME_F_FRAME_POINTER flag is not being set currently by GAS/GNU ld at all.

> > > +               dbg("no fde/fre entries\n");
> > > +               return -EINVAL;
> > > +       }
> > > +
> > > +       header_end = sec->sframe_start + SFRAME_HEADER_SIZE(shdr);
> > > +       if (header_end >= sec->sframe_end) {
> >
> > if we allow zero FDEs/FREs, header_end == sec->sframe_end is legal, right?
>
> I suppose so, but again I'm not seeing any reason to support that.
>
> > > +               dbg("header doesn't fit in section\n");
> > > +               return -EINVAL;
> > > +       }
> > > +
> > > +       num_fdes   = shdr.num_fdes;
> > > +       fdes_start = header_end + shdr.fdes_off;
> > > +       fdes_end   = fdes_start + (num_fdes * sizeof(struct sframe_fde));
> > > +
> > > +       fres_start = header_end + shdr.fres_off;
> > > +       fres_end   = fres_start + shdr.fre_len;
> > > +
> >
> > maybe use check_add_overflow() in all the above calculation, at least
> > on 32-bit arches this all can overflow and it's not clear if below
> > sanity check detects all possible overflows
>
> Ok, I'll look into it.
>
> > > +struct sframe_preamble {
> > > +       u16     magic;
> > > +       u8      version;
> > > +       u8      flags;
> > > +} __packed;
> > > +
> > > +struct sframe_header {
> > > +       struct sframe_preamble preamble;
> > > +       u8      abi_arch;
> > > +       s8      cfa_fixed_fp_offset;
> > > +       s8      cfa_fixed_ra_offset;
> > > +       u8      auxhdr_len;
> > > +       u32     num_fdes;
> > > +       u32     num_fres;
> > > +       u32     fre_len;
> > > +       u32     fdes_off;
> > > +       u32     fres_off;
> > > +} __packed;
> > > +
> > > +struct sframe_fde {
> > > +       s32     start_addr;
> > > +       u32     func_size;
> > > +       u32     fres_off;
> > > +       u32     fres_num;
> > > +       u8      info;
> > > +       u8      rep_size;
> > > +       u16 padding;
> > > +} __packed;
> >
> > I couldn't understand from SFrame itself, but why do sframe_header,
> > sframe_preamble, and sframe_fde have to be marked __packed, if it's
> > all naturally aligned (intentionally and by design)?..
>
> Right, but the spec says they're all packed.  Maybe the point is that
> some future sframe version is free to introduce unaligned fields.

SFrame specification aims to keep SFrame header and SFrame FDE members 
at aligned boundaries in future versions.

Only SFrame FRE related accesses may have unaligned accesses.