Re: Stop/unstop process & xsig
On 24/04/21(Sat) 12:49, Mark Kettenis wrote:
> > Date: Sat, 24 Apr 2021 12:23:17 +0200
> > From: Martin Pieuchot
> >
> > 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 - 1.278
> > > +++ kern/kern_sig.c 20 Mar 2021 12:16:51 -
> > > @@ -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) {
> >
Re: Stop/unstop process & xsig
> Date: Sat, 24 Apr 2021 12:23:17 +0200
> From: Martin Pieuchot
>
> 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 - 1.278
> > +++ kern/kern_sig.c 20 Mar 2021 12:16:51 -
> > @@ -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;
> > +
Re: Stop/unstop process & xsig
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?
> 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 - 1.278
> +++ kern/kern_sig.c 20 Mar 2021 12:16:51 -
> @@ -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);
> 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;
>
> /*
>* If the new signal is being masked, look for other
>* signals.
>*/
> - signum = pr->ps_xsig;
> 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(>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)
> +
Stop/unstop process & xsig
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?
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 - 1.278
+++ kern/kern_sig.c 20 Mar 2021 12:16:51 -
@@ -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);
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;
/*
* If the new signal is being masked, look for other
* signals.
*/
- signum = pr->ps_xsig;
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(>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
