[GIT PULL] objtool/urgent for v5.15-rc4

[GIT PULL] objtool/urgent for v5.15-rc4

[Borislav Petkov]

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

Re: [GIT PULL] objtool/urgent for v5.15-rc4

[pr-tracker-bot]

The pull request you sent on Sun, 3 Oct 2021 11:43:33 +0200:

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

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/52c3c170623d994c468c1ee9cc36c56bbd6d6e56

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

Re: [GIT PULL] objtool/urgent for v5.15-rc4

[Linus Torvalds]

On Sun, Oct 3, 2021 at 2:43 AM Borislav Petkov <bp@suse.de> wrote:
>
> - Handle symbol relocations properly due to changes in the toolchains
> which remove section symbols now

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

Re: [GIT PULL] objtool/urgent for v5.15-rc4

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1887 bytes --]

On Sun, Oct 3, 2021 at 11:38 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> 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.

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

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 1211 bytes --]

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

diff --git a/tools/objtool/special.c b/tools/objtool/special.c
index f58ecc50fb10..f1428e32a505 100644
--- a/tools/objtool/special.c
+++ b/tools/objtool/special.c
@@ -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;
 		}
 

Re: [GIT PULL] objtool/urgent for v5.15-rc4

[Linus Torvalds]

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

On Sun, Oct 3, 2021 at 12:02 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Sun, Oct 3, 2021 at 11:38 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > 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.
>
> 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

Re: [GIT PULL] objtool/urgent for v5.15-rc4

[Josh Poimboeuf]

On Sun, Oct 03, 2021 at 12:10:38PM -0700, Linus Torvalds wrote:
> 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.

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.

> > > 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.
> >
> > 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.

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).

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

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.


diff --git a/tools/objtool/special.c b/tools/objtool/special.c
index f58ecc50fb10..0217ac3fa7ff 100644
--- a/tools/objtool/special.c
+++ b/tools/objtool/special.c
@@ -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)

Re: [GIT PULL] objtool/urgent for v5.15-rc4

[Paolo Bonzini]

On 04/10/21 01:02, Josh Poimboeuf wrote:
> That said, I have no idea what's going in that code or why
> kvm_fastop_exception() is clearing %esi.

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