Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 31 Jan 2022 16:30:25 -0800
From:   Josh Poimboeuf <jpoimboe@redhat.com>
To:     Jann Horn <jannh@google.com>
Cc:     Ingo Molnar <mingo@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        "H. Peter Anvin" <hpa@zytor.com>, linux-kernel@vger.kernel.org,
        Miroslav Benes <mbenes@suse.cz>
Subject: Re: [RFC PATCH] x86/dumpstack: Fix unwind failure due to
 off-by-one-frame
Message-ID: <20220201003025.yb2wpmw4dprmsdww@treble>
References: <20220127005555.2766254-1-jannh@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20220127005555.2766254-1-jannh@google.com>
Precedence: bulk

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