On Mon, Apr 22, 2024 at 6:25 PM Sathvika Vasireddy <sv@xxxxxxxxxxxxx> wrote: > > Implement build-time fixup of alternate feature relative addresses for > the out-of-line (else) patch code. Initial posting to achieve the same > using another tool can be found at [1]. Idea is to implement this using > objtool instead of introducing another tool since it already has elf > parsing and processing covered. > > Introduce --ftr-fixup as an option to objtool to do feature fixup at > build-time. > > Couple of issues and warnings encountered while implementing feature > fixup using objtool are as follows: > > 1. libelf is creating corrupted vmlinux file after writing necessary > changes to the file. Due to this, kexec is not able to load new > kernel. > > It gives the following error: > ELF Note corrupted ! > Cannot determine the file type of vmlinux > > To fix this issue, after opening vmlinux file, make a call to > elf_flagelf (e, ELF_C_SET, ELF_F_LAYOUT). This instructs libelf not > to touch the segment and section layout. It informs the library > that the application will take responsibility for the layout of the > file and that the library should not insert any padding between > sections. > > 2. Fix can't find starting instruction warnings when run on vmlinux > > Objtool throws a lot of can't find starting instruction warnings > when run on vmlinux with --ftr-fixup option. > > These warnings are seen because find_insn() function looks for > instructions at offsets that are relative to the start of the section. > In case of individual object files (.o), there are no can't find > starting instruction warnings seen because the actual offset > associated with an instruction is itself a relative offset since the > sections start at offset 0x0. > > However, in case of vmlinux, find_insn() function fails to find > instructions at the actual offset associated with an instruction > since the sections in vmlinux do not start at offset 0x0. Due to > this, find_insn() will look for absolute offset and not the relative > offset. This is resulting in a lot of can't find starting instruction > warnings when objtool is run on vmlinux. > > To fix this, pass offset that is relative to the start of the section > to find_insn(). > > find_insn() is also looking for symbols of size 0. But, objtool does > not store empty STT_NOTYPE symbols in the rbtree. Due to this, > for empty symbols, objtool is throwing can't find starting > instruction warnings. Fix this by ignoring symbols that are of > size 0 since objtool does not add them to the rbtree. > > 3. Objtool is throwing unannotated intra-function call warnings > when run on vmlinux with --ftr-fixup option. > > One such example: > > vmlinux: warning: objtool: .text+0x3d94: > unannotated intra-function call > > .text + 0x3d94 = c000000000008000 + 3d94 = c0000000000081d4 > > c0000000000081d4: 45 24 02 48 bl c00000000002a618 > <system_reset_exception+0x8> > > c00000000002a610 <system_reset_exception>: > c00000000002a610: 0e 01 4c 3c addis r2,r12,270 > c00000000002a610: R_PPC64_REL16_HA .TOC. > c00000000002a614: f0 6c 42 38 addi r2,r2,27888 > c00000000002a614: R_PPC64_REL16_LO .TOC.+0x4 > c00000000002a618: a6 02 08 7c mflr r0 > > This is happening because we should be looking for destination > symbols that are at absolute offsets instead of relative offsets. > After fixing dest_off to point to absolute offset, there are still > a lot of these warnings shown. > > In the above example, objtool is computing the destination > offset to be c00000000002a618, which points to a completely > different instruction. find_call_destination() is looking for this > offset and failing. Instead, we should be looking for destination > offset c00000000002a610 which points to system_reset_exception > function. > > Even after fixing the way destination offset is computed, and > after looking for dest_off - 0x8 in cases where the original offset > is not found, there are still a lot of unannotated intra-function > call warnings generated. This is due to symbols that are not > properly annotated. > > So, for now, as a hack to curb these warnings, do not emit > unannotated intra-function call warnings when objtool is run > with --ftr-fixup option. > > TODO: > This patch enables build time feature fixup only for powerpc little > endian configs. There are boot failures with big endian configs. > Posting this as an initial RFC to get some review comments while I work > on big endian issues. > > [1] > https://lore.kernel.org/linuxppc-dev/20170521010130.13552-1-npiggin@xxxxxxxxx/ > > Co-developed-by: Nicholas Piggin <npiggin@xxxxxxxxx> > Signed-off-by: Nicholas Piggin <npiggin@xxxxxxxxx> > Signed-off-by: Sathvika Vasireddy <sv@xxxxxxxxxxxxx> > --- > arch/Kconfig | 3 + > arch/powerpc/Kconfig | 5 + > arch/powerpc/Makefile | 5 + > arch/powerpc/include/asm/feature-fixups.h | 11 +- > arch/powerpc/kernel/vmlinux.lds.S | 14 +- > arch/powerpc/lib/feature-fixups.c | 13 + > scripts/Makefile.lib | 7 + > scripts/Makefile.vmlinux | 15 +- > tools/objtool/arch/powerpc/special.c | 329 ++++++++++++++++++++++ > tools/objtool/arch/x86/special.c | 49 ++++ > tools/objtool/builtin-check.c | 2 + > tools/objtool/check.c | 38 ++- > tools/objtool/elf.c | 4 + > tools/objtool/include/objtool/builtin.h | 1 + > tools/objtool/include/objtool/special.h | 43 +++ > 15 files changed, 530 insertions(+), 9 deletions(-) > > diff --git a/arch/Kconfig b/arch/Kconfig > index 9f066785bb71..8defdf86a69e 100644 > --- a/arch/Kconfig > +++ b/arch/Kconfig > @@ -1206,6 +1206,9 @@ config HAVE_UACCESS_VALIDATION > bool > select OBJTOOL > > +config HAVE_OBJTOOL_FTR_FIXUP > + bool > + > config HAVE_STACK_VALIDATION > bool > help > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig > index 1c4be3373686..806285a28231 100644 > --- a/arch/powerpc/Kconfig > +++ b/arch/powerpc/Kconfig > @@ -23,6 +23,11 @@ config 64BIT > bool > default y if PPC64 > > +config HAVE_OBJTOOL_FTR_FIXUP > + bool > + default y if CPU_LITTLE_ENDIAN && PPC64 > + select OBJTOOL > + > config LIVEPATCH_64 > def_bool PPC64 > depends on LIVEPATCH > diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile > index 65261cbe5bfd..bc81847d5c3d 100644 > --- a/arch/powerpc/Makefile > +++ b/arch/powerpc/Makefile > @@ -112,6 +112,11 @@ LDFLAGS_vmlinux-$(CONFIG_RELOCATABLE) := -pie > LDFLAGS_vmlinux-$(CONFIG_RELOCATABLE) += -z notext > LDFLAGS_vmlinux := $(LDFLAGS_vmlinux-y) > > +# --emit-relocs required for post-link fixup of alternate feature > +# text section relocations. > +LDFLAGS_vmlinux += --emit-relocs > +KBUILD_LDFLAGS_MODULE += --emit-relocs > + > ifdef CONFIG_PPC64 > ifndef CONFIG_PPC_KERNEL_PCREL > ifeq ($(call cc-option-yn,-mcmodel=medium),y) > diff --git a/arch/powerpc/include/asm/feature-fixups.h b/arch/powerpc/include/asm/feature-fixups.h > index 77824bd289a3..006e2493c7c3 100644 > --- a/arch/powerpc/include/asm/feature-fixups.h > +++ b/arch/powerpc/include/asm/feature-fixups.h > @@ -30,12 +30,19 @@ > > #define START_FTR_SECTION(label) label##1: > > +#ifdef CONFIG_CPU_LITTLE_ENDIAN > #define FTR_SECTION_ELSE_NESTED(label) \ > label##2: \ > - .pushsection __ftr_alt_##label,"a"; \ > + .pushsection __ftr_alt_##label, "ax"; \ > .align 2; \ > label##3: > - > +#else > +#define FTR_SECTION_ELSE_NESTED(label) \ > +label##2 : \ > + .pushsection __ftr_alt_##label, "a"; \ > + .align 2; \ > +label##3 : > +#endif > > #ifndef CONFIG_CC_IS_CLANG > #define CHECK_ALT_SIZE(else_size, body_size) \ > diff --git a/arch/powerpc/kernel/vmlinux.lds.S b/arch/powerpc/kernel/vmlinux.lds.S > index f420df7888a7..6b1c61e8af47 100644 > --- a/arch/powerpc/kernel/vmlinux.lds.S > +++ b/arch/powerpc/kernel/vmlinux.lds.S > @@ -105,8 +105,13 @@ SECTIONS > .text : AT(ADDR(.text) - LOAD_OFFSET) { > ALIGN_FUNCTION(); > #endif > - /* careful! __ftr_alt_* sections need to be close to .text */ > - *(.text.hot .text.hot.* TEXT_MAIN .text.fixup .text.unlikely .text.unlikely.* .fixup __ftr_alt_* .ref.text); > +#ifdef CONFIG_CPU_LITTLE_ENDIAN > + *(.text.hot .text.hot.* TEXT_MAIN .text.fixup .text.unlikely > + .text.unlikely.* .fixup .ref.text); > +#else > + *(.text.hot .text.hot.* TEXT_MAIN .text.fixup .text.unlikely > + .text.unlikely.* .fixup __ftr_alt_* .ref.text); > +#endif > *(.tramp.ftrace.text); > NOINSTR_TEXT > SCHED_TEXT > @@ -276,6 +281,11 @@ SECTIONS > _einittext = .; > *(.tramp.ftrace.init); > } :text > +#ifdef CONFIG_CPU_LITTLE_ENDIAN > + .__ftr_alternates.text : AT(ADDR(.__ftr_alternates.text) - LOAD_OFFSET) { > + *(__ftr_alt*); > + } > +#endif > > /* .exit.text is discarded at runtime, not link time, > * to deal with references from __bug_table > diff --git a/arch/powerpc/lib/feature-fixups.c b/arch/powerpc/lib/feature-fixups.c > index 4f82581ca203..8c5eb7c8612f 100644 > --- a/arch/powerpc/lib/feature-fixups.c > +++ b/arch/powerpc/lib/feature-fixups.c > @@ -44,6 +44,18 @@ static u32 *calc_addr(struct fixup_entry *fcur, long offset) > return (u32 *)((unsigned long)fcur + offset); > } > > +#ifdef CONFIG_CPU_LITTLE_ENDIAN > +static int patch_alt_instruction(u32 *src, u32 *dest, u32 *alt_start, u32 *alt_end) > +{ > + ppc_inst_t instr; > + > + instr = ppc_inst_read(src); > + > + raw_patch_instruction(dest, instr); > + > + return 0; > +} > +#else > static int patch_alt_instruction(u32 *src, u32 *dest, u32 *alt_start, u32 *alt_end) > { > int err; > @@ -66,6 +78,7 @@ static int patch_alt_instruction(u32 *src, u32 *dest, u32 *alt_start, u32 *alt_e > > return 0; > } > +#endif > > static int patch_feature_section_mask(unsigned long value, unsigned long mask, > struct fixup_entry *fcur) > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib > index c65bb0fbd136..8fff27b9bdcb 100644 > --- a/scripts/Makefile.lib > +++ b/scripts/Makefile.lib > @@ -290,6 +290,13 @@ ifneq ($(objtool-args-y),) > cmd_objtool = $(if $(objtool-enabled), ; $(objtool) $(objtool-args) $@) > endif > > +cmd_objtool_vmlinux := > +ifeq ($(CONFIG_HAVE_OBJTOOL_FTR_FIXUP),y) > +cmd_objtool_vmlinux = $(if $(objtool-enabled), ; $(objtool) $(objtool-args) $@) > +vmlinux: > + $(cmd_objtool_vmlinux) This is complete garbage. At first, I thought it was a build rule for vmlinux, but it is not because $(cmd_objtool_vmlinux) is indented by 4 spaces, not a tab. Of course, you cannot add a vmlinux build rule here. If it had been a tab instead of 4 spaces, Make would have shown a warning. > +endif > + > cmd_gen_objtooldep = $(if $(objtool-enabled), { echo ; echo '$@: $$(wildcard $(objtool))' ; } >> $(dot-target).cmd) > > endif # CONFIG_OBJTOOL -- Best Regards Masahiro Yamada
