linux-kernel - Re: [PATCH 3/3] arm64: stacktrace: Convert to ARCH

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 16 Jul 2020 17:20:22 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Miroslav Benes <mbenes@...e.cz>
Cc:     Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        Heiko Carstens <hca@...ux.ibm.com>,
        Vasily Gorbik <gor@...ux.ibm.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Borislav Petkov <bp@...en8.de>,
        "H. Peter Anvin" <hpa@...or.com>,
        Christian Borntraeger <borntraeger@...ibm.com>,
        Ingo Molnar <mingo@...nel.org>,
        Jiri Slaby <jirislaby@...nel.org>, x86@...nel.org,
        linux-arm-kernel@...ts.infradead.org, linux-s390@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] arm64: stacktrace: Convert to ARCH_STACKWALK

On Thu, Jul 16, 2020 at 01:56:13PM +0200, Miroslav Benes wrote:
> On Wed, 15 Jul 2020, Mark Brown wrote:

> > -void save_stack_trace(struct stack_trace *trace)
> > -{
> > -	__save_stack_trace(current, trace, 0);
> > +	walk_stackframe(task, &frame, consume_entry, cookie);
> >  }

> just an idea for further improvement (and it might be a matter of taste). 

Yeah, there's some more stuff that can be done - the reason I'm looking
at this code is to do reliable stack trace which is going to require at
least some changes to the actual unwinder, this just seemed like a
useful block moving things forwards in itself and I particularly wanted
feedback on patch 1.

> Wouldn't it be slightly better to do one more step and define "struct 
> unwind_state" instead of "struct stackframe" and also some iterator for 
> the unwinding and use that right in new arch_stack_walk() instead of 
> walk_stackframe()? I mean, take the unbounded loop, "inline" it to 
> arch_stack_walk() and replace the loop with the iterator. The body of the 
> iterator would call to unwind_frame() and consume_entry() and that's it. 
> It would make arm64 implementation very similar to x86 and s390 and thus 
> easier to follow when one switches between architectures all the time.

That's definitely on the radar, the unwinding stuff needs other changes
for the reliable stack trace (if nothing else we need to distinguish
between "errors" due to reaching the bottom of the stack and errors due
to bogosity) which so far looked sensible to bundle up together.

> Tangential to this patch, but another idea for improvement is in 
> unwind_frame(). If I am not missing something, everything in 
> CONFIG_FUNCTION_GRAPH_TRACER could be replaced by a simple call to 
> ftrace_graph_ret_addr(). Again see for example unwind_next_frame() in
> arch/s390/kernel/unwind_bc.c (x86 has it too).

Yes, I'd noticed some divergence there and was going to look into it.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)