Re: [PATCH v2 30/44] x86/unwind: add new unwind interface and implementations

[Andy Lutomirski]

On Thu, Aug 11, 2016 at 7:28 AM, Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
> On Thu, Aug 11, 2016 at 12:18:54AM -0700, Andy Lutomirski wrote:
>> > > I admit I don't fully understand the use case.  If someone wants to
>> > > start a trace in the middle, shouldn't they just pass regs in?
>> >
>> > The regs aren't always available.  Some callers just want to skip the
>> > first few frames so the stack dump code itself isn't traced.
>>
>> I suspect that all users are okay with your algorithm simply because
>> they don't switch stacks.  Maybe the thing to do is to stop advancing
>> when sp is passed or if the stack switches at all.
>
> There are actually some cases which could be broken by the sloppy "while
> state->bp < sp" check.  When starting with sp from 'regs->sp', sp often
> points to a different stack than the current one.  It seemed to work in
> my testing, but I guess I got lucky, with irq percpu stack addresses
> being smaller than the thread stack addresses.
>
>> Could you point me at a user that passes anything other than regs->sp
>> here?  On brief inspection, I haven't found any at all.
>
> For example, see show_stack().

Is that a non-trivial case?  show_stack() is generating sp *and* bp.
Why not just pass both of them all the way in to show_trace_log_lvl
(which the existing code already does) and then pass it into
unwind_start?

Alternatively, since that risks causing a bit of loss if you implement
DWARF, you could add an unwind_start_here() function that captures the
state in the calling function and pass the result all the way through.
Or you could write a silly asm helper that literally fills in a struct
pt_regs for the current context (although that could get a bit awkward
for 32-bit, since pt_regs doesn't really contain sp there).

If there are genuinely zero non-trivial cases, then this should fully
solve the problem, no?

Aside: the current code looks a bit silly to me:

    unsigned long stack;
    ...
        sp = &stack;
        bp = stack_frame(current, NULL);

Why not just force a frame pointer and read out sp and bp using asm?

--Andy