> 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);
> >
>
>
