Here's a 3rd approach to solve the TOCTOU race in single_thread_set().
The issue being that the lock serializing access to `ps_single' is not
held when calling single_thread_check().
The approach below is controversial because it extends the scope of the
SCHED_LOCK(). On the other hand, the two others approaches that both
add a new lock to avoid this race ignore the fact that accesses to
`ps_single' are currently not clearly serialized w/o KERNEL_LOCK().
So the diff below improves the situation in that regard and do not add
more complexity due to the use of multiple locks. After having looked
for a way to split the SCHED_LOCK() I believe this is the simplest
approach.
I deliberately used a *_locked() function to avoid grabbing the lock
recursively as I'm trying to get rid of the recursion, see the other
thread on tech@.
That said the uses of `ps_single' in ptrace_ctrl() are not covered by
this diff and I'd be glad to hear some comments about them. This is
fine as long as all the code using `ps_single' runs under KERNEL_LOCK()
but since we're trying to get the single_thread_* API out of it, this
need to be addressed.
Note that this diff introduces a helper for initializing ps_single*
values in order to keep all the accesses of those fields in the same
file.
ok?
Index: kern/kern_fork.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_fork.c,v
retrieving revision 1.226
diff -u -p -r1.226 kern_fork.c
--- kern/kern_fork.c 25 Oct 2020 01:55:18 -0000 1.226
+++ kern/kern_fork.c 4 Nov 2020 12:52:54 -0000
@@ -563,10 +563,7 @@ thread_fork(struct proc *curp, void *sta
* if somebody else wants to take us to single threaded mode,
* count ourselves in.
*/
- if (pr->ps_single) {
- atomic_inc_int(&pr->ps_singlecount);
- atomic_setbits_int(&p->p_flag, P_SUSPSINGLE);
- }
+ single_thread_init(p);
/*
* Return tid to parent thread and copy it out to userspace
Index: kern/kern_sig.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_sig.c,v
retrieving revision 1.263
diff -u -p -r1.263 kern_sig.c
--- kern/kern_sig.c 16 Sep 2020 13:50:42 -0000 1.263
+++ kern/kern_sig.c 4 Nov 2020 12:38:35 -0000
@@ -1932,11 +1932,27 @@ userret(struct proc *p)
p->p_cpu->ci_schedstate.spc_curpriority = p->p_usrpri;
}
+void
+single_thread_init(struct proc *p)
+{
+ struct process *pr = p->p_p;
+ int s;
+
+ SCHED_LOCK(s);
+ if (pr->ps_single) {
+ atomic_inc_int(&pr->ps_singlecount);
+ atomic_setbits_int(&p->p_flag, P_SUSPSINGLE);
+ }
+ SCHED_UNLOCK(s);
+}
+
int
-single_thread_check(struct proc *p, int deep)
+_single_thread_check_locked(struct proc *p, int deep)
{
struct process *pr = p->p_p;
+ SCHED_ASSERT_LOCKED();
+
if (pr->ps_single != NULL && pr->ps_single != p) {
do {
int s;
@@ -1949,14 +1965,12 @@ single_thread_check(struct proc *p, int
return (EINTR);
}
- SCHED_LOCK(s);
- if (pr->ps_single == NULL) {
- SCHED_UNLOCK(s);
+ if (pr->ps_single == NULL)
continue;
- }
if (atomic_dec_int_nv(&pr->ps_singlecount) == 0)
wakeup(&pr->ps_singlecount);
+
if (pr->ps_flags & PS_SINGLEEXIT) {
SCHED_UNLOCK(s);
KERNEL_LOCK();
@@ -1967,13 +1981,24 @@ single_thread_check(struct proc *p, int
/* not exiting and don't need to unwind, so suspend */
p->p_stat = SSTOP;
mi_switch();
- SCHED_UNLOCK(s);
} while (pr->ps_single != NULL);
}
return (0);
}
+int
+single_thread_check(struct proc *p, int deep)
+{
+ int s, error;
+
+ SCHED_LOCK(s);
+ error = _single_thread_check_locked(p, deep);
+ SCHED_UNLOCK(s);
+
+ return error;
+}
+
/*
* Stop other threads in the process. The mode controls how and
* where the other threads should stop:
@@ -1995,8 +2020,12 @@ single_thread_set(struct proc *p, enum s
KERNEL_ASSERT_LOCKED();
KASSERT(curproc == p);
- if ((error = single_thread_check(p, deep)))
+ SCHED_LOCK(s);
+ error = _single_thread_check_locked(p, deep);
+ if (error) {
+ SCHED_UNLOCK(s);
return error;
+ }
switch (mode) {
case SINGLE_SUSPEND:
@@ -2014,7 +2043,6 @@ single_thread_set(struct proc *p, enum s
panic("single_thread_mode = %d", mode);
#endif
}
- SCHED_LOCK(s);
pr->ps_singlecount = 0;
membar_producer();
pr->ps_single = p;
Index: sys/proc.h
===================================================================
RCS file: /cvs/src/sys/sys/proc.h,v
retrieving revision 1.300
diff -u -p -r1.300 proc.h
--- sys/proc.h 16 Sep 2020 08:01:15 -0000 1.300
+++ sys/proc.h 4 Nov 2020 12:34:48 -0000
@@ -600,6 +600,7 @@ enum single_thread_mode {
SINGLE_UNWIND, /* other threads to unwind and stop */
SINGLE_EXIT /* other threads to unwind and then exit */
};
+void single_thread_init(struct proc *);
int single_thread_set(struct proc *, enum single_thread_mode, int);
int single_thread_wait(struct process *, int);
void single_thread_clear(struct proc *, int);
