Skip to content

Commit

Permalink
riscv_fork.c: Fix race condition when handling parent integer registers
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
pussuw authored and xiaoxiang781216 committed Sep 27, 2024
1 parent 172d2a8 commit 9ef76e3
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 13 deletions.
6 changes: 6 additions & 0 deletions arch/risc-v/include/irq.h
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
17 changes: 5 additions & 12 deletions arch/risc-v/src/common/riscv_fork.c
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand All @@ -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 */
Expand Down Expand Up @@ -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 */

Expand All @@ -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().
*/
Expand Down
2 changes: 1 addition & 1 deletion arch/risc-v/src/common/riscv_swint.c
Original file line number Diff line number Diff line change
Expand Up @@ -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 */

Expand Down

0 comments on commit 9ef76e3

Please sign in to comment.