Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753338AbdF2VKX (ORCPT ); Thu, 29 Jun 2017 17:10:23 -0400 Received: from mail.kernel.org ([198.145.29.99]:39698 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751827AbdF2VKV (ORCPT ); Thu, 29 Jun 2017 17:10:21 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org AE80A22BE1 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=luto@kernel.org MIME-Version: 1.0 In-Reply-To: <20170629190559.ttw52ahwtsjynayx@treble> References: <20170629175333.bicpvbwo4d5pdbak@treble> <20170629190559.ttw52ahwtsjynayx@treble> From: Andy Lutomirski Date: Thu, 29 Jun 2017 14:09:54 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v2 6/8] x86/entry: add unwind hint annotations To: Josh Poimboeuf Cc: Andy Lutomirski , X86 ML , "linux-kernel@vger.kernel.org" , live-patching@vger.kernel.org, Linus Torvalds , Jiri Slaby , Ingo Molnar , "H. Peter Anvin" , Peter Zijlstra , Mike Galbraith Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2496 Lines: 50 On Thu, Jun 29, 2017 at 12:05 PM, Josh Poimboeuf wrote: > On Thu, Jun 29, 2017 at 11:50:18AM -0700, Andy Lutomirski wrote: >> On Thu, Jun 29, 2017 at 10:53 AM, Josh Poimboeuf wrote: >> > There's a bug here that will need a small change to the entry code. >> > >> > Mike Galbraith reported: >> > >> > WARNING: can't dereference registers at ffffc900089d7e08 for ip ffffffff81740bbb >> > >> > After some looking I found that it's caused by the following code >> > snippet in the 'interrupt' macro in entry_64.S: >> > >> > /* >> > * Save previous stack pointer, optionally switch to interrupt stack. >> > * irq_count is used to check if a CPU is already on an interrupt stack >> > * or not. While this is essentially redundant with preempt_count it is >> > * a little cheaper to use a separate counter in the PDA (short of >> > * moving irq_enter into assembly, which would be too much work) >> > */ >> > movq %rsp, %rdi >> > incl PER_CPU_VAR(irq_count) >> > cmovzq PER_CPU_VAR(irq_stack_ptr), %rsp >> > UNWIND_HINT_REGS base=rdi >> > pushq %rdi >> > UNWIND_HINT_REGS indirect=1 >> > >> > The problem is that it's changing the stack pointer *before* writing the >> > previous stack pointer (push %rdi). So when unwinding from an NMI which >> > hit between the rsp write and the rdi push, the unwinder tries to access >> > the regs on the previous stack (by reading rdi), but the previous stack >> > pointer isn't there yet, so the access is considered out of bounds. >> >> Ugh, that code. Does this problem go away with this patch applied: >> >> https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=x86/entry_ist&id=2231ec7e0bcc1a2bc94a17081511ab54cc6badd1 >> >> If so, want to update the patch for new kernels (shouldn't conflict >> with anything except your unwind hints)? > > I don't think that patch will fix it, because it still updates rsp > *before* writing the old rsp on the new stack. So there's still a > window where the "previous stack" pointer is missing. But it's in a register. Is undwarf not able to grok that? I have no fundamental problem with pushing it to the new stack first, but the actual asm is nastier because we don't have an addressing mode that's *(*(gs:blahblahblah)) = reg. At least my patch makes all the copied of this code identical so the problem can be solved only once.