Messages in this thread Patch in this message | | | Date | Wed, 16 Dec 2020 14:01:58 -0600 | From | Josh Poimboeuf <> | Subject | Re: New objtool warning.. |
| |
On Tue, Dec 15, 2020 at 09:32:29PM -0800, Linus Torvalds wrote: > > The asm code looks like this: > > > > cmpb $4, %al #, _30 > > jne .L176 #, > > ... > > cmpb $12, %al #, _30 > > jne .L176 #, > > ... > > .L176: > > # drivers/gpu/drm/drm_edid.c:3118: unreachable(); > > #APP > > # 3118 "drivers/gpu/drm/drm_edid.c" 1 > > 320: # > > .pushsection .discard.unreachable > > .long 320b - . # > > .popsection > > # 0 "" 2 > > #NO_APP > > .. this falls through.. > > > > So you *should* find that label that then falls through in that > > ".discard.unreachable" section, and so it should be possible to teach > > objdump about that (insane) unreachable code that way. No?
So this is kind of tricky, because the unreachable() annotation usually means "the previous instruction is a dead end". Most of the time, the next instruction -- the one actually pointed to by the annotation -- is actually reachable from another path.
For example, here's a typical usage of unreachable():
je not_a_bug ud2
.pushsection .discard.unreachable .long not_a_bug - . .popsection
not_a_bug: ... normal non-buggy code ...
The 'not_a_bug' label is pointed to by the unreachable annotation, but it's actually reachable.
In your .o, .discard.unreachable points to 0xbb3, so objtool marks the previous instruction (0xbae) as a dead end:
bae: e9 30 ff ff ff jmpq ae3 <do_cvt_mode+0xd3> bb3: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1) bba: 00 00 00 00 bbe: 66 90 xchg %ax,%ax
And there's another path to 0xbb3 from the switch statement, so objtool assumes it's reachable.
So maybe we need to make objtool's unreachable logic a little more nuanced: If the previous instruction is an unconditional jump, then consider *the annotated instruction itself* to be a dead end.
I'm not quite able to convince myself this wouldn't produce false positives. It did immediately produce one false positive in no_context(), but that should be easily fixable (see patch).
I can run it through more testing, if you don't see any obvious problems with it.
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c index f1f1b5a0956a..c888821bb40c 100644 --- a/arch/x86/mm/fault.c +++ b/arch/x86/mm/fault.c @@ -699,7 +699,6 @@ no_context(struct pt_regs *regs, unsigned long error_code, */ asm volatile ("movq %[stack], %%rsp\n\t" "call handle_stack_overflow\n\t" - "1: jmp 1b" : ASM_CALL_CONSTRAINT : "D" ("kernel stack overflow (page fault)"), "S" (regs), "d" (address), diff --git a/tools/objtool/check.c b/tools/objtool/check.c index c6ab44543c92..267e8b88ca3a 100644 --- a/tools/objtool/check.c +++ b/tools/objtool/check.c @@ -370,9 +370,12 @@ static int add_dead_ends(struct objtool_file *file) return -1; } insn = find_insn(file, reloc->sym->sec, reloc->addend); - if (insn) - insn = list_prev_entry(insn, list); - else if (reloc->addend == reloc->sym->sec->len) { + if (insn) { + struct instruction *prev = list_prev_entry(insn, list); + if (prev->type != INSN_JUMP_UNCONDITIONAL) + insn = prev; + + } else if (reloc->addend == reloc->sym->sec->len) { insn = find_last_insn(file, reloc->sym->sec); if (!insn) { WARN("can't find unreachable insn at %s+0x%x",
| |