On Fri, Jan 24, 2025 at 10:00:52AM -0800, Andrii Nakryiko wrote: > On Tue, Jan 21, 2025 at 6:32 PM Josh Poimboeuf <jpoim...@kernel.org> 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. > > + 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. -- Josh