From f5021021aeb14a36011b954d19076c61c5e0ea99 Mon Sep 17 00:00:00 2001 From: buxiasen Date: Fri, 19 Jul 2024 23:42:18 +0800 Subject: [PATCH] up_backtrace: fix maybe backtrace the exiting thread when the thread to backtrace is exiting, get_tcb and up_backtrace in different critical section may cause try to dump invalid pointer, have to ensure the nxsched_get_tcb and up_backtrace inside same critical section procedure. Signed-off-by: buxiasen --- arch/arm/src/common/arm_backtrace_fp.c | 13 ++++++++----- arch/arm/src/common/arm_backtrace_sp.c | 13 ++++++++----- arch/arm/src/common/arm_backtrace_unwind.c | 13 ++++++++----- arch/arm/src/tlsr82/tc32/tc32_backtrace.c | 13 ++++++++----- arch/arm64/src/common/arm64_backtrace.c | 12 ++++++++---- arch/risc-v/src/common/riscv_backtrace.c | 13 ++++++++----- arch/xtensa/src/common/xtensa_backtrace.c | 13 ++++++++----- include/nuttx/arch.h | 8 ++++++++ sched/sched/sched_backtrace.c | 16 ++++++++++------ 9 files changed, 74 insertions(+), 40 deletions(-) diff --git a/arch/arm/src/common/arm_backtrace_fp.c b/arch/arm/src/common/arm_backtrace_fp.c index cfb624a415..930974c7bf 100644 --- a/arch/arm/src/common/arm_backtrace_fp.c +++ b/arch/arm/src/common/arm_backtrace_fp.c @@ -99,13 +99,20 @@ static int backtrace(uintptr_t *base, uintptr_t *limit, * Returned Value: * up_backtrace() returns the number of addresses returned in buffer * + * Assumptions: + * Have to make sure tcb keep safe during function executing, it means + * 1. Tcb have to be self or not-running. In SMP case, the running task + * PC & SP cannot be backtrace, as whose get from tcb is not the newest. + * 2. Tcb have to keep not be freed. In task exiting case, have to + * make sure the tcb get from pid and up_backtrace in one critical + * section procedure. + * ****************************************************************************/ int up_backtrace(struct tcb_s *tcb, void **buffer, int size, int skip) { struct tcb_s *rtcb = running_task(); - irqstate_t flags; int ret; if (size <= 0 || !buffer) @@ -149,15 +156,11 @@ int up_backtrace(struct tcb_s *tcb, } else { - flags = enter_critical_section(); - ret = backtrace(tcb->stack_base_ptr, tcb->stack_base_ptr + tcb->adj_stack_size, (void *)tcb->xcp.regs[REG_FP], (void *)tcb->xcp.regs[REG_PC], buffer, size, &skip); - - leave_critical_section(flags); } return ret; diff --git a/arch/arm/src/common/arm_backtrace_sp.c b/arch/arm/src/common/arm_backtrace_sp.c index afca4ec3bf..c56b59e2e8 100644 --- a/arch/arm/src/common/arm_backtrace_sp.c +++ b/arch/arm/src/common/arm_backtrace_sp.c @@ -230,6 +230,14 @@ void up_backtrace_init_code_regions(void **regions) * Returned Value: * up_backtrace() returns the number of addresses returned in buffer * + * Assumptions: + * Have to make sure tcb keep safe during function executing, it means + * 1. Tcb have to be self or not-running. In SMP case, the running task + * PC & SP cannot be backtrace, as whose get from tcb is not the newest. + * 2. Tcb have to keep not be freed. In task exiting case, have to + * make sure the tcb get from pid and up_backtrace in one critical + * section procedure. + * ****************************************************************************/ nosanitize_address @@ -237,7 +245,6 @@ int up_backtrace(struct tcb_s *tcb, void **buffer, int size, int skip) { struct tcb_s *rtcb = running_task(); - irqstate_t flags; unsigned long sp; int ret; @@ -287,8 +294,6 @@ int up_backtrace(struct tcb_s *tcb, { ret = 0; - flags = enter_critical_section(); - if (tcb->xcp.regs[REG_PC] && skip-- <= 0) { buffer[ret++] = (void *)tcb->xcp.regs[REG_PC]; @@ -299,8 +304,6 @@ int up_backtrace(struct tcb_s *tcb, tcb->stack_base_ptr + tcb->adj_stack_size, sp, &buffer[ret], size - ret, &skip); - - leave_critical_section(flags); } return ret; diff --git a/arch/arm/src/common/arm_backtrace_unwind.c b/arch/arm/src/common/arm_backtrace_unwind.c index 8b670244a4..c76521fefb 100644 --- a/arch/arm/src/common/arm_backtrace_unwind.c +++ b/arch/arm/src/common/arm_backtrace_unwind.c @@ -698,6 +698,14 @@ again: * Returned Value: * up_backtrace() returns the number of addresses returned in buffer * + * Assumptions: + * Have to make sure tcb keep safe during function executing, it means + * 1. Tcb have to be self or not-running. In SMP case, the running task + * PC & SP cannot be backtrace, as whose get from tcb is not the newest. + * 2. Tcb have to keep not be freed. In task exiting case, have to + * make sure the tcb get from pid and up_backtrace in one critical + * section procedure. + * ****************************************************************************/ int up_backtrace(struct tcb_s *tcb, @@ -705,7 +713,6 @@ int up_backtrace(struct tcb_s *tcb, { struct tcb_s *rtcb = running_task(); struct unwind_frame_s frame; - irqstate_t flags; int ret; if (size <= 0 || !buffer) @@ -748,8 +755,6 @@ int up_backtrace(struct tcb_s *tcb, } else { - flags = enter_critical_section(); - frame.fp = tcb->xcp.regs[REG_FP]; frame.sp = tcb->xcp.regs[REG_SP]; frame.lr = tcb->xcp.regs[REG_LR]; @@ -758,8 +763,6 @@ int up_backtrace(struct tcb_s *tcb, tcb->adj_stack_size; ret = backtrace_unwind(&frame, buffer, size, &skip); - - leave_critical_section(flags); } return ret; diff --git a/arch/arm/src/tlsr82/tc32/tc32_backtrace.c b/arch/arm/src/tlsr82/tc32/tc32_backtrace.c index d94ad56903..d4127e2bea 100644 --- a/arch/arm/src/tlsr82/tc32/tc32_backtrace.c +++ b/arch/arm/src/tlsr82/tc32/tc32_backtrace.c @@ -448,13 +448,20 @@ void up_backtrace_init_code_regions(void **regions) * Returned Value: * up_backtrace() returns the number of addresses returned in buffer * + * Assumptions: + * Have to make sure tcb keep safe during function executing, it means + * 1. Tcb have to be self or not-running. In SMP case, the running task + * PC & SP cannot be backtrace, as whose get from tcb is not the newest. + * 2. Tcb have to keep not be freed. In task exiting case, have to + * make sure the tcb get from pid and up_backtrace in one critical + * section procedure. + * ****************************************************************************/ nosanitize_address int up_backtrace(struct tcb_s *tcb, void **buffer, int size, int skip) { struct tcb_s *rtcb = running_task(); - irqstate_t flags; void *sp; int ret = 0; @@ -511,8 +518,6 @@ int up_backtrace(struct tcb_s *tcb, void **buffer, int size, int skip) } else { - flags = enter_critical_section(); - if (skip-- <= 0) { buffer[ret++] = (void *)tcb->xcp.regs[REG_PC]; @@ -533,8 +538,6 @@ int up_backtrace(struct tcb_s *tcb, void **buffer, int size, int skip) &buffer[ret], size - ret, &skip); } } - - leave_critical_section(flags); } return ret; diff --git a/arch/arm64/src/common/arm64_backtrace.c b/arch/arm64/src/common/arm64_backtrace.c index a0c5b67af5..0d3f855f1b 100644 --- a/arch/arm64/src/common/arm64_backtrace.c +++ b/arch/arm64/src/common/arm64_backtrace.c @@ -102,6 +102,14 @@ static int backtrace(uintptr_t *base, uintptr_t *limit, * Returned Value: * up_backtrace() returns the number of addresses returned in buffer * + * Assumptions: + * Have to make sure tcb keep safe during function executing, it means + * 1. Tcb have to be self or not-running. In SMP case, the running task + * PC & SP cannot be backtrace, as whose get from tcb is not the newest. + * 2. Tcb have to keep not be freed. In task exiting case, have to + * make sure the tcb get from pid and up_backtrace in one critical + * section procedure. + * ****************************************************************************/ int up_backtrace(struct tcb_s *tcb, @@ -109,7 +117,6 @@ int up_backtrace(struct tcb_s *tcb, { struct tcb_s *rtcb = (struct tcb_s *)arch_get_current_tcb(); struct regs_context * p_regs; - irqstate_t flags; int ret; if (rtcb == NULL) @@ -158,7 +165,6 @@ int up_backtrace(struct tcb_s *tcb, } else { - flags = enter_critical_section(); p_regs = (struct regs_context *)tcb->xcp.regs; ret = backtrace(tcb->stack_base_ptr, @@ -166,8 +172,6 @@ int up_backtrace(struct tcb_s *tcb, (void *)p_regs->regs[REG_X29], (void *)p_regs->elr, buffer, size, &skip); - - leave_critical_section(flags); } return ret; diff --git a/arch/risc-v/src/common/riscv_backtrace.c b/arch/risc-v/src/common/riscv_backtrace.c index 01f2e05c11..0aeb2f8a5d 100644 --- a/arch/risc-v/src/common/riscv_backtrace.c +++ b/arch/risc-v/src/common/riscv_backtrace.c @@ -124,12 +124,19 @@ static int backtrace(uintptr_t *base, uintptr_t *limit, * Returned Value: * up_backtrace() returns the number of addresses returned in buffer * + * Assumptions: + * Have to make sure tcb keep safe during function executing, it means + * 1. Tcb have to be self or not-running. In SMP case, the running task + * PC & SP cannot be backtrace, as whose get from tcb is not the newest. + * 2. Tcb have to keep not be freed. In task exiting case, have to + * make sure the tcb get from pid and up_backtrace in one critical + * section procedure. + * ****************************************************************************/ int up_backtrace(struct tcb_s *tcb, void **buffer, int size, int skip) { struct tcb_s *rtcb = running_task(); - irqstate_t flags; int ret; if (size <= 0 || !buffer) @@ -181,8 +188,6 @@ int up_backtrace(struct tcb_s *tcb, void **buffer, int size, int skip) } else { - flags = enter_critical_section(); - #ifdef CONFIG_ARCH_KERNEL_STACK if (tcb->xcp.ustkptr != NULL) { @@ -200,8 +205,6 @@ int up_backtrace(struct tcb_s *tcb, void **buffer, int size, int skip) (void *)tcb->xcp.regs[REG_EPC], buffer, size, &skip); } - - leave_critical_section(flags); } return ret; diff --git a/arch/xtensa/src/common/xtensa_backtrace.c b/arch/xtensa/src/common/xtensa_backtrace.c index a68016cdb6..cd6ab3c89f 100644 --- a/arch/xtensa/src/common/xtensa_backtrace.c +++ b/arch/xtensa/src/common/xtensa_backtrace.c @@ -217,12 +217,19 @@ static int backtrace_stack(uintptr_t *base, uintptr_t *limit, * Returned Value: * up_backtrace() returns the number of addresses returned in buffer * + * Assumptions: + * Have to make sure tcb keep safe during function executing, it means + * 1. Tcb have to be self or not-running. In SMP case, the running task + * PC & SP cannot be backtrace, as whose get from tcb is not the newest. + * 2. Tcb have to keep not be freed. In task exiting case, have to + * make sure the tcb get from pid and up_backtrace in one critical + * section procedure. + * ****************************************************************************/ int up_backtrace(struct tcb_s *tcb, void **buffer, int size, int skip) { struct tcb_s *rtcb = running_task(); - irqstate_t flags; int ret; if (size <= 0 || !buffer) @@ -285,15 +292,11 @@ int up_backtrace(struct tcb_s *tcb, void **buffer, int size, int skip) { /* For non-current task, only check in stack. */ - flags = enter_critical_section(); - ret = backtrace_stack(tcb->stack_base_ptr, tcb->stack_base_ptr + tcb->adj_stack_size, (void *)tcb->xcp.regs[REG_A1], (void *)tcb->xcp.regs[REG_A0], buffer, size, &skip); - - leave_critical_section(flags); } return ret; diff --git a/include/nuttx/arch.h b/include/nuttx/arch.h index 8ae0303b8f..f100a8a148 100644 --- a/include/nuttx/arch.h +++ b/include/nuttx/arch.h @@ -495,6 +495,14 @@ void up_dump_register(FAR void *regs); * Returned Value: * up_backtrace() returns the number of addresses returned in buffer * + * Assumptions: + * Have to make sure tcb keep safe during function executing, it means + * 1. Tcb have to be self or not-running. In SMP case, the running task + * PC & SP cannot be backtrace, as whose get from tcb is not the newest. + * 2. Tcb have to keep not be freed. In task exiting case, have to + * make sure the tcb get from pid and up_backtrace in one critical + * section procedure. + * ****************************************************************************/ int up_backtrace(FAR struct tcb_s *tcb, diff --git a/sched/sched/sched_backtrace.c b/sched/sched/sched_backtrace.c index cbe2fa4b9a..326ecd7470 100644 --- a/sched/sched/sched_backtrace.c +++ b/sched/sched/sched_backtrace.c @@ -43,17 +43,21 @@ #ifdef CONFIG_ARCH_HAVE_BACKTRACE int sched_backtrace(pid_t tid, FAR void **buffer, int size, int skip) { - FAR struct tcb_s *rtcb = NULL; - + FAR struct tcb_s *rtcb; + irqstate_t flags; + int ret = 0; if (tid >= 0) { - rtcb = nxsched_get_tcb(tid); - if (rtcb == NULL) + flags = enter_critical_section(); + rtcb = nxsched_get_tcb(tid); + if (rtcb != NULL) { - return 0; + ret = up_backtrace(rtcb, buffer, size, skip); } + + leave_critical_section(flags); } - return up_backtrace(rtcb, buffer, size, skip); + return ret; } #endif