objtool/urgent for v5.15-rc4

Hi Linus,

please pull a single objtool urgent fix for 5.15.

Thx.

---

The following changes since commit e4e737bb5c170df6135a127739a9e6148ee3da82:

Linux 5.15-rc2 (2021-09-19 17:28:22 -0700)

are available in the Git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git tags/objtool_urgent_for_v5.15_rc4

for you to fetch changes up to 24ff652573754fe4c03213ebd26b17e86842feb3:

objtool: Teach get_alt_entry() about more relocation types (2021-10-01 13:57:47 +0200)

----------------------------------------------------------------
- Handle symbol relocations properly due to changes in the toolchains which remove section symbols now

----------------------------------------------------------------
Peter Zijlstra (1):
objtool: Teach get_alt_entry() about more relocation types

 tools/objtool/special.c | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg

Ugh.

This actually causes a new warning for me:

    arch/x86/kvm/emulate.o: warning: objtool: __ex_table+0x4: don't

know how to handle reloc symbol type: kvm_fastop_exception

on an x86-64 allmodconfig build (and my normal clang build for my actual default config too).

Looking at the kvm code, that kvm_fastop_exception thing is some funky sh*t.

I _think_ the problem is that 'kvm_fastop_exception' is done with bare asm at the top-level and that triggers some odd interaction with other section data, but I really don't know.

Anyway, that thing is in my public tree now, because it's better to get it out and fixed and have the kvm people look at it.

Linus

No, it's the fact that it is marked as a global function (why?) that it then causes problems.

Now, I don't actually see why it would cause problems (the same way I don't see why it's marked global). But removing that

".global kvm_fastop_exception \n"

works.

I suspect it makes the linker do the relocation for us before objtool runs, because now that it's a local name, there is no worry about multiply defined symbols of the same name or anything like that.

I also suspect that the reason for the warning is that the symbol type has never been declared, so it's not marked as a STT_FUNC in the relocation information.

So independently of this kvm_fastop_exception issue, I'd suggest the attached patch for objtool to make the warning more informative for people who try to debug this.

So I have a fix ("remove the global declaration"), but I really don't like how random this is.

I also tried to instead keep the symbol global, and just mark kvm_fastop_exception as a function (and add the proper size annotation), but that only causes more objtool warnings for the (generated asm) functions that *use* that symbol. Because they also don't seem to be properly annotated.

Again, removing the global annotation works around the problem, but the real underlying issue does seem to be that "funky sh*t" going on in arch/x86/kvm/emulate.c.

So I'd like more people to look at this.

In the meantime, I think the exception handling for kvm divide/multiply emulation is badly broken right now. Hmm?

Linus

 tools/objtool/special.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

index f58ecc50fb10..f1428e32a505 100644
@@ -110,8 +110,10 @@ static int get_alt_entry(struct elf *elf, struct special_entry *entry,
 		return -1;
 	}
 	if (!reloc2sec_off(orig_reloc, &alt->orig_sec, &alt->orig_off)) {
-		WARN_FUNC("don't know how to handle reloc symbol type: %s",
-			   sec, offset + entry->orig, orig_reloc->sym->name);
+		WARN_FUNC("don't know how to handle reloc symbol type %d: %s",
+			   sec, offset + entry->orig,
+			   orig_reloc->sym->type,
+			   orig_reloc->sym->name);
 		return -1;
 	}
 

@@ -132,8 +134,10 @@ static int get_alt_entry(struct elf *elf, struct special_entry *entry,
 			return 1;
 
 		if (!reloc2sec_off(new_reloc, &alt->new_sec, &alt->new_off)) {
-			WARN_FUNC("don't know how to handle reloc symbol type: %s",
-				  sec, offset + entry->new, new_reloc->sym->name);
+			WARN_FUNC("don't know how to handle reloc symbol type %d: %s",
+				  sec, offset + entry->new,
+				  new_reloc->sym->type,
+				  new_reloc->sym->name);
 			return -1;
 		}
 

Replying to myself just to add more proper people to the cc.

I'm also wondering how I could possibly be the only person who saw the warning.

I don't think I am, and I think that people who signed off on commit 24ff65257375 ("objtool: Teach get_alt_entry() about more relocation types") and claimed to have "tested" it, clearly didn't actually do so.

PeterZ/Josh/Nathan: see the thread at

https://lore.kernel.org/lkml/CAHk-=wiZwq-0LknKhXN4M+T8jbxn_2i9mcKpO+OaBSSq_Eh7tg@mail.gmail.com/

if you need more context, but I suspect you can figure it out just from this email too.

Linus

Sorry about that. I think Peter and I failed to run this through regression testing. We can work on tightening up our process.

Definitely *not* Nathan's fault. His 'Tested-by' only means that this fixed his particular issue.

Right, basically objtool's complaining that it doesn't know how to handle the NOTYPE symbol. But really, any non-object symbol should be straightforward. I may just remove these warnings altogether in favor of something much simpler (something like the patch below).

The warning is harmless, so it doesn't necessarily mean anything's broken. That said, I have no idea what's going in that code or why kvm_fastop_exception() is clearing %esi.

index f58ecc50fb10..0217ac3fa7ff 100644
@@ -58,22 +58,11 @@ void __weak arch_handle_alternative(unsigned short feature, struct special_alt *
 {
 }
 
-static bool reloc2sec_off(struct reloc *reloc, struct section **sec, unsigned long *off)
+static void reloc_to_sec_off(struct reloc *reloc, struct section **sec,
+			     unsigned long *off)
 {
-	switch (reloc->sym->type) {
-	case STT_FUNC:
-		*sec = reloc->sym->sec;
-		*off = reloc->sym->offset + reloc->addend;
-		return true;
-
-	case STT_SECTION:
-		*sec = reloc->sym->sec;
-		*off = reloc->addend;
-		return true;
-
-	default:
-		return false;
-	}
+	*sec = reloc->sym->sec;
+	*off = reloc->sym->offset + reloc->addend;
 }
 
 static int get_alt_entry(struct elf *elf, struct special_entry *entry,

@@ -109,11 +98,7 @@ static int get_alt_entry(struct elf *elf, struct special_entry *entry,
 		WARN_FUNC("can't find orig reloc", sec, offset + entry->orig);
 		return -1;
 	}
-	if (!reloc2sec_off(orig_reloc, &alt->orig_sec, &alt->orig_off)) {
-		WARN_FUNC("don't know how to handle reloc symbol type: %s",
-			   sec, offset + entry->orig, orig_reloc->sym->name);
-		return -1;
-	}
+	reloc_to_sec_off(orig_reloc, &alt->orig_sec, &alt->orig_off);
 
 	if (!entry->group || alt->new_len) {
 		new_reloc = find_reloc_by_dest(elf, sec, offset + entry->new);

@@ -131,11 +116,7 @@ static int get_alt_entry(struct elf *elf, struct special_entry *entry,
 		if (arch_is_retpoline(new_reloc->sym))
 			return 1;
 
-		if (!reloc2sec_off(new_reloc, &alt->new_sec, &alt->new_off)) {
-			WARN_FUNC("don't know how to handle reloc symbol type: %s",
-				  sec, offset + entry->new, new_reloc->sym->name);
-			return -1;
-		}
+		reloc_to_sec_off(new_reloc, &alt->new_sec, &alt->new_off);
 
 		/* _ASM_EXTABLE_EX hack */
 		if (alt->new_off >= 0x7ffffff0)

It's handled here (which definitely qualifies as "funky sh*t"):

        asm("push %[flags]; popf; " CALL_NOSPEC " ; pushf; pop %[flags]\n"
            : "+a"(ctxt->dst.val), "+d"(ctxt->src.val), [flags]"+D"(flags),
              [thunk_target]"+S"(fop), ASM_CALL_CONSTRAINT
            : "c"(ctxt->src2.val));

        ctxt->eflags = (ctxt->eflags & ~EFLAGS_MASK) | (flags & EFLAGS_MASK);
        if (!fop) /* exception is returned in fop variable */
                return emulate_de(ctxt);

and documented here:

/*
* fastop functions have a special calling convention:
*

 * dst:    rax        (in/out)
 * src:    rdx        (in/out)
 * src2:   rcx        (in)
 * flags:  rflags     (in/out)
 * ex:     rsi        (in:fastop pointer, out:zero if exception)

*
* Moreover, they are all exactly FASTOP_SIZE bytes long, so functions for
* different operand sizes can be reached by calculation, rather than a jump
* table (which would be bigger than the code).
*/

The fastop stuff saves quite a few clock cycles and lines of code, by avoiding complicated emulation of x86 flags. I'll check out the .global annotations, since they are indeed unnecessary.

Paolo