From b2b6fda62b75114a7dad9e5a321ed2c135115484 Mon Sep 17 00:00:00 2001 From: Ville Juven Date: Thu, 26 Sep 2024 13:03:12 +0300 Subject: [PATCH] riscv_fork.c: Fix race condition when handling parent integer registers We need to record the parent's integer register context upon exception entry to a separate non-volatile area. Why? Because xcp.regs can move due to a context switch within the fork() system call, be it either via interrupt or a synchronization point. Fix this by adding a "sregs" area where the saved user context is placed. The critical section within fork() is also unnecessary. --- arch/risc-v/include/irq.h | 6 ++++++ arch/risc-v/src/common/riscv_fork.c | 17 +++++------------ arch/risc-v/src/common/riscv_swint.c | 2 +- 3 files changed, 12 insertions(+), 13 deletions(-) diff --git a/arch/risc-v/include/irq.h b/arch/risc-v/include/irq.h index fb16b84b17799..cd2665cd88631 100644 --- a/arch/risc-v/include/irq.h +++ b/arch/risc-v/include/irq.h @@ -613,6 +613,12 @@ struct xcptcontext uintreg_t *regs; +#ifdef CONFIG_LIB_SYSCALL + /* User integer registers upon system call entry */ + + uintreg_t *sregs; +#endif + /* FPU register save area */ #if defined(CONFIG_ARCH_FPU) && defined(CONFIG_ARCH_LAZYFPU) diff --git a/arch/risc-v/src/common/riscv_fork.c b/arch/risc-v/src/common/riscv_fork.c index 6f17f8d5401e7..76a784a4f5929 100644 --- a/arch/risc-v/src/common/riscv_fork.c +++ b/arch/risc-v/src/common/riscv_fork.c @@ -108,19 +108,14 @@ pid_t riscv_fork(const struct fork_s *context) uintptr_t newtop; uintptr_t stacktop; uintptr_t stackutil; - irqstate_t flags; #ifdef CONFIG_SCHED_THREAD_LOCAL uintptr_t tp; #endif UNUSED(context); - /* parent regs may change in irq, we should disable irq here */ - - flags = up_irq_save(); - /* Allocate and initialize a TCB for the child task. */ - child = nxtask_setup_fork((start_t)parent->xcp.regs[REG_RA]); + child = nxtask_setup_fork((start_t)parent->xcp.sregs[REG_RA]); if (!child) { sinfo("nxtask_setup_fork failed\n"); @@ -130,15 +125,15 @@ pid_t riscv_fork(const struct fork_s *context) /* Copy parent user stack to child */ stacktop = (uintptr_t)parent->stack_base_ptr + parent->adj_stack_size; - DEBUGASSERT(stacktop > parent->xcp.regs[REG_SP]); - stackutil = stacktop - parent->xcp.regs[REG_SP]; + DEBUGASSERT(stacktop > parent->xcp.sregs[REG_SP]); + stackutil = stacktop - parent->xcp.sregs[REG_SP]; /* Copy goes to child's user stack top */ newtop = (uintptr_t)child->cmn.stack_base_ptr + child->cmn.adj_stack_size; newsp = newtop - stackutil; - memcpy((void *)newsp, (const void *)parent->xcp.regs[REG_SP], stackutil); + memcpy((void *)newsp, (const void *)parent->xcp.sregs[REG_SP], stackutil); #ifdef CONFIG_SCHED_THREAD_LOCAL /* Save child's thread pointer */ @@ -169,7 +164,7 @@ pid_t riscv_fork(const struct fork_s *context) /* Copy the parent integer context (overwrites child's SP and TP) */ - memcpy(child->cmn.xcp.regs, parent->xcp.regs, XCPTCONTEXT_SIZE); + memcpy(child->cmn.xcp.regs, parent->xcp.sregs, XCPTCONTEXT_SIZE); /* Save FPU */ @@ -183,8 +178,6 @@ pid_t riscv_fork(const struct fork_s *context) child->cmn.xcp.regs[REG_TP] = tp; #endif - up_irq_restore(flags); - /* And, finally, start the child task. On a failure, nxtask_start_fork() * will discard the TCB by calling nxtask_abort_fork(). */ diff --git a/arch/risc-v/src/common/riscv_swint.c b/arch/risc-v/src/common/riscv_swint.c index 4cf68ac0aa9c7..cfe5838102316 100644 --- a/arch/risc-v/src/common/riscv_swint.c +++ b/arch/risc-v/src/common/riscv_swint.c @@ -161,7 +161,7 @@ uintptr_t dispatch_syscall(unsigned int nbr, uintptr_t parm1, /* Set the user register context to TCB */ - rtcb->xcp.regs = context; + rtcb->xcp.sregs = context; /* Indicate that we are in a syscall handler */