On 04/11/20(Wed) 11:19, Martin Pieuchot wrote: > 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.
Anyone? With this only the `ps_threads' iteration must receive some love in order to take the single_thread_* API out of the KERNEL_LOCK(). For that I just sent a SMR_TAILQ conversion diff. Combined with the diff to remove the recursive attribute of the SCHED_LOCK() we're ready to split it into multiple mutexes. Does that make any sense? Comments? Oks? > 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); >