Re: [PATCH 02/16] x86/dumpstack: Add get_stack_info() support for the SYSENTER stack

[Josh Poimboeuf]

On Mon, Nov 20, 2017 at 12:46:13PM -0800, Andy Lutomirski wrote:
> On Mon, Nov 20, 2017 at 12:42 PM, Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
> > On Mon, Nov 20, 2017 at 09:07:33AM -0800, Andy Lutomirski wrote:
> >> +bool in_SYSENTER_stack(unsigned long *stack, struct stack_info *info)
> >
> > Can you make it lowercase for consistency with the other in_*_stack()
> > functions?  For example, in_irq_stack() is all lowercase even though
> > "IRQ" is normally written in uppercase.
> >
> > But also, I'm wondering whether this get_stack_info() support is even
> > really needed.
> >
> > As currently written, the trampoline code doesn't have any ORC data
> > associated with it.  So the unwinder would never have the need to
> > actually read the SYSENTER stack.
> >
> > You _could_ add an UNWIND_HINT_IRET_REGS annotation after the simulated
> > iret frame is written, which would allow the unwinder to dump those regs
> > when unwinding from an NMI.
> 
> There's some ORC data in the non-trampoline  SYSENTER path

But that's *after* the stack switch to the real kernel stack, right?

> but, more importantly, the OOPS unwinder will just bail without this
> patch.  With the patch, we get a valid unwind, except that everything
> has a ?  in front.

Hm.  I can't even fathom how that's possible.  Are you talking about the
"unwind from NMI to SYSENTER stack" path?  Or any unwind to a syscall?
Either way I'm baffled...  If the unwinder only encounters the SYSENTER
stack at the end, how could that cause everything beforehand to have a
question mark?

> > But there's only a tiny window where that would be possible: only a few
> > instructions.  I'm not sure that would be worth the effort, unless we
> > got to the point where we expect to have 100% unwinder coverage.  But
> > that's currently unrealistic anyway because of generated code and
> > runtime patching.
> 
> I tripped it myself several times when debugging this code.

Again I don't see how this patch would help if there's no ORC data for
the code which uses the SYSENTER stack.  I must be missing something.

-- 
Josh