Re: [PATCH v2 1/5] x86/livepatch: set function alignment to ensure minimal function size

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Dec 01, 2023 at 11:59:09AM +0100, Jan Beulich wrote:
> On 01.12.2023 11:21, Roger Pau Monné wrote:
> > On Fri, Dec 01, 2023 at 10:41:45AM +0100, Jan Beulich wrote:
> >> On 01.12.2023 09:50, Roger Pau Monné wrote:
> >>> On Fri, Dec 01, 2023 at 07:53:29AM +0100, Jan Beulich wrote:
> >>>> On 30.11.2023 18:37, Roger Pau Monné wrote:
> >>>>> On Thu, Nov 30, 2023 at 05:55:07PM +0100, Jan Beulich wrote:
> >>>>>> On 28.11.2023 11:03, Roger Pau Monne wrote:
> >>>>>>> The minimal function size requirements for livepatch are either 5 
> >>>>>>> bytes (for
> >>>>>>> jmp) or 9 bytes (for endbr + jmp).  Ensure that functions are always 
> >>>>>>> at least
> >>>>>>> that size by requesting the compiled to align the functions to 8 or 
> >>>>>>> 16 bytes,
> >>>>>>> depending on whether Xen is build with IBT support.
> >>>>>>
> >>>>>> How is alignment going to enforce minimum function size? If a function 
> >>>>>> is
> >>>>>> last in a section, there may not be any padding added (ahead of 
> >>>>>> linking at
> >>>>>> least). The trailing padding also isn't part of the function.
> >>>>>
> >>>>> If each function lives in it's own section (by using
> >>>>> -ffunction-sections), and each section is aligned, then I think we can
> >>>>> guarantee that there will always be enough padding space?
> >>>>>
> >>>>> Even the last function/section on the .text block would still be
> >>>>> aligned, and as long as the function alignment <= SECTION_ALIGN
> >>>>> there will be enough padding left.  I should add some build time
> >>>>> assert that CONFIG_CC_FUNCTION_ALIGNMENT <= SECTION_ALIGN.
> >>>>
> >>>> I'm not sure of there being a requirement for a section to be padded to
> >>>> its alignment. If the following section has smaller alignment, it could
> >>>> be made start earlier. Of course our linker scripts might guarantee
> >>>> this ...
> >>>
> >>> I do think so, given our linker script arrangements for the .text
> >>> section:
> >>>
> >>> DECL_SECTION(.text) {
> >>>     [...]
> >>> } PHDR(text) = 0x9090
> >>>
> >>> . = ALIGN(SECTION_ALIGN);
> >>>
> >>> The end of the text section is aligned to SECTION_ALIGN, so as long as
> >>> SECTION_ALIGN >= CONFIG_CC_FUNCTION_ALIGNMENT the alignment should
> >>> guarantee a minimal function size.
> >>>
> >>> Do you think it would be clearer if I add the following paragraph:
> >>>
> >>> "Given the Xen linker script arrangement of the .text section, we can
> >>> ensure that when all functions are aligned to the given boundary the
> >>> function size will always be a multiple of such alignment, even for
> >>> the last function in .text, as the linker script aligns the end of the
> >>> section to SECTION_ALIGN."
> >>
> >> I think this would be useful to have there. Beyond that, assembly code
> >> also needs considering btw.
> > 
> > Assembly will get dealt with once we start to also have separate
> > sections for each assembly function.  We cannot patch assembly code at
> > the moment anyway, due to lack of debug symbols.
> 
> Well, yes, that's one part of it. The other is that some .text coming
> from an assembly source may follow one coming from some C source, and
> if the assembly one then isn't properly aligned, padding space again
> wouldn't necessarily be large enough. This may be alright now (where
> .text is the only thing that can come from .S and would be linked
> ahead of all .text.*, being the only thing that can come from .c),

What about adding:

#ifdef CONFIG_CC_SPLIT_SECTIONS
       *(.text.*)
#endif
#ifdef CONFIG_CC_FUNCTION_ALIGNMENT
       /* Ensure enough padding regardless of next section alignment. */
       . = ALIGN(CONFIG_CC_FUNCTION_ALIGNMENT)
#endif

In order to assert that the end of .text.* is also aligned?

> but
> it might subtly when assembly code is also switched to per-function
> sections (you may recall that a patch to this effect is already
> pending: "common: honor CONFIG_CC_SPLIT_SECTIONS also for assembly
> functions").

Yes, I think such patch should also honor the required alignment
specified in CONFIG_CC_FUNCTION_ALIGNMENT.

Thanks, Roger.