Re: [ptrace] please review follow fork/exec changes

On Fri, Feb 03, 2012 at 04:01:46PM -0800, Dmitry Mikulin wrote:
> 
> >Please provide more details, I am looking forward for the panic
> >message and backtrace.
> 
> I can't seem to get the panic with the latest source base, but tracing 
> doesn't appear to work with vfork(). I attached a modified test case to 
> closer model what gdb is doing. If you change fork() to vfork() in simple.c 
> the parent doesn't get the events the same way it does under fork().
I see what is going on. The wait loop for P_PPWAIT in do_fork() simply
do not allow the ptracestop() in the syscall return path to be reached.
There seems to be more problems. In particular, I do not see anything
which would prevent the child from being reapped while the loop is
executing (assume that the parent is multithreaded and other thread
processed SIGCHLD and called wait).

Lets deal with these bugs after your proposal for interface changes is
dealt with.

> 
> >The lack of the notification for parent is exactly what the patch I
> >posted fixes. More exactly, it is the lack of notification for parent
> >with PT_CONTINUE loop. I will commit this today.
> >
> >Regarding a single notification. Currently, parent arrives at the
> >syscall return code only after the child is attached to the debugger.
> >See the cv_wait() in kern_fork.c:739. In other words, if you get the
> >PL_FLAG_FORK, the child is already attached (at last it shall be). My
> >scescx.c code illustrates this well, IMO.
> >
> >You still get a separate stop from the child, but I do not see how is it
> >harmful.
> 
> There a couple of tweaks to the interface that I'd like to have:
> - set PL_FLAG_FORKED in the child so gdb can identify the stop as a fork() 
> event.
> - add a switch similar to PT_FOLLOW_FORK to enable/disable exec() 
> notifications. Gdb gets confused by the events it hasn't explicitly asked 
> for and having a switch makes it easier to filter out unwanted stops.
> 
> Do you think it's possible/makes sense?

Yes, I agree with the proposal to add flag to the child lwp info.
I think it will be easier if the flag is different from PL_FLAG_FORKED.
I named it PL_FLAG_CHILD.

PT_FOLLOW_EXEC is easy to implement, but my question is, how can debugger
operate (correctly) if it ignores exec events ? After exec, the whole
cached state of the debuggee must be invalidated, and since debugger
ignores the notification when the invalidation shall be done, it probably
gets very confused.

Anyway, below is the combined patch that adds PL_FLAG_CHILD and
PT_FOLLOW_EXEC. If you agree with this, I will commit them (separately).

diff --git a/sys/kern/kern_exec.c b/sys/kern/kern_exec.c
index 135f798..c756777 100644
--- a/sys/kern/kern_exec.c
+++ b/sys/kern/kern_exec.c
_at__at_ -889,7 +889,8 _at__at_ exec_fail_dealloc:
 
 	if (error == 0) {
 		PROC_LOCK(p);
-		td->td_dbgflags |= TDB_EXEC;
+		if ((p->p_flag & P_NOFOLLOWEXEC) == 0)
+			td->td_dbgflags |= TDB_EXEC;
 		PROC_UNLOCK(p);
 
 		/*
diff --git a/sys/kern/kern_fork.c b/sys/kern/kern_fork.c
index 60639c9..e447c93 100644
--- a/sys/kern/kern_fork.c
+++ b/sys/kern/kern_fork.c
_at__at_ -1035,7 +1035,9 _at__at_ fork_return(struct thread *td, struct trapframe *frame)
 			p->p_oppid = p->p_pptr->p_pid;
 			proc_reparent(p, dbg);
 			sx_xunlock(&proctree_lock);
+			td->td_dbgflags |= TDB_CHILD;
 			ptracestop(td, SIGSTOP);
+			td->td_dbgflags &= ~TDB_CHILD;
 		} else {
 			/*
 			 * ... otherwise clear the request.
diff --git a/sys/kern/sys_process.c b/sys/kern/sys_process.c
index 4510380..f58039a 100644
--- a/sys/kern/sys_process.c
+++ b/sys/kern/sys_process.c
_at__at_ -660,6 +660,7 _at__at_ kern_ptrace(struct thread *td, int req, pid_t pid, void *addr, int data)
 	case PT_TO_SCX:
 	case PT_SYSCALL:
 	case PT_FOLLOW_FORK:
+	case PT_FOLLOW_EXEC:
 	case PT_DETACH:
 		sx_xlock(&proctree_lock);
 		proctree_locked = 1;
_at__at_ -873,6 +874,12 _at__at_ kern_ptrace(struct thread *td, int req, pid_t pid, void *addr, int data)
 		else
 			p->p_flag &= ~P_FOLLOWFORK;
 		break;
+	case PT_FOLLOW_EXEC:
+		if (data)
+			p->p_flag &= ~P_NOFOLLOWEXEC;
+		else
+			p->p_flag |= P_NOFOLLOWEXEC;
+		break;
 
 	case PT_STEP:
 	case PT_CONTINUE:
_at__at_ -936,7 +943,8 _at__at_ kern_ptrace(struct thread *td, int req, pid_t pid, void *addr, int data)
 					p->p_sigparent = SIGCHLD;
 			}
 			p->p_oppid = 0;
-			p->p_flag &= ~(P_TRACED | P_WAITED | P_FOLLOWFORK);
+			p->p_flag &= ~(P_TRACED | P_WAITED | P_FOLLOWFORK |
+			    P_NOFOLLOWEXEC);
 
 			/* should we send SIGCHLD? */
 			/* childproc_continued(p); */
_at__at_ -1145,6 +1153,8 _at__at_ kern_ptrace(struct thread *td, int req, pid_t pid, void *addr, int data)
 			pl->pl_flags |= PL_FLAG_FORKED;
 			pl->pl_child_pid = td2->td_dbg_forked;
 		}
+		if (td2->td_dbgflags & TDB_CHILD)
+			pl->pl_flags |= PL_FLAG_CHILD;
 		pl->pl_sigmask = td2->td_sigmask;
 		pl->pl_siglist = td2->td_siglist;
 		strcpy(pl->pl_tdname, td2->td_name);
diff --git a/sys/sys/proc.h b/sys/sys/proc.h
index 76f3355..91bc86e 100644
--- a/sys/sys/proc.h
+++ b/sys/sys/proc.h
_at__at_ -384,6 +384,7 _at__at_ do {									\
 				      process */
 #define	TDB_STOPATFORK	0x00000080 /* Stop at the return from fork (child
 				      only) */
+#define	TDB_CHILD	0x00000100 /* New child indicator for ptrace() */
 
 /*
  * "Private" flags kept in td_pflags:
_at__at_ -613,6 +614,7 _at__at_ struct proc {
 #define	P_HWPMC		0x800000 /* Process is using HWPMCs */
 
 #define	P_JAILED	0x1000000 /* Process is in jail. */
+#define	P_NOFOLLOWEXEC	0x2000000 /* Do not report execs with ptrace. */
 #define	P_INEXEC	0x4000000 /* Process is in execve(). */
 #define	P_STATCHILD	0x8000000 /* Child process stopped or exited. */
 #define	P_INMEM		0x10000000 /* Loaded into memory. */
diff --git a/sys/sys/ptrace.h b/sys/sys/ptrace.h
index 2583d59..05c758c 100644
--- a/sys/sys/ptrace.h
+++ b/sys/sys/ptrace.h
_at__at_ -64,6 +64,7 _at__at_
 #define	PT_SYSCALL	22
 
 #define	PT_FOLLOW_FORK	23
+#define	PT_FOLLOW_EXEC	24
 
 #define PT_GETREGS      33	/* get general-purpose registers */
 #define PT_SETREGS      34	/* set general-purpose registers */
_at__at_ -106,7 +107,8 _at__at_ struct ptrace_lwpinfo {
 #define	PL_FLAG_SCX	0x08	/* syscall leave point */
 #define	PL_FLAG_EXEC	0x10	/* exec(2) succeeded */
 #define	PL_FLAG_SI	0x20	/* siginfo is valid */
-#define	PL_FLAG_FORKED	0x40	/* new child */
+#define	PL_FLAG_FORKED	0x40	/* child born */
+#define	PL_FLAG_CHILD	0x80	/* I am from child */
 	sigset_t	pl_sigmask;	/* LWP signal mask */
 	sigset_t	pl_siglist;	/* LWP pending signal */
 	struct __siginfo pl_siginfo;	/* siginfo for signal */