[Crash-utility] Re: Fix irq_stack_size on ARM64

On 7/16/24 4:22 PM, Tao Liu wrote: > Hi Yeping, > > Thanks for the fix. > > On Thu, Jul 11, 2024 at 1:38 PM <wonderzyp(a)gmail.com&gt; wrote: >> When using the crash tool to parse the ARM64 dump file with KASAN enabled, I found that using the bt -a command will cause this tool to crash, the following is the backtrace infomation. >> >> (gdb) bt >> #0 0x00005635ac2b166b in arm64_unwind_frame (frame=0x7ffdaf35cb70, bt=0x7ffdaf35d430) >> at arm64.c:2821 >> #1 arm64_back_trace_cmd (bt=0x7ffdaf35d430) at arm64.c:3306 >> #2 0x00005635ac27b108 in back_trace (bt=bt@entry=0x7ffdaf35d430) at kernel.c:3239 >> #3 0x00005635ac2880ae in cmd_bt () at kernel.c:2863 >> #4 0x00005635ac1f16dc in exec_command () at main.c:893 >> #5 0x00005635ac1f192a in main_loop () at main.c:840 >> #6 0x00005635ac50df81 in captured_main (data=<optimized out>) at main.c:1284 >> #7 gdb_main (args=<optimized out>) at main.c:1313 >> #8 0x00005635ac50e000 in gdb_main_entry (argc=<optimized out>, argv=<optimized out>) >> at main.c:1338 >> #9 0x00005635ac1ea2a5 in main (argc=5, argv=0x7ffdaf35dde8) at main.c:721 >> Eventually, I found that it was may caused by not setting irq_stack_size properly, and provide this patch to solve it. >> > Could you please re-draft your commit message? The original one looks > informal. E.g: > > A segfault issue was observed on KASAN enabled arm64 kernel due to the > incorrect irq_stack_size, see the following stack trace: > ... > The issue was caused by ...., and this patch will fix the issue by .... > >> From 34b28aa8c11e77d20adec4f7705a14d239c8a55f Mon Sep 17 00:00:00 2001 >> From: wonderzyp <wonderzyp(a)qq.com&gt; >> Date: Mon, 8 Jul 2024 20:11:38 +0800 >> Subject: [PATCH 1131/1131] set_arm64_irq_stack_size >> >> Signed-off-by: Yeping Zheng <wonderzyp(a)gmail.com&gt; >> --- >> arm64.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 47 insertions(+), 2 deletions(-) >> >> diff --git a/arm64.c b/arm64.c >> index b3040d7..39d891b 100644 >> --- a/arm64.c >> +++ b/arm64.c >> @@ -93,6 +93,7 @@ static void arm64_calc_VA_BITS(void); >> static int arm64_is_uvaddr(ulong, struct task_context *); >> static void arm64_calc_KERNELPACMASK(void); >> static int arm64_get_vmcoreinfo(unsigned long *vaddr, const char *label, int base); >> +static ulong arm64_set_irq_stack_size(struct machine_specific *ms); >> >> struct kernel_range { >> unsigned long modules_vaddr, modules_end; >> @@ -2223,8 +2224,14 @@ arm64_irq_stack_init(void) >> if (MEMBER_EXISTS("thread_union", "stack")) { >> if ((sz = MEMBER_SIZE("thread_union", "stack")) > 0) >> ms->irq_stack_size = sz; >> - } else >> - ms->irq_stack_size = ARM64_IRQ_STACK_SIZE; >> + } else { >> + ulong res = arm64_set_irq_stack_size(ms); >> + if (res > 0){ >> + ms->irq_stack_size = res; >> + } else { >> + ms->irq_stack_size = ARM64_IRQ_STACK_SIZE; >> + } >> + } >> >> machdep->flags |= IRQ_STACKS; >> >> @@ -4921,6 +4928,44 @@ static void arm64_calc_KERNELPACMASK(void) >> } >> } >> >> +static ulong arm64_set_irq_stack_size(struct machine_specific *ms) >> +{ >> + char *string; >> + int ret; >> + int KASAN_THREAD_SHIFT = 0; >> + int MIN_THREAD_SHIFT; >> + ulong ARM64_PAGE_SHIFT; >> + ulong THREAD_SHIFT = 0; >> + ulong THREAD_SIZE; > I guess the upper case of variable names is not encouraged, though it > is the variable that comes from kernel config file. > >> + if (kt->ikconfig_flags & IKCONFIG_AVAIL) { >> + if ((ret = get_kernel_config("CONFIG_KASAN_GENERIC", NULL) == IKCONFIG_Y) || >> + (ret = get_kernel_config("CONFIG_KASAN_SW_TAGS", NULL) == IKCONFIG_Y)) { >> + KASAN_THREAD_SHIFT = 1; >> + } >> + } >> + MIN_THREAD_SHIFT = 14 + KASAN_THREAD_SHIFT; >> + >> + if (kt->ikconfig_flags & IKCONFIG_AVAIL) { > Could the if condition be merged with the prior one? > >> + if ((ret = get_kernel_config("CONFIG_VMAP_STACK", NULL)) == IKCONFIG_Y){ >> + if ((ret = get_kernel_config("CONFIG_ARM64_PAGE_SHIFT", &string)) == The "CONFIG_ARM64_PAGE_SHIFT " has been removed since kernel v6.9-rc1, so this can not work on the latest kernel. See: d3e5bab923d3 ("arch: simplify architecture specific page size configuration") In addition, the IKCONFIG is not available in most distributions, it can not cover this case. There is a similar discussion there: https://www.mail-archive.com/devel@lists.crash-utility.osci.io/msg00880.html Can you try to handle the current issue with a similar solution? >> IKCONFIG_STR){ >> + ARM64_PAGE_SHIFT = atol(string); >> + } >> + if (MIN_THREAD_SHIFT < ARM64_PAGE_SHIFT){ >> + THREAD_SHIFT = ARM64_PAGE_SHIFT; >> + } else { >> + THREAD_SHIFT = MIN_THREAD_SHIFT; >> + } >> + } >> + } >> + >> + if (THREAD_SHIFT == 0) { >> + return -1; >> + } >> + >> + THREAD_SIZE = ((1UL) << THREAD_SHIFT); >> + return THREAD_SIZE; >> +} > I'm OK with the approach above, since it directly came from the kernel > source. However I'm not a fan of checking kernel configs, there might > be kernels which are compiled without CONFIG_IKCONFIG. > > Could we add an approach here, to get the value from disassembly when > CONFIG_IKCONFIG is negative? > > kernel source: arch/arm64/kernel/entry.S: > > .macro kernel_ventry, el:req, ht:req, regsize:req, label:req > .... > add sp, sp, x0 // sp' = sp + x0 > sub x0, sp, x0 // x0' = sp' - x0 = (sp + x0) - x0 = sp > tbnz x0, #THREAD_SHIFT, 0f <<<<<<<< > > $ objdump -d vmlinux > ... > ffff800080010800 <vectors>: > ffff800080010800: d10543ff sub sp, sp, #0x150 > ffff800080010804: 8b2063ff add sp, sp, x0 > ffff800080010808: cb2063e0 sub x0, sp, x0 > ffff80008001080c: 37800080 tbnz w0, #16, > ffff80008001081c <vectors+0x1c> <<<<<<<<<< > > It is easy to get the THREAD_SHIFT value by disassembling the tbnz > instruction. What do you think @Lianbo Jiang This is a good idea, but it still relies on compiler. As we discussed, usually I would recommend finding its values via some kernel symbols.

Thanks Lianbo > > Thanks, > Tao Liu > >> + >> #endif /* ARM64 */ >> >> >> -- >> 2.25.1 >>