Received-SPF: pass (google.com: domain of linux-kernel+bounces-109360-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Wed, 20 Mar 2024 19:40:33 +0000
From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Jiangfeng Xiao <xiaojiangfeng@huawei.com>
Cc: arnd@arndb.de, keescook@chromium.org, haibo.li@mediatek.com,
	angelogioacchino.delregno@collabora.com, amergnat@baylibre.com,
	akpm@linux-foundation.org, dave.hansen@linux.intel.com,
	douzhaolei@huawei.com, gustavoars@kernel.org, jpoimboe@kernel.org,
	kepler.chenxin@huawei.com, kirill.shutemov@linux.intel.com,
	linux-hardening@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-mm@kvack.org, linux-arm-kernel@lists.infradead.org,
	nixiaoming@huawei.com, peterz@infradead.org, wangbing6@huawei.com,
	wangfangpeng1@huawei.com, jannh@google.com, willy@infradead.org,
	David.Laight@aculab.com
Subject: Re: [PATCH v2] ARM: unwind: improve unwinders for noreturn case
Message-ID: <Zfs7sT6Pxy5yjnPC@shell.armlinux.org.uk>
References: <1709516385-7778-1-git-send-email-xiaojiangfeng@huawei.com>
 <1710906278-23851-1-git-send-email-xiaojiangfeng@huawei.com>
 <ZfqiD8Yw0oOVHW/p@shell.armlinux.org.uk>
 <84a57ca8-8963-ca24-8bd1-ddc5c33bf4da@huawei.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <84a57ca8-8963-ca24-8bd1-ddc5c33bf4da@huawei.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>

On Wed, Mar 20, 2024 at 11:30:05PM +0800, Jiangfeng Xiao wrote:
> 
> 
> On 2024/3/20 16:45, Russell King (Oracle) wrote:
> > On Wed, Mar 20, 2024 at 11:44:38AM +0800, Jiangfeng Xiao wrote:
> >> This is an off-by-one bug which is common in unwinders,
> >> due to the fact that the address on the stack points
> >> to the return address rather than the call address.
> >>
> >> So, for example, when the last instruction of a function
> >> is a function call (e.g., to a noreturn function), it can
> >> cause the unwinder to incorrectly try to unwind from
> >> the function after the callee.
> >>
> >> foo:
> >> ...
> >> 	bl	bar
> >> ... end of function and thus next function ...
> >>
> >> which results in LR pointing into the next function.
> >>
> >> Fixed this by subtracting 1 from frmae->pc in the call frame
> >> (but not exception frames) like ORC on x86 does.
> > 
> > The reason that I'm not accepting this patch is because the above says
> > that it fixes it by subtracting 1 from the PC value, but the patch is
> > *way* more complicated than that and there's no explanation why.
> > 
> > For example, the following are unexplained:
> > 
> > - Why do we always need ex_frame
> 
> ```
> bar:
> ...
> ... end of function bar ...
> 
> foo:
>     BUG();
> ... end of function foo ...
> ```
> 
> For example, when the first instruction of function 'foo'
> is a undefined instruction, after function 'foo' is executed
> to trigger an exception, 'arm_get_current_stackframe' assigns
> 'regs->ARM_pc' to 'frame.pc'.
> 
> If we always decrement frame.pc by 1, unwinder will incorrectly
> try to unwind from the function 'bar' before the function 'foo'.
> 
> So we need to 'ext_frame' to distinguish this case
> where we don't need to subtract 1.

This just sounds wrong to me. We have two different cases:

1) we're unwinding a frame where PC points at the offending instruction.
   This may or may not be in the exception text.
2) we're unwinding a frame that has been created because of a branch,
   where the PC points at the next instruction _after_ that callsite.

While we're unwinding, we will mostly hit the second type of frames, but
we'll only hit the first type on the initial frame. Some exception
entries will have the PC pointing at the next instruction to be
executed, others will have it pointing at the offending instruction
(e.g. if it needs to be retried.)

So, I don't see what being in the exception/entry text really has much
to do with any decision making here. I think you've found that it works
for your case, but it won't _always_ work and you're just shifting the
"bug" with how these traces work from one issue to a different but
similar issue.

> > - What is the purpose of the change in format string for the display of
> >   backtraces
> ```
> unwind_frame(&frame);
> dump_backtrace_entry(...from) //from = frame.pc
> 	printk("...%pS\n", ...(void *)from);
> ```
> %pB will do sprint_backtrace and print the symbol at (from - 1) address
> %pS will do sprint_symbol_build_id and print the symbol at (from) address

The quote in printk-formats.rst states:

        %pS     versatile_init+0x0/0x110
	%pB     prev_fn_of_versatile_init+0x88/0x88

This is rather ambiguous on its own, since the definition of "previous
function" is ambiguous. Given the offset and size stated there, it's
also not obvious what pointer was passed. It would be nice if these
examples actually said what the pointer passed in actually was.

I had been interpreting "prev_fn_of_" to mean the caller of
versatile_init() but it could also be the preceeding function in the
kernel text to versatile_init() - that is where the ambiguity comes
from.

So, the question I now have is... if %pB prints the symbol corresponding
with "from - 1", then

- with the frame pointer walker, from will always be the return address
  found on the stack for the function we are currently in.
- with the unwinder it will be whatever the unwinder computes as the LR
  register unless the unwind instructions place a non-zero value in PC.

Is there a case where the unwinder gets this wrong?

I think what would help is if you split this patch up, and addressed
each part separately, describing the issue that each part is addressing
giving an example that clearly explains what the patch is doing.
However, please note my comments above that using the fact that we're
in an exception frame doesn't actually tell you anything about whether
you need to correct the PC value or not.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!