Messages in this thread Patch in this message | | | Date | Sun, 12 Jan 2020 01:42:58 -0700 | From | Nathan Chancellor <> | Subject | Re: [RFC v5 00/57] objtool: Add support for arm64 |
| |
On Thu, Jan 09, 2020 at 04:02:03PM +0000, Julien Thierry wrote: > Hi, > > This patch series is the continuation of Raphael's work [1]. All the > patches can be retrieved from: > git clone -b arm64-objtool-v5 https://github.com/julien-thierry/linux.git > > There still are some outstanding issues but the series is starting to > get pretty big so it is probably good to start having some discussions > on the current state of things. > > It felt necessary to split some of the patches (especially the arm64 > decoder). In order to give Raphael credit for his work I used the > "Suggested-by" tag. If this is not the right way to give credit or if > it should be present on more patches do let me know. > > There still are some shortcomings. On defconfig here are the remaining > warnings: > * arch/arm64/crypto/crct10dif-ce-core.o: warning: objtool: crc_t10dif_pmull_p8()+0xf0: unsupported intra-function call > * arch/arm64/kernel/cpu_errata.o: warning: objtool: qcom_link_stack_sanitization()+0x4: unsupported intra-function call > Objtool currently does not support bl from a procedure to itself. This > is also an issue with retpolines. I need to investigate more to figure > out whether something can be done for this or if this file should not be > validated by objtool. > > * arch/arm64/kernel/efi-entry.o: warning: objtool: entry()+0xb0: sibling call from callable instruction with modified stack frame > The EFI entry jumps to code mapped by EFI. Objtool cannot know statically where the code flow is going. > > * arch/arm64/kernel/entry.o: warning: objtool: .entry.tramp.text+0x404: unsupported intra-function call > Need to figure out what is needed to handle aarch64 trampolines. x86 > explicitly annotates theirs with ANNOTATE_NOSPEC_ALTERNATIVE and > patching them as alternatives. > > * arch/arm64/kernel/head.o: warning: objtool: .head.text+0x58: can't find jump dest instruction at .head.text+0x80884 > This is actually a constant that turns out to be a valid branch opcode. > A possible solution could be to introduce a marco that explicitly > annotates constants placed in code sections. > > * arch/arm64/kernel/hibernate-asm.o: warning: objtool: el1_sync()+0x4: unsupported instruction in callable function > Symbols el<x>_* shouldn't be considered as callable functions. Should we > use SYM_CODE_END instead of PROC_END? > > * arch/arm64/kvm/hyp/hyp-entry.o: warning: objtool: .hyp.text: empty alternative at end of section > This is due to the arm64 alternative_cb. Currently, the feature > corresponding to the alternative_cb is defined as the current number of > features supported by the kernel, meaning the identifier is not fixed > accross kernel versions. This makes it a bit hard to detect these > alternative_cb for external tools. > > Would it be acceptable to set a fixed identifier for alternative_cb? > (probably 0xFF so it is always higher than the number of real features) > > * drivers/ata/libata-scsi.o: warning: objtool: ata_sas_queuecmd() falls through to next function ata_scsi_scan_host() > This is due to a limitation in the switch table metadata interpretation. > The compiler might create a table of unsigned offsets and then > compute the final offset as follows: > > ldrb offset_reg, [<offset_table>, <offset_idx>, uxtw] > adr base_reg, <base_addr> > add res_addr, base_reg, offset_reg, sxtb #2 > > Effectively using the loaded offset as a signed value. > I don't have a simple way to solve this at the moment, I'd like to > avoid decoding the instructions to check which ones might sign extend > the loaded offset. > > * kernel/bpf/core.o: warning: objtool: ___bpf_prog_run()+0x44: sibling call from callable instruction with modified stack frame > This is because the function uses a C jump table which differ from > basic jump tables. Also, the code generated for C jump tables on arm64 > does not follow the same form as the one for x86. So the existing x86 objtool > code handling C jump tables can't be used. > > I'll focus on understanding the arm64 pattern so objtool can handle them. > > > In the mean time, any feedback on the current state is appreciated. > > * Patches 1 to 18 adapts the current objtool code to make it easier to > support new architectures. > * Patches 19 to 45 add the support for arm64 architecture to objtool. > * Patches 46 to 57 fix warnings reported by objtool on the existing > arm64 code. > > Changes since RFCv4[1]: > * Rebase on v5.5-rc5 > * Misc cleanup/bug fixes > * Fix some new objtool warnings reported on arm64 objects > * Make ORC subcommand optional since arm64 does not currently support it > * Support branch instructions in alternative sections when they jump > within the same set of alternative instructions > * Replace the "extra" stack_op with a list of stack_op > * Split the decoder into multiple patches to ease review > * Mark constants generated by load literal instructions as bytes that > should not be reached by execution flow > * Rework the switch table handling > > [1] https://lkml.org/lkml/2019/8/16/400 > > Thanks, > > Julien > > --> > > Julien Thierry (43): > objtool: check: Remove redundant checks on operand type > objtool: check: Clean instruction state before each function > validation > objtool: check: Use arch specific values in restore_reg() > objtool: check: Ignore empty alternative groups > objtool: Give ORC functions consistent name > objtool: Make ORC support optional > objtool: Split generic and arch specific CFI definitions > objtool: Abstract alternative special case handling > objtool: check: Allow jumps from an alternative group to itself > objtool: Do not look for STT_NOTYPE symbols > objtool: Support addition to set frame pointer > objtool: Support restoring BP from the stack without POP > objtool: Make stack validation more generic > objtool: Support multiple stack_op per instruction > objtool: arm64: Decode unknown instructions > objtool: arm64: Decode simple data processing instructions > objtool: arm64: Decode add/sub immediate instructions > objtool: arm64: Decode logical data processing instructions > objtool: arm64: Decode system instructions not affecting the flow > objtool: arm64: Decode calls to higher EL > objtool: arm64: Decode brk instruction > objtool: arm64: Decode instruction triggering context switch > objtool: arm64: Decode branch instructions with PC relative immediates > objtool: arm64: Decode branch to register instruction > objtool: arm64: Decode basic load/stores > objtool: arm64: Decode load/store with register offset > objtool: arm64: Decode load/store register pair instructions > objtool: arm64: Decode FP/SIMD load/store instructions > objtool: arm64: Decode load/store exclusive > objtool: arm64: Decode atomic load/store > objtool: arm64: Decode pointer auth load instructions > objtool: arm64: Decode load acquire/store release > objtool: arm64: Decode load/store with memory tag > objtool: arm64: Decode load literal > objtool: arm64: Decode register data processing instructions > objtool: arm64: Decode FP/SIMD data processing instructions > objtool: arm64: Decode SVE instructions > objtool: arm64: Implement functions to add switch tables alternatives > arm64: Generate no-ops to pad executable section > arm64: Move constant to rodata > arm64: Mark sigreturn32.o as containing non standard code > arm64: entry: Avoid empty alternatives entries > arm64: crypto: Remove redundant branch > > Raphael Gault (14): > objtool: Add abstraction for computation of symbols offsets > objtool: orc: Refactor ORC API for other architectures to implement. > objtool: Move registers and control flow to arch-dependent code > objtool: Refactor switch-tables code to support other architectures > objtool: arm64: Add required implementation for supporting the aarch64 > architecture in objtool. > gcc-plugins: objtool: Add plugin to detect switch table on arm64 > objtool: arm64: Enable stack validation for arm64 > arm64: alternative: Mark .altinstr_replacement as containing > executable instructions > arm64: assembler: Add macro to annotate asm function having non > standard stack-frame. > arm64: sleep: Prevent stack frame warnings from objtool > arm64: kvm: Annotate non-standard stack frame functions > arm64: kernel: Add exception on kuser32 to prevent stack analysis > arm64: crypto: Add exceptions for crypto object to prevent stack > analysis > arm64: kernel: Annotate non-standard stack frame functions > > arch/arm64/Kconfig | 2 + > arch/arm64/crypto/Makefile | 3 + > arch/arm64/crypto/sha1-ce-core.S | 3 +- > arch/arm64/crypto/sha2-ce-core.S | 3 +- > arch/arm64/crypto/sha3-ce-core.S | 3 +- > arch/arm64/crypto/sha512-ce-core.S | 3 +- > arch/arm64/include/asm/alternative.h | 2 +- > arch/arm64/kernel/Makefile | 4 + > arch/arm64/kernel/entry.S | 4 +- > arch/arm64/kernel/hyp-stub.S | 3 + > arch/arm64/kernel/relocate_kernel.S | 5 + > arch/arm64/kernel/sleep.S | 5 + > arch/arm64/kvm/hyp-init.S | 3 + > arch/arm64/kvm/hyp/entry.S | 3 + > include/linux/frame.h | 19 +- > scripts/Makefile.gcc-plugins | 2 + > scripts/gcc-plugins/Kconfig | 4 + > .../arm64_switch_table_detection_plugin.c | 94 + > tools/objtool/Build | 4 +- > tools/objtool/Makefile | 13 +- > tools/objtool/arch.h | 14 +- > tools/objtool/arch/arm64/Build | 5 + > tools/objtool/arch/arm64/arch_special.c | 262 ++ > tools/objtool/arch/arm64/bit_operations.c | 69 + > tools/objtool/arch/arm64/decode.c | 2866 +++++++++++++++++ > .../objtool/arch/arm64/include/arch_special.h | 23 + > .../arch/arm64/include/bit_operations.h | 31 + > tools/objtool/arch/arm64/include/cfi_regs.h | 44 + > .../objtool/arch/arm64/include/insn_decode.h | 206 ++ > tools/objtool/arch/x86/Build | 3 + > tools/objtool/arch/x86/arch_special.c | 182 ++ > tools/objtool/arch/x86/decode.c | 29 +- > tools/objtool/arch/x86/include/arch_special.h | 28 + > tools/objtool/arch/x86/include/cfi_regs.h | 25 + > tools/objtool/{ => arch/x86}/orc_dump.c | 4 +- > tools/objtool/{ => arch/x86}/orc_gen.c | 114 +- > tools/objtool/cfi.h | 21 +- > tools/objtool/check.c | 461 +-- > tools/objtool/check.h | 13 +- > tools/objtool/elf.c | 3 +- > tools/objtool/objtool.c | 2 + > tools/objtool/orc.h | 38 +- > tools/objtool/special.c | 44 +- > tools/objtool/special.h | 13 + > 44 files changed, 4282 insertions(+), 400 deletions(-) > create mode 100644 scripts/gcc-plugins/arm64_switch_table_detection_plugin.c > create mode 100644 tools/objtool/arch/arm64/Build > create mode 100644 tools/objtool/arch/arm64/arch_special.c > create mode 100644 tools/objtool/arch/arm64/bit_operations.c > create mode 100644 tools/objtool/arch/arm64/decode.c > create mode 100644 tools/objtool/arch/arm64/include/arch_special.h > create mode 100644 tools/objtool/arch/arm64/include/bit_operations.h > create mode 100644 tools/objtool/arch/arm64/include/cfi_regs.h > create mode 100644 tools/objtool/arch/arm64/include/insn_decode.h > create mode 100644 tools/objtool/arch/x86/arch_special.c > create mode 100644 tools/objtool/arch/x86/include/arch_special.h > create mode 100644 tools/objtool/arch/x86/include/cfi_regs.h > rename tools/objtool/{ => arch/x86}/orc_dump.c (98%) > rename tools/objtool/{ => arch/x86}/orc_gen.c (62%) > > -- > 2.21.0 >
Hi Julien,
The 0day bot reported a couple of issues with clang with this series; the full report is available here (clang reports are only sent to our mailing lists for manual triage for the time being):
https://groups.google.com/d/msg/clang-built-linux/MJbl_xPxawg/mWjgDgZgBwAJ
The first obvious issue is that this series appears to depend on a GCC plugin? I'll be quite honest, objtool and everything it does is rather over my head but I see this warning during configuration (allyesconfig):
WARNING: unmet direct dependencies detected for GCC_PLUGIN_SWITCH_TABLES Depends on [n]: GCC_PLUGINS [=n] && ARM64 [=y] Selected by [y]: - ARM64 [=y] && STACK_VALIDATION [=y]
Followed by the actual error:
error: unable to load plugin './scripts/gcc-plugins/arm64_switch_table_detection_plugin.so': './scripts/gcc-plugins/arm64_switch_table_detection_plugin.so: cannot open shared object file: No such file or directory'
If this plugin is absolutely necessary and can't be implemented in another way so that clang can be used, seems like STACK_VALIDATION should only be selected on ARM64 when CONFIG_CC_IS_GCC is not zero.
The second issue I see is the -Wenum-conversion warnings; they are pretty trivial to fix (see commit e7e83dd3ff1d ("objtool: Fix Clang enum conversion warning") upstream and the below diff).
Would you mind addressing these in a v6 if you happen to do one?
Cheers, Nathan
diff --git a/tools/objtool/arch/arm64/decode.c b/tools/objtool/arch/arm64/decode.c index 5a5f82b5cb81..1ed6bf0c85ce 100644 --- a/tools/objtool/arch/arm64/decode.c +++ b/tools/objtool/arch/arm64/decode.c @@ -1518,7 +1518,7 @@ int arm_decode_ld_st_regs_unsc_imm(u32 instr, enum insn_type *type, op->dest.type = OP_DEST_REG_INDIRECT; op->dest.reg = rn; op->dest.offset = SIGN_EXTEND(imm9, 9); - op->src.type = OP_DEST_REG; + op->src.type = OP_SRC_REG; op->src.reg = rt; op->src.offset = 0; break; @@ -1605,7 +1605,7 @@ int arm_decode_ld_st_regs_unsigned(u32 instr, enum insn_type *type, op->dest.type = OP_DEST_REG_INDIRECT; op->dest.reg = rn; op->dest.offset = imm12; - op->src.type = OP_DEST_REG; + op->src.type = OP_SRC_REG; op->src.reg = rt; op->src.offset = 0; } @@ -1772,7 +1772,7 @@ int arm_decode_ld_st_imm_unpriv(u32 instr, enum insn_type *type, op->dest.type = OP_DEST_REG_INDIRECT; op->dest.reg = rn; op->dest.offset = SIGN_EXTEND(imm9, 9); - op->src.type = OP_DEST_REG; + op->src.type = OP_SRC_REG; op->src.reg = rt; op->src.offset = 0; break; @@ -1852,7 +1852,7 @@ int arm_decode_atomic(u32 instr, enum insn_type *type, list_add_tail(&op->list, ops_list); op->src.reg = rn; - op->src.type = OP_DEST_REG_INDIRECT; + op->src.type = OP_SRC_REG_INDIRECT; op->src.offset = 0; op->dest.type = OP_DEST_REG; op->dest.reg = rt; @@ -2187,7 +2187,7 @@ int arm_decode_ldapr_stlr_unsc_imm(u32 instr, enum insn_type *type, break; default: /* store */ - op->dest.type = OP_SRC_REG_INDIRECT; + op->dest.type = OP_DEST_REG_INDIRECT; op->dest.reg = rn; op->dest.offset = SIGN_EXTEND(imm9, 9); op->src.type = OP_SRC_REG;
| |