lkml.org 
[lkml]   [2022]   [Jan]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCH] x86/dumpstack: Fix unwind failure due to off-by-one-frame
On Thu, Jan 27, 2022 at 01:55:55AM +0100, Jann Horn wrote:
> (emphasis on the "RFC", not the "PATCH"...)
>
> I've hit a bug where __dump_stack() ends up printing a stack trace that
> consists purely of guesses (all printed frames start with "? ").
>
> Debugging the issue, I found that show_trace_log_lvl() is looking at a
> stack that looks like this:
>
> function stored value pointer in show_trace_log_lvl()
> ====================================================================
> show_stack saved RIP
> show_stack saved RBP <-- stack
> show_trace_log_lvl saved RIP <-- unwind_get_return_address_ptr(...)
> show_trace_log_lvl ...
> show_trace_log_lvl ...
>
> show_trace_log_lvl() then iterates up the stack with its `stack`
> variable; but because `unwind_get_return_address_ptr(&state)` is below the
> starting point, the two never compile equal, and so `reliable` is never
> set to 1.

Thanks for reporting! If I understand correctly, this only happens
when show_stack() has an 8-byte stack size.

> Poking around a bit, I see two issues.
>
> The first issue is that __unwind_start() tries to figure out whether
> `first_frame` is inside the current frame before even having looked up
> the ORC entry that determines where the current frame ends.
> That can't work and results in being off-by-one-frame in some cases no
> matter how we twist the comparison between `state->sp` and `first_frame`.

> The second issue is that show_trace_log_lvl() asks __unwind_start() to
> stop when it finds the frame containing `stack`, but then tries
> comparing `unwind_get_return_address_ptr(&state)` (which has to be below
> `stack`, since it is part of the lower frame) with `stack`.
> That can't work if __unwind_start() is working properly - we'll have to
> unwind up another frame.
>
> This patch is an attempt to fix that, but I guess there might still be
> issues with it in the interaction with show_regs_if_on_stack() in
> show_trace_log_lvl(), or something like that?
>
> Another option might be to rework even more how ORC stack walking works,
> and always compute the location of the next frame in __unwind_start()
> and unwind_next(), such that it becomes possible to query for the top
> of the current frame?
>
> Or a completely different approach, do more special-casing of different
> unwinding scenarios in __unwind_start(), such that unwinding a remote
> task doesn't go through the skip-ahead loop, and unwinding the current
> task from a starting point is always guaranteed to skip the given frame
> and stop at the following one? Or something along those lines?
>
> That would also make it more obviously correct what happens if a
> function specifies its own frame as the starting point wrt to changes to
> that frame's contents before the call to unwind_next()... now that I'm
> typing this out, I think that might be the best option?

If I understand correctly, this last proposal is what the current
__unwind_start() code already attempts to do (but obviously fails in the
above off-by-one case). It tries to start at the first frame it finds
*beyond* the given 'first_frame' pointer, rather than the frame
including it. That makes the logic simpler, since you don't have to
find the size of the frame.

So I think this bug could be fixed by reverting commit f1d9a2abff66
("x86/unwind/orc: Don't skip the first frame for inactive tasks").

Can you confirm?

If that fixes it, we may need to do a little more special-casing like
you suggest to get the expected results for the different use cases.

--
Josh

\
 
 \ /
  Last update: 2022-02-01 01:31    [W:0.048 / U:0.276 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site