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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux&nblp;USB Development]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite Secrets]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux