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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.01.2024 12:02, Roger Pau Monne wrote:
> The minimal function size requirements for an x86 livepatch are either 5 bytes
> (for jmp) or 9 bytes (for endbr + jmp), 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.  Different compilers
> handle the option differently, as clang will ignore -falign-functions value
> if it's smaller than the one that would be set by the optimization level, 
> while
> gcc seems to always honor the function alignment passed in -falign-functions.
> In order to cope with this behavior and avoid that setting -falign-functions
> results in an alignment inferior to what the optimization level would have
> selected force x86 release builds to use a function alignment of 16 bytes.

Nit: A comma after "selected" may help readability.

> --- a/xen/Kconfig
> +++ b/xen/Kconfig
> @@ -37,6 +37,27 @@ 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.
> +#
> +# Requires clang >= 7.0.0
> +config CC_HAS_FUNCTION_ALIGNMENT
> +     def_bool $(cc-option,-falign-functions)

Nit: Maybe better have a blank line here?

> +config FUNCTION_ALIGNMENT_4B
> +     bool
> +config FUNCTION_ALIGNMENT_8B
> +     bool
> +config FUNCTION_ALIGNMENT_16B
> +     bool
> +config FUNCTION_ALIGNMENT
> +     int
> +     depends on CC_HAS_FUNCTION_ALIGNMENT
> +     default 16 if FUNCTION_ALIGNMENT_16B
> +     default  8 if  FUNCTION_ALIGNMENT_8B
> +     default  4 if  FUNCTION_ALIGNMENT_4B
> +
>  source "arch/$(SRCARCH)/Kconfig"
>  
>  config DEFCONFIG_LIST
>[...]
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -99,6 +99,10 @@ SECTIONS
>         *(.text)
>  #ifdef CONFIG_CC_SPLIT_SECTIONS
>         *(.text.*)
> +#endif
> +#ifdef CONFIG_FUNCTION_ALIGNMENT
> +       /* Ensure enough distance with the next placed section. */
> +       . = ALIGN(CONFIG_FUNCTION_ALIGNMENT);
>  #endif
>         *(.text.__x86_indirect_thunk_*)

I continue to fail to see how an alignment directive can guarantee minimum
distance. In the worst case such a directive inserts nothing at all. IOW
at the very least there's a non-spelled-out assumption here about the last
item in the earlier section having suitable alignment and thus, if small
in size, being suitably padded. Personally I don't think merely spelling
out such a requirement would help - it would end up being a trap for
someone to fall into.

I'm further curious why .text.__x86_indirect_thunk_* is left past the
inserted alignment. While pretty unlikely, isn't it in principle possible
for the thunks there to also need patching? Aren't we instead requiring
then that assembly functions (and thunks) all be suitably aligned as well?

Jan