Re: arm64: Support overflow stack panic

[Hong Yang3 杨红 <hong.yang3@xxxxxxx>]

Hi Kazu

Thanks for your time to review the patches and all the detailed comments, the 3rd patch attached with all the refinement.

By the way, do I need to squash all the 3 patches as one before them get accepted?

Thanks

Hong

---

From: HAGIO KAZUHITO(?尾　一仁) <k-hagio-ab@xxxxxxx>
Sent: Friday, November 19, 2021 16:21
To: Hong Yang3 杨红 <hong.yang3@xxxxxxx>
Cc: Discussion list for crash utility usage, maintenance and development <crash-utility@xxxxxxxxxx>
Subject: RE: arm64: Support overflow stack panic

 

注意：此封邮件来自于公司外部，请注意信息安全！
Attention: This email comes from outside of the company, please pay attention to the information security!

Hi Hong,

Thanks for the refinement.

> +     if (symbol_exists("overflow_stack") &&
> +         (sp = per_cpu_symbol_search("overflow_stack")) &&
> +         get_symbol_type("overflow_stack", NULL, req)) {
> +             if (CRASHDEBUG(1)) {
> +                     fprintf(fp, "overflow_stack: \n");
> +                     fprintf(fp, "  type: %x, %s\n",
> +                             (int)req->typecode,
> +                             (req->typecode == TYPE_CODE_PTR) ?
> +                                             "TYPE_CODE_PTR" : "other");

Given that overflow_stack is array, TYPE_CODE_ARRAY would be better.

> @@ -2673,6 +2724,12 @@ arm64_back_trace_cmd(struct bt_info *bt)
>                       bt->hp->eip : GET_STACK_ULONG(bt->hp->esp);
>               stackframe.sp = bt->hp->esp + 8;
>               bt->flags &= ~BT_REGS_NOT_FOUND;
> +     } else if (arm64_on_overflow_stack(bt->tc->processor, bt->frameptr)) {
> +             arm64_set_overflow_stack(bt);
> +             bt->flags |= BT_OVERFLOW_STACK;

I would prefer to place this into the else block below

> +             stackframe.sp = bt->stkptr;
> +             stackframe.pc = bt->instptr;
> +             stackframe.fp = bt->frameptr;

then we can remove these three lines.

>       } else {
>               if (arm64_on_irq_stack(bt->tc->processor, bt->frameptr)) {
>                       arm64_set_irq_stack(bt);

> +static int
> +arm64_in_alternate_stack(int cpu, ulong stkptr)
> +{
> +     struct machine_specific *ms = machdep->machspec;
> +
> +     return arm64_in_alternate_stackv(cpu, stkptr,
> +                     ms->irq_stacks, ms->irq_stack_size);
> +}

Given that in_alternate_stack() should check if it's in non-process stacks,
I think we should check also for overflow_stacks here.  So how about making
these function like this?

arm64_in_alternate_stack
  arm64_on_irq_stack
  arm64_on_overflow_stack

arm64_on_irq_stack
  arm64_in_alternate_stackv(irq_stacks)

arm64_on_overflow_stack
  arm64_in_alternate_stackv(overflow_stacks)

Thanks,
Kazu

-----Original Message-----
> Hi Kazu
>
> Here are the latest patches for supporting to run bt command against a core dump with kernel stack overflow
> exception for arm64.
>
> Please help to review and advise if any further change needed.
>
> Tested bt command with options:
>
> bt
> bt -a
> bt -c 3
>
> By the way, 'mach' command also updated to show overflow stacks info as same as IRQ stacks.
>
> Thanks
> Hong
> ________________________________
>
> From: HAGIO KAZUHITO(?尾　一仁) <k-hagio-ab@xxxxxxx>
> Sent: Wednesday, November 17, 2021 15:23
> To: Hong Yang3 杨红 <hong.yang3@xxxxxxx>
> Cc: Discussion list for crash utility usage, maintenance and development <crash-utility@xxxxxxxxxx>
> Subject: RE: arm64: Support overflow stack panic
>
> 注意：此封邮件来自于公司外部，请注意信息安全！
> Attention: This email comes from outside of the company, please pay attention to the information security!
>
> Hi Hong,
>
> Thank you for the patch and sending it to this list.
>
> -----Original Message-----
> > Hi Crash
> >
> > I'll keep refining the patch before it get approved:
>
> OK, so we will wait for the refined patch.
>
> Thanks,
> Kazu
>
> >
> >
> > 1.    Fix the error in arm64_overflow_stack_init() which saved the overflow stack address into
> > ms->irqstacks[], which would cause bt command crash on other cpus. The normal IRQ stacks should be used
> > for bt command for other cpus.
> > 2.    In addition to unwind on the overflow stack, try to go through the IRQ stack to find more useful
> > information
> >
> > Kernel stack overflow case would be rarely but I'd like to sharp the crash to cover this kind of issue.
> >
> > Best regards
> > Hong
> > ________________________________
> >
> > From: Hong Yang3 杨红
> > Sent: Tuesday, November 16, 2021 9:55
> > To: crash-utility@xxxxxxxxxx <crash-utility@xxxxxxxxxx>
> > Subject: arm64: Support overflow stack panic
> >
> > Hi All
> >
> > When I was trying to open a core of an overflow stack panic result, the bt command caused a segment fault,
> > after a while I figured out the overflow stack is not supported by crash utility.
> >
> > This patch is trying to initialize the overflow stack information on startup stage, and the bt command
> works
> > as expected to dump the correct call trace in the overflow stack, currently  it only apply to arm64 target.
> >
> > I'm not sure if any other sub command also need to be fixed for full support for the overflow stack, please
> > advise and I'll try to improve the patch.
> >
> > Thanks
> > Hong YANG

From 2a5e482ef4592c1328d7ff88f3fa9b3f593d4d09 Mon Sep 17 00:00:00 2001
From: Hong YANG <hong.yang3@xxxxxxx>
Date: Fri, 19 Nov 2021 18:27:01 +0800
Subject: [PATCH 3/3] arm64: Refine overflow stack checking funcs

Signed-off-by: Hong YANG <hong.yang3@xxxxxxx>
---
 arm64.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/arm64.c b/arm64.c
index 54a1037..af32e2c 100644
--- a/arm64.c
+++ b/arm64.c
@@ -1742,8 +1742,8 @@ arm64_overflow_stack_init(void)
 			fprintf(fp, "overflow_stack: \n");
 			fprintf(fp, "  type: %x, %s\n",
 				(int)req->typecode,
-				(req->typecode == TYPE_CODE_PTR) ?
-						"TYPE_CODE_PTR" : "other");
+				(req->typecode == TYPE_CODE_ARRAY) ?
+						"TYPE_CODE_ARRAY" : "other");
 			fprintf(fp, "  target_typecode: %x, %s\n",
 				(int)req->target_typecode,
 				req->target_typecode == TYPE_CODE_INT ?
@@ -2728,16 +2728,13 @@ arm64_back_trace_cmd(struct bt_info *bt)
 			bt->hp->eip : GET_STACK_ULONG(bt->hp->esp);
 		stackframe.sp = bt->hp->esp + 8;
 		bt->flags &= ~BT_REGS_NOT_FOUND;
-	} else if (arm64_on_overflow_stack(bt->tc->processor, bt->frameptr)) {
-		arm64_set_overflow_stack(bt);
-		bt->flags |= BT_OVERFLOW_STACK;
-		stackframe.sp = bt->stkptr;
-		stackframe.pc = bt->instptr;
-		stackframe.fp = bt->frameptr;
 	} else {
 		if (arm64_on_irq_stack(bt->tc->processor, bt->frameptr)) {
 			arm64_set_irq_stack(bt);
 			bt->flags |= BT_IRQSTACK;
+		} else if (arm64_on_overflow_stack(bt->tc->processor, bt->frameptr)) {
+			arm64_set_overflow_stack(bt);
+			bt->flags |= BT_OVERFLOW_STACK;
 		}
 		stackframe.sp = bt->stkptr;
 		stackframe.pc = bt->instptr;
@@ -3990,16 +3987,10 @@ arm64_on_process_stack(struct bt_info *bt, ulong stkptr)
 	return FALSE;
 }
 
-static int
-arm64_on_irq_stack(int cpu, ulong stkptr)
-{
-	return arm64_in_alternate_stack(cpu, stkptr);
-}
-
 static int
 arm64_in_alternate_stackv(int cpu, ulong stkptr, ulong *stacks, ulong stack_size)
 {
-	if (!stack_size || (cpu >= kt->cpus))
+	if ((cpu >= kt->cpus) || (stacks == NULL) || !stack_size)
 		return FALSE;
 
 	if ((stkptr >= stacks[cpu]) &&
@@ -4011,6 +4002,13 @@ arm64_in_alternate_stackv(int cpu, ulong stkptr, ulong *stacks, ulong stack_size
 
 static int
 arm64_in_alternate_stack(int cpu, ulong stkptr)
+{
+	return (arm64_on_irq_stack(cpu, stkptr) ||
+		arm64_on_overflow_stack(cpu, stkptr));
+}
+
+static int
+arm64_on_irq_stack(int cpu, ulong stkptr)
 {
 	struct machine_specific *ms = machdep->machspec;
 
-- 
2.25.1

--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility