On 05/03/21(Fri) 11:30, Martin Pieuchot wrote:
> On 04/03/21(Thu) 11:45, Mark Kettenis wrote:
> > > Date: Thu, 4 Mar 2021 11:19:23 +0100
> > > From: Martin Pieuchot <m...@openbsd.org>
> > >
> > > On 04/03/21(Thu) 11:01, Mark Kettenis wrote:
> > > > > Date: Thu, 4 Mar 2021 10:54:48 +0100
> > > > > From: Patrick Wildt <patr...@blueri.se>
> > > > >
> > > > > 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?
> > > >
> > > > Not a big fan of READ_ONCE() and WRITE_ONCE(), but apparently those
> > > > are needed to comply with the alpha memory model. At least in some
> > > > cases...
> > >
> > > Updated diff using READ_ONCE(), ok?
> >
> > If you use READ_ONCE() you shoul also use WRITE_ONCE() everywhere
> > where you modify ps_single isn't it?
>
> I don't know, I'm learning how to do it. I'd appreciate if somebody could
> come with a READ_ONCE(9) manual explaining how this API should be used.
>
> Updated diff including the WRITE_ONCE().
Any ok?
> 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 5 Mar 2021 10:28:05 -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 = READ_ONCE(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 = READ_ONCE(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/kern_sig.c
> ===================================================================
> RCS file: /cvs/src/sys/kern/kern_sig.c,v
> retrieving revision 1.274
> diff -u -p -r1.274 kern_sig.c
> --- kern/kern_sig.c 4 Mar 2021 09:02:37 -0000 1.274
> +++ kern/kern_sig.c 5 Mar 2021 10:28:05 -0000
> @@ -2040,7 +2040,7 @@ single_thread_set(struct proc *p, enum s
> }
> pr->ps_singlecount = 0;
> membar_producer();
> - pr->ps_single = p;
> + WRITE_ONCE(pr->ps_single, p);
> TAILQ_FOREACH(q, &pr->ps_threads, p_thr_link) {
> if (q == p)
> continue;
> @@ -2131,7 +2131,7 @@ single_thread_clear(struct proc *p, int
> KERNEL_ASSERT_LOCKED();
>
> SCHED_LOCK(s);
> - pr->ps_single = NULL;
> + WRITE_ONCE(pr->ps_single, NULL);
> atomic_clearbits_int(&pr->ps_flags, PS_SINGLEUNWIND | PS_SINGLEEXIT);
> TAILQ_FOREACH(q, &pr->ps_threads, p_thr_link) {
> if (q == p || (q->p_flag & P_SUSPSINGLE) == 0)
> 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 5 Mar 2021 10:28:06 -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 = READ_ONCE(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 = READ_ONCE(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 = READ_ONCE(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 = READ_ONCE(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:
>
