Re: [RFC PATCH 2/2] objtool/powerpc: Enhance objtool to fixup alternate feature relative addresses

Masahiro Yamada
2024-05-06T16:51:18Z

On Mon, Apr 22, 2024 at 6:25 PM Sathvika Vasireddy <sv@linux.ibm.com> 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@gmail.com/
>
> Co-developed-by: Nicholas Piggin <npiggin@gmail.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> Signed-off-by: Sathvika Vasireddy <sv@linux.ibm.com>
> ---
> 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