Hi Yeping, Thanks for the fix. On Thu, Jul 11, 2024 at 1:38 PM <wonderzyp(a)gmail.com&gt; wrote: 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 .... I guess the upper case of variable names is not encouraged, though it is the variable that comes from kernel config file. Could the if condition be merged with the prior one? 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 Thanks, Tao Liu

Hi Yeping, Thanks for the patch. I re-thought about the issue. I want to ask a few questions: 1) What's your kernel version? Because I checked the latest kernel source 6.10, that the related code doesn't change: * #define IRQ_STACK_SIZE THREAD_SIZE * * and finding a solid usage of THREAD_SIZE is hard, but: * * union thread_union { * ... * unsigned long stack[THREAD_SIZE/sizeof(long)]; * }; So I guess we can still get the THREAD_SIZE by the old crash code right? if (MEMBER_EXISTS("thread_union", "stack")) { if ((sz = MEMBER_SIZE("thread_union", "stack")) > 0) ms->irq_stack_size = sz; Could you please debug with your vmcore why code doesn't enter the above if conditions, so it goes into your arm64_set_irq_stack_size() check? 2) The commit message seems mis-formatted in your attachment patch. Thanks, Tao Liu On Wed, Jul 17, 2024 at 5:16 PM yp z <wonderzyp(a)gmail.com&gt; wrote:

From: wonderzyp <wonderzyp(a)qq.com&gt; A segfault issue was observed due to the incorrect irq_stack_size on ARM64, see the following stack trace: The issue may be encountered when thread_union symbol not found in vmlinux due to compiling optimization. This patch will try the following 2 methods to get the irq_stack_size when thread_union symbol unavailable: 1. Try getting the value from IKconfig. 2. Try getting the value from kernel code disassembly, to get THREAD_SHIFT directly from tbnz instruction. In arch/arm64/kernel/entry.S: .macro kernel_ventry, el:req, ht:req, regsize:req, label:req ... add sp, sp, x0 sub x0, sp, x0 tbnz x0, #THREAD_SHIFT, 0f $ gdb vmlinux (gdb) disass vectors Dump of assembler code for function vectors: ... 0xffff800080010804 <+4>: add sp, sp, x0 0xffff800080010808 <+8>: sub x0, sp, x0 0xffff80008001080c <+12>: tbnz w0, #16, 0xffff80008001081c <vectors+28> Signed-off-by: Yeping.Zheng <yeping.zheng(a)nio.com&gt; --- Hi Yeping, Could you please have a test on this patch? I re-drafted the commit message and cleaned up some code, but the patch is NOT tested, especially the tbnz part, not sure if it can work as expected. In addition, the "tbnz" part is a little bit tricky, not sure if @Lianbo likes this approach or not. Thanks, Tao Liu --- arm64.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 67 insertions(+), 2 deletions(-) diff --git a/arm64.c b/arm64.c index b3040d7..363ebab 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(void); struct kernel_range { unsigned long modules_vaddr, modules_end; @@ -2223,8 +2224,10 @@ 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->irq_stack_size = (res > 0) ? res : ARM64_IRQ_STACK_SIZE; + } machdep->flags |= IRQ_STACKS; @@ -4921,6 +4924,68 @@ static void arm64_calc_KERNELPACMASK(void) } } +static ulong arm64_set_irq_stack_size(void) +{ + char *string; + int kasan_thread_shift = 0; + int min_thread_shift; + ulong arm64_page_shift; + ulong thread_shift = 0; + ulong thread_size; + + char buf1[BUFSIZE]; + char *pos1, *pos2; + int errflag = 0; + + if (kt->ikconfig_flags & IKCONFIG_AVAIL) { + if ((get_kernel_config("CONFIG_KASAN_GENERIC", NULL) == IKCONFIG_Y) || + (get_kernel_config("CONFIG_KASAN_SW_TAGS", NULL) == IKCONFIG_Y)) + kasan_thread_shift = 1; + min_thread_shift = 14 + kasan_thread_shift; + + if (get_kernel_config("CONFIG_VMAP_STACK", NULL) == IKCONFIG_Y) { + if (get_kernel_config("CONFIG_ARM64_PAGE_SHIFT", &string) == 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; + } + } + } else { + sprintf(buf1, "x/32i vectors"); + open_tmpfile(); + if (!gdb_pass_through(buf1, pc->tmpfile, GNU_RETURN_ON_ERROR)) { + goto out; + } + rewind(pc->tmpfile); + while (fgets(buf1, BUFSIZE, pc->tmpfile)) { + if ((pos1 = strstr(buf1, "tbnz"))) { + if ((pos2 = strchr(pos1, '#'))) { + thread_shift = stol(pos2 + 1, + RETURN_ON_ERROR|QUIET, + &errflag); + if (errflag) { + thread_shift = 0; + } else if (CRASHDEBUG(1)) { + error(INFO, "Detect thread shift" + " via tbnz %ld\n", thread_shift); + } + break; + } + } + } +out: + close_tmpfile(); + } + + if (!thread_shift) + return 0; + else + return ((1UL) << thread_shift); +} + #endif /* ARM64 */ -- 2.40.1

Hi Tao, I'm sorry I forget my vmlinux is with IKCONFIG enabled, so yesterday's test did not cover the branch where tbnz is located. I found that that patch not work well due to this lines: By disable the `QUIET` flag, the crash tool output error: crash: not a valid number: 15, 0xffffffc00801081c <vectors+28> I think stol() function cannot extract a single digit from char* with non-numeric characters, so I use sscanf() in following patch which can work well. --- arm64.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/arm64.c b/arm64.c index 81c4eeb..26af590 100644 --- a/arm64.c +++ b/arm64.c @@ -5031,16 +5031,12 @@ static ulong arm64_set_irq_stack_size(void) while (fgets(buf1, BUFSIZE, pc->tmpfile)) { if ((pos1 = strstr(buf1, "tbnz"))) { if ((pos2 = strchr(pos1, '#'))) { - thread_shift = stol(pos2 + 1, - RETURN_ON_ERROR|QUIET, - &errflag); - if (errflag) { - thread_shift = 0; - } else if (CRASHDEBUG(1)) { - error(INFO, "Detect thread shift" - " via tbnz %ld\n", thread_shift); + if (sscanf(pos2 + 1, "%ld", &thread_shift) == 1) { + if (CRASHDEBUG(1)){ + error(INFO, "Detect thread shift via tbnz %ld\n", thread_shift); + } + break; } - break; } } } -- 2.25.1 Thanks, Yeping.ZHENG

On 7/16/24 4:22 PM, Tao Liu wrote: 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? 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

On Fri, Jul 19, 2024 at 6:40 AM Tao Liu <ltao(a)redhat.com&gt; wrote: 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

Hi Yeping, On Mon, Jul 29, 2024 at 2:20 PM yp z <wonderzyp(a)gmail.com&gt; wrote: This looks fine to me. 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

Hi Yeping, Thanks for the update, for the patch LGTM, so ack. Thanks, Tao Liu On Tue, Jul 30, 2024 at 7:40 PM yp z <wonderzyp(a)gmail.com&gt; wrote:

Hi Lianbo, Thank you for your suggestion. Also, we expect it to enter the else branch only when there is no I think you are right, the else branch can work well regardless of whether vmcoreinfo is present or not. So I changed the execution order of the code: Step 1. Try to get THREAD_SHIFT from tbnz instruction. Step 2. If cannot get THREAD_SHIFT by step 1, change the thread_shift when KASAN is enabled and with vmcoreinfo. Please help review this new patch. Thanks Yeping lijiang <lijiang(a)redhat.com&gt; 于2024年7月31日周三 11:23写道： > Hi, Yeping > Thank you for the update. > > I copied the code from your attachment here: > > +static ulong arm64_set_irq_stack_size(void) > +{ > + int min_thread_shift = 14; > + ulong thread_shift = 0; > + char buf1[BUFSIZE]; > + char *pos1, *pos2; > + int errflag = 0; > + > + if (kernel_symbol_exists("kasan_enable_current")) { > + min_thread_shift += 1; > + thread_shift = (min_thread_shift < machdep->pageshift) ? > machdep->pageshift : min_thread_shift; > + } else { > > The above if-branch can work well when there is the vmcoreinfo data and > KASAN is enabled, otherwise it may not work, Eg: the KASAN is enabled and > there is no the vmcoreinfo data. > > Also, we expect it to enter the else branch only when there is no > > Thanks > Lianbo > > + sprintf(buf1, "x/32i vectors"); > + open_tmpfile(); > + if (!gdb_pass_through(buf1, pc->tmpfile, GNU_RETURN_ON_ERROR)) { > + goto out; > + } > + rewind(pc->tmpfile); > + while (fgets(buf1, BUFSIZE, pc->tmpfile)) { > + if ((pos1 = strstr(buf1, "tbnz"))) { > + 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; > + } > + } > + } > +out: > + close_tmpfile(); > + } > + > + if (!thread_shift) > + return 0; > + else > + return ((1UL) << thread_shift); > +} > + > #endif /* ARM64 */ > > On Tue, Jul 30, 2024 at 3:40 PM yp z <wonderzyp(a)gmail.com&gt; wrote: > >> 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 >>> >>> > >> >>> >>> > >>> >>> >>> >>>

Hi Lianbo, doc: Thank to Dave for pointing that out. I apologize for not realizing this before, and resend it as following. whether vmcoreinfo is present or not. So I changed the execution order of the code: when KASAN is enabled and with vmcoreinfo. --- arm64.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 47 insertions(+), 2 deletions(-) diff --git a/arm64.c b/arm64.c index 78e6609..49799e6 100644 --- a/arm64.c +++ b/arm64.c @@ -94,6 +94,7 @@ static int arm64_is_uvaddr(ulong, struct task_context *); static void arm64_calc_KERNELPACMASK(void); static void arm64_recalc_KERNELPACMASK(void); static int arm64_get_vmcoreinfo(unsigned long *vaddr, const char *label, int base); +static ulong arm64_set_irq_stack_size(void); struct kernel_range { unsigned long modules_vaddr, modules_end; @@ -2234,8 +2235,10 @@ 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->irq_stack_size = (res > 0) ? res : ARM64_IRQ_STACK_SIZE; + } machdep->flags |= IRQ_STACKS; @@ -4950,6 +4953,48 @@ static void arm64_recalc_KERNELPACMASK(void){ } } +static ulong arm64_set_irq_stack_size(void) +{ + int min_thread_shift = 14; + ulong thread_shift = 0; + char buf1[BUFSIZE]; + char *pos1, *pos2; + int errflag = 0; + + sprintf(buf1, "x/32i vectors"); + open_tmpfile(); + if (!gdb_pass_through(buf1, pc->tmpfile, GNU_RETURN_ON_ERROR)) { + goto out; + } + rewind(pc->tmpfile); + while (fgets(buf1, BUFSIZE, pc->tmpfile)) { + if ((pos1 = strstr(buf1, "tbnz"))) { + 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; + } + } + } +out: + close_tmpfile(); + + if ((!thread_shift) && kernel_symbol_exists("kasan_enable_current")) { + min_thread_shift += 1; + thread_shift = (min_thread_shift < machdep->pageshift) ? machdep->pageshift : min_thread_shift; + } + + if (!thread_shift) + return 0; + else + return ((1UL) << thread_shift); +} + #endif /* ARM64 */ Thanks yeping. Dave Young <dyoung(a)redhat.com&gt; 于2024年7月31日周三 14:58写道： > > > Hi, sending a patch as attachment is bad, you can refer to the kernel doc: > https://www.kernel.org/doc/html/v4.14/process/email-clients.html > > "Patches for the Linux kernel are submitted via email, preferably as > inline text in the body of the email. Some maintainers accept > attachments, but then the attachments should have content-type > text/plain. However, attachments are generally frowned upon because it > makes quoting portions of the patch more difficult in the patch review > process." > > Crash is different from kernel but the email process is similar, I > think you can resend it with a proper format instead of maintainers > manually copying them in the email body for review. > > Thanks > Dave > >

Hi Lianbo, I have checked my vmlinux again, and found that not contained the symbol "vmcoreinfo_data" and "vmcoreinfo_size", but it has "kasan_enable_current". So I think check "kasan_enable_current" when have symol "vmcoreinfo_data" won't work as excepted. /* Having the vmcoreinfo data */ Maybe the previous patch is more suitable for this situation ? + if (kernel_symbol_exists("kasan_enable_current")) { I found that my vmlinux not set CONFIG_KEXEC and other related kernel dump configurations. Maybe this is the reason that not contained "vmcoreinfo_data" symbol. By the way, my kernel was running as a guest OS, and the dump file was generated by the hypervisor. Thanks yeping lijiang <lijiang(a)redhat.com&gt; 于2024年7月31日周三 17:36写道： > On Wed, Jul 31, 2024 at 3:34 PM yp z <wonderzyp(a)gmail.com&gt; wrote: > >> Hi Lianbo, >> >> > Hi, sending a patch as attachment is bad, you can refer to the kernel >> doc: >> >> Thank to Dave for pointing that out. I apologize for not realizing this >> before, and resend it as following. >> >> > I think you are right, the else branch can work well regardless of >> whether vmcoreinfo is present or not. So I changed the execution order of >> the code: >> > Step 1. Try to get THREAD_SHIFT from tbnz instruction. >> > Step 2. If cannot get THREAD_SHIFT by step 1, change the thread_shift >> when KASAN is enabled and with vmcoreinfo. >> > Please help review this new patch. >> > > Hmm, No. I would recommend doing it as below: > > ... > /* Having the vmcoreinfo data and enabling KASAN */ > if (kernel_symbol_exists("vmcoreinfo_data") && > kernel_symbol_exists("vmcoreinfo_size") && > kernel_symbol_exists("kasan_enable_current")) { > min_thread_shift += 1; > thread_shift = (min_thread_shift < machdep->pageshift) ? > machdep->pageshift : min_thread_shift; > } else { > sprintf(buf1, "x/32i vectors"); > ... > } > ... > > if (!thread_shift) > return 0; > > return ((1UL) << thread_shift); > } > > > > When having the vmcoreinfo data and disabling KASAN, crash tool can work > well without your patches, It should be like this: > > ... > /* Having the vmcoreinfo data */ > } else { > sprintf(buf1, "x/32i vectors"); > ... > } > ... > > if (!thread_shift) > return 0; > > return ((1UL) << thread_shift); > } > > Just an idea. Can you help double check? Yeping. > > Thanks > Lianbo > > >> >> --- >> arm64.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 47 insertions(+), 2 deletions(-) >> >> diff --git a/arm64.c b/arm64.c >> index 78e6609..49799e6 100644 >> --- a/arm64.c >> +++ b/arm64.c >> @@ -94,6 +94,7 @@ static int arm64_is_uvaddr(ulong, struct task_context >> *); >> static void arm64_calc_KERNELPACMASK(void); >> static void arm64_recalc_KERNELPACMASK(void); >> static int arm64_get_vmcoreinfo(unsigned long *vaddr, const char *label, >> int base); >> +static ulong arm64_set_irq_stack_size(void); >> >> struct kernel_range { >> unsigned long modules_vaddr, modules_end; >> @@ -2234,8 +2235,10 @@ 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->irq_stack_size = (res > 0) ? res : >> ARM64_IRQ_STACK_SIZE; >> + } >> >> machdep->flags |= IRQ_STACKS; >> >> @@ -4950,6 +4953,48 @@ static void arm64_recalc_KERNELPACMASK(void){ >> } >> } >> >> +static ulong arm64_set_irq_stack_size(void) >> +{ >> + int min_thread_shift = 14; >> + ulong thread_shift = 0; >> + char buf1[BUFSIZE]; >> + char *pos1, *pos2; >> + int errflag = 0; >> + >> + sprintf(buf1, "x/32i vectors"); >> + open_tmpfile(); >> + if (!gdb_pass_through(buf1, pc->tmpfile, GNU_RETURN_ON_ERROR)) { >> + goto out; >> + } >> + rewind(pc->tmpfile); >> + while (fgets(buf1, BUFSIZE, pc->tmpfile)) { >> + if ((pos1 = strstr(buf1, "tbnz"))) { >> + 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; >> + } >> + } >> + } >> +out: >> + close_tmpfile(); >> + >> + if ((!thread_shift) && >> kernel_symbol_exists("kasan_enable_current")) { >> + min_thread_shift += 1; >> + thread_shift = (min_thread_shift < machdep->pageshift) ? >> machdep->pageshift : min_thread_shift; >> + } >> + >> + if (!thread_shift) >> + return 0; >> + else >> + return ((1UL) << thread_shift); >> +} >> + >> #endif /* ARM64 */ >> >> >> Thanks >> yeping. >> >> Dave Young <dyoung(a)redhat.com&gt; 于2024年7月31日周三 14:58写道： >> >>> > Please help review this new patch. >>> >>> Hi, sending a patch as attachment is bad, you can refer to the kernel >>> doc: >>> https://www.kernel.org/doc/html/v4.14/process/email-clients.html >>> >>> "Patches for the Linux kernel are submitted via email, preferably as >>> inline text in the body of the email. Some maintainers accept >>> attachments, but then the attachments should have content-type >>> text/plain. However, attachments are generally frowned upon because it >>> makes quoting portions of the patch more difficult in the patch review >>> process." >>> >>> Crash is different from kernel but the email process is similar, I >>> think you can resend it with a proper format instead of maintainers >>> manually copying them in the email body for review. >>> >>> Thanks >>> Dave >>> >>>

Hi Tao, I have tested this new patch-v2, and it works well in both branches. Thanks, Yeping Tao Liu <ltao(a)redhat.com&gt; 于2024年8月14日周三 07:41写道：

Hi, I think KASAN not only affects irq_stack_size but also normal stack size. Should we need to modify normal stack size as well? Thanks, Kuan-Ying Lee