Re: [PATCH] arm64: stacktrace: don't trace arch_stack_walk()

From: Mark Rutland <mark.rutland@arm.com>
To: Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Chen Jun <chenjun102@huawei.com>,
	Marco Elver <elver@google.com>, Mark Brown <broonie@kernel.org>
Subject: Re: [PATCH] arm64: stacktrace: don't trace arch_stack_walk()
Date: Mon, 22 Mar 2021 13:01:58 +0000	[thread overview]
Message-ID: <20210322130158.GA78652@C02TD0UTHF1T.local> (raw)
In-Reply-To: <20210319190205.GI6832@arm.com>

On Fri, Mar 19, 2021 at 07:02:06PM +0000, Catalin Marinas wrote:
> On Fri, Mar 19, 2021 at 06:41:06PM +0000, Mark Rutland wrote:
> > We recently converted arm64 to use arch_stack_walk() in commit:
> > 
> >   5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK")
> > 
> > The core stacktrace code expects that (when tracing the current task)
> > arch_stack_walk() starts a trace at its caller, and does not include
> > itself in the trace. However, arm64's arch_stack_walk() includes itself,
> > and so traces include one more entry than callers expect. The core
> > stacktrace code which calls arch_stack_walk() tries to skip a number of
> > entries to prevent itself appearing in a trace, and the additional entry
> > prevents skipping one of the core stacktrace functions, leaving this in
> > the trace unexpectedly.
> > 
> > We can fix this by having arm64's arch_stack_walk() begin the trace with
> > its caller. The first value returned by the trace will be
> > __builtin_return_address(0), i.e. the caller of arch_stack_walk(). The
> > first frame record to be unwound will be __builtin_frame_address(1),
> > i.e. the caller's frame record. To prevent surprises, arch_stack_walk()
> > is also marked noinline.

[...]

> > Fixes: 5fc57df2f6fd ("arm64: stacktrace: Convert to ARCH_STACKWALK")
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Chen Jun <chenjun102@huawei.com>
> > Cc: Marco Elver <elver@google.com>
> > Cc: Mark Brown <broonie@kernel.org>
> > Cc: Will Deacon <will@kernel.org>
> 
> Thanks Mark. I think we should add a cc stable, just with Fixes doesn't
> always seem to end up in a stable kernel:
> 
> Cc: <stable@vger.kernel.org> # 5.10.x

Makes sense to me, sure.

> With that:
> 
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Thanks!

Will, I assume you're happy to fold in the above when picking this. If
you'd prefer I repost with that folded in, please let me know!

Mark.