> Date: Thu, 4 Mar 2021 10:34:24 +0100 > From: Martin Pieuchot <m...@openbsd.org> > > Running t/rw/msleep(9) w/o KERNEL_LOCK() implies that a thread can > change the value of `ps_single' while one of its siblings might be > dereferencing it. > > To prevent inconsistencies in the code executed by sibling thread, the > diff below makes sure `ps_single' is dereferenced only once in various > parts of the kernel. > > ok?
I think that means that ps_single has to be declared "volatile". > Index: kern/kern_exit.c > =================================================================== > RCS file: /cvs/src/sys/kern/kern_exit.c,v > retrieving revision 1.196 > diff -u -p -r1.196 kern_exit.c > --- kern/kern_exit.c 15 Feb 2021 09:35:59 -0000 1.196 > +++ kern/kern_exit.c 4 Mar 2021 09:29:27 -0000 > @@ -274,6 +274,8 @@ exit1(struct proc *p, int xexit, int xsi > */ > if (qr->ps_flags & PS_TRACED && > !(qr->ps_flags & PS_EXITING)) { > + struct proc *st; > + > process_untrace(qr); > > /* > @@ -281,9 +283,9 @@ exit1(struct proc *p, int xexit, int xsi > * direct the signal to the active > * thread to avoid deadlock. > */ > - if (qr->ps_single) > - ptsignal(qr->ps_single, SIGKILL, > - STHREAD); > + st = qr->ps_single; > + if (st != NULL) > + ptsignal(st, SIGKILL, STHREAD); > else > prsignal(qr, SIGKILL); > } else { > @@ -510,7 +512,7 @@ dowait4(struct proc *q, pid_t pid, int * > { > int nfound; > struct process *pr; > - struct proc *p; > + struct proc *p, *st; > int error; > > if (pid == 0) > @@ -541,10 +543,11 @@ loop: > proc_finish_wait(q, p); > return (0); > } > + > + st = pr->ps_single; > if (pr->ps_flags & PS_TRACED && > - (pr->ps_flags & PS_WAITED) == 0 && pr->ps_single && > - pr->ps_single->p_stat == SSTOP && > - (pr->ps_single->p_flag & P_SUSPSINGLE) == 0) { > + (pr->ps_flags & PS_WAITED) == 0 && st != NULL && > + st->p_stat == SSTOP && (st->p_flag & P_SUSPSINGLE) == 0) { > if (single_thread_wait(pr, 0)) > goto loop; > > Index: kern/sys_process.c > =================================================================== > RCS file: /cvs/src/sys/kern/sys_process.c,v > retrieving revision 1.86 > diff -u -p -r1.86 sys_process.c > --- kern/sys_process.c 8 Feb 2021 10:51:02 -0000 1.86 > +++ kern/sys_process.c 4 Mar 2021 09:29:27 -0000 > @@ -273,7 +273,7 @@ sys_ptrace(struct proc *p, void *v, regi > int > ptrace_ctrl(struct proc *p, int req, pid_t pid, caddr_t addr, int data) > { > - struct proc *t; /* target thread */ > + struct proc *st, *t; /* target thread */ > struct process *tr; /* target process */ > int error = 0; > int s; > @@ -433,8 +433,9 @@ ptrace_ctrl(struct proc *p, int req, pid > * from where it stopped." > */ > > - if (pid < THREAD_PID_OFFSET && tr->ps_single) > - t = tr->ps_single; > + st = tr->ps_single; > + if (pid < THREAD_PID_OFFSET && st != NULL) > + t = st; > > /* If the address parameter is not (int *)1, set the pc. */ > if ((int *)addr != (int *)1) > @@ -464,8 +465,9 @@ ptrace_ctrl(struct proc *p, int req, pid > * from where it stopped." > */ > > - if (pid < THREAD_PID_OFFSET && tr->ps_single) > - t = tr->ps_single; > + st = tr->ps_single; > + if (pid < THREAD_PID_OFFSET && st != NULL) > + t = st; > > #ifdef PT_STEP > /* > @@ -495,8 +497,9 @@ ptrace_ctrl(struct proc *p, int req, pid > break; > > case PT_KILL: > - if (pid < THREAD_PID_OFFSET && tr->ps_single) > - t = tr->ps_single; > + st = tr->ps_single; > + if (pid < THREAD_PID_OFFSET && st != NULL) > + t = st; > > /* just send the process a KILL signal. */ > data = SIGKILL; > @@ -536,6 +539,7 @@ int > ptrace_kstate(struct proc *p, int req, pid_t pid, void *addr) > { > struct process *tr; /* target process */ > + struct proc *st; > struct ptrace_event *pe = addr; > int error; > > @@ -582,9 +586,9 @@ ptrace_kstate(struct proc *p, int req, p > tr->ps_ptmask = pe->pe_set_event; > break; > case PT_GET_PROCESS_STATE: > - if (tr->ps_single) > - tr->ps_ptstat->pe_tid = > - tr->ps_single->p_tid + THREAD_PID_OFFSET; > + st = tr->ps_single; > + if (st != NULL) > + tr->ps_ptstat->pe_tid = st->p_tid + THREAD_PID_OFFSET; > memcpy(addr, tr->ps_ptstat, sizeof *tr->ps_ptstat); > break; > default: > >