Re: Read `ps_single' once
On 08/03/21(Mon) 12:37, Claudio Jeker wrote:
> On Mon, Mar 08, 2021 at 12:11:54PM +0100, Martin Pieuchot wrote:
> [...]
> > This diff targets a specific problem which is to make sure `ps_single'
> > dereferences are coherent if this value is being modified w/o KERNEL_LOCK().
> > It doesn't revisit/clarify the relation between the uses of `ps_single'
> > in ptrace and parking code. This can, IMHO, be done in a later step.
>
> It only ensures that ps_single is coherent but not that data read from
> that pointer is coherent.
Yes, that's exactly the point of this diff.
> > > > > @@ -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;
> > > > >
> > >
> > > Here you access p_stat and p_flag, as far as I remember p_stat is also
> > > protected by SCHED_LOCK. p_flag is atomic and maybe the check should be
> > > turned. So this decision may not be stable.
> >
> > This is an incoherency which is fine as long as this code is executed
> > with the KERNEL_LOCK().
>
> It is not if the signal handling is no longer using the KERNEL_LOCK.
But it is currently ;)
> Then the thread could be in the process of being stopped and the race
> to enter single_thread_wait() could be lost. The KERNEL_LOCK() alone does
> not prevent single_thread_set() from running.
Sure, there's plenty of races in the existing code if the KERNEL)_LOCK()
is removed. I'm trying to move forward step by step. I sent a simple diff
that fixes a simple problem. Are you suggesting I should send a huge diff?
Re: Read `ps_single' once
On Mon, Mar 08, 2021 at 12:11:54PM +0100, Martin Pieuchot wrote:
> On 08/03/21(Mon) 11:57, Claudio Jeker wrote:
> > On Mon, Mar 08, 2021 at 11:06:44AM +0100, Martin Pieuchot wrote:
> > > 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
> > > > > >
> > > > > > On 04/03/21(Thu) 11:01, Mark Kettenis wrote:
> > > > > > > > Date: Thu, 4 Mar 2021 10:54:48 +0100
> > > > > > > > From: Patrick Wildt
> > > > > > > >
> > > > > > > > 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
> > > > > > > > > >
> > > > > > > > > > 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?
> >
> > The one thing that bothers me is that we decided that ps_single needs the
> > SCHED_LOCK but now this becomes a bit of a mishmash.
>
> I hear what you're saying.
>
> I'm currently concentrating on moving cursig() out of the KERNEL_LOCK()
> and I'd appreciate not be blocked on discussions on which locking/lock-free
> solution is the best for making the parking code mp-safe.
The problem is that you may introduce hard to debug issues in the parking
code. Been there done that.
> This diff targets a specific problem which is to make sure `ps_single'
> dereferences are coherent if this value is being modified w/o KERNEL_LOCK().
> It doesn't revisit/clarify the relation between the uses of `ps_single'
> in ptrace and parking code. This can, IMHO, be done in a later step.
It only ensures that ps_single is coherent but not that data read from
that pointer is coherent.
> > > > @@ -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;
> > > >
> >
> > Here you access p_stat and p_flag, as far as I remember p_stat is also
> > protected by SCHED_LOCK. p_flag is atomic and maybe the check should be
> > turned. So this decision may not be stable.
>
> This is an incoherency which is fine as long as this code is executed
> with the KERNEL_LOCK().
It is not if the signal handling is no longer using the KERNEL_LOCK.
Then the thread could be in the process of being stopped and the race
to enter single_thread_wait() could be lost. The KERNEL_LOCK() alone does
not prevent single_thread_set() from running.
> > > > Index: kern/sys_process.c
> > > > ===
> > > > RCS file: /cvs/src/sys/kern/sys_process.c,v
> > > > retrieving revision 1.86
> > > > diff -u -p -r1.86
Re: Read `ps_single' once
On 08/03/21(Mon) 11:57, Claudio Jeker wrote:
> On Mon, Mar 08, 2021 at 11:06:44AM +0100, Martin Pieuchot wrote:
> > 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
> > > > >
> > > > > On 04/03/21(Thu) 11:01, Mark Kettenis wrote:
> > > > > > > Date: Thu, 4 Mar 2021 10:54:48 +0100
> > > > > > > From: Patrick Wildt
> > > > > > >
> > > > > > > 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
> > > > > > > > >
> > > > > > > > > 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?
>
> The one thing that bothers me is that we decided that ps_single needs the
> SCHED_LOCK but now this becomes a bit of a mishmash.
I hear what you're saying.
I'm currently concentrating on moving cursig() out of the KERNEL_LOCK()
and I'd appreciate not be blocked on discussions on which locking/lock-free
solution is the best for making the parking code mp-safe.
This diff targets a specific problem which is to make sure `ps_single'
dereferences are coherent if this value is being modified w/o KERNEL_LOCK().
It doesn't revisit/clarify the relation between the uses of `ps_single'
in ptrace and parking code. This can, IMHO, be done in a later step.
> > > @@ -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;
> > >
>
> Here you access p_stat and p_flag, as far as I remember p_stat is also
> protected by SCHED_LOCK. p_flag is atomic and maybe the check should be
> turned. So this decision may not be stable.
This is an incoherency which is fine as long as this code is executed
with the KERNEL_LOCK().
> > > 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.c8 Feb 2021 10:51:02 - 1.86
> > > +++ kern/sys_process.c5 Mar 2021 10:28:06 -
> > > @@ -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)
> > >
Re: Read `ps_single' once
On Mon, Mar 08, 2021 at 11:06:44AM +0100, Martin Pieuchot wrote:
> 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
> > > >
> > > > On 04/03/21(Thu) 11:01, Mark Kettenis wrote:
> > > > > > Date: Thu, 4 Mar 2021 10:54:48 +0100
> > > > > > From: Patrick Wildt
> > > > > >
> > > > > > 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
> > > > > > > >
> > > > > > > > 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?
The one thing that bothers me is that we decided that ps_single needs the
SCHED_LOCK but now this becomes a bit of a mishmash.
> > 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.c15 Feb 2021 09:35:59 - 1.196
> > +++ kern/kern_exit.c5 Mar 2021 10:28:05 -
> > @@ -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 {
I seriously wonder if that should not be moved into prsignal() but this
should be ok.
> > @@ -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;
> >
Here you access p_stat and p_flag, as far as I remember p_stat is also
protected by SCHED_LOCK. p_flag is atomic and maybe the check should be
turned. So this decision may not be stable.
> > 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 - 1.274
> > +++ kern/kern_sig.c 5 Mar 2021 10:28:05 -
> > @@ -2040,7 +2040,7 @@ single_thread_set(struct proc *p, enum s
> >
Re: Read `ps_single' once
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
> > >
> > > On 04/03/21(Thu) 11:01, Mark Kettenis wrote:
> > > > > Date: Thu, 4 Mar 2021 10:54:48 +0100
> > > > > From: Patrick Wildt
> > > > >
> > > > > 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
> > > > > > >
> > > > > > > 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 - 1.196
> +++ kern/kern_exit.c 5 Mar 2021 10:28:05 -
> @@ -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 - 1.274
> +++ kern/kern_sig.c 5 Mar 2021 10:28:05 -
> @@ -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, >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(>ps_flags, PS_SINGLEUNWIND | PS_SINGLEEXIT);
> TAILQ_FOREACH(q, >ps_threads, p_thr_link) {
> if (q == p || (q->p_flag & P_SUSPSINGLE) == 0)
> Index:
Re: Read `ps_single' once
On 04/03/21(Thu) 11:45, Mark Kettenis wrote:
> > Date: Thu, 4 Mar 2021 11:19:23 +0100
> > From: Martin Pieuchot
> >
> > On 04/03/21(Thu) 11:01, Mark Kettenis wrote:
> > > > Date: Thu, 4 Mar 2021 10:54:48 +0100
> > > > From: Patrick Wildt
> > > >
> > > > 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
> > > > > >
> > > > > > 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().
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.c15 Feb 2021 09:35:59 - 1.196
+++ kern/kern_exit.c5 Mar 2021 10:28:05 -
@@ -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 - 1.274
+++ kern/kern_sig.c 5 Mar 2021 10:28:05 -
@@ -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, >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(>ps_flags, PS_SINGLEUNWIND | PS_SINGLEEXIT);
TAILQ_FOREACH(q, >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
---
Re: Read `ps_single' once
> Date: Thu, 4 Mar 2021 11:19:23 +0100
> From: Martin Pieuchot
>
> On 04/03/21(Thu) 11:01, Mark Kettenis wrote:
> > > Date: Thu, 4 Mar 2021 10:54:48 +0100
> > > From: Patrick Wildt
> > >
> > > 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
> > > > >
> > > > > 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?
That is one of the reasons I dislike these macros...
> 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 - 1.196
> +++ kern/kern_exit.c 4 Mar 2021 10:15:22 -
> @@ -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.c8 Feb 2021 10:51:02 - 1.86
> +++ kern/sys_process.c4 Mar 2021 10:15:57 -
> @@ -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 =
Re: Read `ps_single' once
On 04/03/21(Thu) 11:01, Mark Kettenis wrote:
> > Date: Thu, 4 Mar 2021 10:54:48 +0100
> > From: Patrick Wildt
> >
> > 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
> > > >
> > > > 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.c15 Feb 2021 09:35:59 - 1.196
+++ kern/kern_exit.c4 Mar 2021 10:15:22 -
@@ -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 - 1.86
+++ kern/sys_process.c 4 Mar 2021 10:15:57 -
@@ -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 &&
Re: Read `ps_single' once
> Date: Thu, 4 Mar 2021 10:54:48 +0100
> From: Patrick Wildt
>
> 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
> > >
> > > 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...
> > > 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 - 1.196
> > > +++ kern/kern_exit.c 4 Mar 2021 09:29:27 -
> > > @@ -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.c8 Feb 2021 10:51:02 - 1.86
> > > +++ kern/sys_process.c4 Mar 2021 09:29:27 -
> > > @@ -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
Re: Read `ps_single' once
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
> >
> > 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.c15 Feb 2021 09:35:59 - 1.196
> > +++ kern/kern_exit.c4 Mar 2021 09:29:27 -
> > @@ -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 - 1.86
> > +++ kern/sys_process.c 4 Mar 2021 09:29:27 -
> > @@ -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. */
> >
Re: Read `ps_single' once
> Date: Thu, 4 Mar 2021 10:34:24 +0100
> From: Martin Pieuchot
>
> 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 - 1.196
> +++ kern/kern_exit.c 4 Mar 2021 09:29:27 -
> @@ -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.c8 Feb 2021 10:51:02 - 1.86
> +++ kern/sys_process.c4 Mar 2021 09:29:27 -
> @@ -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
