From owner-freebsd-stable@FreeBSD.ORG Mon Jan 18 01:48:28 2010 Return-Path: Delivered-To: freebsd-stable@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 2146F1065679 for ; Mon, 18 Jan 2010 01:48:28 +0000 (UTC) (envelope-from davidxu@freebsd.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by mx1.freebsd.org (Postfix) with ESMTP id E663B8FC08; Mon, 18 Jan 2010 01:48:27 +0000 (UTC) Received: from apple.my.domain (localhost [127.0.0.1]) by freefall.freebsd.org (8.14.3/8.14.3) with ESMTP id o0I1mOfL022561; Mon, 18 Jan 2010 01:48:25 GMT (envelope-from davidxu@freebsd.org) Message-ID: <4B53BDE8.9000509@freebsd.org> Date: Mon, 18 Jan 2010 09:48:24 +0800 From: David Xu User-Agent: Thunderbird 2.0.0.9 (X11/20080612) MIME-Version: 1.0 To: Kostik Belousov References: <4B4D0293.3040704@rogers.com> <201001140941.46748.tijl@coosemans.org> <4B4FC56A.4020007@freebsd.org> <201001161451.39399.tijl@coosemans.org> <4B526C69.6050300@freebsd.org> <20100117195307.GJ62907@deviant.kiev.zoral.com.ua> In-Reply-To: <20100117195307.GJ62907@deviant.kiev.zoral.com.ua> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: Gardner Bell , Tijl Coosemans , freebsd-stable@freebsd.org Subject: Re: process in STOP state X-BeenThere: freebsd-stable@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Production branch of FreeBSD source code List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 18 Jan 2010 01:48:28 -0000 Kostik Belousov wrote: > On Sun, Jan 17, 2010 at 09:48:25AM +0800, David Xu wrote: >> Tijl Coosemans wrote: >>> On Friday 15 January 2010 02:31:22 David Xu wrote: >>> >>>> Tijl Coosemans wrote: >>>> >>>>>> Besides weird formatting of procstat -k output, I do not see >>>>>> anything wrong in the state of the process. It got SIGSTOP, I am >>>>>> sure. Attaching gdb helps because debugger gets signal reports >>>>>> instead of target process getting the signal actions on signal >>>>>> delivery. >>>>>> >>>>>> The only question is why the process gets SIGSTOP at all. >>>>>> >>>>> Wine uses ptrace(2) sometimes. The SIGSTOP could have come from >>>>> that. I recently submitted >>>>> http://www.freebsd.org/cgi/query-pr.cgi?pr=142757 describing a >>>>> problem with ptrace and signals, so you might want to give the >>>>> kernel patch a try. >>>>> >>>> The problem in your patch is that ksi pointer can not be hold across >>>> thread sleeping, because once the process is resumed, there is no >>>> guarantee that the thread will run first, once the signal came from >>>> process's signal queue, other threads can remove the signal, and here >>>> your sigqueue_take(ksi) is dangerous code. >>>> >>> If other threads can run before the current thread then there's a >>> second problem next to the one in the PR (current thread deletes >>> signal that shouldn't be deleted). Then those other threads can see >>> that the SIGSTOP bit (or another signal) is still set and stop the >>> process a second time. This might be what happens in the OP's case. >>> >>> So, the signal has to be cleared before suspending the process, but >>> then other threads can still deliver other signals which might change >>> delivery order and I don't see any way around that besides introducing >>> a per process signal lock that is also kept while the process is >>> stopped. Comments? >>> >>> >> I don't have an idea now, we ever delivered signal to thread's queue, >> though it may lose signal if thread exits, but it does not have the problem >> you have described here, we may need to rethink how to fix the signal-lost >> problem but still deliver signal to thread's queue, just an idea. >> > > I do think that signal shall be removed from the queue before > ptracestop(), and I do not see a problem with other threads taking other > signals. ptracestop() stops the whole process, and delivery order when > signals go to different threads is somewhat vague due to scheduling > events etc. So even if we guarantee that thread goes to userland for > signal delivery before another thread, that another thread may still run > handler observable earlier due to scheduler, swapping etc. > > I think your fix might be slightly reworked, making three points: > 1. do remove signal from queue, and reinsert it into the _head_ of the > _thread_ queue after. We already got it from the head, and current > thread did not masked the signal. Since thread cannot change the mask in > between, this signal is the first to be delivered to this thread, and > current thread is still eligible for delivery. I added KSI_HEAD flag to > adjust behaviour of sigqueue_add(). > > 2. Instead of adding signal to sq_kill, I just call sigqueue_add > with NULL ksi. > > 3. To remove ksi from the queue, but not freeing it, I had to copy/paste > the sigqueue_get() into sigqueue_steal(). It returns pointer to the > removed ksi. I did not found good way to merge _get and steal. > > diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c > index 1c21bc5..80e5a07 100644 > --- a/sys/kern/kern_sig.c > +++ b/sys/kern/kern_sig.c > @@ -316,6 +316,42 @@ sigqueue_get(sigqueue_t *sq, int signo, ksiginfo_t *si) > return (signo); > } > > +static int > +sigqueue_steal(sigqueue_t *sq, int signo, ksiginfo_t **si) > +{ > + struct proc *p; > + struct ksiginfo *ksi, *next; > + int count; > + > + KASSERT(sq->sq_flags & SQ_INIT, ("sigqueue not inited")); > + p = sq->sq_proc; > + count = 0; > + > + if (!SIGISMEMBER(sq->sq_signals, signo)) > + return (0); > + > + if (SIGISMEMBER(sq->sq_kill, signo)) { > + count++; > + SIGDELSET(sq->sq_kill, signo); > + } > + > + TAILQ_FOREACH_SAFE(ksi, &sq->sq_list, ksi_link, next) { > + if (ksi->ksi_signo == signo) { > + if (count == 0) { > + TAILQ_REMOVE(&sq->sq_list, ksi, ksi_link); > + ksi->ksi_sigq = NULL; > + *si = ksi; > + } > + if (++count > 1) > + break; > + } > + } > + > + if (count <= 1) > + SIGDELSET(sq->sq_signals, signo); > + return (signo); > +} > + > void > sigqueue_take(ksiginfo_t *ksi) > { > @@ -357,7 +393,10 @@ sigqueue_add(sigqueue_t *sq, int signo, ksiginfo_t *si) > > /* directly insert the ksi, don't copy it */ > if (si->ksi_flags & KSI_INS) { > - TAILQ_INSERT_TAIL(&sq->sq_list, si, ksi_link); > + if (si->ksi_flags & KSI_HEAD) > + TAILQ_INSERT_HEAD(&sq->sq_list, si, ksi_link); > + else > + TAILQ_INSERT_TAIL(&sq->sq_list, si, ksi_link); > si->ksi_sigq = sq; > goto out_set_bit; > } > @@ -2492,6 +2531,7 @@ issignal(struct thread *td, int stop_allowed) > struct sigacts *ps; > struct sigqueue *queue; > sigset_t sigpending; > + ksiginfo_t *psi; > int sig, prop, newsig; > > p = td->td_proc; > @@ -2529,24 +2569,24 @@ issignal(struct thread *td, int stop_allowed) > if (p->p_flag & P_TRACED && (p->p_flag & P_PPWAIT) == 0) { > /* > * If traced, always stop. > + * Remove old signal from queue before the stop. > + * XXX shrug off debugger, it causes siginfo to > + * be thrown away. > */ > + queue = &td->td_sigqueue; > + psi = NULL; > + if (sigqueue_steal(queue, sig, &psi) == 0) { > + queue = &p->p_sigqueue; > + sigqueue_steal(queue, sig, &psi); > + } > + > mtx_unlock(&ps->ps_mtx); > newsig = ptracestop(td, sig); > mtx_lock(&ps->ps_mtx); > > if (sig != newsig) { > - ksiginfo_t ksi; > - > - queue = &td->td_sigqueue; > - /* > - * clear old signal. > - * XXX shrug off debugger, it causes siginfo to > - * be thrown away. > - */ > - if (sigqueue_get(queue, sig, &ksi) == 0) { > - queue = &p->p_sigqueue; > - sigqueue_get(queue, sig, &ksi); > - } > + if (psi != NULL && ksiginfo_tryfree(psi)) > + p->p_pendingcnt--; > > /* > * If parent wants us to take the signal, > @@ -2561,10 +2601,14 @@ issignal(struct thread *td, int stop_allowed) > * Put the new signal into td_sigqueue. If the > * signal is being masked, look for other signals. > */ > - SIGADDSET(queue->sq_signals, sig); > + sigqueue_add(queue, sig, NULL); > if (SIGISMEMBER(td->td_sigmask, sig)) > continue; > signotify(td); > + } else { > + if (psi != NULL) > + psi->ksi_flags |= KSI_INS | KSI_HEAD; > + sigqueue_add(&td->td_sigqueue, sig, psi); > } > > /* > diff --git a/sys/sys/signalvar.h b/sys/sys/signalvar.h > index c9455c2..c35fe69 100644 > --- a/sys/sys/signalvar.h > +++ b/sys/sys/signalvar.h > @@ -234,6 +234,7 @@ typedef struct ksiginfo { > #define KSI_EXT 0x02 /* Externally managed ksi. */ > #define KSI_INS 0x04 /* Directly insert ksi, not the copy */ > #define KSI_SIGQ 0x08 /* Generated by sigqueue, might ret EGAIN. */ > +#define KSI_HEAD 0x10 /* Insert into head, not tail. */ > #define KSI_COPYMASK (KSI_TRAP|KSI_SIGQ) > > #define KSI_ONQ(ksi) ((ksi)->ksi_sigq != NULL) In the patch, even if you removed the KSI from queue, current thread still holds the pointer and sleeps, this might be not a good idea since other code try to implement reliable signal like SIGCHLD and AIO signal and mqueue, those code may remove the signal from queue and free it, if they run before the current thread, they will do. otherwise, we have to use unreliable signals, that means, when a realtime signal is delivered, sig_info is lost and only a bit set in sq_kill like sigqueue() syscall does, this is not perfect.