Re: [PATCH v4 1/2] x86/unwind: add ORC unwinder

[Andy Lutomirski]

On Tue, Aug 8, 2017 at 12:13 PM, Josh Poimboeuf <jpoimboe@xxxxxxxxxx> 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 <jpoimboe@xxxxxxxxxx> 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.