Messages in this thread | | | Date | Mon, 31 Jan 2022 16:30:25 -0800 | From | Josh Poimboeuf <> | Subject | Re: [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
| |