| Message ID | 20231214101719.18770-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/x86: add testing for self modifying code and livepatch | expand |
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 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. > --- 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.) > +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. > --- 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)? > --- 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? Jan
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.
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
On Thu, Dec 14, 2023 at 02:53:13PM +0100, Jan Beulich wrote: > 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. Hm, yes, checked with godbolt now and GCC behaves the opposite of clang, and will always attempt to honor the alignment passed in falign-functions. Maybe for release builds we should select a 16b alignment on x86? Arm seems to use 4 byte alignment by default when using -O2. > > 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). That's the intention, that the highest selected FUNCTION_ALIGNMENT_X ends up defining the value of FUNCTION_ALIGNMENT. > >>> --- 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? Yes, livepatch-build-tools requires functions to be in separate sections in order to be able to patch them. Contents of .text.cold or .text.unlikely won't be detected by the patch generation scripts as far as I'm aware (like it doesn't detect changes to .fixup currently). There might be ways to create patches for those manually by building a custom livepatch payload, but in that case the function size is left to the user to calculate. Thanks, Roger.
On 14.12.2023 16:20, Roger Pau Monné wrote: > On Thu, Dec 14, 2023 at 02:53:13PM +0100, Jan Beulich wrote: >> 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. > > Hm, yes, checked with godbolt now and GCC behaves the opposite of > clang, and will always attempt to honor the alignment passed in > falign-functions. > > Maybe for release builds we should select a 16b alignment on x86? Might make sense, yes. Iirc there was a time where 16-byte alignment was the default for functions in gcc. Jan
diff --git a/xen/Kconfig b/xen/Kconfig index 134e6e68ad84..7a61e130771c 100644 --- 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) +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 + source "arch/$(SRCARCH)/Kconfig" config DEFCONFIG_LIST diff --git a/xen/Makefile b/xen/Makefile index 21832d640225..162cb2bda1c5 100644 --- 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 +CFLAGS += -falign-functions=$(CONFIG_FUNCTION_ALIGNMENT) +endif CFLAGS += -nostdinc -fno-builtin -fno-common CFLAGS += -Werror -Wredundant-decls -Wwrite-strings -Wno-pointer-arith 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 59b80d122fd0..afaf1e996b0e 100644 --- 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) *(.gnu.warning) 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..5b3332300d44 100644 --- 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_*) diff --git a/xen/common/Kconfig b/xen/common/Kconfig index 310ad4229cdf..c9a21c3c8a07 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 ---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 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. The compiler option -falign-functions is not available on at least clang 3.8, so introduce a Kconfig check for it and make the livepatch option depend on the compiler supporting the option. The naming of the option(s) CONFIG_FUNCTION_ALIGNMENT is explicitly not mentioning CC in preparation for the option also being used by assembly code. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- 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/Kconfig | 18 ++++++++++++++++++ xen/Makefile | 3 +++ xen/arch/arm/livepatch.c | 2 ++ xen/arch/arm/xen.lds.S | 4 ++++ xen/arch/x86/livepatch.c | 4 ++++ xen/arch/x86/xen.lds.S | 4 ++++ xen/common/Kconfig | 5 ++++- 7 files changed, 39 insertions(+), 1 deletion(-)