Re: [PATCH v3 1/4] x86/livepatch: align functions to ensure minimal distance between entry points

["Roger Pau Monné" <roger.pau@citrix.com>]

On Thu, Dec 14, 2023 at 12:18:13PM +0100, Jan Beulich wrote:
> On 14.12.2023 11:17, Roger Pau Monne wrote:
> > The minimal function size requirements for livepatch are either 5 bytes (for
> > jmp) or 9 bytes (for endbr + jmp) on x86, and always 4 bytes on Arm.  Ensure
> > that distance between functions entry points is always at least of the minimal
> > required size for livepatch instruction replacement to be successful.
> > 
> > Add an additional align directive to the linker script, in order to ensure that
> > the next section placed after the .text.* (per-function sections) is also
> > aligned to the required boundary, so that the distance of the last function
> > entry point with the next symbol is also of minimal size.
> > 
> > Note that it's possible for the compiler to end up using a higher function
> > alignment regardless of the passed value, so this change just make sure that
> > the minimum required for livepatch to work is present.
> 
> That's a possibility which we don't need to be concerned about. Yet isn't it
> also possible that we override a larger, deemed better (e.g. performance-wise)
> value?

I'm kind of confused, the compiler will always choose the higher
alignment.  For example non-debug builds on my box end up with
function sections aligned to 16 instead of the 8 passed in the
-falign-functions= parameter:

$ clang -MMD -MP -MF arch/x86/.traps.o.d -m64 -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wno-unused-but-set-variable -Wno-unused-local-typedefs   -Werror=unknown-warning-option -O2 -fomit-frame-pointer -falign-functions=8 -nostdinc -fno-builtin -fno-common -Werror -Wredundant-decls -Wwrite-strings -Wno-pointer-arith -Wdeclaration-after-statement -Wvla -pipe -D__XEN__ -include ./include/xen/config.h -ffunction-sections -fdata-sections -mretpoline-external-thunk  -I./include -I./arch/x86/include -I./arch/x86/include/generated -I./arch/x86/include/asm/mach-generic -I./arch/x86/include/asm/mach-default -DXEN_IMG_OFFSET=0x200000 -msoft-float -fno-pie -fno-stack-protector -fno-exceptions -fno-asynchronous-unwind-tables -Wnested-externs -DHAVE_AS_VMX -DHAVE_AS_SSE4_2 -DHAVE_AS_EPT -DHAVE_AS_RDRAND -DHAVE_AS_FSGSBASE -DHAVE_AS_XSAVEOPT -DHAVE_AS_RDSEED -DHAVE_AS_CLAC_STAC -DHAVE_AS_CLWB -DHAVE_AS_QUOTED_SYM -DHAVE_AS_INVPCID -DHAVE_AS_MOVDIR -DHAVE_AS_ENQCMD -mno-red-zone -fpic -mno-mmx -mno-sse -mskip-rax-setup -fcf-protection=none -Wa,-I./include -Wa,-I./include '-D__OBJECT_LABEL__=arch/x86/traps.o'   -DXEN_BUILD_EFI -DBUILD_ID_EFI -c arch/x86/traps.c -o arch/x86/.traps.o.tmp -MQ arch/x86/traps.o

$ readelf -WS xen/arch/x86/traps.o

There are 107 section headers, starting at offset 0xe2e0:

Section Headers:
  [Nr] Name              Type            Addr             Off    Size   ES Flg Lk Inf Al
  [ 0]                   NULL            0000000000000000 000000 000000 00      0   0  0
  [ 1] .text             PROGBITS        0000000000000000 000040 000000 00  AX  0   0  4
  [ 2] .text.show_code   PROGBITS        0000000000000000 000040 000287 00  AX  0   0 16
  [ 3] .rela.text.show_code RELA            0000000000000000 008520 000450 18   I 104   2  8
  [ 4] .altinstructions  PROGBITS        0000000000000000 0002c7 00024c 00   A  0   0  1
  [ 5] .rela.altinstructions RELA            0000000000000000 008970 0007e0 18   I 104   4  8
  [ 6] .discard          PROGBITS        0000000000000000 000513 000054 00   A  0   0  1
  [ 7] .altinstr_replacement PROGBITS        0000000000000000 000567 000018 00  AX  0   0  1
  [ 8] .ex_table         PROGBITS        0000000000000000 000580 000028 00   A  0   0  4
  [ 9] .rela.ex_table    RELA            0000000000000000 009150 0000f0 18   I 104   8  8
  [10] .text.get_stack_trace_bottom PROGBITS        0000000000000000 0005b0 000046 00  AX  0   0 16
  [11] .text.get_stack_dump_bottom PROGBITS        0000000000000000 000600 00003d 00  AX  0   0 16
  [12] .text.show_stack_overflow PROGBITS        0000000000000000 000640 000158 00  AX  0   0 16
[...]

> I'm somewhat concerned of that case. IOW is -falign-functions= really
> the right option to use here? (There may not be one which we would actually
> prefer to use.) Specifically -falign-functions (without a value, i.e. using a
> machine dependent default) is implied by -O2 and -O3, as per 13.2 gcc doc.

Right, and that still works fine AFAICT, see how in the example above
functions ended up aligned to 16 even when -falign-functions=8 was
provided on the command line.

> > --- a/xen/Kconfig
> > +++ b/xen/Kconfig
> > @@ -37,6 +37,24 @@ config CC_HAS_VISIBILITY_ATTRIBUTE
> >  config CC_SPLIT_SECTIONS
> >  	bool
> >  
> > +# Set function alignment.
> > +#
> > +# Allow setting on a boolean basis, and then convert such selection to an
> > +# integer for the build system and code to consume more easily.
> > +config CC_HAS_FUNCTION_ALIGNMENT
> > +	def_bool $(cc-option,-falign-functions=8)
> 
> How does this check make sure 4- or 16-byte alignment are also accepted as
> valid? (Requesting 8-byte alignment would be at least bogus on e.g. IA-64.)

I was confused and expected the compiler to round up to the next
supported value by the arch, but that doesn't seem to be written down
in the GCC manual at least.

One option would be to simply test for -falign-functions with no
specific alignment, and leave Kconfig code to set a suitable value on
a per-arch basis.

> > +config FUNCTION_ALIGNMENT_4B
> > +	bool
> > +config FUNCTION_ALIGNMENT_8B
> > +	bool
> > +config FUNCTION_ALIGNMENT_16B
> > +	bool
> > +config FUNCTION_ALIGNMENT
> > +	int
> > +	default 16 if FUNCTION_ALIGNMENT_16B
> > +	default  8 if  FUNCTION_ALIGNMENT_8B
> > +	default  4 if  FUNCTION_ALIGNMENT_4B
> 
> While for the immediate purpose here no "depends on CC_HAS_FUNCTION_ALIGNMENT"
> is okay, I still wonder if it wouldn't better be added in case further
> "select"s appear.

Will adjust in next version.

> > --- a/xen/Makefile
> > +++ b/xen/Makefile
> > @@ -390,6 +390,9 @@ CFLAGS += -fomit-frame-pointer
> >  endif
> >  
> >  CFLAGS-$(CONFIG_CC_SPLIT_SECTIONS) += -ffunction-sections -fdata-sections
> > +ifdef CONFIG_FUNCTION_ALIGNMENT
> 
> ... e.g. this meaningless? Or is the lack of a default value leading to
> the option remaining undefined (rather than assigning some "default"
> default, e.g. 0)?

If no default value the option remain undefined, and -falign-functions
is not passed on the compiler command line.  This is required in case
the compiler doesn't support -falign-functions.

> > --- a/xen/arch/arm/xen.lds.S
> > +++ b/xen/arch/arm/xen.lds.S
> > @@ -44,6 +44,10 @@ SECTIONS
> >  #ifdef CONFIG_CC_SPLIT_SECTIONS
> >         *(.text.*)
> >  #endif
> > +#ifdef CONFIG_FUNCTION_ALIGNMENT
> > +       /* Ensure enough distance with the next placed section. */
> > +       . = ALIGN(CONFIG_FUNCTION_ALIGNMENT);
> > +#endif
> >  
> >         *(.fixup)
> 
> Seeing .fixup nicely in context - can't that (and other specialized
> sections containing code) also be patched?

The current livepatch-build-tools logic doesn't seem to detect changes
to .fixup, so I've added this to my list of stuff to fix for
livepatch.  I do see the livepatch code in the hypervisor has support
for loading extra .ex_table sections, so I assume at some point it was
considered to add support for .fixup.  My current thinking is that
.fixup itself won't be changed, and that instead a new .fixup will be
loaded, and the newly loaded .ex_table will reference such section.

Thanks, Roger.