Linux-PCI Archive on lore.kernel.org
 help / color / Atom feed
From: Josh Poimboeuf <jpoimboe@redhat.com>
To: Sami Tolvanen <samitolvanen@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
	Jann Horn <jannh@google.com>,
	the arch/x86 maintainers <x86@kernel.org>,
	Masahiro Yamada <masahiroy@kernel.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	Will Deacon <will@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"Paul E. McKenney" <paulmck@kernel.org>,
	Kees Cook <keescook@chromium.org>,
	Nick Desaulniers <ndesaulniers@google.com>,
	clang-built-linux <clang-built-linux@googlegroups.com>,
	Kernel Hardening <kernel-hardening@lists.openwall.com>,
	linux-arch <linux-arch@vger.kernel.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	linux-kbuild <linux-kbuild@vger.kernel.org>,
	kernel list <linux-kernel@vger.kernel.org>,
	linux-pci@vger.kernel.org
Subject: Re: [PATCH v6 22/25] x86/asm: annotate indirect jumps
Date: Mon, 9 Nov 2020 20:29:24 -0600
Message-ID: <20201110022924.tekltjo25wtrao7z@treble> (raw)
In-Reply-To: <CABCJKuee7hUQSiksdRMYNNx05bW7pWaDm4fQ__znGQ99z9-dEw@mail.gmail.com>

On Mon, Nov 09, 2020 at 03:11:41PM -0800, Sami Tolvanen wrote:
> On Fri, Oct 23, 2020 at 10:36 AM Sami Tolvanen <samitolvanen@google.com> wrote:
> >
> > On Wed, Oct 21, 2020 at 05:22:59PM -0700, Sami Tolvanen wrote:
> > > There are a couple of differences, like the first "undefined stack
> > > state" warning pointing to set_bringup_idt_handler.constprop.0()
> > > instead of __switch_to_asm(). I tried running this with --backtrace,
> > > but objtool segfaults at the first .entry.text warning:
> >
> > Looks like it segfaults when calling BT_FUNC() for an instruction that
> > doesn't have a section (?). Applying this patch allows objtool to finish
> > with --backtrace:
> >
> > diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> > index c216dd4d662c..618b0c4f2890 100644
> > --- a/tools/objtool/check.c
> > +++ b/tools/objtool/check.c
> > @@ -2604,7 +2604,7 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
> >                                 ret = validate_branch(file, func,
> >                                                       insn->jump_dest, state);
> >                                 if (ret) {
> > -                                       if (backtrace)
> > +                                       if (backtrace && insn->sec)
> >                                                 BT_FUNC("(branch)", insn);
> >                                         return ret;
> >                                 }
> >
> >
> > Running objtool -barfld on an allyesconfig+LTO vmlinux.o prints out the
> > following, ignoring the crypto warnings for now:
> 
> OK, I spent some time looking at these warnings and the configs needed
> to reproduce them without building allyesconfig:
> 
> CONFIG_XEN
> 
> __switch_to_asm()+0x0: undefined stack state
>   xen_hypercall_set_trap_table()+0x0: <=== (sym)
> 
> CONFIG_XEN_PV
> 
> .entry.text+0xffd: sibling call from callable instruction with
> modified stack frame
>   .entry.text+0xfcb: (branch)
>   .entry.text+0xfb5: (alt)
>   .entry.text+0xfb0: (alt)
>   .entry.text+0xf78: (branch)
>   .entry.text+0x9c: (branch)
>   xen_syscall_target()+0x15: (branch)
>   xen_syscall_target()+0x0: <=== (sym)
> .entry.text+0x1754: unsupported instruction in callable function
>   .entry.text+0x171d: (branch)
>   .entry.text+0x1707: (alt)
>   .entry.text+0x1701: (alt)
>   xen_syscall32_target()+0x15: (branch)
>   xen_syscall32_target()+0x0: <=== (sym)
> .entry.text+0x1634: redundant CLD
> 
> Backtrace doesn’t print out anything useful for the “redundant CLD”
> error, but it occurs when validate_branch is looking at
> xen_sysenter_target.
> 
> do_suspend_lowlevel()+0x116: sibling call from callable instruction
> with modified stack frame
>   do_suspend_lowlevel()+0x9a: (branch)
>   do_suspend_lowlevel()+0x0: <=== (sym)
> 
> .entry.text+0x48: stack state mismatch: cfa1=7-8 cfa2=-1+0
>   .altinstr_replacement+0xffffffffffffffff: (branch)
>   .entry.text+0x21: (alt)
>   .entry.text+0x1c: (alt)
>   .entry.text+0x10: <=== (hint)
> .entry.text+0x15fd: stack state mismatch: cfa1=7-8 cfa2=-1+0
>   .altinstr_replacement+0xffffffffffffffff: (branch)
>   .entry.text+0x15dc: (alt)
>   .entry.text+0x15d7: (alt)
>   .entry.text+0x15d0: <=== (hint)
> .entry.text+0x168c: stack state mismatch: cfa1=7-8 cfa2=-1+0
>   .altinstr_replacement+0xffffffffffffffff: (branch)
>   .entry.text+0x166b: (alt)
>   .entry.text+0x1666: (alt)
>   .entry.text+0x1660: <=== (hint)

I can't make much sense of most of these warnings.  Disassembly would
help.

(Also, something like the patch below should help objtool show more
symbol names.)

> It looks like the stack state mismatch warnings can be fixed by adding
> unwind hints also to entry_SYSCALL_64_after_hwframe,
> entry_SYSENTER_compat_after_hwframe, and
> entry_SYSCALL_compat_after_hwframe. Does that sound correct?

No, those code paths should already have the hints they need, unless I'm
missing something.

> CONFIG_AMD_MEM_ENCRYPT
> 
> .head.text+0xfb: unsupported instruction in callable function
>   .head.text+0x207: (branch)
>   sev_es_play_dead()+0xff: (branch)
>   sev_es_play_dead()+0xd2: (branch)
>   sev_es_play_dead()+0xa8: (alt)
>   sev_es_play_dead()+0x144: (branch)
>   sev_es_play_dead()+0x10b: (branch)
>   sev_es_play_dead()+0x1f: (branch)
>   sev_es_play_dead()+0x0: <=== (sym)
> 
> This happens because sev_es_play_dead calls start_cpu0. It always has,
> but objtool hasn’t been able to follow the call when processing only
> sev-es.o. Any thoughts on the preferred way to fix this one?

Objtool isn't supposed to traverse through call instructions like that.
Is LTO inlining the call or something?

> CONFIG_CRYPTO_CRC32C_INTEL
> 
> __x86_retpoline_rdi()+0x10: return with modified stack frame
>   __x86_retpoline_rdi()+0x0: (branch)
>   .altinstr_replacement+0x147: (branch)
>   .text+0xaf4c7: (alt)
>   .text+0xb03b0: (branch)
>   .text+0xaf482: (branch)
>   crc_pcl()+0x10: (branch)
>   crc_pcl()+0x0: <=== (sym)
> 
> __x86_retpoline_rdi()+0x0: stack state mismatch: cfa1=7+32 cfa2=7+8
>   .altinstr_replacement+0x265: (branch)
>   __x86_indirect_thunk_rdi()+0x0: (alt)
>   __x86_indirect_thunk_rdi()+0x0: <=== (sym)
> 
> This is different from the warnings in the rest of the arch/x86/crypto
> code. Do we need some kind of a hint before the JMP_NOSPEC in crc_pcl?

I'll need to look more into that one.

> CONFIG_FUNCTION_TRACER
> 
> __x86_retpoline_rdi()+0x0: stack state mismatch: cfa1=7+32 cfa2=-1+0
>   .altinstr_replacement+0x111: (branch)
>   .text+0x28a5: (alt)
>   .text+0x2880: <=== (hint)
> 
> This unwind hint is in return_to_handler. Removing it obviously stops
> the warning and doesn’t seem to result in any other complaints from
> objtool. Is this hint correct?

The hint is supposed to be there.  I don't understand this one either.
Did it inline the call to ftrace_return_to_handler()?

> The remaining warnings are all “unsupported stack pointer realignment”
> issues in the crypto code and can be reproduced with the following
> configs:
> 
> CONFIG_CRYPTO_AES_NI_INTEL
> CONFIG_CRYPTO_CAMELLIA_AESNI_AVX2_X86_64
> CONFIG_CRYPTO_SHA1_SSSE3
> CONFIG_CRYPTO_SHA256_SSSE3
> CONFIG_CRYPTO_SHA512_SSSE3
> 
> Josh, have you had a chance to look at the crypto patches you mentioned earlier?

I've been traveling for several weeks, but now my work schedule is
getting more normal, so I'll hopefully be able to spend time on this.

How would I recreate all these warnings?

Is it

  https://github.com/samitolvanen/linux.git lto-v6

plus a certain version of clang?

Also, any details on how to build clang would be appreciated, it's been
a while since I tried.


Here's the patch for hopefully making the warnings more helpful:


diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index c6ab44543c92..e5f5cb107664 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2432,6 +2432,9 @@ static int validate_branch(struct objtool_file *file, struct symbol *func,
 	sec = insn->sec;
 
 	while (1) {
+
+		if (insn->offset == 0x48)
+			WARN_FUNC("yo", sec, insn->offset);
 		next_insn = next_insn_same_sec(file, insn);
 
 		if (file->c_file && func && insn->func && func != insn->func->pfunc) {
diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index 4e1d7460574b..ced7e4754cba 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -217,6 +217,21 @@ struct symbol *find_func_containing(struct section *sec, unsigned long offset)
 	return NULL;
 }
 
+struct symbol *find_symbol_preceding(struct section *sec, unsigned long offset)
+{
+	struct symbol *sym;
+
+	/*
+	 * This is slow, but used for warning messages.
+	 */
+	while (1) {
+		sym = find_symbol_by_offset(sec, offset);
+		if (sym || !offset)
+			return sym;
+		offset--;
+	}
+}
+
 struct symbol *find_symbol_by_name(const struct elf *elf, const char *name)
 {
 	struct symbol *sym;
diff --git a/tools/objtool/elf.h b/tools/objtool/elf.h
index 807f8c670097..841902ed381e 100644
--- a/tools/objtool/elf.h
+++ b/tools/objtool/elf.h
@@ -136,10 +136,11 @@ struct symbol *find_func_by_offset(struct section *sec, unsigned long offset);
 struct symbol *find_symbol_by_offset(struct section *sec, unsigned long offset);
 struct symbol *find_symbol_by_name(const struct elf *elf, const char *name);
 struct symbol *find_symbol_containing(const struct section *sec, unsigned long offset);
+struct symbol *find_func_containing(struct section *sec, unsigned long offset);
+struct symbol *find_symbol_preceding(struct section *sec, unsigned long offset);
 struct reloc *find_reloc_by_dest(const struct elf *elf, struct section *sec, unsigned long offset);
 struct reloc *find_reloc_by_dest_range(const struct elf *elf, struct section *sec,
 				     unsigned long offset, unsigned int len);
-struct symbol *find_func_containing(struct section *sec, unsigned long offset);
 int elf_rebuild_reloc_section(struct elf *elf, struct section *sec);
 
 #define for_each_sec(file, sec)						\
diff --git a/tools/objtool/warn.h b/tools/objtool/warn.h
index 7799f60de80a..33da0f2ed9d5 100644
--- a/tools/objtool/warn.h
+++ b/tools/objtool/warn.h
@@ -22,6 +22,8 @@ static inline char *offstr(struct section *sec, unsigned long offset)
 	unsigned long name_off;
 
 	func = find_func_containing(sec, offset);
+	if (!func)
+		func = find_symbol_preceding(sec, offset);
 	if (func) {
 		name = func->name;
 		name_off = offset - func->offset;


  reply index

Thread overview: 73+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-13  0:31 [PATCH v6 00/25] Add support for Clang LTO Sami Tolvanen
2020-10-13  0:31 ` [PATCH v6 01/25] kbuild: preprocess module linker script Sami Tolvanen
2020-10-13  0:31 ` [PATCH v6 02/25] objtool: Add a pass for generating __mcount_loc Sami Tolvanen
2020-10-14 16:50   ` Ingo Molnar
2020-10-14 18:21     ` Peter Zijlstra
2020-10-15 20:10       ` Josh Poimboeuf
2020-10-13  0:31 ` [PATCH v6 03/25] objtool: Don't autodetect vmlinux.o Sami Tolvanen
2020-10-13  0:31 ` [PATCH v6 04/25] tracing: move function tracer options to Kconfig Sami Tolvanen
2020-10-13  0:31 ` [PATCH v6 05/25] tracing: add support for objtool mcount Sami Tolvanen
2020-10-13  0:31 ` [PATCH v6 06/25] x86, build: use " Sami Tolvanen
2020-10-13  0:31 ` [PATCH v6 07/25] treewide: remove DISABLE_LTO Sami Tolvanen
2020-10-14 22:43   ` Kees Cook
2020-10-17  1:46     ` Masahiro Yamada
2020-10-13  0:31 ` [PATCH v6 08/25] kbuild: add support for Clang LTO Sami Tolvanen
2020-10-13  0:31 ` [PATCH v6 09/25] kbuild: lto: fix module versioning Sami Tolvanen
2020-10-13  0:31 ` [PATCH v6 10/25] objtool: Split noinstr validation from --vmlinux Sami Tolvanen
2020-10-13  0:31 ` [PATCH v6 11/25] kbuild: lto: postpone objtool Sami Tolvanen
2020-10-13  0:31 ` [PATCH v6 12/25] kbuild: lto: limit inlining Sami Tolvanen
2020-10-13  0:31 ` [PATCH v6 13/25] kbuild: lto: merge module sections Sami Tolvanen
2020-10-14 22:49   ` Kees Cook
2020-10-20 16:42     ` Sami Tolvanen
2020-10-13  0:31 ` [PATCH v6 14/25] kbuild: lto: remove duplicate dependencies from .mod files Sami Tolvanen
2020-10-14 22:50   ` Kees Cook
2020-12-03 17:59     ` Masahiro Yamada
2020-12-03 18:47       ` Sami Tolvanen
2020-10-13  0:31 ` [PATCH v6 15/25] init: lto: ensure initcall ordering Sami Tolvanen
2020-10-13  0:31 ` [PATCH v6 16/25] init: lto: fix PREL32 relocations Sami Tolvanen
2020-10-14 22:53   ` Kees Cook
2020-10-15  0:12   ` Jann Horn
2020-10-13  0:31 ` [PATCH v6 17/25] PCI: Fix PREL32 relocations for LTO Sami Tolvanen
2020-10-14 22:58   ` Kees Cook
2020-10-13  0:31 ` [PATCH v6 18/25] modpost: lto: strip .lto from module names Sami Tolvanen
2020-10-13  0:31 ` [PATCH v6 19/25] scripts/mod: disable LTO for empty.c Sami Tolvanen
2020-10-13  0:31 ` [PATCH v6 20/25] efi/libstub: disable LTO Sami Tolvanen
2020-10-13  0:31 ` [PATCH v6 21/25] drivers/misc/lkdtm: disable LTO for rodata.o Sami Tolvanen
2020-10-13  0:32 ` [PATCH v6 22/25] x86/asm: annotate indirect jumps Sami Tolvanen
2020-10-14 22:46   ` Kees Cook
2020-10-14 23:23   ` Jann Horn
2020-10-15 10:22     ` Peter Zijlstra
2020-10-15 20:39       ` Josh Poimboeuf
2020-10-20 16:45         ` Sami Tolvanen
2020-10-20 18:52           ` Josh Poimboeuf
2020-10-20 19:24             ` Sami Tolvanen
2020-10-21  8:56               ` Peter Zijlstra
2020-10-21  9:08                 ` Peter Zijlstra
2020-10-21  9:32                 ` Peter Zijlstra
2020-10-21 21:27                   ` Josh Poimboeuf
2020-10-22  7:25                     ` Peter Zijlstra
2020-10-23 17:48                       ` Sami Tolvanen
2020-10-23 18:04                         ` Nick Desaulniers
2020-10-21 15:01                 ` Sami Tolvanen
2020-10-22  0:22                 ` Sami Tolvanen
2020-10-23 17:36                   ` Sami Tolvanen
2020-11-09 23:11                     ` Sami Tolvanen
2020-11-10  2:29                       ` Josh Poimboeuf [this message]
2020-11-10  3:18                         ` Nick Desaulniers
2020-11-10  4:48                         ` Sami Tolvanen
2020-11-10 16:11                           ` Josh Poimboeuf
2020-11-10 17:46                         ` Josh Poimboeuf
2020-11-10 18:59                           ` Sami Tolvanen
2020-11-13 19:54                             ` Josh Poimboeuf
2020-11-13 20:24                               ` Sami Tolvanen
2020-11-13 20:52                                 ` Josh Poimboeuf
2020-11-13 22:34                                 ` Josh Poimboeuf
2020-11-13 22:54                                   ` Sami Tolvanen
2020-11-13 22:56                                     ` Josh Poimboeuf
2020-11-13 23:31                                   ` Sami Tolvanen
2020-11-14  0:49                                     ` Josh Poimboeuf
2020-10-21  9:51             ` Peter Zijlstra
2020-10-21 18:30               ` Josh Poimboeuf
2020-10-13  0:32 ` [PATCH v6 23/25] x86, vdso: disable LTO only for vDSO Sami Tolvanen
2020-10-13  0:32 ` [PATCH v6 24/25] x86, cpu: disable LTO for cpu.c Sami Tolvanen
2020-10-13  0:32 ` [PATCH v6 25/25] x86, build: allow LTO_CLANG and THINLTO to be selected Sami Tolvanen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201110022924.tekltjo25wtrao7z@treble \
    --to=jpoimboe@redhat.com \
    --cc=clang-built-linux@googlegroups.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=jannh@google.com \
    --cc=keescook@chromium.org \
    --cc=kernel-hardening@lists.openwall.com \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kbuild@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=masahiroy@kernel.org \
    --cc=ndesaulniers@google.com \
    --cc=paulmck@kernel.org \
    --cc=peterz@infradead.org \
    --cc=rostedt@goodmis.org \
    --cc=samitolvanen@google.com \
    --cc=will@kernel.org \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-PCI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pci/0 linux-pci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pci linux-pci/ https://lore.kernel.org/linux-pci \
		linux-pci@vger.kernel.org
	public-inbox-index linux-pci

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pci


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git