From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752490AbdHHUJc (ORCPT ); Tue, 8 Aug 2017 16:09:32 -0400 Received: from mail.kernel.org ([198.145.29.99]:54048 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752465AbdHHUJa (ORCPT ); Tue, 8 Aug 2017 16:09:30 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4BB132392A 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: <20170808191344.kw5nlicmq4g6fpii@treble> References: <0a6cbfb40f8da99b7a45a1a8302dc6aef16ec812.1500938583.git.jpoimboe@redhat.com> <20170728164844.tee7ujqluv2fgarf@sasha-lappy> <20170728175234.pmou7z6dosbi7krh@treble> <20170728182954.imivl45vrc7toarp@sasha-lappy> <20170728185720.fwczcezbkrlxmwqb@treble> <20170728195912.panyn4p6e6ovueey@sasha-lappy> <20170729035437.fmnxmeedrq55bpwm@treble> <20170808185809.qaug762cj7nw3bcd@treble> <20170808191344.kw5nlicmq4g6fpii@treble> From: Andy Lutomirski Date: Tue, 8 Aug 2017 13:09:08 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v4 1/2] x86/unwind: add ORC unwinder To: Josh Poimboeuf Cc: Linus Torvalds , "Levin, Alexander (Sasha Levin)" , "x86@kernel.org" , "linux-kernel@vger.kernel.org" , "live-patching@vger.kernel.org" , Andy Lutomirski , 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 On Tue, Aug 8, 2017 at 12:13 PM, Josh Poimboeuf wrote: > On Tue, Aug 08, 2017 at 12:03:51PM -0700, Linus Torvalds wrote: >> On Tue, Aug 8, 2017 at 11:58 AM, Josh Poimboeuf wrote: >> > >> > Take for example the lock_is_held_type() function. In vmlinux, it has >> > the following instruction: >> > >> > callq *0xffffffff85a94880 (pv_irq_ops.save_fl) >> > >> > At runtime, that instruction is patched and replaced with a fast inline >> > version of arch_local_save_flags() which eliminates the call: >> > >> > pushfq >> > pop %rax >> > >> > The problem is when an interrupt hits after the push: >> > >> > pushfq >> > --- irq --- >> > pop %rax >> >> That should actually be something easily fixable, for an odd reason: >> the instruction boundaries are different. >> >> > I'm not sure what the solution should be. It will probably need to be >> > one of the following: >> > >> > a) either don't allow runtime "alternative" patches to mess with the >> > stack pointer (objtool could enforce this); or >> > >> > b) come up with some way to register such patches with the ORC >> > unwinder at runtime. >> >> c) just add ORC data for the alternative statically and _unconditionally_. >> >> No runtime registration. Just an unconditional entry for the >> particular IP that comes after the "pushfq". It cannot match the >> "callq" instruction, since it would be in the middle of that >> instruction. >> >> Basically, just do a "union" of the ORC data for all the alternatives. >> >> Now, objtool should still verify that the instruction pointers for >> alternatives are unique - or that they share the same ORC unwinder >> information if they are not. >> >> But in cases like this, when the instruction boundaires are different, >> things should "just work", with no need for any special cases. >> >> Hmm? > > Yeah, that might work. Objtool already knows about alternatives, so it > might not be too hard. I'll try it. But this one's not an actual alternative, right? It's a pv op. I would advocate that we make it an alternative after all. I frickin' hate the PV irq ops. It would like roughly like this: ALTERNATIVE "pushfq; popq %rax", "callq *pv_irq_ops.save_fl", X86_FEATURE_GODDAMN_PV_IRQ_OPS (The obvious syntax error and the naming should probably be fixed. Also, this needs to live in an #ifdef because it needs to build on kernels with pv support. It should also properly register itself as a pv patch site.) Semi-serious question: can we maybe delete lguest and 32-bit Xen PV support some time soon? As far as I know, 32-bit Xen PV *hosts* are all EOL and have no security support, 32-bit Xen PV guest dom0 may not work (I've never tried, but it would certainly be nutty on a 64-bit hypervisor), and lguest is, um, not seriously maintained any more. [1] [1] A while back I complained that I couldn't get lguest to boot. Someone replied with multiple workarounds for known bugs that make it not boot. I have a hard time believing that anyone uses it for anything other than trying to test that it still works.