Am Thu, Mar 04, 2021 at 10:42:24AM +0100 schrieb Mark Kettenis:
> > 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".
Isn't there the READ_ONCE(x) macro, that does exactly that?
> > 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:
> >
> >
>
