A error stack trace of bt cmd observed: crash> bt 1 PID: 1 TASK: c000000003714b80 CPU: 2 COMMAND: "systemd" #0 [c0000000037735c0] _end at c0000000037154b0 (unreliable) #1 [c000000003773770] __switch_to at c00000000001fa9c #2 [c0000000037737d0] __schedule at c00000000112e4ec #3 [c0000000037738b0] schedule at c00000000112ea80 ... The #0 stack trace is incorrect, the function address shouldn't exceed _end. The reason is for kernel>=v6.2, the offset of pt_regs to sp changed from STACK_FRAME_OVERHEAD, i.e 112, to STACK_SWITCH_FRAME_REGS. For CONFIG_PPC64_ELF_ABI_V1, it's 112, for ABI_V2, it's 48. So the nip will read a wrong value from stack when ABI_V2 enabled. To determine if ABI_V2 enabled is tricky. This patch do it by check the following: In arch/powerpc/include/asm/ppc_asm.h: #ifdef CONFIG_PPC64_ELF_ABI_V2 #define STK_GOT 24 #else #define STK_GOT 40 In arch/powerpc/kernel/tm.S: _GLOBAL(tm_reclaim) mfcr r5 mflr r0 stw r5, 8(r1) std r0, 16(r1) std r2, STK_GOT(r1) ... So a disassemble on tm_reclaim, and extract the STK_GOT value from std instruction is used as the approach. After the patch: crash> bt 1 PID: 1 TASK: c000000003714b80 CPU: 2 COMMAND: "systemd" #0 [c0000000037737d0] __schedule at c00000000112e4ec #1 [c0000000037738b0] schedule at c00000000112ea80 ... Signed-off-by: Tao Liu <ltao(a)redhat.com&gt; --- defs.h | 1 + ppc64.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 58 insertions(+), 3 deletions(-) diff --git a/defs.h b/defs.h index 2231cb6..d5cb8cc 100644 --- a/defs.h +++ b/defs.h @@ -4643,6 +4643,7 @@ struct efi_memory_desc_t { #define MSR_PR_LG 14 /* Problem State / Privilege Level */ /* Used to find the user or kernel-mode frame*/ +#define STACK_SWITCH_FRAME_REGS 48 #define STACK_FRAME_OVERHEAD 112 #define EXCP_FRAME_MARKER 0x7265677368657265 diff --git a/ppc64.c b/ppc64.c index e8930a1..6e5f155 100644 --- a/ppc64.c +++ b/ppc64.c @@ -72,6 +72,7 @@ static ulong pud_page_vaddr_l4(ulong pud); static ulong pmd_page_vaddr_l4(ulong pmd); static int is_opal_context(ulong sp, ulong nip); void opalmsg(void); +static bool is_ppc64_elf_abi_v2(void); static int is_opal_context(ulong sp, ulong nip) { @@ -2813,6 +2814,51 @@ ppc64_get_sp(ulong task) return sp; } +static bool +is_ppc64_elf_abi_v2(void) +{

Hi Tao, On 24/09/22 10:25PM, Aditya Gupta wrote: > Hi Tao, > > > On 18/09/24 05:12, Tao Liu wrote: > > A error stack trace of bt cmd observed: > > > > crash> bt 1 > > PID: 1 TASK: c000000003714b80 CPU: 2 COMMAND: "systemd" > > #0 [c0000000037735c0] _end at c0000000037154b0 (unreliable) > > #1 [c000000003773770] __switch_to at c00000000001fa9c > > #2 [c0000000037737d0] __schedule at c00000000112e4ec > > #3 [c0000000037738b0] schedule at c00000000112ea80 > > ... > > > > The #0 stack trace is incorrect, the function address shouldn't exceed _end. > > The reason is for kernel>=v6.2, the offset of pt_regs to sp changed from > > STACK_FRAME_OVERHEAD, i.e 112, to STACK_SWITCH_FRAME_REGS. For > > CONFIG_PPC64_ELF_ABI_V1, it's 112, for ABI_V2, it's 48. So the nip will read a > > wrong value from stack when ABI_V2 enabled. > > > > To determine if ABI_V2 enabled is tricky. This patch do it by check the > > following: > > > > In arch/powerpc/include/asm/ppc_asm.h: > > #ifdef CONFIG_PPC64_ELF_ABI_V2 > > #define STK_GOT 24 > > #else > > #define STK_GOT 40 > > > > In arch/powerpc/kernel/tm.S: > > _GLOBAL(tm_reclaim) > > mfcr r5 > > mflr r0 > > stw r5, 8(r1) > > std r0, 16(r1) > > std r2, STK_GOT(r1) > > ... > > > > So a disassemble on tm_reclaim, and extract the STK_GOT value from std > > instruction is used as the approach. > > > > After the patch: > > crash> bt 1 > > PID: 1 TASK: c000000003714b80 CPU: 2 COMMAND: "systemd" > > #0 [c0000000037737d0] __schedule at c00000000112e4ec > > #1 [c0000000037738b0] schedule at c00000000112ea80 > > ... > > > > Signed-off-by: Tao Liu <ltao(a)redhat.com&gt; > > --- > > defs.h | 1 + > > ppc64.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--- > > 2 files changed, 58 insertions(+), 3 deletions(-) > > > > diff --git a/defs.h b/defs.h > > index 2231cb6..d5cb8cc 100644 > > --- a/defs.h > > +++ b/defs.h > > @@ -4643,6 +4643,7 @@ struct efi_memory_desc_t { > > #define MSR_PR_LG 14 /* Problem State / Privilege Level */ > > /* Used to find the user or kernel-mode frame*/ > > +#define STACK_SWITCH_FRAME_REGS 48 > > #define STACK_FRAME_OVERHEAD 112 > > #define EXCP_FRAME_MARKER 0x7265677368657265 > > diff --git a/ppc64.c b/ppc64.c > > index e8930a1..6e5f155 100644 > > --- a/ppc64.c > > +++ b/ppc64.c > > @@ -72,6 +72,7 @@ static ulong pud_page_vaddr_l4(ulong pud); > > static ulong pmd_page_vaddr_l4(ulong pmd); > > static int is_opal_context(ulong sp, ulong nip); > > void opalmsg(void); > > +static bool is_ppc64_elf_abi_v2(void); > > static int is_opal_context(ulong sp, ulong nip) > > { > > @@ -2813,6 +2814,51 @@ ppc64_get_sp(ulong task) > > return sp; > > } > > +static bool > > +is_ppc64_elf_abi_v2(void) > > +{ > > > Thanks for fixing these issues on ppc64 ! > > > It is good. I am also checking if there is a way to implement > 'is_ppc64_elf_abi_v2' by getting ABI version from the ELF notes, as 'file' > command somehow does know: > > $ file vmlinux-little-endian vmlinux-little-endian: ELF 64-bit LSB > executable, 64-bit PowerPC or cisco 7500, OpenPOWER ELF V2 ABI, version 1 > (SYSV), statically linked, > BuildID[sha1]=f6449970e6af6cddeed1e260393e2377ccc0f369, not stripped > > That might be more dependable (if we can do that). How about this approach ? I feel this might be more dependable, as the file command also uses this, as well as fadump (PowerPC firmware-assisted dump) in the kernel. > As a sidenote, there is still some issue either in patch #1 or #3 > causing all secondary CPUs (where swapper task is running) to show all > threads as '_end' What do you think ?

Either way what you chose, I do realise the idea to utilise the 'std r2, STK_GOT(r1)' in such a way, was interesting and smart. static bool is_ppc64_elf_abi_v2(void) { const char *kernel_file = pc->namelist; int fd, swap; char eheader[BUFSIZE]; Elf64_Ehdr *elf64; if ((fd = open(kernel_file, O_RDONLY)) < 0) { error(INFO, "%s: %s\n", kernel_file, strerror(errno)); return FALSE; } if (read(fd, eheader, BUFSIZE) != BUFSIZE) { error(INFO, "%s: %s\n", kernel_file, strerror(errno)); close(fd); return FALSE; } close(fd); if (!STRNEQ(eheader, ELFMAG) || eheader[EI_VERSION] != EV_CURRENT) return FALSE; elf64 = (Elf64_Ehdr *)&eheader[0]; swap = (((eheader[EI_DATA] == ELFDATA2LSB) && (__BYTE_ORDER == __BIG_ENDIAN)) || ((eheader[EI_DATA] == ELFDATA2MSB) && (__BYTE_ORDER == __LITTLE_ENDIAN))); /* * The ABI version is stored in E_FLAGS in the ELF header, atleast for * ppc64 binaries. * * These logic is also used by the 'file' command, which states this in * its magic file: * * >18 leshort 20 PowerPC or cisco 4500, * >18 leshort 21 64-bit PowerPC or cisco 7500, * >>48 lelong 0 Unspecified or Power ELF V1 ABI, * >>48 lelong 1 Power ELF V1 ABI, * >>48 lelong 2 OpenPOWER ELF V2 ABI, * * The '>18' above means, data at 18 (0x12), which is basically * 'elf64->e_machine', which will be 20 for PPC, and 21 for PPC64 * * Similarly, '>>48' is offset 48 (0x30) in the file, which is * basically 'elf64->e_flags', which has the ELF ABI version */ if (elf64->e_flags == 2) { /* ABI v2 */ return TRUE; } else if (elf64->e_flags == 1) { /* ABI v1 */ return FALSE; } error(WARNING, "Unstable elf_abi v1/v2 detection.\n"); return FALSE; } Also, we can those values in the ELF header: $ hexdump vmlinux-little-endian 0000000 457f 464c 0102 0001 0000 0000 0000 0000 0000010 0002 0015 (= 21 = E_MACHINE) 0001 0000 0000 0000 0000 c000 0000020 0040 0000 0000 0000 8bf8 0040 0000 0000 0000030 0002 (= E_FLAGS = ABI) 0000 0040 0038 0002 0040 0027 0026 0000040 0001 0000 0007 0000 0000 0001 0000 0000 Also, I see 'pc->namelist' is basically the kernel filename, is that correct ? Can that be renamed to kernel_filename or something ? Thanks, Aditya Gupta > > Thanks, > > Aditya Gupta > > > + char buf1[BUFSIZE]; > > + char *pos1, *pos2; > > + int errflag = 0; > > + ulong stk_got = 0; > > + static bool ret = false; > > + static bool checked = false; > > + > > + if (checked == true || !symbol_exists("tm_reclaim")) > > + return ret; > > + > > + sprintf(buf1, "x/16i tm_reclaim"); > > + open_tmpfile(); > > + if (!gdb_pass_through(buf1, pc->tmpfile, GNU_RETURN_ON_ERROR)) > > + goto out; > > + checked = true; > > + rewind(pc->tmpfile); > > + while (fgets(buf1, BUFSIZE, pc->tmpfile)) { > > + // "std r2, STK_GOT(r1)" is expected > > + if (strstr(buf1, "std") && > > + strstr(buf1, "(r1)") && > > + (pos1 = strstr(buf1, "r2,"))) { > > + pos1 += strlen("r2,"); > > + for (pos2 = pos1; *pos2 != '\0' && *pos2 != '('; pos2++); > > + *pos2 = '\0'; > > + stk_got = stol(pos1, RETURN_ON_ERROR|QUIET, &errflag); > > + break; > > + } > > + } > > + > > + if (!errflag) { > > + switch (stk_got) { > > + case 24: > > + ret = true; > > + case 40: > > + goto out; > > + } > > + } > > + error(WARNING, "Unstable elf_abi v1/v2 detection.\n"); > > +out: > > + close_tmpfile(); > > + return ret; > > +} > > /* > > * get the SP and PC values for idle tasks. > > @@ -2834,9 +2880,17 @@ get_ppc64_frame(struct bt_info *bt, ulong *getpc, ulong *getsp) > > sp = ppc64_get_sp(task); > > if (!INSTACK(sp, bt)) > > goto out; > > - readmem(sp+STACK_FRAME_OVERHEAD, KVADDR, &regs, > > - sizeof(struct ppc64_pt_regs), > > - "PPC64 pt_regs", FAULT_ON_ERROR); > > + > > + if (THIS_KERNEL_VERSION >= LINUX(6,2,0) && is_ppc64_elf_abi_v2()) { > > + readmem(sp+STACK_SWITCH_FRAME_REGS, KVADDR, &regs, > > + sizeof(struct ppc64_pt_regs), > > + "PPC64 pt_regs", FAULT_ON_ERROR); > > + } else { > > + readmem(sp+STACK_FRAME_OVERHEAD, KVADDR, &regs, > > + sizeof(struct ppc64_pt_regs), > > + "PPC64 pt_regs", FAULT_ON_ERROR); > > + } > > + > > ip = regs.nip; > > closest = closest_symbol(ip); > > if (STREQ(closest, ".__switch_to") || STREQ(closest, "__switch_to")) {

Hi Tao, On 24/09/22 10:25PM, Aditya Gupta wrote: > Hi Tao, > > > On 18/09/24 05:12, Tao Liu wrote: > > A error stack trace of bt cmd observed: > > > > crash> bt 1 > > PID: 1 TASK: c000000003714b80 CPU: 2 COMMAND: "systemd" > > #0 [c0000000037735c0] _end at c0000000037154b0 (unreliable) > > #1 [c000000003773770] __switch_to at c00000000001fa9c > > #2 [c0000000037737d0] __schedule at c00000000112e4ec > > #3 [c0000000037738b0] schedule at c00000000112ea80 > > ... > > > > The #0 stack trace is incorrect, the function address shouldn't exceed _end. > > The reason is for kernel>=v6.2, the offset of pt_regs to sp changed from > > STACK_FRAME_OVERHEAD, i.e 112, to STACK_SWITCH_FRAME_REGS. For > > CONFIG_PPC64_ELF_ABI_V1, it's 112, for ABI_V2, it's 48. So the nip will read a > > wrong value from stack when ABI_V2 enabled. > > > > To determine if ABI_V2 enabled is tricky. This patch do it by check the > > following: > > > > In arch/powerpc/include/asm/ppc_asm.h: > > #ifdef CONFIG_PPC64_ELF_ABI_V2 > > #define STK_GOT 24 > > #else > > #define STK_GOT 40 > > > > In arch/powerpc/kernel/tm.S: > > _GLOBAL(tm_reclaim) > > mfcr r5 > > mflr r0 > > stw r5, 8(r1) > > std r0, 16(r1) > > std r2, STK_GOT(r1) > > ... > > > > So a disassemble on tm_reclaim, and extract the STK_GOT value from std > > instruction is used as the approach. > > > > After the patch: > > crash> bt 1 > > PID: 1 TASK: c000000003714b80 CPU: 2 COMMAND: "systemd" > > #0 [c0000000037737d0] __schedule at c00000000112e4ec > > #1 [c0000000037738b0] schedule at c00000000112ea80 > > ... > > > > Signed-off-by: Tao Liu <ltao(a)redhat.com&gt; > > --- > > defs.h | 1 + > > ppc64.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--- > > 2 files changed, 58 insertions(+), 3 deletions(-) > > > > diff --git a/defs.h b/defs.h > > index 2231cb6..d5cb8cc 100644 > > --- a/defs.h > > +++ b/defs.h > > @@ -4643,6 +4643,7 @@ struct efi_memory_desc_t { > > #define MSR_PR_LG 14 /* Problem State / Privilege Level */ > > /* Used to find the user or kernel-mode frame*/ > > +#define STACK_SWITCH_FRAME_REGS 48 > > #define STACK_FRAME_OVERHEAD 112 > > #define EXCP_FRAME_MARKER 0x7265677368657265 > > diff --git a/ppc64.c b/ppc64.c > > index e8930a1..6e5f155 100644 > > --- a/ppc64.c > > +++ b/ppc64.c > > @@ -72,6 +72,7 @@ static ulong pud_page_vaddr_l4(ulong pud); > > static ulong pmd_page_vaddr_l4(ulong pmd); > > static int is_opal_context(ulong sp, ulong nip); > > void opalmsg(void); > > +static bool is_ppc64_elf_abi_v2(void); > > static int is_opal_context(ulong sp, ulong nip) > > { > > @@ -2813,6 +2814,51 @@ ppc64_get_sp(ulong task) > > return sp; > > } > > +static bool > > +is_ppc64_elf_abi_v2(void) > > +{ > > > Thanks for fixing these issues on ppc64 ! > > > It is good. I am also checking if there is a way to implement > 'is_ppc64_elf_abi_v2' by getting ABI version from the ELF notes, as 'file' > command somehow does know: > > $ file vmlinux-little-endian vmlinux-little-endian: ELF 64-bit LSB > executable, 64-bit PowerPC or cisco 7500, OpenPOWER ELF V2 ABI, version 1 > (SYSV), statically linked, > BuildID[sha1]=f6449970e6af6cddeed1e260393e2377ccc0f369, not stripped > > That might be more dependable (if we can do that). How about this approach ? I feel this might be more dependable, as the file command also uses this, as well as fadump (PowerPC firmware-assisted dump) in the kernel. > As a sidenote, there is still some issue either in patch #1 or #3 > causing all secondary CPUs (where swapper task is running) to show all > threads as '_end' What do you think ?

Either way what you chose, I do realise the idea to utilise the 'std r2, STK_GOT(r1)' in such a way, was interesting and smart. static bool is_ppc64_elf_abi_v2(void) { const char *kernel_file = pc->namelist; int fd, swap; char eheader[BUFSIZE]; Elf64_Ehdr *elf64; if ((fd = open(kernel_file, O_RDONLY)) < 0) { error(INFO, "%s: %s\n", kernel_file, strerror(errno)); return FALSE; } if (read(fd, eheader, BUFSIZE) != BUFSIZE) { error(INFO, "%s: %s\n", kernel_file, strerror(errno)); close(fd); return FALSE; } close(fd); if (!STRNEQ(eheader, ELFMAG) || eheader[EI_VERSION] != EV_CURRENT) return FALSE; elf64 = (Elf64_Ehdr *)&eheader[0]; swap = (((eheader[EI_DATA] == ELFDATA2LSB) && (__BYTE_ORDER == __BIG_ENDIAN)) || ((eheader[EI_DATA] == ELFDATA2MSB) && (__BYTE_ORDER == __LITTLE_ENDIAN)));

/* * The ABI version is stored in E_FLAGS in the ELF header, atleast for * ppc64 binaries. * * These logic is also used by the 'file' command, which states this in * its magic file: * * >18 leshort 20 PowerPC or cisco 4500, * >18 leshort 21 64-bit PowerPC or cisco 7500, * >>48 lelong 0 Unspecified or Power ELF V1 ABI, * >>48 lelong 1 Power ELF V1 ABI, * >>48 lelong 2 OpenPOWER ELF V2 ABI, * * The '>18' above means, data at 18 (0x12), which is basically * 'elf64->e_machine', which will be 20 for PPC, and 21 for PPC64 * * Similarly, '>>48' is offset 48 (0x30) in the file, which is * basically 'elf64->e_flags', which has the ELF ABI version */ if (elf64->e_flags == 2) { /* ABI v2 */ return TRUE; } else if (elf64->e_flags == 1) { /* ABI v1 */ return FALSE; } error(WARNING, "Unstable elf_abi v1/v2 detection.\n"); return FALSE; } Also, we can those values in the ELF header: $ hexdump vmlinux-little-endian 0000000 457f 464c 0102 0001 0000 0000 0000 0000 0000010 0002 0015 (= 21 = E_MACHINE) 0001 0000 0000 0000 0000 c000 0000020 0040 0000 0000 0000 8bf8 0040 0000 0000 0000030 0002 (= E_FLAGS = ABI) 0000 0040 0038 0002 0040 0027 0026 0000040 0001 0000 0007 0000 0000 0001 0000 0000

Also, I see 'pc->namelist' is basically the kernel filename, is that correct ? Can that be renamed to kernel_filename or something ?

Thanks, Aditya Gupta > > Thanks, > > Aditya Gupta > > > + char buf1[BUFSIZE]; > > + char *pos1, *pos2; > > + int errflag = 0; > > + ulong stk_got = 0; > > + static bool ret = false; > > + static bool checked = false; > > + > > + if (checked == true || !symbol_exists("tm_reclaim")) > > + return ret; > > + > > + sprintf(buf1, "x/16i tm_reclaim"); > > + open_tmpfile(); > > + if (!gdb_pass_through(buf1, pc->tmpfile, GNU_RETURN_ON_ERROR)) > > + goto out; > > + checked = true; > > + rewind(pc->tmpfile); > > + while (fgets(buf1, BUFSIZE, pc->tmpfile)) { > > + // "std r2, STK_GOT(r1)" is expected > > + if (strstr(buf1, "std") && > > + strstr(buf1, "(r1)") && > > + (pos1 = strstr(buf1, "r2,"))) { > > + pos1 += strlen("r2,"); > > + for (pos2 = pos1; *pos2 != '\0' && *pos2 != '('; pos2++); > > + *pos2 = '\0'; > > + stk_got = stol(pos1, RETURN_ON_ERROR|QUIET, &errflag); > > + break; > > + } > > + } > > + > > + if (!errflag) { > > + switch (stk_got) { > > + case 24: > > + ret = true; > > + case 40: > > + goto out; > > + } > > + } > > + error(WARNING, "Unstable elf_abi v1/v2 detection.\n"); > > +out: > > + close_tmpfile(); > > + return ret; > > +} > > /* > > * get the SP and PC values for idle tasks. > > @@ -2834,9 +2880,17 @@ get_ppc64_frame(struct bt_info *bt, ulong *getpc, ulong *getsp) > > sp = ppc64_get_sp(task); > > if (!INSTACK(sp, bt)) > > goto out; > > - readmem(sp+STACK_FRAME_OVERHEAD, KVADDR, &regs, > > - sizeof(struct ppc64_pt_regs), > > - "PPC64 pt_regs", FAULT_ON_ERROR); > > + > > + if (THIS_KERNEL_VERSION >= LINUX(6,2,0) && is_ppc64_elf_abi_v2()) { > > + readmem(sp+STACK_SWITCH_FRAME_REGS, KVADDR, &regs, > > + sizeof(struct ppc64_pt_regs), > > + "PPC64 pt_regs", FAULT_ON_ERROR); > > + } else { > > + readmem(sp+STACK_FRAME_OVERHEAD, KVADDR, &regs, > > + sizeof(struct ppc64_pt_regs), > > + "PPC64 pt_regs", FAULT_ON_ERROR); > > + } > > + > > ip = regs.nip; > > closest = closest_symbol(ip); > > if (STREQ(closest, ".__switch_to") || STREQ(closest, "__switch_to")) {

Hi Tao, On 04/10/24 10:40, Tao Lou wrote: > Hi Aditya & lianbo, > > On Tue, Sep 24, 2024 at 2:28 AM Aditya Gupta <adityag(a)linux.ibm.com&gt; wrote: >> Hi Tao, >> >> On 24/09/22 10:25PM, Aditya Gupta wrote: >>> Hi Tao, >>> >>> >>> On 18/09/24 05:12, Tao Liu wrote: >>>> A error stack trace of bt cmd observed: >>>> >>>> crash> bt 1 >>>> PID: 1 TASK: c000000003714b80 CPU: 2 COMMAND: "systemd" >>>> #0 [c0000000037735c0] _end at c0000000037154b0 (unreliable) >>>> #1 [c000000003773770] __switch_to at c00000000001fa9c >>>> #2 [c0000000037737d0] __schedule at c00000000112e4ec >>>> #3 [c0000000037738b0] schedule at c00000000112ea80 >>>> ... >>>> >>>> The #0 stack trace is incorrect, the function address shouldn't exceed _end. >>>> The reason is for kernel>=v6.2, the offset of pt_regs to sp changed from >>>> STACK_FRAME_OVERHEAD, i.e 112, to STACK_SWITCH_FRAME_REGS. For >>>> CONFIG_PPC64_ELF_ABI_V1, it's 112, for ABI_V2, it's 48. So the nip will read a >>>> wrong value from stack when ABI_V2 enabled. >>>> >>>> To determine if ABI_V2 enabled is tricky. This patch do it by check the >>>> following: >>>> >>>> In arch/powerpc/include/asm/ppc_asm.h: >>>> #ifdef CONFIG_PPC64_ELF_ABI_V2 >>>> #define STK_GOT 24 >>>> #else >>>> #define STK_GOT 40 >>>> >>>> In arch/powerpc/kernel/tm.S: >>>> _GLOBAL(tm_reclaim) >>>> mfcr r5 >>>> mflr r0 >>>> stw r5, 8(r1) >>>> std r0, 16(r1) >>>> std r2, STK_GOT(r1) >>>> ... >>>> >>>> So a disassemble on tm_reclaim, and extract the STK_GOT value from std >>>> instruction is used as the approach. >>>> >>>> After the patch: >>>> crash> bt 1 >>>> PID: 1 TASK: c000000003714b80 CPU: 2 COMMAND: "systemd" >>>> #0 [c0000000037737d0] __schedule at c00000000112e4ec >>>> #1 [c0000000037738b0] schedule at c00000000112ea80 >>>> ... >>>> >>>> Signed-off-by: Tao Liu <ltao(a)redhat.com&gt; >>>> --- >>>> defs.h | 1 + >>>> ppc64.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--- >>>> 2 files changed, 58 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/defs.h b/defs.h >>>> index 2231cb6..d5cb8cc 100644 >>>> --- a/defs.h >>>> +++ b/defs.h >>>> @@ -4643,6 +4643,7 @@ struct efi_memory_desc_t { >>>> #define MSR_PR_LG 14 /* Problem State / Privilege Level */ >>>> /* Used to find the user or kernel-mode frame*/ >>>> +#define STACK_SWITCH_FRAME_REGS 48 >>>> #define STACK_FRAME_OVERHEAD 112 >>>> #define EXCP_FRAME_MARKER 0x7265677368657265 >>>> diff --git a/ppc64.c b/ppc64.c >>>> index e8930a1..6e5f155 100644 >>>> --- a/ppc64.c >>>> +++ b/ppc64.c >>>> @@ -72,6 +72,7 @@ static ulong pud_page_vaddr_l4(ulong pud); >>>> static ulong pmd_page_vaddr_l4(ulong pmd); >>>> static int is_opal_context(ulong sp, ulong nip); >>>> void opalmsg(void); >>>> +static bool is_ppc64_elf_abi_v2(void); >>>> static int is_opal_context(ulong sp, ulong nip) >>>> { >>>> @@ -2813,6 +2814,51 @@ ppc64_get_sp(ulong task) >>>> return sp; >>>> } >>>> +static bool >>>> +is_ppc64_elf_abi_v2(void) >>>> +{ >>> >>> Thanks for fixing these issues on ppc64 ! >>> >>> >>> It is good. I am also checking if there is a way to implement >>> 'is_ppc64_elf_abi_v2' by getting ABI version from the ELF notes, as 'file' >>> command somehow does know: >>> >>> $ file vmlinux-little-endian vmlinux-little-endian: ELF 64-bit LSB >>> executable, 64-bit PowerPC or cisco 7500, OpenPOWER ELF V2 ABI, version 1 >>> (SYSV), statically linked, >>> BuildID[sha1]=f6449970e6af6cddeed1e260393e2377ccc0f369, not stripped >>> >>> That might be more dependable (if we can do that). >> How about this approach ? >> >> I feel this might be more dependable, as the file command also uses >> this, as well as fadump (PowerPC firmware-assisted dump) in the kernel. >> >>> As a sidenote, there is still some issue either in patch #1 or #3 >>> causing all secondary CPUs (where swapper task is running) to show all >>> threads as '_end' >> What do you think ? > I think your approach will work, thanks for your code! Please see my > comments inlined in the code: > >> Either way what you chose, I do realise the idea to utilise the >> 'std r2, STK_GOT(r1)' in such a way, was interesting and smart. >> >> static bool >> is_ppc64_elf_abi_v2(void) >> { >> const char *kernel_file = pc->namelist; >> int fd, swap; >> char eheader[BUFSIZE]; >> Elf64_Ehdr *elf64; >> >> if ((fd = open(kernel_file, O_RDONLY)) < 0) { >> error(INFO, "%s: %s\n", kernel_file, strerror(errno)); >> return FALSE; >> } >> if (read(fd, eheader, BUFSIZE) != BUFSIZE) { >> error(INFO, "%s: %s\n", kernel_file, strerror(errno)); >> close(fd); >> return FALSE; >> } >> close(fd); >> >> if (!STRNEQ(eheader, ELFMAG) || eheader[EI_VERSION] != EV_CURRENT) >> return FALSE; >> >> elf64 = (Elf64_Ehdr *)&eheader[0]; >> >> swap = (((eheader[EI_DATA] == ELFDATA2LSB) && >> (__BYTE_ORDER == __BIG_ENDIAN)) || >> ((eheader[EI_DATA] == ELFDATA2MSB) && >> (__BYTE_ORDER == __LITTLE_ENDIAN))); > The above code seems duplicated with the one in is_kernel(), in fact, > is_kernel() will be invoked at the very beginning to determine if one > file is vmlinux, so I would suggest to add our abi_v2 check here, > like: > > in ppc64.c: > int ppc64_elf_abi = 0; > > static bool is_ppc64_elf_abi_v2(void) > { > if (ppc64_elf_abi == 1) > ... > else if (ppc64_elf_abi == 2) > ... > else > ... > } > > in symbols.c:is_kernel(): > ... > } else if ((elf64->e_ident[EI_CLASS] == ELFCLASS64) && > ((swap16(elf64->e_type, swap) == ET_EXEC) || > ... > case EM_PPC64: > if (machine_type_mismatch(file, "PPC64", NULL, 0)) > goto bailout; > + else > + ppc64_elf_abi = swap32(elf64->e_flags, swap); > break; > > So we can initial the abi version at crash start only once, and get > rid of the code duplication. Yes, I actually copied pasted the initial part from is_kernel. The approach is good, just to clarify, we know that is_kernel will be called everytime before this check right ?

Thanks, Aditya Gupta > >> /* >> * The ABI version is stored in E_FLAGS in the ELF header, atleast for >> * ppc64 binaries. >> * >> * These logic is also used by the 'file' command, which states this in >> * its magic file: >> * >> * >18 leshort 20 PowerPC or cisco 4500, >> * >18 leshort 21 64-bit PowerPC or cisco 7500, >> * >>48 lelong 0 Unspecified or Power ELF V1 ABI, >> * >>48 lelong 1 Power ELF V1 ABI, >> * >>48 lelong 2 OpenPOWER ELF V2 ABI, >> * >> * The '>18' above means, data at 18 (0x12), which is basically >> * 'elf64->e_machine', which will be 20 for PPC, and 21 for PPC64 >> * >> * Similarly, '>>48' is offset 48 (0x30) in the file, which is >> * basically 'elf64->e_flags', which has the ELF ABI version >> */ >> if (elf64->e_flags == 2) { >> /* ABI v2 */ >> return TRUE; >> } else if (elf64->e_flags == 1) { >> /* ABI v1 */ >> return FALSE; >> } >> >> error(WARNING, "Unstable elf_abi v1/v2 detection.\n"); >> return FALSE; >> } >> >> Also, we can those values in the ELF header: >> >> $ hexdump vmlinux-little-endian >> 0000000 457f 464c 0102 0001 0000 0000 0000 0000 >> 0000010 0002 0015 (= 21 = E_MACHINE) 0001 0000 0000 0000 0000 c000 >> 0000020 0040 0000 0000 0000 8bf8 0040 0000 0000 >> 0000030 0002 (= E_FLAGS = ABI) 0000 0040 0038 0002 0040 0027 0026 >> 0000040 0001 0000 0007 0000 0000 0001 0000 0000 >> > I agree with the elf header check on the vmlinux file, it is much > simpler. Also I compiled the kernel with CONFIG_PPC64_ELF_ABI_V1 on, > the file cmd will give: Power ELF V1 ABI string. So the approach can > work. > >> Also, I see 'pc->namelist' is basically the kernel filename, is that >> correct ? Can that be renamed to kernel_filename or something ? >> > Yes, by reading through the code, pc->namelist is vmlinux, and > pc->namelist_debug is the debug symbol if vmlinux doesn't have the > symbol. But I didn't see the need for the rename. > > Thanks, > Tao Liu > >> Thanks, >> Aditya Gupta >> >>> Thanks, >>> >>> Aditya Gupta >>> >>>> + char buf1[BUFSIZE]; >>>> + char *pos1, *pos2; >>>> + int errflag = 0; >>>> + ulong stk_got = 0; >>>> + static bool ret = false; >>>> + static bool checked = false; >>>> + >>>> + if (checked == true || !symbol_exists("tm_reclaim")) >>>> + return ret; >>>> + >>>> + sprintf(buf1, "x/16i tm_reclaim"); >>>> + open_tmpfile(); >>>> + if (!gdb_pass_through(buf1, pc->tmpfile, GNU_RETURN_ON_ERROR)) >>>> + goto out; >>>> + checked = true; >>>> + rewind(pc->tmpfile); >>>> + while (fgets(buf1, BUFSIZE, pc->tmpfile)) { >>>> + // "std r2, STK_GOT(r1)" is expected >>>> + if (strstr(buf1, "std") && >>>> + strstr(buf1, "(r1)") && >>>> + (pos1 = strstr(buf1, "r2,"))) { >>>> + pos1 += strlen("r2,"); >>>> + for (pos2 = pos1; *pos2 != '\0' && *pos2 != '('; pos2++); >>>> + *pos2 = '\0'; >>>> + stk_got = stol(pos1, RETURN_ON_ERROR|QUIET, &errflag); >>>> + break; >>>> + } >>>> + } >>>> + >>>> + if (!errflag) { >>>> + switch (stk_got) { >>>> + case 24: >>>> + ret = true; >>>> + case 40: >>>> + goto out; >>>> + } >>>> + } >>>> + error(WARNING, "Unstable elf_abi v1/v2 detection.\n"); >>>> +out: >>>> + close_tmpfile(); >>>> + return ret; >>>> +} >>>> /* >>>> * get the SP and PC values for idle tasks. >>>> @@ -2834,9 +2880,17 @@ get_ppc64_frame(struct bt_info *bt, ulong *getpc, ulong *getsp) >>>> sp = ppc64_get_sp(task); >>>> if (!INSTACK(sp, bt)) >>>> goto out; >>>> - readmem(sp+STACK_FRAME_OVERHEAD, KVADDR, &regs, >>>> - sizeof(struct ppc64_pt_regs), >>>> - "PPC64 pt_regs", FAULT_ON_ERROR); >>>> + >>>> + if (THIS_KERNEL_VERSION >= LINUX(6,2,0) && is_ppc64_elf_abi_v2()) { >>>> + readmem(sp+STACK_SWITCH_FRAME_REGS, KVADDR, &regs, >>>> + sizeof(struct ppc64_pt_regs), >>>> + "PPC64 pt_regs", FAULT_ON_ERROR); >>>> + } else { >>>> + readmem(sp+STACK_FRAME_OVERHEAD, KVADDR, &regs, >>>> + sizeof(struct ppc64_pt_regs), >>>> + "PPC64 pt_regs", FAULT_ON_ERROR); >>>> + } >>>> + >>>> ip = regs.nip; >>>> closest = closest_symbol(ip); >>>> if (STREQ(closest, ".__switch_to") || STREQ(closest, "__switch_to")) {

On Tue, Oct 8, 2024 at 10:46 PM Tao Liu <ltao(a)redhat.com&gt; wrote: > > Hi Aditya, > > On Tue, Oct 8, 2024 at 10:18 PM Aditya Gupta <adityag(a)linux.ibm.com&gt; wrote: > > > > Hi Tao, > > > > > > On 04/10/24 10:40, Tao Lou wrote: > > > Hi Aditya & lianbo, > > > > > > On Tue, Sep 24, 2024 at 2:28 AM Aditya Gupta <adityag(a)linux.ibm.com&gt; wrote: > > >> Hi Tao, > > >> > > >> On 24/09/22 10:25PM, Aditya Gupta wrote: > > >>> Hi Tao, > > >>> > > >>> > > >>> On 18/09/24 05:12, Tao Liu wrote: > > >>>> A error stack trace of bt cmd observed: > > >>>> > > >>>> crash> bt 1 > > >>>> PID: 1 TASK: c000000003714b80 CPU: 2 COMMAND: "systemd" > > >>>> #0 [c0000000037735c0] _end at c0000000037154b0 (unreliable) > > >>>> #1 [c000000003773770] __switch_to at c00000000001fa9c > > >>>> #2 [c0000000037737d0] __schedule at c00000000112e4ec > > >>>> #3 [c0000000037738b0] schedule at c00000000112ea80 > > >>>> ... > > >>>> > > >>>> The #0 stack trace is incorrect, the function address shouldn't exceed _end. > > >>>> The reason is for kernel>=v6.2, the offset of pt_regs to sp changed from > > >>>> STACK_FRAME_OVERHEAD, i.e 112, to STACK_SWITCH_FRAME_REGS. For > > >>>> CONFIG_PPC64_ELF_ABI_V1, it's 112, for ABI_V2, it's 48. So the nip will read a > > >>>> wrong value from stack when ABI_V2 enabled. > > >>>> > > >>>> To determine if ABI_V2 enabled is tricky. This patch do it by check the > > >>>> following: > > >>>> > > >>>> In arch/powerpc/include/asm/ppc_asm.h: > > >>>> #ifdef CONFIG_PPC64_ELF_ABI_V2 > > >>>> #define STK_GOT 24 > > >>>> #else > > >>>> #define STK_GOT 40 > > >>>> > > >>>> In arch/powerpc/kernel/tm.S: > > >>>> _GLOBAL(tm_reclaim) > > >>>> mfcr r5 > > >>>> mflr r0 > > >>>> stw r5, 8(r1) > > >>>> std r0, 16(r1) > > >>>> std r2, STK_GOT(r1) > > >>>> ... > > >>>> > > >>>> So a disassemble on tm_reclaim, and extract the STK_GOT value from std > > >>>> instruction is used as the approach. > > >>>> > > >>>> After the patch: > > >>>> crash> bt 1 > > >>>> PID: 1 TASK: c000000003714b80 CPU: 2 COMMAND: "systemd" > > >>>> #0 [c0000000037737d0] __schedule at c00000000112e4ec > > >>>> #1 [c0000000037738b0] schedule at c00000000112ea80 > > >>>> ... > > >>>> > > >>>> Signed-off-by: Tao Liu <ltao(a)redhat.com&gt; > > >>>> --- > > >>>> defs.h | 1 + > > >>>> ppc64.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--- > > >>>> 2 files changed, 58 insertions(+), 3 deletions(-) > > >>>> > > >>>> diff --git a/defs.h b/defs.h > > >>>> index 2231cb6..d5cb8cc 100644 > > >>>> --- a/defs.h > > >>>> +++ b/defs.h > > >>>> @@ -4643,6 +4643,7 @@ struct efi_memory_desc_t { > > >>>> #define MSR_PR_LG 14 /* Problem State / Privilege Level */ > > >>>> /* Used to find the user or kernel-mode frame*/ > > >>>> +#define STACK_SWITCH_FRAME_REGS 48 > > >>>> #define STACK_FRAME_OVERHEAD 112 > > >>>> #define EXCP_FRAME_MARKER 0x7265677368657265 > > >>>> diff --git a/ppc64.c b/ppc64.c > > >>>> index e8930a1..6e5f155 100644 > > >>>> --- a/ppc64.c > > >>>> +++ b/ppc64.c > > >>>> @@ -72,6 +72,7 @@ static ulong pud_page_vaddr_l4(ulong pud); > > >>>> static ulong pmd_page_vaddr_l4(ulong pmd); > > >>>> static int is_opal_context(ulong sp, ulong nip); > > >>>> void opalmsg(void); > > >>>> +static bool is_ppc64_elf_abi_v2(void); > > >>>> static int is_opal_context(ulong sp, ulong nip) > > >>>> { > > >>>> @@ -2813,6 +2814,51 @@ ppc64_get_sp(ulong task) > > >>>> return sp; > > >>>> } > > >>>> +static bool > > >>>> +is_ppc64_elf_abi_v2(void) > > >>>> +{ > > >>> > > >>> Thanks for fixing these issues on ppc64 ! > > >>> > > >>> > > >>> It is good. I am also checking if there is a way to implement > > >>> 'is_ppc64_elf_abi_v2' by getting ABI version from the ELF notes, as 'file' > > >>> command somehow does know: > > >>> > > >>> $ file vmlinux-little-endian vmlinux-little-endian: ELF 64-bit LSB > > >>> executable, 64-bit PowerPC or cisco 7500, OpenPOWER ELF V2 ABI, version 1 > > >>> (SYSV), statically linked, > > >>> BuildID[sha1]=f6449970e6af6cddeed1e260393e2377ccc0f369, not stripped > > >>> > > >>> That might be more dependable (if we can do that). > > >> How about this approach ? > > >> > > >> I feel this might be more dependable, as the file command also uses > > >> this, as well as fadump (PowerPC firmware-assisted dump) in the kernel. > > >> > > >>> As a sidenote, there is still some issue either in patch #1 or #3 > > >>> causing all secondary CPUs (where swapper task is running) to show all > > >>> threads as '_end' > > >> What do you think ? > > > I think your approach will work, thanks for your code! Please see my > > > comments inlined in the code: > > > > > >> Either way what you chose, I do realise the idea to utilise the > > >> 'std r2, STK_GOT(r1)' in such a way, was interesting and smart. > > >> > > >> static bool > > >> is_ppc64_elf_abi_v2(void) > > >> { > > >> const char *kernel_file = pc->namelist; > > >> int fd, swap; > > >> char eheader[BUFSIZE]; > > >> Elf64_Ehdr *elf64; > > >> > > >> if ((fd = open(kernel_file, O_RDONLY)) < 0) { > > >> error(INFO, "%s: %s\n", kernel_file, strerror(errno)); > > >> return FALSE; > > >> } > > >> if (read(fd, eheader, BUFSIZE) != BUFSIZE) { > > >> error(INFO, "%s: %s\n", kernel_file, strerror(errno)); > > >> close(fd); > > >> return FALSE; > > >> } > > >> close(fd); > > >> > > >> if (!STRNEQ(eheader, ELFMAG) || eheader[EI_VERSION] != EV_CURRENT) > > >> return FALSE; > > >> > > >> elf64 = (Elf64_Ehdr *)&eheader[0]; > > >> > > >> swap = (((eheader[EI_DATA] == ELFDATA2LSB) && > > >> (__BYTE_ORDER == __BIG_ENDIAN)) || > > >> ((eheader[EI_DATA] == ELFDATA2MSB) && > > >> (__BYTE_ORDER == __LITTLE_ENDIAN))); > > > The above code seems duplicated with the one in is_kernel(), in fact, > > > is_kernel() will be invoked at the very beginning to determine if one > > > file is vmlinux, so I would suggest to add our abi_v2 check here, > > > like: > > > > > > in ppc64.c: > > > int ppc64_elf_abi = 0; > > > > > > static bool is_ppc64_elf_abi_v2(void) > > > { > > > if (ppc64_elf_abi == 1) > > > ... > > > else if (ppc64_elf_abi == 2) > > > ... > > > else > > > ... > > > } > > > > > > in symbols.c:is_kernel(): > > > ... > > > } else if ((elf64->e_ident[EI_CLASS] == ELFCLASS64) && > > > ((swap16(elf64->e_type, swap) == ET_EXEC) || > > > ... > > > case EM_PPC64: > > > if (machine_type_mismatch(file, "PPC64", NULL, 0)) > > > goto bailout; > > > + else > > > + ppc64_elf_abi = swap32(elf64->e_flags, swap); > > > break; > > > > > > So we can initial the abi version at crash start only once, and get > > > rid of the code duplication. > > > > Yes, I actually copied pasted the initial part from is_kernel. > > > > The approach is good, just to clarify, we know that is_kernel will be > > called everytime before this check right ? > > Actually I used another approach rather than is_kernel(), see [1] for > v2 patchset of ppc64 fixing. > The reason is keeping a ppc64 specific variable in the multi-cpu arch > used function is_kernel(), it is hard to make the code as clean. > Hi Aditya, There is one more question about the abi_v1/2. Say if I have a abi_v2 machine, but I want to run a abi_v1 elf executable file. I don't know 1) is it possible to run v1 elf on a v2 kernel machine; 2) if it can, which STACK_FRAME_OVERHEAD will the kernel use, 48 or 112? If STACK_FRAME_OVERHEAD will vary depending on abi_v1/2 of elf executable file, then our approach needs to change.

Currently I have failed to build a abi_v1 elf executable file on the abi_v2 machine, which reports: $ gcc test.c -mabi=elfv1 fatal error: gnu/stubs-64-v1.h: No such file or directory But I don't know where to install the missing header or rpms, or if it is doable. Any ideas? Thanks, Tao Liu > [1]: https://www.mail-archive.com/devel@lists.crash-utility.osci.io/msg01170.html > > Thanks, > Tao Liu > > > > > > > Thanks, > > Aditya Gupta > > > > > > > >> /* > > >> * The ABI version is stored in E_FLAGS in the ELF header, atleast for > > >> * ppc64 binaries. > > >> * > > >> * These logic is also used by the 'file' command, which states this in > > >> * its magic file: > > >> * > > >> * >18 leshort 20 PowerPC or cisco 4500, > > >> * >18 leshort 21 64-bit PowerPC or cisco 7500, > > >> * >>48 lelong 0 Unspecified or Power ELF V1 ABI, > > >> * >>48 lelong 1 Power ELF V1 ABI, > > >> * >>48 lelong 2 OpenPOWER ELF V2 ABI, > > >> * > > >> * The '>18' above means, data at 18 (0x12), which is basically > > >> * 'elf64->e_machine', which will be 20 for PPC, and 21 for PPC64 > > >> * > > >> * Similarly, '>>48' is offset 48 (0x30) in the file, which is > > >> * basically 'elf64->e_flags', which has the ELF ABI version > > >> */ > > >> if (elf64->e_flags == 2) { > > >> /* ABI v2 */ > > >> return TRUE; > > >> } else if (elf64->e_flags == 1) { > > >> /* ABI v1 */ > > >> return FALSE; > > >> } > > >> > > >> error(WARNING, "Unstable elf_abi v1/v2 detection.\n"); > > >> return FALSE; > > >> } > > >> > > >> Also, we can those values in the ELF header: > > >> > > >> $ hexdump vmlinux-little-endian > > >> 0000000 457f 464c 0102 0001 0000 0000 0000 0000 > > >> 0000010 0002 0015 (= 21 = E_MACHINE) 0001 0000 0000 0000 0000 c000 > > >> 0000020 0040 0000 0000 0000 8bf8 0040 0000 0000 > > >> 0000030 0002 (= E_FLAGS = ABI) 0000 0040 0038 0002 0040 0027 0026 > > >> 0000040 0001 0000 0007 0000 0000 0001 0000 0000 > > >> > > > I agree with the elf header check on the vmlinux file, it is much > > > simpler. Also I compiled the kernel with CONFIG_PPC64_ELF_ABI_V1 on, > > > the file cmd will give: Power ELF V1 ABI string. So the approach can > > > work. > > > > > >> Also, I see 'pc->namelist' is basically the kernel filename, is that > > >> correct ? Can that be renamed to kernel_filename or something ? > > >> > > > Yes, by reading through the code, pc->namelist is vmlinux, and > > > pc->namelist_debug is the debug symbol if vmlinux doesn't have the > > > symbol. But I didn't see the need for the rename. > > > > > > Thanks, > > > Tao Liu > > > > > >> Thanks, > > >> Aditya Gupta > > >> > > >>> Thanks, > > >>> > > >>> Aditya Gupta > > >>> > > >>>> + char buf1[BUFSIZE]; > > >>>> + char *pos1, *pos2; > > >>>> + int errflag = 0; > > >>>> + ulong stk_got = 0; > > >>>> + static bool ret = false; > > >>>> + static bool checked = false; > > >>>> + > > >>>> + if (checked == true || !symbol_exists("tm_reclaim")) > > >>>> + return ret; > > >>>> + > > >>>> + sprintf(buf1, "x/16i tm_reclaim"); > > >>>> + open_tmpfile(); > > >>>> + if (!gdb_pass_through(buf1, pc->tmpfile, GNU_RETURN_ON_ERROR)) > > >>>> + goto out; > > >>>> + checked = true; > > >>>> + rewind(pc->tmpfile); > > >>>> + while (fgets(buf1, BUFSIZE, pc->tmpfile)) { > > >>>> + // "std r2, STK_GOT(r1)" is expected > > >>>> + if (strstr(buf1, "std") && > > >>>> + strstr(buf1, "(r1)") && > > >>>> + (pos1 = strstr(buf1, "r2,"))) { > > >>>> + pos1 += strlen("r2,"); > > >>>> + for (pos2 = pos1; *pos2 != '\0' && *pos2 != '('; pos2++); > > >>>> + *pos2 = '\0'; > > >>>> + stk_got = stol(pos1, RETURN_ON_ERROR|QUIET, &errflag); > > >>>> + break; > > >>>> + } > > >>>> + } > > >>>> + > > >>>> + if (!errflag) { > > >>>> + switch (stk_got) { > > >>>> + case 24: > > >>>> + ret = true; > > >>>> + case 40: > > >>>> + goto out; > > >>>> + } > > >>>> + } > > >>>> + error(WARNING, "Unstable elf_abi v1/v2 detection.\n"); > > >>>> +out: > > >>>> + close_tmpfile(); > > >>>> + return ret; > > >>>> +} > > >>>> /* > > >>>> * get the SP and PC values for idle tasks. > > >>>> @@ -2834,9 +2880,17 @@ get_ppc64_frame(struct bt_info *bt, ulong *getpc, ulong *getsp) > > >>>> sp = ppc64_get_sp(task); > > >>>> if (!INSTACK(sp, bt)) > > >>>> goto out; > > >>>> - readmem(sp+STACK_FRAME_OVERHEAD, KVADDR, &regs, > > >>>> - sizeof(struct ppc64_pt_regs), > > >>>> - "PPC64 pt_regs", FAULT_ON_ERROR); > > >>>> + > > >>>> + if (THIS_KERNEL_VERSION >= LINUX(6,2,0) && is_ppc64_elf_abi_v2()) { > > >>>> + readmem(sp+STACK_SWITCH_FRAME_REGS, KVADDR, &regs, > > >>>> + sizeof(struct ppc64_pt_regs), > > >>>> + "PPC64 pt_regs", FAULT_ON_ERROR); > > >>>> + } else { > > >>>> + readmem(sp+STACK_FRAME_OVERHEAD, KVADDR, &regs, > > >>>> + sizeof(struct ppc64_pt_regs), > > >>>> + "PPC64 pt_regs", FAULT_ON_ERROR); > > >>>> + } > > >>>> + > > >>>> ip = regs.nip; > > >>>> closest = closest_symbol(ip); > > >>>> if (STREQ(closest, ".__switch_to") || STREQ(closest, "__switch_to")) { > >

Previously the newsp read from stack is unchecked, it would expect the newsp hold a valid sp pointer of the next stack frame by default, which is not always true, see the following stack: R1: c00000000a327660 NIP: c0000000002b9984 #0 [c00000000a327660] crash_setup_regs at c0000000002b9984 c00000000a327660: c00000000015a7b0 c000000001bc8600 c00000000a327670: 0000000000000000 c000000002e13b54 c00000000a327680: c00000000a327850 0000000000004000 c00000000a327690: c0000000002ba078 c0000000026f2ca8 The newsp will be c00000000015a7b0, which is not a valid stack pointer and as a result it will fail the stack back trace afterwards. This usually happens at the beginning of back trace, once we get the correct sp value, the stack can be back traced successfully.

The same issue also exists in x86_64 arch. However I cannot dive into

gcc code why newer versions will make the inline function like this. Or we can modify crash_setup_regs() to be the following: static inline void __attribute__((always_inline)) crash_setup_regs(...) Any thoughts? Thanks, Tao Liu > > Anyways this patch is also good, doesn't seem problematic for the > previous working cases. > > > Thanks, > > Aditya Gupta > > > This patch will search and check for the valid newsp at the beginning of > > stack back trace. For the above example, a valid newsp would be assigned > > as c00000000a327850. > > > > Before: > > crash> bt > > ... > > #0 [c00000000a327660] crash_setup_regs at c0000000002b9984 > > After: > > crash> bt > > ... > > #0 [c00000000a327660] crash_setup_regs at c0000000002b9984 > > #1 [c00000000a327850] panic at c000000000167de4 > > #2 [c00000000a3278f0] sysrq_handle_crash at c000000000a4d3a8 > > #3 [c00000000a327950] __handle_sysrq at c000000000a4dec4 > > #4 [c00000000a3279f0] write_sysrq_trigger at c000000000a4e984 > > ... > > Signed-off-by: Tao Liu <ltao(a)redhat.com&gt; > > --- > > ppc64.c | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/ppc64.c b/ppc64.c > > index 6e5f155..8af7b8a 100644 > > --- a/ppc64.c > > +++ b/ppc64.c > > @@ -2148,6 +2148,19 @@ ppc64_back_trace(struct gnu_request *req, struct bt_info *bt) > > > > while (INSTACK(req->sp, bt)) { > > newsp = *(ulong *)&bt->stackbuf[req->sp - bt->stackbase]; > > + > > + if (!newpc) { > > + ulong *up; > > + int i; > > + for (i = 0, up = (ulong *)&bt->stackbuf[req->sp - bt->stackbase]; > > + i < (req->sp - bt->stackbase) / sizeof(ulong); i++, up++) { > > + if (INSTACK(*up, bt) && is_kernel_text(*(up + 2))) { > > + newsp = *up; > > + break; > > + } > > + } > > + } > > + > > if ((req->name = closest_symbol(req->pc)) == NULL) { > > if (CRASHDEBUG(1)) { > > error(FATAL, >

Hi Tao, On 03/10/24 16:04, Tao Liu wrote: > Hi Aditya & lianbo, > > On Mon, Sep 23, 2024 at 5:00 AM Aditya Gupta <adityag(a)linux.ibm.com&gt; wrote: >> Hi Tao, >> >> On 18/09/24 05:12, Tao Liu wrote: >>> Previously the newsp read from stack is unchecked, it would expect >>> the newsp hold a valid sp pointer of the next stack frame by default, >>> which is not always true, see the following stack: >>> >>> R1: c00000000a327660 NIP: c0000000002b9984 >>> #0 [c00000000a327660] crash_setup_regs at c0000000002b9984 >>> c00000000a327660: c00000000015a7b0 c000000001bc8600 >>> c00000000a327670: 0000000000000000 c000000002e13b54 >>> c00000000a327680: c00000000a327850 0000000000004000 >>> c00000000a327690: c0000000002ba078 c0000000026f2ca8 >>> >>> The newsp will be c00000000015a7b0, which is not a valid stack pointer >>> and as a result it will fail the stack back trace afterwards. This usually >>> happens at the beginning of back trace, once we get the correct sp value, >>> the stack can be back traced successfully. >> Seems weird that this issue occurred in the first place. The stack frame >> should have a valid previous sp at 0th offset to sp, according to the >> PowerISA document. > I finally figured out the reason for the faulty stack. The reason is > due to gcc, the previous inlined function crash_setup_regs is no > longer inlined, which led to a malformed stack, see the following: > > crash> bt -f > R1: c00000000a327660 NIP: c0000000002b9984 > [NIP : crash_setup_regs+68] > [LR : __crash_kexec+156] > #0 [c00000000a327660] crash_setup_regs at c0000000002b9984 > c00000000a327660: c00000000015a7b0 c000000001bc8600 > c00000000a327670: 0000000000000000 c000000002e13b54 > > crash> dis __crash_kexec > ... > 0xc0000000002ba054 <__crash_kexec+148>: addi r3,r1,40 > 0xc0000000002ba058 <__crash_kexec+152>: bl 0xc0000000002b9948 > <crash_setup_regs+8> > 0xc0000000002ba05c <__crash_kexec+156>: bl 0xc0000000002b7c58 > <crash_save_vmcoreinfo+8> > 0xc0000000002ba060 <__crash_kexec+160>: nop > 0xc0000000002ba064 <__crash_kexec+164>: addi r3,r1,40 > 0xc0000000002ba068 <__crash_kexec+168>: bl 0xc00000000015a6e8 > <machine_crash_shutdown+8> > 0xc0000000002ba06c <__crash_kexec+172>: nop > 0xc0000000002ba070 <__crash_kexec+176>: ld r3,0(r29) > 0xc0000000002ba074 <__crash_kexec+180>: bl 0xc00000000015a738 > <machine_kexec+8> > 0xc0000000002ba078 <__crash_kexec+184>: nop > ... > > crash> dis crash_setup_regs > ... > 0xc0000000002b9948 <crash_setup_regs+8>: mflr r0 > 0xc0000000002b994c <crash_setup_regs+12>: cmpdi r4,0 > 0xc0000000002b9950 <crash_setup_regs+16>: std r0,16(r1) > 0xc0000000002b9954 <crash_setup_regs+20>: stdu r1,-32(r1) > 0xc0000000002b9958 <crash_setup_regs+24>: beq > 0xc0000000002b9980 <crash_setup_regs+64> > ... > 0xc0000000002b9980 <crash_setup_regs+64>: bl > 0xc00000000007a964 <ppc_save_regs> > 0xc0000000002b9984 <crash_setup_regs+68>: nop > ... > > crash> dis ppc_save_regs > 0xc00000000007a964 <ppc_save_regs>: addi r3,r3,-48 > 0xc00000000007a968 <ppc_save_regs+4>: std r0,48(r3) > 0xc00000000007a96c <ppc_save_regs+8>: std r2,64(r3) > ... > 0xc00000000007a9fc <ppc_save_regs+152>: mflr r0 > 0xc00000000007aa00 <ppc_save_regs+156>: std r0,304(r3) > ... > > As we can see, NIP is c0000000002b9984, which is right after "bl > ppc_save_regs", which is expected because the NIP is assigned as the > value of LR in instruction 0xc00000000007a9fc and 0xc00000000007aa00, > and R1(sp) is also stored in ppc_save_regs. Since there is no stack > push in ppc_save_regs, the stack pointer remains to be the same as > crash_setup_regs()'s. crash_setup_regs() have stack push in > 0xc0000000002b9954, so it has a lower stack frame than its parent > function __crash_kexec. > > Then the execution will finally enter into machine_kexec(): > > crash_setup_regs(&fixed_regs, regs); > crash_save_vmcoreinfo(); > machine_crash_shutdown(&fixed_regs); > machine_kexec(kexec_crash_image); > > So the stack frame will be modified, that's why the c00000000a327660 > address doesn't hold a valid stack address. However if function > crash_setup_regs() is inlined, it will have the same stack as its > parent function __crash_kexec, and later function calling will grow > down the stack thus will not modify this stack frame. > > In fact, the crash_setup_regs() should always be inlined, in order to > have the same stack frame as its parent function __crash_kexec()'s. We > can see the function should be inlined in different archs: > > static inline void crash_setup_regs(...) > > But according to the disassembly above, crash_setup_regs() is not > inlined, which leads to this bug. For the above kernel, it's > 6.10.0-15.el10.ppc64le compiled by gcc (GCC) 14.1.1. If we compile the > same kernel code with rhel9's gcc, aka gcc (GCC) 11.5.0, then the > function can be inlined as expected. > > The same issue also exists in x86_64 arch. However I cannot dive into > gcc code why newer versions will make the inline function like this. > Or we can modify crash_setup_regs() to be the following: > > static inline void __attribute__((always_inline)) crash_setup_regs(...) > > Any thoughts? Interesting. I had fixed a similar issue in the kernel earlier, with the ppc64 register saving part. Let's see, GCC apart, crash & gdb should have been able to handle this inlining/not inlined right ?

Will look into it. Currently on leave till next 2 days. Will look after that.

Thanks, Aditya Gupta > > Thanks, > Tao Liu > > > > > > > >> Anyways this patch is also good, doesn't seem problematic for the >> previous working cases. >> >> >> Thanks, >> >> Aditya Gupta >> >>> This patch will search and check for the valid newsp at the beginning of >>> stack back trace. For the above example, a valid newsp would be assigned >>> as c00000000a327850. >>> >>> Before: >>> crash> bt >>> ... >>> #0 [c00000000a327660] crash_setup_regs at c0000000002b9984 >>> After: >>> crash> bt >>> ... >>> #0 [c00000000a327660] crash_setup_regs at c0000000002b9984 >>> #1 [c00000000a327850] panic at c000000000167de4 >>> #2 [c00000000a3278f0] sysrq_handle_crash at c000000000a4d3a8 >>> #3 [c00000000a327950] __handle_sysrq at c000000000a4dec4 >>> #4 [c00000000a3279f0] write_sysrq_trigger at c000000000a4e984 >>> ... >>> Signed-off-by: Tao Liu <ltao(a)redhat.com&gt; >>> --- >>> ppc64.c | 13 +++++++++++++ >>> 1 file changed, 13 insertions(+) >>> >>> diff --git a/ppc64.c b/ppc64.c >>> index 6e5f155..8af7b8a 100644 >>> --- a/ppc64.c >>> +++ b/ppc64.c >>> @@ -2148,6 +2148,19 @@ ppc64_back_trace(struct gnu_request *req, struct bt_info *bt) >>> >>> while (INSTACK(req->sp, bt)) { >>> newsp = *(ulong *)&bt->stackbuf[req->sp - bt->stackbase]; >>> + >>> + if (!newpc) { >>> + ulong *up; >>> + int i; >>> + for (i = 0, up = (ulong *)&bt->stackbuf[req->sp - bt->stackbase]; >>> + i < (req->sp - bt->stackbase) / sizeof(ulong); i++, up++) { >>> + if (INSTACK(*up, bt) && is_kernel_text(*(up + 2))) { >>> + newsp = *up; >>> + break; >>> + } >>> + } >>> + } >>> + >>> if ((req->name = closest_symbol(req->pc)) == NULL) { >>> if (CRASHDEBUG(1)) { >>> error(FATAL,