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?
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 10:15:22 -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/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 10:15:57 -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:
