Date: Mon, 13 Feb 2012 14:04:24 -0800 From: Dmitry Mikulin <dmitrym@juniper.net> To: Konstantin Belousov <kostikbel@gmail.com> Cc: freebsd-current Current <freebsd-current@freebsd.org>, Marcel Moolenaar <marcelm@juniper.net> Subject: Re: [ptrace] please review follow fork/exec changes Message-ID: <4F3988E8.2040705@juniper.net> In-Reply-To: <20120213152825.GH3283@deviant.kiev.zoral.com.ua> References: <20120204204218.GC3283@deviant.kiev.zoral.com.ua> <4F3043E2.6090607@juniper.net> <20120207121022.GC3283@deviant.kiev.zoral.com.ua> <4F318D74.9030506@juniper.net> <4F31C89C.7010705@juniper.net> <4F3318AD.6000607@juniper.net> <20120209122908.GD3283@deviant.kiev.zoral.com.ua> <4F34311A.9050702@juniper.net> <20120210001725.GJ3283@deviant.kiev.zoral.com.ua> <4F3478B3.9040809@juniper.net> <20120213152825.GH3283@deviant.kiev.zoral.com.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
--------------010105090202050104070301 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit On 02/13/2012 07:28 AM, Konstantin Belousov wrote: > I looked at the orphan.patch. > > Am I right that the orphans are the real childs of the process which > are temporarily reparented to the debugger ? Whatever they are, a comment > should be added to proc.h describing what does it mean. Done. > > Please provide me with a test case that demonstrates the issue > solved by the change. <ftp://ftp.springdaemons.com/soft/>I attached 2 source files that compile into separate binaries. scescx should be able The problem I'm trying to solve is to allow a parent to collect it's child exit status while we're following its child. Gdb detaches from the parent upon successful switch-over from parent to child. At this point due to re-parenting the parent loses the child to gdb and if it's in a wait() it'll get a return status that it has no children to wait for. > > The new LIST_FOREACH(&q->p_orphans) body is copy/pasted, together > with the comments, from the LIST_FOREACH(&q->p_children). Can the > common code be moved into some function ? Moved the common code into a function. Didn't have time to test though. > > Shouldn't there be some assertion in proc_reparent() for the case when > we remove child from the orphans list, that the child is no longer > debugged ? Hmm... Not sure I understand... > > Why in proc_reparent(), in the case of P_TRACED child, you do > PROC_UNLOC/PROC_LOCK ? No idea how it ended up like that... I'll clean it up. > > It seems that now wait4(2) can be called from the real (non-debugger) > parent first and result in the call to proc_reap(), isn't it ? We would > then just reparent the child back to the caller, still leaving the > zombie and confusing debugger. When either gdb or the real parent gets to proc_reap() the process wouldn't get destroyed, it'll get caught by the following clause: if (p->p_oppid && (t = pfind(p->p_oppid)) != NULL) { and the real parent with get the child back into the children's list while gdb will get it into the orphan list. The second time around when proc_reap() is entered, p->p_oppid will be 0 and the process will get really reaped. Does it make sense? And proc_reparent() attempts to keep the orphan list clean and not have the same entries and the list of siblings. The idea is to keep the real parent alive ling enough to collect the statuc of the child running under gdb. --------------010105090202050104070301 Content-Type: text/x-patch; name="orphan-1.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="orphan-1.diff" Index: sys/proc.h =================================================================== --- sys/proc.h (revision 231328) +++ sys/proc.h (working copy) @@ -507,8 +507,6 @@ struct proc { /* The following fields are all zeroed upon creation in fork. */ #define p_startzero p_oppid pid_t p_oppid; /* (c + e) Save ppid in ptrace. XXX */ - int p_dbg_child; /* (c + e) # of debugged children in - ptrace. */ struct vmspace *p_vmspace; /* (b) Address space. */ u_int p_swtick; /* (c) Tick when swapped in or out. */ struct itimerval p_realtimer; /* (c) Alarm timer. */ @@ -576,6 +574,11 @@ struct proc { after fork. */ uint64_t p_prev_runtime; /* (c) Resource usage accounting. */ struct racct *p_racct; /* (b) Resource accounting. */ + /* An orphan is process that has beed re-parented to the debugger + as a result of attaching to it. Need to keep track of tham to + be able to collect the exit status of what used to be children. */ + LIST_ENTRY(proc) p_orphan; /* (e) List of orphan processes. */ + LIST_HEAD(, proc) p_orphans; /* (e) Pointer to list of orphans. */ }; #define p_session p_pgrp->pg_session @@ -867,7 +870,7 @@ void procinit(void); void proc_linkup0(struct proc *p, struct thread *td); void proc_linkup(struct proc *p, struct thread *td); void proc_reap(struct thread *td, struct proc *p, int *status, int options, - struct rusage *rusage); + struct rusage *rusage, int child); void proc_reparent(struct proc *child, struct proc *newparent); struct pstats *pstats_alloc(void); void pstats_fork(struct pstats *src, struct pstats *dst); Index: kern/kern_exit.c =================================================================== --- kern/kern_exit.c (revision 231328) +++ kern/kern_exit.c (working copy) @@ -680,7 +680,7 @@ sys_wait4(struct thread *td, struct wait_args *uap */ void proc_reap(struct thread *td, struct proc *p, int *status, int options, - struct rusage *rusage) + struct rusage *rusage, int child) { struct proc *q, *t; @@ -720,7 +720,6 @@ proc_reap(struct thread *td, struct proc *p, int * if (p->p_oppid && (t = pfind(p->p_oppid)) != NULL) { PROC_LOCK(p); proc_reparent(p, t); - p->p_pptr->p_dbg_child--; p->p_oppid = 0; PROC_UNLOCK(p); pksignal(t, SIGCHLD, p->p_ksi); @@ -738,7 +737,10 @@ proc_reap(struct thread *td, struct proc *p, int * sx_xlock(&allproc_lock); LIST_REMOVE(p, p_list); /* off zombproc */ sx_xunlock(&allproc_lock); - LIST_REMOVE(p, p_sibling); + if (child) + LIST_REMOVE(p, p_sibling); + else + LIST_REMOVE(p, p_orphan); leavepgrp(p); #ifdef PROCDESC if (p->p_procdesc != NULL) @@ -803,12 +805,54 @@ proc_reap(struct thread *td, struct proc *p, int * sx_xunlock(&allproc_lock); } +static int +proc_to_reap (struct thread *td, struct proc *p, pid_t pid, int *status, + int options, struct rusage *rusage, int child) +{ + struct proc *q; + + q = td->td_proc; + PROC_LOCK(p); + if (pid != WAIT_ANY && + p->p_pid != pid && p->p_pgid != -pid) { + PROC_UNLOCK(p); + return (0); + } + if (p_canwait(td, p)) { + PROC_UNLOCK(p); + return (0); + } + + /* + * This special case handles a kthread spawned by linux_clone + * (see linux_misc.c). The linux_wait4 and linux_waitpid + * functions need to be able to distinguish between waiting + * on a process and waiting on a thread. It is a thread if + * p_sigparent is not SIGCHLD, and the WLINUXCLONE option + * signifies we want to wait for threads and not processes. + */ + if ((p->p_sigparent != SIGCHLD) ^ + ((options & WLINUXCLONE) != 0)) { + PROC_UNLOCK(p); + return (0); + } + + PROC_SLOCK(p); + if (p->p_state == PRS_ZOMBIE) { + proc_reap(td, p, status, options, rusage, child); + return (-1); + } + PROC_SUNLOCK(p); + PROC_UNLOCK(p); + return (1); +} + int kern_wait(struct thread *td, pid_t pid, int *status, int options, struct rusage *rusage) { struct proc *p, *q; - int error, nfound; + int error, nfound, ret; AUDIT_ARG_PID(pid); AUDIT_ARG_VALUE(options); @@ -831,37 +875,16 @@ loop: nfound = 0; sx_xlock(&proctree_lock); LIST_FOREACH(p, &q->p_children, p_sibling) { - PROC_LOCK(p); - if (pid != WAIT_ANY && - p->p_pid != pid && p->p_pgid != -pid) { - PROC_UNLOCK(p); + ret = proc_to_reap (td, p, pid, status, options, rusage, 1); + if (ret == 0) continue; - } - if (p_canwait(td, p)) { - PROC_UNLOCK(p); - continue; - } + else if (ret == 1) + nfound++; + else + return (0); - /* - * This special case handles a kthread spawned by linux_clone - * (see linux_misc.c). The linux_wait4 and linux_waitpid - * functions need to be able to distinguish between waiting - * on a process and waiting on a thread. It is a thread if - * p_sigparent is not SIGCHLD, and the WLINUXCLONE option - * signifies we want to wait for threads and not processes. - */ - if ((p->p_sigparent != SIGCHLD) ^ - ((options & WLINUXCLONE) != 0)) { - PROC_UNLOCK(p); - continue; - } - - nfound++; + PROC_LOCK(p); PROC_SLOCK(p); - if (p->p_state == PRS_ZOMBIE) { - proc_reap(td, p, status, options, rusage); - return (0); - } if ((p->p_flag & P_STOPPED_SIG) && (p->p_suspcount == p->p_numthreads) && (p->p_flag & P_WAITED) == 0 && @@ -893,16 +916,21 @@ loop: if (status) *status = SIGCONT; - return (0); } PROC_UNLOCK(p); } + LIST_FOREACH(p, &q->p_orphans, p_orphan) { + ret = proc_to_reap (td, p, pid, status, options, rusage, 0); + if (ret == 0) + continue; + else if (ret == 1) + nfound++; + else + return (0); + } if (nfound == 0) { sx_xunlock(&proctree_lock); - if (td->td_proc->p_dbg_child) - return (0); - else - return (ECHILD); + return (ECHILD); } if (options & WNOHANG) { sx_xunlock(&proctree_lock); @@ -929,6 +957,7 @@ loop: void proc_reparent(struct proc *child, struct proc *parent) { + struct proc *p; sx_assert(&proctree_lock, SX_XLOCKED); PROC_LOCK_ASSERT(child, MA_OWNED); @@ -940,5 +969,15 @@ proc_reparent(struct proc *child, struct proc *par PROC_UNLOCK(child->p_pptr); LIST_REMOVE(child, p_sibling); LIST_INSERT_HEAD(&parent->p_children, child, p_sibling); + + LIST_FOREACH(p, &parent->p_orphans, p_orphan) { + if (p == child) { + LIST_REMOVE(child, p_orphan); + break; + } + } + if (child->p_flag & P_TRACED) + LIST_INSERT_HEAD(&child->p_pptr->p_orphans, child, p_orphan); + child->p_pptr = parent; } Index: kern/kern_fork.c =================================================================== --- kern/kern_fork.c (revision 231328) +++ kern/kern_fork.c (working copy) @@ -590,6 +590,7 @@ do_fork(struct thread *td, int flags, struct proc LIST_INSERT_AFTER(p1, p2, p_pglist); PGRP_UNLOCK(p1->p_pgrp); LIST_INIT(&p2->p_children); + LIST_INIT(&p2->p_orphans); callout_init(&p2->p_itcallout, CALLOUT_MPSAFE); Index: kern/sys_process.c =================================================================== --- kern/sys_process.c (revision 231328) +++ kern/sys_process.c (working copy) @@ -841,8 +841,6 @@ kern_ptrace(struct thread *td, int req, pid_t pid, p->p_flag |= P_TRACED; p->p_oppid = p->p_pptr->p_pid; if (p->p_pptr != td->td_proc) { - /* Remember that a child is being debugged(traced). */ - p->p_pptr->p_dbg_child++; proc_reparent(p, td->td_proc); } data = SIGSTOP; @@ -931,7 +929,6 @@ kern_ptrace(struct thread *td, int req, pid_t pid, PROC_UNLOCK(pp); PROC_LOCK(p); proc_reparent(p, pp); - p->p_pptr->p_dbg_child--; if (pp == initproc) p->p_sigparent = SIGCHLD; } Index: kern/sys_procdesc.c =================================================================== --- kern/sys_procdesc.c (revision 231328) +++ kern/sys_procdesc.c (working copy) @@ -367,7 +367,7 @@ procdesc_close(struct file *fp, struct thread *td) * procdesc_reap(). */ PROC_SLOCK(p); - proc_reap(curthread, p, NULL, 0, NULL); + proc_reap(curthread, p, NULL, 0, NULL, 0); } else { /* * If the process is not yet dead, we need to kill it, but we --------------010105090202050104070301--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4F3988E8.2040705>