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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.12.2023 14:37, Roger Pau Monné wrote:
> 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.

Will it? Before writing the reply I went through gcc's respective doc
section, without finding such a guarantee.

>  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.

Perhaps; this ...

>>> +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

... makes sure the highest alignment ever selected from anywhere will be
used (should an arch need to select any of these).

>>> --- 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.

Oh, sorry, I meant to delete that comment, which really was only the 2nd
half of something I had before actually deciding to try it out (see the
unmotivated ... at the beginning).

>>> --- 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.

Hmm, yes, that's a fair explanation for .fixup not needing special
handling. Yet then I would still be worried of other snippets, e.g.
stuff ending up in e.g. .text.cold or .text.unlikely. Would they all
also be dealt with in similar ways?

Jan