linux-kernel - Re: [PATCH v2 2/2] arm64: fix dump_backtrace() to show correct pt

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]

Message-Id: <465BAA45-CF32-4131-9EC6-7A7C463693A5@gmail.com>
Date:	Wed, 21 Oct 2015 20:32:47 +0900
From:	Jungseok Lee <jungseoklee85@...il.com>
To:	AKASHI Takahiro <takahiro.akashi@...aro.org>
Cc:	catalin.marinas@....com, will.deacon@....com, james.morse@....com,
	mark.rutland@....com, broonie@...nel.org, david.griego@...aro.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] arm64: fix dump_backtrace() to show correct pt_regs at interrupt

On Oct 20, 2015, at 5:00 PM, AKASHI Takahiro wrote:

Hi Akashi,

> Adding an extra dummy stack frame for interrupt has one side-effect that
> dump_backtrace() shows inccorect data of struct pt_regs at
> "Exception stack ..." because we are still on an interrupt stack when
> dump_backtrace() encounters an interrupt handler's frame.
> 
> This patch reuses __in_irqentry_text() macro, which was added by
>  commit 9a5ad7d0e3e1 ("arm64: Add __exception_irq_entry definition for
>                        function graph"),
> and allows dump_backtrace() to recognize an interrupt handler's
> frame and fetch a correct pointer (sp) to struct pt_regs on an process
> stack.

A concern is the way __irq_entry and IRQENTRY_TEXT are implemented. As
you know well, they are bound to only function graph tracer. IMHO, it
would be better to factor them out generically and then utilize them
in arch and ftrace side. However, other hackers, ARM64 maintainers
or Arnd Bergmann, would leave more valuable comments than me ;)

Other than that, the approach is straightforwrd.

> One of drawbacks in this approach is that we will sometimes see
> __irqentry_text_start and gic_handle_irq interchangeably, especially,
> when "perf report --call-graph" shows stack traces because both symbols
> has the same address.
> (Please note that this can happen even without this patch if
> CONFIG_FUNCTION_GRAPH_TRACER is enabled.)
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@...aro.org>
> ---
> arch/arm64/include/asm/exception.h |    7 +++----
> arch/arm64/include/asm/traps.h     |    7 -------
> arch/arm64/kernel/traps.c          |    9 +++++++--
> arch/arm64/kernel/vmlinux.lds.S    |    7 +++++++
> 4 files changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/exception.h b/arch/arm64/include/asm/exception.h
> index 6cb7e1a..8415005 100644
> --- a/arch/arm64/include/asm/exception.h
> +++ b/arch/arm64/include/asm/exception.h
> @@ -21,10 +21,9 @@
> #include <linux/ftrace.h>

This can be dropped.

> #define __exception	__attribute__((section(".exception.text")))
> -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> +/* We always need this definition for dump_backtrace */
> +#undef __irq_entry
> +#define __irq_entry	__attribute__((__section__(".irqentry.text")))
> #define __exception_irq_entry	__irq_entry
> -#else
> -#define __exception_irq_entry	__exception
> -#endif
> 
> #endif	/* __ASM_EXCEPTION_H */
> diff --git a/arch/arm64/include/asm/traps.h b/arch/arm64/include/asm/traps.h
> index 0cc2f29..8f05d3b 100644
> --- a/arch/arm64/include/asm/traps.h
> +++ b/arch/arm64/include/asm/traps.h
> @@ -34,7 +34,6 @@ struct undef_hook {
> void register_undef_hook(struct undef_hook *hook);
> void unregister_undef_hook(struct undef_hook *hook);
> 
> -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> static inline int __in_irqentry_text(unsigned long ptr)
> {
> 	extern char __irqentry_text_start[];
> @@ -43,12 +42,6 @@ static inline int __in_irqentry_text(unsigned long ptr)
> 	return ptr >= (unsigned long)&__irqentry_text_start &&
> 	       ptr < (unsigned long)&__irqentry_text_end;
> }
> -#else
> -static inline int __in_irqentry_text(unsigned long ptr)
> -{
> -	return 0;
> -}
> -#endif
> 
> static inline int in_exception_text(unsigned long ptr)
> {
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index e9b9b53..8fc3d4b 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -179,10 +179,15 @@ static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
> 		ret = unwind_frame(&frame);
> 		if (ret < 0)
> 			break;
> -		stack = frame.sp;
> -		if (in_exception_text(where))
> +		if (__in_irqentry_text(where)) {
> +			stack = *(unsigned long *)(frame.fp + 0x18);
> +			dump_mem("", "Interrupt stack", stack,
> +				 stack + sizeof(struct pt_regs), false);

According to entry.S in case of \el == 1, the stack, might look as follows.

    ----------- <- High address (x21)
    |         |
    |         |
    | pt_regs |
    |         |
    |         |
    ----------- <- Low address

So, I think 'bottom' is stack-sizeof(struct pt_regs) and 'top' is stack.
Am I missing here?

Best Regards
Jungseok Lee--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/