> Date: Sat, 24 Apr 2021 12:23:17 +0200 > From: Martin Pieuchot <m...@openbsd.org> > > On 20/03/21(Sat) 13:25, Martin Pieuchot wrote: > > Diff below refactors routines to stop/unstop processes and save the signal > > number which will/can be transmitted it in wait4(2). It does the following: > > > > - Move the "hack" involving P_SINTR to avoid grabbing the SCHED_LOCK() > > recursively inside proc_stop(). > > > > - Introduce proc_unstop(), the symmetric routine to proc_stop(). > > > > - Manipulate `ps_xsig' only in proc_stop/unstop(). > > > > Ok? > > Anyone?
This is not ok... > > > Index: kern/kern_sig.c > > =================================================================== > > RCS file: /cvs/src/sys/kern/kern_sig.c,v > > retrieving revision 1.278 > > diff -u -p -r1.278 kern_sig.c > > --- kern/kern_sig.c 12 Mar 2021 10:13:28 -0000 1.278 > > +++ kern/kern_sig.c 20 Mar 2021 12:16:51 -0000 > > @@ -124,7 +124,7 @@ const int sigprop[NSIG + 1] = { > > > > void setsigvec(struct proc *, int, struct sigaction *); > > > > -void proc_stop(struct proc *p, int); > > +int proc_stop(struct proc *p, int, int); > > void proc_stop_sweep(void *); > > void *proc_stop_si; > > > > @@ -1061,8 +1061,7 @@ ptsignal(struct proc *p, int signum, enu > > if (pr->ps_flags & PS_PPWAIT) > > goto out; > > atomic_clearbits_int(siglist, mask); > > - pr->ps_xsig = signum; > > - proc_stop(p, 0); > > + proc_stop(p, signum, 0); > > goto out; > > } > > /* > > @@ -1170,17 +1169,12 @@ out: > > * > > * while (signum = cursig(curproc)) > > * postsig(signum); > > - * > > - * Assumes that if the P_SINTR flag is set, we're holding both the > > - * kernel and scheduler locks. > > */ > > int > > cursig(struct proc *p) > > { > > struct process *pr = p->p_p; > > int sigpending, signum, mask, prop; > > - int dolock = (p->p_flag & P_SINTR) == 0; > > - int s; > > > > KERNEL_ASSERT_LOCKED(); > > > > @@ -1217,31 +1211,22 @@ cursig(struct proc *p) > > */ > > if (((pr->ps_flags & (PS_TRACED | PS_PPWAIT)) == PS_TRACED) && > > signum != SIGKILL) { > > - pr->ps_xsig = signum; > > > > single_thread_set(p, SINGLE_SUSPEND, 0); > > - > > - if (dolock) > > - SCHED_LOCK(s); > > - proc_stop(p, 1); > > - if (dolock) > > - SCHED_UNLOCK(s); > > - > > + signum = proc_stop(p, signum, 1); At this point the process will sleep since proc_stop() calls mi_switch(). At that point the debugger may clear or change the value of ps_xsig. > > single_thread_clear(p, 0); > > > > /* > > * If we are no longer being traced, or the parent > > * didn't give us a signal, look for more signals. > > */ > > - if ((pr->ps_flags & PS_TRACED) == 0 || > > - pr->ps_xsig == 0) > > + if ((pr->ps_flags & PS_TRACED) == 0 || signum == 0) > > continue; So this change is wrong. > > > > /* > > * If the new signal is being masked, look for other > > * signals. > > */ > > - signum = pr->ps_xsig; So this assignment is *not* redundant. > > mask = sigmask(signum); > > if ((p->p_sigmask & mask) != 0) > > continue; > > @@ -1286,12 +1271,7 @@ cursig(struct proc *p) > > (pr->ps_pgrp->pg_jobc == 0 && > > prop & SA_TTYSTOP)) > > break; /* == ignore */ > > - pr->ps_xsig = signum; > > - if (dolock) > > - SCHED_LOCK(s); > > - proc_stop(p, 1); > > - if (dolock) > > - SCHED_UNLOCK(s); > > + proc_stop(p, signum, 1); > > break; > > } else if (prop & SA_IGNORE) { > > /* > > @@ -1331,15 +1311,21 @@ keep: > > * Put the argument process into the stopped state and notify the parent > > * via wakeup. Signals are handled elsewhere. The process must not be > > * on the run queue. > > + * > > + * Assumes that if the P_SINTR flag is set, we're holding the scheduler > > + * lock. > > */ > > -void > > -proc_stop(struct proc *p, int sw) > > +int > > +proc_stop(struct proc *p, int signum, int sw) > > { > > struct process *pr = p->p_p; > > + int dolock = (p->p_flag & P_SINTR) == 0; > > + int s; > > > > -#ifdef MULTIPROCESSOR > > - SCHED_ASSERT_LOCKED(); > > -#endif > > + pr->ps_xsig = signum; > > + > > + if (dolock) > > + SCHED_LOCK(s); > > > > p->p_stat = SSTOP; > > atomic_clearbits_int(&pr->ps_flags, PS_WAITED); > > @@ -1352,6 +1338,13 @@ proc_stop(struct proc *p, int sw) > > softintr_schedule(proc_stop_si); > > if (sw) > > mi_switch(); > > + > > + if (dolock) > > + SCHED_UNLOCK(s); > > + > > + signum = pr->ps_xsig; > > + > > + return signum; > > } > > > > /* > > @@ -1376,6 +1369,27 @@ proc_stop_sweep(void *v) > > } > > } > > > > +void > > +proc_unstop(struct proc *p, int signum) > > +{ > > + struct process *pr = p->p_p; > > + > > + KASSERT(signum >= 0); > > + KASSERT(p->p_stat == SSTOP); > > + > > + if (signum != 0) > > + pr->ps_xsig = signum; > > + > > + /* > > + * If we're being traced (possibly because someone attached us > > + * while we were stopped), check for a signal from the debugger. > > + */ > > + if ((pr->ps_flags & PS_TRACED) != 0 && pr->ps_xsig != 0) > > + atomic_setbits_int(&p->p_siglist, sigmask(pr->ps_xsig)); > > + > > + setrunnable(p); > > +} > > + > > /* > > * Take the action for the specified signal > > * from the current set of pending signals. > > @@ -2042,7 +2056,7 @@ single_thread_set(struct proc *p, enum s > > if (q->p_flag & P_WEXIT) { > > if (mode == SINGLE_EXIT) { > > if (q->p_stat == SSTOP) { > > - setrunnable(q); > > + proc_unstop(q, 0); > > atomic_inc_int(&pr->ps_singlecount); > > } > > } > > @@ -2069,7 +2083,7 @@ single_thread_set(struct proc *p, enum s > > break; > > case SSTOP: > > if (mode == SINGLE_EXIT) { > > - setrunnable(q); > > + proc_unstop(q, 0); > > atomic_inc_int(&pr->ps_singlecount); > > } > > break; > > @@ -2138,7 +2152,7 @@ single_thread_clear(struct proc *p, int > > */ > > if (q->p_stat == SSTOP && (q->p_flag & flag) == 0) { > > if (q->p_wchan == 0) > > - setrunnable(q); > > + proc_unstop(q, 0); > > else > > q->p_stat = SSLEEP; > > } > > Index: kern/sched_bsd.c > > =================================================================== > > RCS file: /cvs/src/sys/kern/sched_bsd.c,v > > retrieving revision 1.65 > > diff -u -p -r1.65 sched_bsd.c > > --- kern/sched_bsd.c 10 Dec 2020 04:26:50 -0000 1.65 > > +++ kern/sched_bsd.c 20 Mar 2021 12:16:51 -0000 > > @@ -456,12 +456,6 @@ setrunnable(struct proc *p) > > default: > > panic("setrunnable"); > > case SSTOP: > > - /* > > - * If we're being traced (possibly because someone attached us > > - * while we were stopped), check for a signal from the debugger. > > - */ > > - if ((pr->ps_flags & PS_TRACED) != 0 && pr->ps_xsig != 0) > > - atomic_setbits_int(&p->p_siglist, sigmask(pr->ps_xsig)); > > prio = p->p_usrpri; > > unsleep(p); > > break; > > 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 20 Mar 2021 12:16:51 -0000 > > @@ -484,9 +484,8 @@ ptrace_ctrl(struct proc *p, int req, pid > > > > /* Finally, deliver the requested signal (or none). */ > > if (t->p_stat == SSTOP) { > > - tr->ps_xsig = data; > > SCHED_LOCK(s); > > - setrunnable(t); > > + proc_unstop(t, data); > > SCHED_UNLOCK(s); > > } else { > > if (data != 0) > > Index: sys/signalvar.h > > =================================================================== > > RCS file: /cvs/src/sys/sys/signalvar.h,v > > retrieving revision 1.46 > > diff -u -p -r1.46 signalvar.h > > --- sys/signalvar.h 4 Mar 2021 09:02:38 -0000 1.46 > > +++ sys/signalvar.h 20 Mar 2021 12:16:51 -0000 > > @@ -103,6 +103,7 @@ struct sigio_ref; > > /* > > * Machine-independent functions: > > */ > > +void proc_unstop(struct proc *, int); > > int coredump(struct proc *p); > > void execsigs(struct proc *p); > > int cursig(struct proc *p); > > > >