[Crash-utility] Re: Fix irq_stack_size on ARM64

Hi Tao, Thanks for your suggestion, and this method works well as expected. > Can we make it simpler, since the value we are looking for is the 2nd > oprand of tbnz, there must be a ',' after the value. > > tbnz w2, #0x1f, 0xffff80008001bed0 > > Can we make it like the following? I haven't tested the following code... > > if ((pos2 = strchr(pos1, '#'))) { > pos2 += 1; > for (pos1 = pos2; *pos2 != '\0' && *pos2 != ','; pos2++); > *pos2 = '\0'; > thread_shift = stol(pos1, RETURN_ON_ERROR|QUIET, &errflag); > if (errflag) { > thread_shift = 0; > } > break; > } Thanks. Yeping.ZHENG Tao Liu <ltao(a)redhat.com&gt; 于2024年7月30日周二 08:55写道： > > Hi Yeping, > > On Mon, Jul 29, 2024 at 2:20 PM yp z <wonderzyp(a)gmail.com&gt; wrote: > > > > Hi Lianbo and Tao, > > Thank you for your suggestions, and I have rewrote a patch in two cases: > > > > [1] with vmcoreinfo: > > + if (kernel_symbol_exists("kasan_enable_current")) { > > + min_thread_shift += 1; > > + thread_shift = (min_thread_shift < machdep->pageshift) ? machdep->pageshift : min_thread_shift; > > + } > > > This looks fine to me. > > >> BTW: can you help point out the current issue is with or without the vmcoreinfo? Yeping > > > > My issue is with vmcoreinfo of "SYMBOL("kasan_enable_current")", and works well with this patch. > > > > [2] without vmcoreinfo: > >> > >> What about somehow the disassembly gives us hex values like: > >> #0xe? It won't work by then. Any ideas for this? > > > > In order to use stol() as expected, I have changed the first non-numeric char to '\0': > > + if ((pos1 = strstr(buf1, "tbnz"))) { > > + if ((pos2 = strchr(pos1, '#'))) { > > + pos2 += 1; > > + pos1 = pos2; > > + while (*pos2 != '\0') { > > + if (!((*pos2 >= '0' && *pos2 <= '9') > > + || (*pos2 >= 'A' && *pos2 <= 'F') > > + || (*pos2 >= 'a' && *pos2 <= 'f'))) { > > There are 'x' 'X' not covered, it will not work for the "#0xe" case. > Can we make it simpler, since the value we are looking for is the 2nd > oprand of tbnz, there must be a ',' after the value. > > tbnz w2, #0x1f, 0xffff80008001bed0 > > Can we make it like the following? I haven't tested the following code... > > if ((pos2 = strchr(pos1, '#'))) { > pos2 += 1; > for (pos1 = pos2; *pos2 != '\0' && *pos2 != ','; pos2++); > *pos2 = '\0'; > thread_shift = stol(pos1, RETURN_ON_ERROR|QUIET, &errflag); > if (errflag) { > thread_shift = 0; > } > break; > } > > Thanks, > Tao Liu > > > + *pos2 = '\0'; > > + break; > > + } > > + ++pos2; > > + } > > + thread_shift = stol(pos1, RETURN_ON_ERROR|QUIET, &errflag); > > > > Thanks. > > Yeping.ZHENG > > > > lijiang <lijiang(a)redhat.com&gt; 于2024年7月26日周五 17:23写道： > >> > >> On Fri, Jul 19, 2024 at 6:40 AM Tao Liu <ltao(a)redhat.com&gt; wrote: > >>> > >>> Hi Lianbo, > >>> > >>> On Thu, Jul 18, 2024 at 10:50 PM Lianbo Jiang <lijiang(a)redhat.com&gt; wrote: > >>> > > >>> > 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. > >>> > >>> Yeah, but I didn't find a suitable symbol so far... In addition, the > >> > >> > >> It's true. For the KASAN, it is easy to find a suitable symbol, but it's not easy for the PAGESIZE or PAGESHIFT. > >> > >> Given that I would suggest covering two cases: > >> [1] with vmcoreinfo(see mm/kasan/common.c) > >> ... > >> int min_thread_shift = 14; > >> if (kernel_symbol_exists("kasan_enable_current")) > >> min_thread_shift += 1; > >> > >> thread_shift = (min_thread_shift < machdep->pageshift) ? machdep->pageshift : min_thread_shift; > >> ... > >> > >> [2] without vmcoreinfo > >> Use Tao's solution(disassemble kernel code) > >> > >> The first one is better than the second one in case of having vmcoreinfo, If I understand correctly. > >> But we still need to cover the above two cases in the crash tools. > >> > >> BTW: can you help point out the current issue is with or without the vmcoreinfo? Yeping > >> > >> Thanks > >> Lianbo > >> > >>> above case is a little different here, the tbnz instruction is not > >>> generated from C code, it is written in assembly in > >>> arch/arm64/kernel/entry.S file. So I guess there would be a lower > >>> possibility if tbnz instruction gets replaced or eliminated by the > >>> compilers. Anyway, it's OK to me if you don't want to accept the > >>> approach. > >>> > >>> Thanks, > >>> Tao Liu > >>> > >>> > >>> > > >>> > > >>> > Thanks > >>> > > >>> > Lianbo > >>> > > >>> > > > >>> > > Thanks, > >>> > > Tao Liu > >>> > > > >>> > >> + > >>> > >> #endif /* ARM64 */ > >>> > >> > >>> > >> > >>> > >> -- > >>> > >> 2.25.1 > >>> > >> > >>> > > >>> >