| 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
| ||
|
Message-ID: <561E009B.3070001@linaro.org>
Date: Wed, 14 Oct 2015 16:13:31 +0900
From: AKASHI Takahiro <takahiro.akashi@...aro.org>
To: James Morse <james.morse@....com>,
Jungseok Lee <jungseoklee85@...il.com>
Cc: catalin.marinas@....com, will.deacon@....com,
linux-arm-kernel@...ts.infradead.org, mark.rutland@....com,
barami97@...il.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/2] arm64: Expand the stack trace feature to support
IRQ stack
On 10/09/2015 11:24 PM, James Morse wrote:
> Hi Jungseok,
>
> On 07/10/15 16:28, Jungseok Lee wrote:
>> Currently, a call trace drops a process stack walk when a separate IRQ
>> stack is used. It makes a call trace information much less useful when
>> a system gets paniked in interrupt context.
>
> panicked
>
>> This patch addresses the issue with the following schemes:
>>
>> - Store aborted stack frame data
>> - Decide whether another stack walk is needed or not via current sp
>> - Loosen the frame pointer upper bound condition
>
> It may be worth merging this patch with its predecessor - anyone trying to
> bisect a problem could land between these two patches, and spend time
> debugging the truncated call traces.
>
>
>> diff --git a/arch/arm64/include/asm/irq.h b/arch/arm64/include/asm/irq.h
>> index 6ea82e8..e5904a1 100644
>> --- a/arch/arm64/include/asm/irq.h
>> +++ b/arch/arm64/include/asm/irq.h
>> @@ -2,13 +2,25 @@
>> #define __ASM_IRQ_H
>>
>> #include <linux/irqchip/arm-gic-acpi.h>
>> +#include <asm/stacktrace.h>
>>
>> #include <asm-generic/irq.h>
>>
>> struct irq_stack {
>> void *stack;
>> + struct stackframe frame;
>> };
>>
>> +DECLARE_PER_CPU(struct irq_stack, irq_stacks);
>
> Good idea, storing this in the per-cpu data makes it immune to stack
> corruption.
Is this the only reason that you have a dummy stack frame in per-cpu data?
By placing this frame in an interrupt stack, I think, we will be able to eliminate
changes in dump_stace(). and
>
>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
>> index 407991b..5124649 100644
>> --- a/arch/arm64/kernel/stacktrace.c
>> +++ b/arch/arm64/kernel/stacktrace.c
>> @@ -43,7 +43,27 @@ int notrace unwind_frame(struct stackframe *frame)
>> low = frame->sp;
>> high = ALIGN(low, THREAD_SIZE);
>>
>> - if (fp < low || fp > high - 0x18 || fp & 0xf)
>> + /*
>> + * A frame pointer would reach an upper bound if a prologue of the
>> + * first function of call trace looks as follows:
>> + *
>> + * stp x29, x30, [sp,#-16]!
>> + * mov x29, sp
>> + *
>> + * Thus, the upper bound is (top of stack - 0x20) with consideration
>
> The terms 'top' and 'bottom' of the stack are confusing, your 'top' appears
> to be the highest address, which is used first, making it the bottom of the
> stack.
>
> I would try to use the terms low/est and high/est, in keeping with the
> variable names in use here.
>
>
>> + * of a 16-byte empty space in THREAD_START_SP.
>> + *
>> + * The value, 0x20, however, does not cover all cases as interrupts
>> + * are handled using a separate stack. That is, a call trace can start
>> + * from elx_irq exception vectors. The symbols could not be promoted
>> + * to candidates for a stack trace under the restriction, 0x20.
>> + *
>> + * The scenario is handled without complexity as 1) considering
>> + * (bottom of stack + THREAD_START_SP) as a dummy frame pointer, the
>> + * content of which is 0, and 2) allowing the case, which changes
>> + * the value to 0x10 from 0x20.
>
> Where has 0x20 come from? The old value was 0x18.
>
> My understanding is the highest part of the stack looks like this:
> high [ off-stack ]
> high - 0x08 [ left free by THREAD_START_SP ]
> high - 0x10 [ left free by THREAD_START_SP ]
> high - 0x18 [#1 x30 ]
> high - 0x20 [#1 x29 ]
>
> So the condition 'fp > high - 0x18' prevents returning either 'left free'
> address, or off-stack-value as a frame. Changing it to 'fp > high - 0x10'
> allows the first half of that reserved area to be a valid stack frame.
>
> This change is breaking perf using incantations [0] and [1]:
>
> Before, with just patch 1/2:
> ---__do_softirq
> |
> |--92.95%-- __handle_domain_irq
> | __irqentry_text_start
> | el1_irq
> |
>
> After, with both patches:
> ---__do_softirq
> |
> |--83.83%-- __handle_domain_irq
> | __irqentry_text_start
> | el1_irq
> | |
> | |--99.39%-- 0x400008040d00000c
> | --0.61%-- [...]
> |
This also shows that walk_stackframe() doesn't walk through a process stack.
Now I'm trying the following hack on top of Jungseok's patch.
(It doesn't traverse from an irq stack to an process stack yet. I need modify
unwind_frame().)
Thanks,
-Takahiro AKASHI
----8<----
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 650cc05..5fbd1ea 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -185,14 +185,12 @@ alternative_endif
mov x23, sp
and x23, x23, #~(THREAD_SIZE - 1)
cmp x20, x23 // check irq re-enterance
+ mov x19, sp
beq 1f
- str x29, [x19, #IRQ_FRAME_FP]
- str x21, [x19, #IRQ_FRAME_SP]
- str x22, [x19, #IRQ_FRAME_PC]
- mov x29, x24
-1: mov x19, sp
- csel x23, x19, x24, eq // x24 = top of irq stack
- mov sp, x23
+ mov sp, x24 // x24 = top of irq stack
+ stp x29, x22, [sp, #-32]!
+ mov x29, sp
+1:
.endm
/*
--
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/
Powered by blists - more mailing lists