Message-ID: <alpine.DEB.2.21.1903270820530.1789@nanos.tec.linutronix.de>
Date:   Wed, 27 Mar 2019 15:20:08 +0100 (CET)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Andi Kleen <andi@...stfloor.org>
cc:     Andi Kleen <ak@...ux.intel.com>, x86@...nel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>
Subject: Re: [PATCH 02/17] x86, lto: Mark all top level asm statements as
 .text

Andi,

On Tue, 26 Mar 2019, Andi Kleen wrote:
> > Well, we better should know the real reason for this wreckage. I mean, the
> > default section for text is suprisingly .text. I don't see a reason why
> > this would be any different for an assembly function implemented in a C
> > file.
> 
> What happens is that when the function before the asm changes
> the section, gcc only changes it back for the next function/variable
> with a different section. But it doesn't change it back for the asm.
> 
> 
> e.g. here
> 
> __attribute__((section("foo"))) void func(void)
> {
> }
> 
> asm("foo:\n");
> 
> gives with gcc -S (might be different with optimization):
> 
>  .section foo,"ax",@progbits <----------------- sets the section
>  .globl func
>  .type func, @function

SNIP

> .LFE0:
>  .size func, .-func
> <--------------------------- no section reset before the asm
> #APP
>  foo:
> 
>  .ident "GCC: (GNU) 8.3.1 20190223 (Red Hat 8.3.1-2)"
>  .section .note.GNU-stack,"",@progbits

Makes sense, but comes as a surprise when the thing is actually marked as a
function.

> But gcc reorders functions even without LTO inside files, so it could
> eventually happen.

Adding

+void __init foo(void)
+{
+       pr_info("foo\n");
+}

right before the kretprobe_trampoline and compiling it with GCC 6.

So one would assume that kretprobe_trampoline now ends up in
.init.text. But it ends up in the .text section because it's reordered and
ends up at the top of .text.

So clearly stuff gets reordered and those top level ASM constructs which
lack a section are just working by chance and we need the annotations and
backport them.

We also need a way to detect such wreckage automatically. This can happen
again and as the GCC behaviour is random there is no guarantee that it's
noticed right away. Josh, can objtool help here or do we need some other
form of checking that?

So independent of the LTO issue, this information needs to be in the
changelog.

For the patch itself. The kprobes/vide/error-inject parts are fine because
these are clearly functions: ".type $NAME, @function\n".

But this hunk not so much:

--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -372,7 +372,7 @@ extern struct paravirt_patch_template pv_ops;
 
>  #define DEF_NATIVE(ops, name, code)					\
> 	__visible extern const char start_##ops##_##name[], end_##ops##_##name[];	\
> -	asm(NATIVE_LABEL("start_", ops, name) code NATIVE_LABEL("end_", ops, name))
> +	asm(".text\n\t" NATIVE_LABEL("start_", ops, name) code NATIVE_LABEL("end_", ops, name))

Because it is NOT text.

That 'code' is never executed in place. It's a patch table, which is used
by the alternative code to patch in the native instructions so the pv_ops
indirection can be avoided on bare metal. It's only copied into a buffer
nothing else. So blindly slapping '.text' on it is just wrong.

But that's not the only thing which is wrong here. DEF_NATIVE is only used
in paravirt_patch_32/64.c and the resulting labels are not used outside of
this either. So why are these labels global and the c declaration __visible
extern?

global was already in the original paravirt code and should have never been
there in the first place.

But __visible? That was added via:

  commit 9a55fdbe941e ("x86, asmlinkage, paravirt: Add __visible/asmlinkage to xen paravirt ops")

with a completely empty changelog. Really helpful.

And further down the road it was again LTO "improved":

commit 824a2870098fa536 ("x86, asmlinkage, paravirt: Don't rely on local assembler labels")

with the following changelog in 2013:

    "The paravirt patching code assumes that it can reference a
     local assembler label between two different top level assembler
     statements. This does not work with LTO
     where the assembler code may end up in different assembler files.
    
     Replace it with extern / global /asm linkage labels."
    
This clearly shows that it was never analyzed proper and even the current
patch lacks any form of proper root cause analysis as the "changelog"
clearly shows:

  "With gcc 8 toplevel assembler statements that do not mark themselves
   as .text may end up in other sections. I had boot crashes because
   various assembler statements ended up in the middle of the initcall
   section."

Admittedly it contains at least some information, which is progress over an
empty changelog. But it's clearly NOT a gcc8 problem and it has absolutely
nothing to do with LTO, which the subject suggests.

Is it really necessary, that I need to:

  - urge you to talk with GCC people?

  - ask about whether this needs to be backported?

  - ask whether this is an LTO only problem?

  - do your homework of analysing the root cause?

  - do your homework of analysing the patched code?

  - do your homework of fixing it proper?

And you ask why it takes ages to get your stuff merged? Yes, it takes ages
because patches based on 'works for me' engineering are simply not
acceptable. You have a proven track record of that and I'm trusting you and
your patches not at all. Done that, got burned often enough. Not going to
happen again. It's solely up to you to change that situation.

Proper fix below.

Thanks,

	tglx

8<-------------------

--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -367,11 +367,15 @@ extern struct paravirt_patch_template pv
 	_paravirt_alt(insn_string, "%c[paravirt_typenum]", "%c[paravirt_clobber]")
 
 /* Simple instruction patching code. */
-#define NATIVE_LABEL(a,x,b) "\n\t.globl " a #x "_" #b "\n" a #x "_" #b ":\n\t"
+#define NATIVE_LABEL(a,x,b) "\n" a #x "_" #b ":\n\t"
 
 #define DEF_NATIVE(ops, name, code)					\
-	__visible extern const char start_##ops##_##name[], end_##ops##_##name[];	\
-	asm(NATIVE_LABEL("start_", ops, name) code NATIVE_LABEL("end_", ops, name))
+	static const char start_##ops##_##name[], end_##ops##_##name[]; \
+	asm(".pushsection .rodata, \"a\"\n"				\
+	    NATIVE_LABEL("start_", ops, name)				\
+	    code							\
+	    NATIVE_LABEL("end_", ops, name)				\
+	    ".popsection\n")
 
 unsigned paravirt_patch_ident_64(void *insnbuf, unsigned len);
 unsigned paravirt_patch_default(u8 type, void *insnbuf,

Powered by blists - more mailing lists