From owner-freebsd-current@freebsd.org Wed Jul 13 04:54:51 2016 Return-Path: Delivered-To: freebsd-current@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 652FFB93F03 for ; Wed, 13 Jul 2016 04:54:51 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id E0EBE17FE; Wed, 13 Jul 2016 04:54:50 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from tom.home (kib@localhost [127.0.0.1]) by kib.kiev.ua (8.15.2/8.15.2) with ESMTPS id u6D4sdDF000440 (version=TLSv1 cipher=DHE-RSA-CAMELLIA256-SHA bits=256 verify=NO); Wed, 13 Jul 2016 07:54:39 +0300 (EEST) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua u6D4sdDF000440 Received: (from kostik@localhost) by tom.home (8.15.2/8.15.2/Submit) id u6D4sdkV000439; Wed, 13 Jul 2016 07:54:39 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Wed, 13 Jul 2016 07:54:39 +0300 From: Konstantin Belousov To: Mark Johnston Cc: freebsd-current@FreeBSD.org Subject: Re: ptrace attach in multi-threaded processes Message-ID: <20160713045439.GT38613@kib.kiev.ua> References: <20160712011938.GA51319@wkstn-mjohnston.west.isilon.com> <20160712055753.GI38613@kib.kiev.ua> <20160712170502.GA71220@wkstn-mjohnston.west.isilon.com> <20160712175150.GP38613@kib.kiev.ua> <20160712182414.GC71220@wkstn-mjohnston.west.isilon.com> <20160713033036.GR38613@kib.kiev.ua> <20160713040210.GA89573@wkstn-mjohnston.west.isilon.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160713040210.GA89573@wkstn-mjohnston.west.isilon.com> User-Agent: Mutt/1.6.1 (2016-04-27) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.1 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on tom.home X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 13 Jul 2016 04:54:51 -0000 On Tue, Jul 12, 2016 at 09:02:10PM -0700, Mark Johnston wrote: > Hm, the only SIGSTOP in my scenario is the one generated by PT_ATTACH. > The problem occurs when this SIGSTOP races with *any* other signal that's > being delivered to the process and for which the process has registered > a handler. For instance, SIGHUP after a log rotation. > > If a real SIGSTOP races with PT_ATTACH, then I would indeed expect to > find the process in a stopped state after the detach. Does this make > more sense? I finally see. Might be something like the patch below is a step in the desired direction. Idea is in the proc_next_xthread(): p_xthread should be set to the next thread with a pending signal. Do you have a test case that demonstrates the issue ? diff --git a/sys/kern/kern_sig.c b/sys/kern/kern_sig.c index 2a5e6de..1106f3a 100644 --- a/sys/kern/kern_sig.c +++ b/sys/kern/kern_sig.c @@ -2525,22 +2525,21 @@ ptracestop(struct thread *td, int sig) PROC_SUNLOCK(p); return (sig); } - /* - * Just make wait() to work, the last stopped thread - * will win. - */ - p->p_xsig = sig; - p->p_xthread = td; - p->p_flag |= (P_STOPPED_SIG|P_STOPPED_TRACE); - sig_suspend_threads(td, p, 0); - if ((td->td_dbgflags & TDB_STOPATFORK) != 0) { - td->td_dbgflags &= ~TDB_STOPATFORK; - cv_broadcast(&p->p_dbgwait); + if (p->p_xthread == NULL) + p->p_xthread = td; + if (p->p_xthread == td) { + p->p_xsig = sig; + p->p_flag |= (P_STOPPED_SIG|P_STOPPED_TRACE); + sig_suspend_threads(td, p, 0); + if ((td->td_dbgflags & TDB_STOPATFORK) != 0) { + td->td_dbgflags &= ~TDB_STOPATFORK; + cv_broadcast(&p->p_dbgwait); + } } stopme: thread_suspend_switch(td, p); if (p->p_xthread == td) - p->p_xthread = NULL; + proc_next_xthread(p); if (!(p->p_flag & P_TRACED)) break; if (td->td_dbgflags & TDB_SUSPEND) { diff --git a/sys/kern/sys_process.c b/sys/kern/sys_process.c index a6037e3..3d9950b 100644 --- a/sys/kern/sys_process.c +++ b/sys/kern/sys_process.c @@ -1057,7 +1057,7 @@ kern_ptrace(struct thread *td, int req, pid_t pid, void *addr, int data) proctree_locked = 0; } p->p_xsig = data; - p->p_xthread = NULL; + proc_next_xthread(p); if ((p->p_flag & (P_STOPPED_SIG | P_STOPPED_TRACE)) != 0) { /* deliver or queue signal */ td2->td_dbgflags &= ~TDB_XSIG; @@ -1065,7 +1065,8 @@ kern_ptrace(struct thread *td, int req, pid_t pid, void *addr, int data) if (req == PT_DETACH) { FOREACH_THREAD_IN_PROC(p, td3) - td3->td_dbgflags &= ~TDB_SUSPEND; + td3->td_dbgflags &= ~(TDB_SUSPEND | + TDB_XSIG); } /* * unsuspend all threads, to not let a thread run, @@ -1376,9 +1377,24 @@ stopevent(struct proc *p, unsigned int event, unsigned int val) do { if (event != S_EXIT) p->p_xsig = val; - p->p_xthread = NULL; + proc_next_xthread(p); p->p_stype = event; /* Which event caused the stop? */ wakeup(&p->p_stype); /* Wake up any PIOCWAIT'ing procs */ msleep(&p->p_step, &p->p_mtx, PWAIT, "stopevent", 0); } while (p->p_step); } + +void +proc_next_xthread(struct proc *p) +{ + struct thread *td; + + PROC_LOCK_ASSERT(p, MA_OWNED); + FOREACH_THREAD_IN_PROC(p, td) { + if (td == p->p_xthread) + continue; + if ((td->td_dbgflags & TDB_XSIG) != 0) + break; + } + p->p_xthread = td; +} diff --git a/sys/sys/proc.h b/sys/sys/proc.h index f533db6..a3132d9 100644 --- a/sys/sys/proc.h +++ b/sys/sys/proc.h @@ -999,6 +999,7 @@ int proc_getenvv(struct thread *td, struct proc *p, struct sbuf *sb); void procinit(void); void proc_linkup0(struct proc *p, struct thread *td); void proc_linkup(struct proc *p, struct thread *td); +void proc_next_xthread(struct proc *p); struct proc *proc_realparent(struct proc *child); void proc_reap(struct thread *td, struct proc *p, int *status, int options); void proc_reparent(struct proc *child, struct proc *newparent);