On 24/04/21(Sat) 12:49, Mark Kettenis wrote:
> > 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.
Indeed, for that exact reason `ps_xsig' is read at the end of
proc_stop() once the thread returned from mi_switch(). Are you saying
this is not enough?
It is similar to move the assignment below just before the test, no? My
understanding is that this is safe because all this code is currently
executed under a lock common to all the threads.
>
> > > 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);
> > >
> >
> >
