Re: New objtool warning..

From: Josh Poimboeuf
Date: Wed Dec 16 2020 - 15:03:49 EST


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",