Re: [PATCH v2] x86: fix early boot crash on gcc-10

From: Borislav Petkov <bp@alien8.de>
To: Michael Matz <matz@suse.de>
Cc: Jakub Jelinek <jakub@redhat.com>,
	Sergei Trofimovich <slyfox@gentoo.org>,
	linux-kernel@vger.kernel.org,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, "H. Peter Anvin" <hpa@zytor.com>,
	Andy Lutomirski <luto@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	x86@kernel.org
Subject: Re: [PATCH v2] x86: fix early boot crash on gcc-10
Date: Wed, 15 Apr 2020 09:48:42 +0200	[thread overview]
Message-ID: <20200415074842.GA31016@zn.tnic> (raw)
In-Reply-To: <alpine.LSU.2.21.2004141343370.11688@wotan.suse.de>

On Tue, Apr 14, 2020 at 01:50:29PM +0000, Michael Matz wrote:
> So this part expects that the caller (!) of trace_hardirqs_on was compiled
> with a frame pointer (in %ebp).

/me looks at the .s file...

options passed comment at the top has -fno-omit-frame-pointer

> Obviously that's not the case as you traced above. Is start_secondary
> the immediate caller in the above case?

Yes, start_secondary() is the function which is marked as
__attribute__((optimize("-fno-stack-protector"))) and it does:

# arch/x86/kernel/smpboot.c:264:        local_irq_enable();
        call    trace_hardirqs_on       #

(the local_irq_enable() is a macro which has the call to
trace_hardirqs_on().

> Look at it's disassembly.  If it doesn't have the usual push
> %ebp/mov%esp,%ebp prologue it probably doesn't use a frame pointer.

Here's the preamble:

        .text
        .p2align 4
        .type   start_secondary, @function
start_secondary:
        pushl   %esi    #
        pushl   %ebx    #
        subl    $28, %esp       #,
# arch/x86/kernel/smpboot.c:226:        cr4_init();
        call    cr4_init        #
	...

Those two pushes on entry are for callee-saved regs, AFAICT, because it
is going to use them later. But no frame setup.

> In that case I would speculate that having a frame pointer
> was (before the change above) only a side-effect of being
> compiled with -fstack-protector, which got now disabled.

Looks like it. And that's 32-bit. 64-bit variant of this looks more like it:

        .text
        .p2align 4
        .type   start_secondary, @function
start_secondary:
        pushq   %rbp    #
        subq    $8, %rsp        #,

although it doesn't save %rsp on the stack. I think it leaves space on
the stack for the canary but I'm not sure.

> But I was under the impression that the upstream kernels build with
> -fno-omit-frame-pointer, so that sounds unexpected.

Yap, see above.

> But I have no better explanation at the moment. If the above
> speculation doesn't make you progress: preprocessed file and a note of
> what the immediate caller of trace_hardirqs_on is in the above case,
> please.

Ok, I'll find you on IRC to talk details.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette