| Message ID | 20240207145547.89689-4-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen: introduce Kconfig function alignment option | expand |
On 07.02.2024 15:55, 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 scripts, 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. Perhaps "... minimal required size"? > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -395,8 +395,11 @@ config CRYPTO > config LIVEPATCH > bool "Live patching support" > default X86 > - depends on "$(XEN_HAS_BUILD_ID)" = "y" > + depends on "$(XEN_HAS_BUILD_ID)" = "y" && CC_HAS_FUNCTION_ALIGNMENT > select CC_SPLIT_SECTIONS > + select FUNCTION_ALIGNMENT_16B if XEN_IBT > + select FUNCTION_ALIGNMENT_8B if X86 > + select FUNCTION_ALIGNMENT_4B if ARM This isn't strictly needed, is it? Would be nice to avoid re-selection of what the default for an arch is anyway, as otherwise this will start looking clumsy when a couple more architectures are added. Preferably with that dropped (or it being clarified why it's still desirable to have): Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On Tue, Feb 13, 2024 at 04:58:38PM +0100, Jan Beulich wrote: > On 07.02.2024 15:55, 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 scripts, 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. > > Perhaps "... minimal required size"? Yes. > > --- a/xen/common/Kconfig > > +++ b/xen/common/Kconfig > > @@ -395,8 +395,11 @@ config CRYPTO > > config LIVEPATCH > > bool "Live patching support" > > default X86 > > - depends on "$(XEN_HAS_BUILD_ID)" = "y" > > + depends on "$(XEN_HAS_BUILD_ID)" = "y" && CC_HAS_FUNCTION_ALIGNMENT > > select CC_SPLIT_SECTIONS > > + select FUNCTION_ALIGNMENT_16B if XEN_IBT > > + select FUNCTION_ALIGNMENT_8B if X86 > > + select FUNCTION_ALIGNMENT_4B if ARM > > This isn't strictly needed, is it? Would be nice to avoid re-selection > of what the default for an arch is anyway, as otherwise this will start > looking clumsy when a couple more architectures are added. My worry was that the default per-arch could change, ie: for example x86 moving from 16 to 8 and then it would hamper livepatch support if IBT is also enabled. I however think it's very unlikely to reduce the default alignment, and in any case we would hit a build time assert if that ever happens. So yes, I'm fine with dropping those. > Preferably > with that dropped (or it being clarified why it's still desirable to > have): > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks, Roger.
On 26.02.2024 12:32, Roger Pau Monné wrote: > On Tue, Feb 13, 2024 at 04:58:38PM +0100, Jan Beulich wrote: >> On 07.02.2024 15:55, 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 scripts, 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. >> >> Perhaps "... minimal required size"? > > Yes. > >>> --- a/xen/common/Kconfig >>> +++ b/xen/common/Kconfig >>> @@ -395,8 +395,11 @@ config CRYPTO >>> config LIVEPATCH >>> bool "Live patching support" >>> default X86 >>> - depends on "$(XEN_HAS_BUILD_ID)" = "y" >>> + depends on "$(XEN_HAS_BUILD_ID)" = "y" && CC_HAS_FUNCTION_ALIGNMENT >>> select CC_SPLIT_SECTIONS >>> + select FUNCTION_ALIGNMENT_16B if XEN_IBT >>> + select FUNCTION_ALIGNMENT_8B if X86 >>> + select FUNCTION_ALIGNMENT_4B if ARM >> >> This isn't strictly needed, is it? Would be nice to avoid re-selection >> of what the default for an arch is anyway, as otherwise this will start >> looking clumsy when a couple more architectures are added. > > My worry was that the default per-arch could change, ie: for example > x86 moving from 16 to 8 and then it would hamper livepatch support if > IBT is also enabled. I however think it's very unlikely to reduce the > default alignment, and in any case we would hit a build time assert if > that ever happens. > > So yes, I'm fine with dropping those. Oh, no - not "those", only "that", i.e. only the last (Arm) one. Jan
On Mon, Feb 26, 2024 at 01:36:32PM +0100, Jan Beulich wrote: > On 26.02.2024 12:32, Roger Pau Monné wrote: > > On Tue, Feb 13, 2024 at 04:58:38PM +0100, Jan Beulich wrote: > >> On 07.02.2024 15:55, 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 scripts, 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. > >> > >> Perhaps "... minimal required size"? > > > > Yes. > > > >>> --- a/xen/common/Kconfig > >>> +++ b/xen/common/Kconfig > >>> @@ -395,8 +395,11 @@ config CRYPTO > >>> config LIVEPATCH > >>> bool "Live patching support" > >>> default X86 > >>> - depends on "$(XEN_HAS_BUILD_ID)" = "y" > >>> + depends on "$(XEN_HAS_BUILD_ID)" = "y" && CC_HAS_FUNCTION_ALIGNMENT > >>> select CC_SPLIT_SECTIONS > >>> + select FUNCTION_ALIGNMENT_16B if XEN_IBT > >>> + select FUNCTION_ALIGNMENT_8B if X86 > >>> + select FUNCTION_ALIGNMENT_4B if ARM > >> > >> This isn't strictly needed, is it? Would be nice to avoid re-selection > >> of what the default for an arch is anyway, as otherwise this will start > >> looking clumsy when a couple more architectures are added. > > > > My worry was that the default per-arch could change, ie: for example > > x86 moving from 16 to 8 and then it would hamper livepatch support if > > IBT is also enabled. I however think it's very unlikely to reduce the > > default alignment, and in any case we would hit a build time assert if > > that ever happens. > > > > So yes, I'm fine with dropping those. > > Oh, no - not "those", only "that", i.e. only the last (Arm) one. Oh, I see what you mean, even x86 selects the default one when IBT is enabled, and when not the requirement for livepatch is < than the default anyway. That's why I said that we could even drop all of them and just rely on the build time assert to catch any changes here. Feel free to drop the ARM one. Thanks, Roger.
On 27.02.2024 09:15, Roger Pau Monné wrote: > On Mon, Feb 26, 2024 at 01:36:32PM +0100, Jan Beulich wrote: >> On 26.02.2024 12:32, Roger Pau Monné wrote: >>> On Tue, Feb 13, 2024 at 04:58:38PM +0100, Jan Beulich wrote: >>>> On 07.02.2024 15:55, 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 scripts, 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. >>>> >>>> Perhaps "... minimal required size"? >>> >>> Yes. >>> >>>>> --- a/xen/common/Kconfig >>>>> +++ b/xen/common/Kconfig >>>>> @@ -395,8 +395,11 @@ config CRYPTO >>>>> config LIVEPATCH >>>>> bool "Live patching support" >>>>> default X86 >>>>> - depends on "$(XEN_HAS_BUILD_ID)" = "y" >>>>> + depends on "$(XEN_HAS_BUILD_ID)" = "y" && CC_HAS_FUNCTION_ALIGNMENT >>>>> select CC_SPLIT_SECTIONS >>>>> + select FUNCTION_ALIGNMENT_16B if XEN_IBT >>>>> + select FUNCTION_ALIGNMENT_8B if X86 >>>>> + select FUNCTION_ALIGNMENT_4B if ARM >>>> >>>> This isn't strictly needed, is it? Would be nice to avoid re-selection >>>> of what the default for an arch is anyway, as otherwise this will start >>>> looking clumsy when a couple more architectures are added. >>> >>> My worry was that the default per-arch could change, ie: for example >>> x86 moving from 16 to 8 and then it would hamper livepatch support if >>> IBT is also enabled. I however think it's very unlikely to reduce the >>> default alignment, and in any case we would hit a build time assert if >>> that ever happens. >>> >>> So yes, I'm fine with dropping those. >> >> Oh, no - not "those", only "that", i.e. only the last (Arm) one. > > Oh, I see what you mean, even x86 selects the default one when IBT is > enabled, and when not the requirement for livepatch is < than the > default anyway. That's why I said that we could even drop all of them > and just rely on the build time assert to catch any changes here. Just to clarify: The default I mean is the architecture imposed one. Leaving aside Thumb mode, Arm instructions are all 32-bit words, and hence less than 4-byte alignment makes no sense (and may even be disallowed by the architecture). Whereas for x86 what you're talking about is just a compiler default, which isn't really guaranteed to never be lower (with -Os for example I'd expect it to be perhaps as low as 1). Jan > Feel free to drop the ARM one. > > Thanks, Roger.
On Tue, Feb 27, 2024 at 09:53:24AM +0100, Jan Beulich wrote: > On 27.02.2024 09:15, Roger Pau Monné wrote: > > On Mon, Feb 26, 2024 at 01:36:32PM +0100, Jan Beulich wrote: > >> On 26.02.2024 12:32, Roger Pau Monné wrote: > >>> On Tue, Feb 13, 2024 at 04:58:38PM +0100, Jan Beulich wrote: > >>>> On 07.02.2024 15:55, 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 scripts, 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. > >>>> > >>>> Perhaps "... minimal required size"? > >>> > >>> Yes. > >>> > >>>>> --- a/xen/common/Kconfig > >>>>> +++ b/xen/common/Kconfig > >>>>> @@ -395,8 +395,11 @@ config CRYPTO > >>>>> config LIVEPATCH > >>>>> bool "Live patching support" > >>>>> default X86 > >>>>> - depends on "$(XEN_HAS_BUILD_ID)" = "y" > >>>>> + depends on "$(XEN_HAS_BUILD_ID)" = "y" && CC_HAS_FUNCTION_ALIGNMENT > >>>>> select CC_SPLIT_SECTIONS > >>>>> + select FUNCTION_ALIGNMENT_16B if XEN_IBT > >>>>> + select FUNCTION_ALIGNMENT_8B if X86 > >>>>> + select FUNCTION_ALIGNMENT_4B if ARM > >>>> > >>>> This isn't strictly needed, is it? Would be nice to avoid re-selection > >>>> of what the default for an arch is anyway, as otherwise this will start > >>>> looking clumsy when a couple more architectures are added. > >>> > >>> My worry was that the default per-arch could change, ie: for example > >>> x86 moving from 16 to 8 and then it would hamper livepatch support if > >>> IBT is also enabled. I however think it's very unlikely to reduce the > >>> default alignment, and in any case we would hit a build time assert if > >>> that ever happens. > >>> > >>> So yes, I'm fine with dropping those. > >> > >> Oh, no - not "those", only "that", i.e. only the last (Arm) one. > > > > Oh, I see what you mean, even x86 selects the default one when IBT is > > enabled, and when not the requirement for livepatch is < than the > > default anyway. That's why I said that we could even drop all of them > > and just rely on the build time assert to catch any changes here. > > Just to clarify: The default I mean is the architecture imposed one. > Leaving aside Thumb mode, Arm instructions are all 32-bit words, and > hence less than 4-byte alignment makes no sense (and may even be > disallowed by the architecture). Whereas for x86 what you're talking > about is just a compiler default, which isn't really guaranteed to > never be lower (with -Os for example I'd expect it to be perhaps as > low as 1). Right, it's a compiler default, but in patch 1 we already set the default alignment for x86 to be 16 bytes. When in your first comment you mentioned "... default for an arch is anyway" I assumed you mean the default in the arch Kconfig file, not what the ISA mandates. Thanks, Roger.
diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c index bbca1e5a5ed3..aa8ae8c38d28 100644 --- a/xen/arch/arm/livepatch.c +++ b/xen/arch/arm/livepatch.c @@ -68,6 +68,8 @@ void arch_livepatch_revive(void) int arch_livepatch_verify_func(const struct livepatch_func *func) { + BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE > CONFIG_FUNCTION_ALIGNMENT); + /* If NOPing only do up to maximum amount we can put in the ->opaque. */ if ( !func->new_addr && (func->new_size > LIVEPATCH_OPAQUE_SIZE || func->new_size % ARCH_PATCH_INSN_SIZE) ) diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S index 470c8f22084f..0126a3afae53 100644 --- a/xen/arch/arm/xen.lds.S +++ b/xen/arch/arm/xen.lds.S @@ -46,6 +46,10 @@ SECTIONS #ifdef CONFIG_CC_SPLIT_SECTIONS *(.text.*) #endif +#ifdef CONFIG_LIVEPATCH + /* Ensure enough distance with the next placed section. */ + . = ALIGN(CONFIG_FUNCTION_ALIGNMENT); +#endif *(.fixup) *(.gnu.warning) diff --git a/xen/arch/ppc/xen.lds.S b/xen/arch/ppc/xen.lds.S index 030e1ee37b55..f300c03a666f 100644 --- a/xen/arch/ppc/xen.lds.S +++ b/xen/arch/ppc/xen.lds.S @@ -32,6 +32,10 @@ SECTIONS #ifdef CONFIG_CC_SPLIT_SECTIONS *(.text.*) #endif +#ifdef CONFIG_LIVEPATCH + /* Ensure enough distance with the next placed section. */ + . = ALIGN(CONFIG_FUNCTION_ALIGNMENT); +#endif *(.fixup) *(.gnu.warning) diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S index 8510a87c4d06..1fb9af11c1cc 100644 --- a/xen/arch/riscv/xen.lds.S +++ b/xen/arch/riscv/xen.lds.S @@ -27,6 +27,10 @@ SECTIONS #ifdef CONFIG_CC_SPLIT_SECTIONS *(.text.*) #endif +#ifdef CONFIG_LIVEPATCH + /* Ensure enough distance with the next placed section. */ + . = ALIGN(CONFIG_FUNCTION_ALIGNMENT); +#endif . = ALIGN(IDENT_AREA_SIZE); _ident_start = .; diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c index ee539f001b73..b00ad7120da9 100644 --- a/xen/arch/x86/livepatch.c +++ b/xen/arch/x86/livepatch.c @@ -109,6 +109,10 @@ int arch_livepatch_verify_func(const struct livepatch_func *func) */ uint8_t needed = ARCH_PATCH_INSN_SIZE; + BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE + + (IS_ENABLED(CONIFG_XEN_IBT) ? ENDBR64_LEN : 0) > + CONFIG_FUNCTION_ALIGNMENT); + if ( is_endbr64(func->old_addr) || is_endbr64_poison(func->old_addr) ) needed += ENDBR64_LEN; diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S index 8930e14fc40e..6649bc16dc48 100644 --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -101,6 +101,10 @@ SECTIONS *(.text.*) #endif *(.text.__x86_indirect_thunk_*) +#ifdef CONFIG_LIVEPATCH + /* Ensure enough distance with the next placed section. */ + . = ALIGN(CONFIG_FUNCTION_ALIGNMENT); +#endif *(.fixup) *(.gnu.warning) diff --git a/xen/common/Kconfig b/xen/common/Kconfig index 310ad4229cdf..63fba175c99f 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -395,8 +395,11 @@ config CRYPTO config LIVEPATCH bool "Live patching support" default X86 - depends on "$(XEN_HAS_BUILD_ID)" = "y" + depends on "$(XEN_HAS_BUILD_ID)" = "y" && CC_HAS_FUNCTION_ALIGNMENT select CC_SPLIT_SECTIONS + select FUNCTION_ALIGNMENT_16B if XEN_IBT + select FUNCTION_ALIGNMENT_8B if X86 + select FUNCTION_ALIGNMENT_4B if ARM ---help--- Allows a running Xen hypervisor to be dynamically patched using binary patches without rebooting. This is primarily used to binarily
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 scripts, 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 livepatch-build-tools will take into account each per-function section alignment in order to decide whether there's enough padding. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v5: - Split chunks into pre-patches to make review easier. Changes since v4: - Split from the rest of the livepatch testing series. - Reword and expand a bit the commit message. - Add a comment about falign-functions clang version requirement. Changes since v3: - Test for compiler option with -falign-functions. - Make FUNCTION_ALIGNMENT depend on CC_HAS_FUNCTION_ALIGNMENT. - Set 16byte function alignment for x86 release builds. Changes since v2: - Add Arm side. - Align end of section in the linker script to ensure enough padding for the last function. - Expand commit message and subject. - Rework Kconfig options. - Check that the compiler supports the option. Changes since v1: - New in this version. --- xen/arch/arm/livepatch.c | 2 ++ xen/arch/arm/xen.lds.S | 4 ++++ xen/arch/ppc/xen.lds.S | 4 ++++ xen/arch/riscv/xen.lds.S | 4 ++++ xen/arch/x86/livepatch.c | 4 ++++ xen/arch/x86/xen.lds.S | 4 ++++ xen/common/Kconfig | 5 ++++- 7 files changed, 26 insertions(+), 1 deletion(-)