Re: [ptrace] please review follow fork/exec changes
On Mon, Feb 13, 2012 at 02:50:45PM -0800, Dmitry Mikulin wrote:
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.
Right, this is what I figured. But I asked about some further implication
of this change:
if real parent spuriosly calls wait4(2) on the child pid after the child
exited, but before the debugger called the wait4(), then exactly the
code you noted above will be run. This results in the child being fully
returned to the original parent.
Next, the wait4() call from debugger gets an error, and zombie will be
kept around until parent calls wait4() for this pid once more.
Am I missed something ?
In this case the process will move from gdb's child list to gdb's orphan
list when the real parent does a wait4(). Next time around the wait loop in
gdb it'll be caught by the orphan's proc_reap().
I do not see how the next debugger loop could find this process at all,
since the first wait4() call reparented it to the original parent.
pgpYsbd4CxqpH.pgp
Description: PGP signature
Re: [ptrace] please review follow fork/exec changes
On Wed, Feb 15, 2012 at 09:22:10AM -0800, Dmitry Mikulin wrote:
On 02/15/2012 08:32 AM, Konstantin Belousov wrote:
On Mon, Feb 13, 2012 at 02:50:45PM -0800, Dmitry Mikulin wrote:
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.
Right, this is what I figured. But I asked about some further implication
of this change:
if real parent spuriosly calls wait4(2) on the child pid after the child
exited, but before the debugger called the wait4(), then exactly the
code you noted above will be run. This results in the child being fully
returned to the original parent.
Next, the wait4() call from debugger gets an error, and zombie will be
kept around until parent calls wait4() for this pid once more.
Am I missed something ?
In this case the process will move from gdb's child list to gdb's orphan
list when the real parent does a wait4(). Next time around the wait loop
in
gdb it'll be caught by the orphan's proc_reap().
I do not see how the next debugger loop could find this process at all,
since the first wait4() call reparented it to the original parent.
Not the debugger loop, the kern_wait() loop. The child get re-parented to
the original parent but moves to the orphan list of the debugger process.
Either the debugger loop which calls wait4/waitpid, or the kern_wait loop
resulting from the debugger calling wait*.
Could you, please, describe, how the patched kernel moves the wait'ed
zombie to the orphan list of the debugger ?
For me, it seems that there is another bug, the child appears both on
the childdren list, and on the orphan list of the real parent.
pgpeVg6zOwT6c.pgp
Description: PGP signature
Re: [ptrace] please review follow fork/exec changes
On 02/15/2012 08:32 AM, Konstantin Belousov wrote:
On Mon, Feb 13, 2012 at 02:50:45PM -0800, Dmitry Mikulin wrote:
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.
Right, this is what I figured. But I asked about some further implication
of this change:
if real parent spuriosly calls wait4(2) on the child pid after the child
exited, but before the debugger called the wait4(), then exactly the
code you noted above will be run. This results in the child being fully
returned to the original parent.
Next, the wait4() call from debugger gets an error, and zombie will be
kept around until parent calls wait4() for this pid once more.
Am I missed something ?
In this case the process will move from gdb's child list to gdb's orphan
list when the real parent does a wait4(). Next time around the wait loop in
gdb it'll be caught by the orphan's proc_reap().
I do not see how the next debugger loop could find this process at all,
since the first wait4() call reparented it to the original parent.
Not the debugger loop, the kern_wait() loop. The child get re-parented to the
original parent but moves to the orphan list of the debugger process.
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: [ptrace] please review follow fork/exec changes
On Wed, Feb 15, 2012 at 09:54:44AM -0800, Dmitry Mikulin wrote:
On 02/15/2012 09:40 AM, Konstantin Belousov wrote:
On Wed, Feb 15, 2012 at 09:22:10AM -0800, Dmitry Mikulin wrote:
On 02/15/2012 08:32 AM, Konstantin Belousov wrote:
On Mon, Feb 13, 2012 at 02:50:45PM -0800, Dmitry Mikulin wrote:
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.
Right, this is what I figured. But I asked about some further
implication
of this change:
if real parent spuriosly calls wait4(2) on the child pid after the
child
exited, but before the debugger called the wait4(), then exactly the
code you noted above will be run. This results in the child being fully
returned to the original parent.
Next, the wait4() call from debugger gets an error, and zombie will be
kept around until parent calls wait4() for this pid once more.
Am I missed something ?
In this case the process will move from gdb's child list to gdb's orphan
list when the real parent does a wait4(). Next time around the wait loop
in
gdb it'll be caught by the orphan's proc_reap().
I do not see how the next debugger loop could find this process at all,
since the first wait4() call reparented it to the original parent.
Not the debugger loop, the kern_wait() loop. The child get re-parented to
the original parent but moves to the orphan list of the debugger process.
Either the debugger loop which calls wait4/waitpid, or the kern_wait loop
resulting from the debugger calling wait*.
Could you, please, describe, how the patched kernel moves the wait'ed
zombie to the orphan list of the debugger ?
For me, it seems that there is another bug, the child appears both on
the childdren list, and on the orphan list of the real parent.
The first attempt to reap the child will get into the
if (p-p_oppid (t = pfind(p-p_oppid)) != NULL) {
clause, which will re-parent it to the real parent. The child will not be
destroyed at this point.
The following loop in proc_reparent() will make sure that the child does
not stay in both lists:
LIST_FOREACH(p, parent-p_orphans, p_orphan) {
if (p == child) {
LIST_REMOVE(child, p_orphan);
break;
}
}
Since the child parent is gdb and it's still being traced, the following
will move it to gdb's orphan list:
if (child-p_flag P_TRACED)
LIST_INSERT_HEAD(child-p_pptr-p_orphans, child, p_orphan);
No, the child parent at this point is no longer the gdb, it is the original
parent. And since P_TRACED is set, the process is inserted also in the
orphans list of the original parent.
This all happens during the first execution of wait4/waitpid from the
real parent, in the proc_reparent.
After this the real parent will get the exit status.
The next pass through the kern_wait() loop called from gdb will catch the
child in its orphan list and will reap it this time for real since
p-p_oppid will be set to 0 in the previous attempt to reap it. Gdb gets
the exit code, the child is destroyed.
No, the child has no longer any assotiation with the debugger process,
since the block in the
if (p-p_oppid (t = pfind(p-p_oppid)) != NULL) {
statement destroyed it.
pgpGeU7C26Grj.pgp
Description: PGP signature
Re: [ptrace] please review follow fork/exec changes
On 02/15/2012 09:40 AM, Konstantin Belousov wrote:
On Wed, Feb 15, 2012 at 09:22:10AM -0800, Dmitry Mikulin wrote:
On 02/15/2012 08:32 AM, Konstantin Belousov wrote:
On Mon, Feb 13, 2012 at 02:50:45PM -0800, Dmitry Mikulin wrote:
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.
Right, this is what I figured. But I asked about some further implication
of this change:
if real parent spuriosly calls wait4(2) on the child pid after the child
exited, but before the debugger called the wait4(), then exactly the
code you noted above will be run. This results in the child being fully
returned to the original parent.
Next, the wait4() call from debugger gets an error, and zombie will be
kept around until parent calls wait4() for this pid once more.
Am I missed something ?
In this case the process will move from gdb's child list to gdb's orphan
list when the real parent does a wait4(). Next time around the wait loop
in
gdb it'll be caught by the orphan's proc_reap().
I do not see how the next debugger loop could find this process at all,
since the first wait4() call reparented it to the original parent.
Not the debugger loop, the kern_wait() loop. The child get re-parented to
the original parent but moves to the orphan list of the debugger process.
Either the debugger loop which calls wait4/waitpid, or the kern_wait loop
resulting from the debugger calling wait*.
Could you, please, describe, how the patched kernel moves the wait'ed
zombie to the orphan list of the debugger ?
For me, it seems that there is another bug, the child appears both on
the childdren list, and on the orphan list of the real parent.
The first attempt to reap the child will get into the
if (p-p_oppid (t = pfind(p-p_oppid)) != NULL) {
clause, which will re-parent it to the real parent. The child will not be
destroyed at this point.
The following loop in proc_reparent() will make sure that the child does not
stay in both lists:
LIST_FOREACH(p, parent-p_orphans, p_orphan) {
if (p == child) {
LIST_REMOVE(child, p_orphan);
break;
}
}
Since the child parent is gdb and it's still being traced, the following will
move it to gdb's orphan list:
if (child-p_flag P_TRACED)
LIST_INSERT_HEAD(child-p_pptr-p_orphans, child, p_orphan);
After this the real parent will get the exit status.
The next pass through the kern_wait() loop called from gdb will catch the child in
its orphan list and will reap it this time for real since p-p_oppid will be
set to 0 in the previous attempt to reap it. Gdb gets the exit code, the child is
destroyed.
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: [ptrace] please review follow fork/exec changes
I'm not sure we are on the same page. Let's do it from the very beginning.
The real parent calls wait4() and enters kern_wait() while the child is in a
zombie state. The child is in the gdb's children list and the real parent's
orphan list.
We enter proc_reap() because the child is caught by the orphan list of the real
parent.
We are at
if (p-p_oppid (t = pfind(p-p_oppid)) != NULL) {
doing proc_reparent() to the real parent of the child.
The child is in the real parent's orphan list and will be removed from it by
proc_reparent and added to the orphan list of gdb.
After we're done re-parenting, the child is in the children list of the real
parent and in the orphan list of gdb.
On the way out of proc_reap we clear p-p_oppid and wait4() returns the exit
code to the real parent.
The child is not yet destroyed and is being waited for by gdb since it's on
gdb's orphan list. We enter proc_reap() through gdb's wait4() and kern_wait().
We don't enter the following this time
if (p-p_oppid (t = pfind(p-p_oppid)) != NULL) {
and go to the actual destruction of the process.
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: [ptrace] please review follow fork/exec changes
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. Please provide me with a test case that demonstrates the issue solved by the change. 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 ? 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 ? Why in proc_reparent(), in the case of P_TRACED child, you do PROC_UNLOC/PROC_LOCK ? 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. pgpZfjlKmTwOb.pgp Description: PGP signature
Re: [ptrace] please review follow fork/exec changes
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.
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 @@
Re: [ptrace] please review follow fork/exec changes
On Mon, Feb 13, 2012 at 02:04:24PM -0800, Dmitry Mikulin wrote:
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.
This text should be put somewhere in the comment. It took me some time
to re-create the reason for the patch during the read.
I will take a look at the example tomorrow, thanks.
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.
Ok. Do not put the space between function name and '('.
Both calls to proc_to_reap() has the space.
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...
proc_reparent() can move the child both to and from the orphan list.
If child is traced, you instert it into the orhpan list.
When removing the child from the orphan list, it means that
debugger finished with the process. My suggestion is to assert this
in proc_reparent (but I am not completely sure that this can be done
easily).
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.
Right, this is what I figured. But I asked about some further implication
of this change:
if real parent spuriosly calls wait4(2) on the child pid after the child
exited, but before the debugger called the wait4(), then exactly the
code you noted above will be run. This results in the child being fully
returned to the original parent.
Next, the wait4() call from debugger gets an error, and zombie will be
kept around until parent calls wait4() for this pid once more.
Am I missed something ?
The idea is to keep the real parent alive ling enough to collect the
statuc of the child running under gdb.
pgpimDQrx4uk7.pgp
Description: PGP signature
Re: [ptrace] please review follow fork/exec changes
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.
This text should be put somewhere in the comment. It took me some time
to re-create the reason for the patch during the read.
I'll find a place in the code to add this comment.
I will take a look at the example tomorrow, thanks.
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.
Ok. Do not put the space between function name and '('.
Both calls to proc_to_reap() has the space.
Habit of a different coding convention... fixed
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...
proc_reparent() can move the child both to and from the orphan list.
If child is traced, you instert it into the orhpan list.
When removing the child from the orphan list, it means that
debugger finished with the process. My suggestion is to assert this
in proc_reparent (but I am not completely sure that this can be done
easily).
Need to think about this one.
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.
Right, this is what I figured. But I asked about some further implication
of this change:
if real parent spuriosly calls wait4(2) on the child pid after the child
exited, but before the debugger called the wait4(), then exactly the
code you noted above will be run. This results in the child being fully
returned to the original parent.
Next, the wait4() call from debugger gets an error, and zombie will be
kept around until parent calls wait4() for this pid once more.
Am I missed something ?
In this case the process will move from gdb's child list to gdb's orphan list
when the real parent does a wait4(). Next time around the wait loop in gdb
it'll be caught by the orphan's proc_reap().
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: [ptrace] please review follow fork/exec changes
On Wed, Feb 08, 2012 at 04:51:57PM -0800, Dmitry Mikulin wrote:
The patch I sent earlier works for me. Just wanted to let you know to
illustrate what I would like to see from the kernel.
I'm trying to see if there's way not to add flags with semantics similar to
TDB_EXEC. I think the problem with TDB_EXEC is that is serves a trigger for
a stop as well as an indicator to return PL_FLAG_EXEC. And in my case I
still want to see all the stops but I only want to see the PL_FLAG_EXEC
when PT_FOLLOW_EXEC is specified.
Do you think the attached patch will do what I'd like without compromising
existing functionality?
The semantic of PL_FLAG_EXEC up until now is very simple: it indicates
that current stop occured during the first return to usermode after
successful exec. The proposed patch breaks the semantic, because now
some stops which satisfy the stated condition are no longer marked with
the flag.
That said, I am lost. You stated that you still need some stops at
exec even when not PT_FOLLOW_EXEC is requested. Why usermode cannot
remember whether the PT_FOLLOW_EXEC was set for the process, and ignore
PL_FLAG_EXEC if not requested ?
I just gave up and added PL_FLAG_EXECF, which is set when PT_FOLLOW_EXEC
was set and exec is active. Would this work for your purposes ?
PL_FLAG_EXECF has the same semantic as PL_FLAG_EXEC had in your
follow-exec.patch. But the stop set is not changed comparing with the
stock src.
Are you fine with PL_FLAG_CHILD part of the changes ? If yes, I will
commit it to make some progress.
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
@@ -1035,7 +1035,9 @@ 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..4f93a79 100644
--- a/sys/kern/sys_process.c
+++ b/sys/kern/sys_process.c
@@ -660,6 +660,7 @@ 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;
@@ -873,6 +874,12 @@ 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_FOLLOWEXEC;
+ else
+ p-p_flag = ~P_FOLLOWEXEC;
+ break;
case PT_STEP:
case PT_CONTINUE:
@@ -936,7 +943,8 @@ 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_FOLLOWEXEC);
/* should we send SIGCHLD? */
/* childproc_continued(p); */
@@ -1139,12 +1147,17 @@ kern_ptrace(struct thread *td, int req, pid_t pid, void
*addr, int data)
pl-pl_flags |= PL_FLAG_SCE;
else if (td2-td_dbgflags TDB_SCX)
pl-pl_flags |= PL_FLAG_SCX;
- if (td2-td_dbgflags TDB_EXEC)
+ if (td2-td_dbgflags TDB_EXEC) {
pl-pl_flags |= PL_FLAG_EXEC;
+ if (p-p_flag P_FOLLOWEXEC)
+ pl-pl_flags |= PL_FLAG_EXECF;
+ }
if (td2-td_dbgflags TDB_FORK) {
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 9ebfe83..bec7223 100644
--- a/sys/sys/proc.h
+++ b/sys/sys/proc.h
@@ -384,6 +384,7 @@ do {
\
process */
#defineTDB_STOPATFORK 0x0080 /* Stop at the return from fork
(child
only) */
+#defineTDB_CHILD
Re: [ptrace] please review follow fork/exec changes
The semantic of PL_FLAG_EXEC up until now is very simple: it indicates that current stop occured during the first return to usermode after successful exec. The proposed patch breaks the semantic, because now some stops which satisfy the stated condition are no longer marked with the flag. That said, I am lost. You stated that you still need some stops at exec even when not PT_FOLLOW_EXEC is requested. Why usermode cannot remember whether the PT_FOLLOW_EXEC was set for the process, and ignore PL_FLAG_EXEC if not requested ? I was trying to avoid making ugly changes in gdb if it was possible not to make ugly changes in the kernel. I changed gdb to work without PT_FOLLOW_EXEC. I just gave up and added PL_FLAG_EXECF, which is set when PT_FOLLOW_EXEC was set and exec is active. Would this work for your purposes ? PL_FLAG_EXECF has the same semantic as PL_FLAG_EXEC had in your follow-exec.patch. But the stop set is not changed comparing with the stock src. Are you fine with PL_FLAG_CHILD part of the changes ? If yes, I will commit it to make some progress. yes, the PL_FLAG_CHILD part works for me. Please commit it and we can move on to the next part of the review. ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: [ptrace] please review follow fork/exec changes
On Thu, Feb 09, 2012 at 12:48:26PM -0800, Dmitry Mikulin wrote:
The semantic of PL_FLAG_EXEC up until now is very simple: it indicates
that current stop occured during the first return to usermode after
successful exec. The proposed patch breaks the semantic, because now
some stops which satisfy the stated condition are no longer marked with
the flag.
That said, I am lost. You stated that you still need some stops at
exec even when not PT_FOLLOW_EXEC is requested. Why usermode cannot
remember whether the PT_FOLLOW_EXEC was set for the process, and ignore
PL_FLAG_EXEC if not requested ?
I was trying to avoid making ugly changes in gdb if it was possible not to
make ugly changes in the kernel. I changed gdb to work without
PT_FOLLOW_EXEC.
So, does the patch below helps you, or did I missed something again ?
I just gave up and added PL_FLAG_EXECF, which is set when PT_FOLLOW_EXEC
was set and exec is active. Would this work for your purposes ?
PL_FLAG_EXECF has the same semantic as PL_FLAG_EXEC had in your
follow-exec.patch. But the stop set is not changed comparing with the
stock src.
Are you fine with PL_FLAG_CHILD part of the changes ? If yes, I will
commit it to make some progress.
yes, the PL_FLAG_CHILD part works for me.
Please commit it and we can move on to the next part of the review.
Committed as r231320. Below is what left for PT_FOLLOWEXEC.
diff --git a/sys/kern/sys_process.c b/sys/kern/sys_process.c
index 2060efe..4f93a79 100644
--- a/sys/kern/sys_process.c
+++ b/sys/kern/sys_process.c
@@ -660,6 +660,7 @@ 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;
@@ -873,6 +874,12 @@ 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_FOLLOWEXEC;
+ else
+ p-p_flag = ~P_FOLLOWEXEC;
+ break;
case PT_STEP:
case PT_CONTINUE:
@@ -936,7 +943,8 @@ 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_FOLLOWEXEC);
/* should we send SIGCHLD? */
/* childproc_continued(p); */
@@ -1139,8 +1147,11 @@ kern_ptrace(struct thread *td, int req, pid_t pid, void
*addr, int data)
pl-pl_flags |= PL_FLAG_SCE;
else if (td2-td_dbgflags TDB_SCX)
pl-pl_flags |= PL_FLAG_SCX;
- if (td2-td_dbgflags TDB_EXEC)
+ if (td2-td_dbgflags TDB_EXEC) {
pl-pl_flags |= PL_FLAG_EXEC;
+ if (p-p_flag P_FOLLOWEXEC)
+ pl-pl_flags |= PL_FLAG_EXECF;
+ }
if (td2-td_dbgflags TDB_FORK) {
pl-pl_flags |= PL_FLAG_FORKED;
pl-pl_child_pid = td2-td_dbg_forked;
diff --git a/sys/sys/proc.h b/sys/sys/proc.h
index 0245e88..bec7223 100644
--- a/sys/sys/proc.h
+++ b/sys/sys/proc.h
@@ -614,6 +614,7 @@ struct proc {
#defineP_HWPMC 0x80 /* Process is using HWPMCs */
#defineP_JAILED0x100 /* Process is in jail. */
+#defineP_FOLLOWEXEC0x200 /* Report execs with ptrace. */
#defineP_INEXEC0x400 /* Process is in execve(). */
#defineP_STATCHILD 0x800 /* Child process stopped or exited. */
#defineP_INMEM 0x1000 /* Loaded into memory. */
diff --git a/sys/sys/ptrace.h b/sys/sys/ptrace.h
index 8a02495..81cebfc 100644
--- a/sys/sys/ptrace.h
+++ b/sys/sys/ptrace.h
@@ -64,6 +64,7 @@
#definePT_SYSCALL 22
#definePT_FOLLOW_FORK 23
+#definePT_FOLLOW_EXEC 24
#define PT_GETREGS 33 /* get general-purpose registers */
#define PT_SETREGS 34 /* set general-purpose registers */
@@ -100,14 +101,15 @@ struct ptrace_lwpinfo {
#definePL_EVENT_NONE 0
#definePL_EVENT_SIGNAL 1
int pl_flags; /* LWP flags. */
-#definePL_FLAG_SA 0x01/* M:N thread */
-#definePL_FLAG_BOUND 0x02/* M:N bound thread */
-#definePL_FLAG_SCE 0x04/* syscall enter point */
-#definePL_FLAG_SCX 0x08/* syscall leave point */
-#definePL_FLAG_EXEC0x10/* exec(2) succeeded */
-#define
Re: [ptrace] please review follow fork/exec changes
On 02/09/2012 04:17 PM, Konstantin Belousov wrote: On Thu, Feb 09, 2012 at 12:48:26PM -0800, Dmitry Mikulin wrote: The semantic of PL_FLAG_EXEC up until now is very simple: it indicates that current stop occured during the first return to usermode after successful exec. The proposed patch breaks the semantic, because now some stops which satisfy the stated condition are no longer marked with the flag. That said, I am lost. You stated that you still need some stops at exec even when not PT_FOLLOW_EXEC is requested. Why usermode cannot remember whether the PT_FOLLOW_EXEC was set for the process, and ignore PL_FLAG_EXEC if not requested ? I was trying to avoid making ugly changes in gdb if it was possible not to make ugly changes in the kernel. I changed gdb to work without PT_FOLLOW_EXEC. So, does the patch below helps you, or did I missed something again ? It works, but I managed to make gdb work without it. So, PT_FOLLOW_EXEC is not needed now. Sorry for the confusion. ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: [ptrace] please review follow fork/exec changes
The patch I sent earlier works for me. Just wanted to let you know to
illustrate what I would like to see from the kernel.
I'm trying to see if there's way not to add flags with semantics similar to
TDB_EXEC. I think the problem with TDB_EXEC is that is serves a trigger for a
stop as well as an indicator to return PL_FLAG_EXEC. And in my case I still
want to see all the stops but I only want to see the PL_FLAG_EXEC when
PT_FOLLOW_EXEC is specified.
Do you think the attached patch will do what I'd like without compromising
existing functionality?
Index: sys/proc.h
===
--- sys/proc.h (revision 231228)
+++ sys/proc.h (working copy)
@@ -384,6 +384,7 @@ do { \
process */
#define TDB_STOPATFORK 0x0080 /* Stop at the return from fork (child
only) */
+#define TDB_CHILD 0x0100 /* New child indicator for ptrace() */
/*
* Private flags kept in td_pflags:
@@ -613,6 +614,7 @@ struct proc {
#define P_HWPMC 0x80 /* Process is using HWPMCs */
#define P_JAILED 0x100 /* Process is in jail. */
+#define P_FOLLOWEXEC 0x200 /* Report execs with ptrace. */
#define P_INEXEC 0x400 /* Process is in execve(). */
#define P_STATCHILD 0x800 /* Child process stopped or exited. */
#define P_INMEM 0x1000 /* Loaded into memory. */
Index: sys/ptrace.h
===
--- sys/ptrace.h (revision 231228)
+++ sys/ptrace.h (working copy)
@@ -64,6 +64,7 @@
#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 */
@@ -106,7 +107,8 @@ 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 */
Index: kern/kern_exec.c
===
--- kern/kern_exec.c (revision 231228)
+++ kern/kern_exec.c (working copy)
@@ -56,6 +56,7 @@ __FBSDID($FreeBSD$);
#include sys/proc.h
#include sys/pioctl.h
#include sys/namei.h
+#include sys/ptrace.h
#include sys/resourcevar.h
#include sys/sdt.h
#include sys/sf_buf.h
Index: kern/kern_fork.c
===
--- kern/kern_fork.c (revision 231228)
+++ kern/kern_fork.c (working copy)
@@ -1035,7 +1035,9 @@ fork_return(struct thread *td, struct trapframe *f
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.
Index: kern/sys_process.c
===
--- kern/sys_process.c (revision 231228)
+++ kern/sys_process.c (working copy)
@@ -660,6 +660,7 @@ kern_ptrace(struct thread *td, int req, pid_t pid,
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;
@@ -873,6 +874,12 @@ kern_ptrace(struct thread *td, int req, pid_t pid,
else
p-p_flag = ~P_FOLLOWFORK;
break;
+ case PT_FOLLOW_EXEC:
+ if (data)
+ p-p_flag |= P_FOLLOWEXEC;
+ else
+ p-p_flag = ~P_FOLLOWEXEC;
+ break;
case PT_STEP:
case PT_CONTINUE:
@@ -936,7 +943,8 @@ kern_ptrace(struct thread *td, int req, pid_t pid,
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_FOLLOWEXEC);
/* should we send SIGCHLD? */
/* childproc_continued(p); */
@@ -1139,12 +1147,15 @@ kern_ptrace(struct thread *td, int req, pid_t pid,
pl-pl_flags |= PL_FLAG_SCE;
else if (td2-td_dbgflags TDB_SCX)
pl-pl_flags |= PL_FLAG_SCX;
- if (td2-td_dbgflags TDB_EXEC)
+ if (td2-td_dbgflags TDB_EXEC
+ (p-p_stops S_PT_SCX || p-p_flag P_FOLLOWEXEC))
pl-pl_flags |= PL_FLAG_EXEC;
if (td2-td_dbgflags TDB_FORK) {
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);
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: [ptrace] please review follow fork/exec changes
On Mon, Feb 06, 2012 at 01:19:30PM -0800, Dmitry Mikulin wrote:
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.
OK.
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.
You're right, the debugger needs to handle exec() events implicitly when it
starts up executables. The problem is that there is OS-independent
machinery in gdb which handles statup fork-exec sequence differently from
when the debuggee itself does an exec(). Basically in the event handling
code I need to be able to distinguish app startup by gdb from an exec done
by the app. Other OS-es have flags like PL_FLAG_EXEC set on demand: they
have an equivalent of PT_FOLLOW_EXEC. I attached a modified patch that
solves the problem. It tries to separate the always-on TDB_EXEC from the
on-demand TDB_FOLLOWEXEC without changing existing functionality. Let me
know if it's acceptable.
So, do you in fact need to distinguish exec stops from syscall exit
against exec stops from PT_FOLLOW_EXEC, or do you need to only get stops
at exec returns from PT_CONTINUE when explicitely requested them ?
I would prefer to not introduce another PL_FLAG_somethingEXEC with the
same semantic as PL_FLAG_EXEC. Instead, would the following patch be fine
for your purposes ? With it, stop on exec should only occur if PT_SCX
is requested, or PT_CONTINUE and PT_FOLLOW_EXEC.
[I am unable to fully test this until tomorrow].
diff --git a/sys/kern/kern_exec.c b/sys/kern/kern_exec.c
index 135f798..67cb1b2 100644
--- a/sys/kern/kern_exec.c
+++ b/sys/kern/kern_exec.c
@@ -56,6 +56,7 @@ __FBSDID($FreeBSD$);
#include sys/proc.h
#include sys/pioctl.h
#include sys/namei.h
+#include sys/ptrace.h
#include sys/resourcevar.h
#include sys/sdt.h
#include sys/sf_buf.h
@@ -889,7 +890,9 @@ exec_fail_dealloc:
if (error == 0) {
PROC_LOCK(p);
- td-td_dbgflags |= TDB_EXEC;
+ if ((p-p_flag P_TRACED) != 0
+ ((P_FOLLOWEXEC) != 0 || (p-p_stops S_PT_SCX) != 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
@@ -1035,7 +1035,9 @@ 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..79bbaed 100644
--- a/sys/kern/sys_process.c
+++ b/sys/kern/sys_process.c
@@ -660,6 +660,7 @@ 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;
@@ -873,6 +874,12 @@ 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_FOLLOWEXEC;
+ else
+ p-p_flag = ~P_FOLLOWEXEC;
+ break;
case PT_STEP:
case PT_CONTINUE:
@@ -936,7 +943,8 @@ 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_FOLLOWEXEC);
/* should we send SIGCHLD? */
/*
Re: [ptrace] please review follow fork/exec changes
On Monday, February 06, 2012 10:12:11 pm David Xu wrote: On 2012/1/26 7:48, Dmitry Mikulin wrote: snip The debugger needs to intercept fork() in both parent and child so it can detach from the old process and attach to the new one. Maybe it'll make more sense in the context of gdb changes. Should I send them too? Don't think Marcel included that patch... Does the orphan list change intended to not lost the child after fork ? But the child shall be traced, so debugger would get the SIGTRAP on the attach on fork returning to usermode. I remember that I explicitely tested this when adding followfork changes. Yes, the debugger gets SIGTRAPs. The problem arises when the real parent of the forked process has the code to collect termination status. Since attaching to a process changes the parent/child relationships, we need to keep track of the children lost due to re-parenting so we can properly attribute their exit status to the real parent. I recall that someone brought a topic in the list said that this should be fixed, debugging a process should not change parent-child relation, instead a new link list data structure should be added to struct proc to trace debugged process, this will make code clean with a small memory overhead. Yes, I have some old patches to start on this, but I hadn't really finished them, and it makes wait() a bit more complicated. It would be nice if ptrace() had its own pwait() or some such instead of overloading wait(). -- John Baldwin ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: [ptrace] please review follow fork/exec changes
So, do you in fact need to distinguish exec stops from syscall exit
against exec stops from PT_FOLLOW_EXEC,
This is pretty much what I need. It's the same stop in syscall return right? I
don't want to change when the stop happens, I want to have an lwpinfo flag that
tells me when a stop occurred in a process under PT_FOLLOW_EXEC.
@@ -889,7 +890,9 @@ exec_fail_dealloc:
if (error == 0) {
PROC_LOCK(p);
- td-td_dbgflags |= TDB_EXEC;
+ if ((p-p_flag P_TRACED) != 0
+ ((P_FOLLOWEXEC) != 0 || (p-p_stops S_PT_SCX) != 0))
+ td-td_dbgflags |= TDB_EXEC;
PROC_UNLOCK(p);
There's a small bug in the patch that makes it not work. The check for
P_FOLLOWEXEC should be:
+ ((p-p_flag P_FOLLOWEXEC) != 0 || (p-p_stops S_PT_SCX) !=
0))
Looks like the patch should work for me but I need to verify.
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: [ptrace] please review follow fork/exec changes
Well, that didn't work... Not sure why since it broke existing gdb. My guess is
we're not getting the exec stops we used to get. Might have to wait till
tomorrow to get more details.
On 02/07/2012 12:45 PM, Dmitry Mikulin wrote:
So, do you in fact need to distinguish exec stops from syscall exit
against exec stops from PT_FOLLOW_EXEC,
This is pretty much what I need. It's the same stop in syscall return right? I
don't want to change when the stop happens, I want to have an lwpinfo flag that
tells me when a stop occurred in a process under PT_FOLLOW_EXEC.
@@ -889,7 +890,9 @@ exec_fail_dealloc:
if (error == 0) {
PROC_LOCK(p);
-td-td_dbgflags |= TDB_EXEC;
+if ((p-p_flag P_TRACED) != 0
+((P_FOLLOWEXEC) != 0 || (p-p_stops S_PT_SCX) != 0))
+td-td_dbgflags |= TDB_EXEC;
PROC_UNLOCK(p);
There's a small bug in the patch that makes it not work. The check for
P_FOLLOWEXEC should be:
+((p-p_flag P_FOLLOWEXEC) != 0 || (p-p_stops S_PT_SCX) != 0))
Looks like the patch should work for me but I need to verify.
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: [ptrace] please review follow fork/exec changes
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.
OK.
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.
You're right, the debugger needs to handle exec() events implicitly when it
starts up executables. The problem is that there is OS-independent machinery in
gdb which handles statup fork-exec sequence differently from when the debuggee
itself does an exec(). Basically in the event handling code I need to be able
to distinguish app startup by gdb from an exec done by the app. Other OS-es
have flags like PL_FLAG_EXEC set on demand: they have an equivalent of
PT_FOLLOW_EXEC. I attached a modified patch that solves the problem. It tries
to separate the always-on TDB_EXEC from the on-demand TDB_FOLLOWEXEC without
changing existing functionality. Let me know if it's acceptable.
Another issue I'm investigating is that after the switch-over to the child gdb
gets a SIGHUP when it continues the child. I think it has to do with the
re-parenting/orphan business. I'll let you know what I find, but if you have an
idea what might be causing it, please let me know.
Thanks.
Dmitry.
Index: kern/kern_exec.c
===
--- kern/kern_exec.c (revision 231088)
+++ kern/kern_exec.c (working copy)
@@ -890,6 +890,8 @@ exec_fail_dealloc:
if (error == 0) {
PROC_LOCK(p);
td-td_dbgflags |= TDB_EXEC;
+ if (p-p_flag P_FOLLOWEXEC)
+ td-td_dbgflags |= TDB_FOLLOWEXEC;
PROC_UNLOCK(p);
/*
Index: kern/kern_fork.c
===
--- kern/kern_fork.c (revision 231088)
+++ kern/kern_fork.c (working copy)
@@ -1035,7 +1035,9 @@ fork_return(struct thread *td, struct trapframe *f
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.
Index: kern/sys_process.c
===
--- kern/sys_process.c (revision 231088)
+++ kern/sys_process.c (working copy)
@@ -660,6 +660,7 @@ kern_ptrace(struct thread *td, int req, pid_t pid,
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;
@@ -873,6 +874,12 @@ kern_ptrace(struct thread *td, int req, pid_t pid,
else
p-p_flag = ~P_FOLLOWFORK;
break;
+ case PT_FOLLOW_EXEC:
+ if (data)
+ p-p_flag = ~P_FOLLOWEXEC;
+ else
+ p-p_flag |= P_FOLLOWEXEC;
+ break;
case PT_STEP:
case PT_CONTINUE:
@@ -936,7 +943,8 @@ kern_ptrace(struct thread *td, int req, pid_t pid,
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_FOLLOWEXEC);
/* should we send SIGCHLD? */
/* childproc_continued(p); */
@@ -1141,10 +1149,14 @@ kern_ptrace(struct thread *td, int req, pid_t pid,
pl-pl_flags |= PL_FLAG_SCX;
if (td2-td_dbgflags TDB_EXEC)
pl-pl_flags |= PL_FLAG_EXEC;
+ if (td2-td_dbgflags TDB_FOLLOWEXEC)
+ pl-pl_flags |= PL_FLAG_FOLLOWEXEC;
if (td2-td_dbgflags TDB_FORK) {
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);
Index: kern/subr_syscall.c
===
--- kern/subr_syscall.c (revision 230847)
+++ kern/subr_syscall.c (working copy)
@@ -216,7 +216,8 @@ syscallret(struct thread *td, int error, struct sy
((td-td_dbgflags (TDB_FORK | TDB_EXEC)) != 0 ||
(p-p_stops S_PT_SCX) != 0))
ptracestop(td, SIGTRAP);
- td-td_dbgflags = ~(TDB_SCX | TDB_EXEC | TDB_FORK);
+ td-td_dbgflags =
+ ~(TDB_SCX | TDB_EXEC | TDB_FOLLOWEXEC | TDB_FORK);
PROC_UNLOCK(p);
}
}
Index: sys/proc.h
===
--- sys/proc.h (revision 231088)
+++
Re: [ptrace] please review follow fork/exec changes
Oops, this should be the part of the patch that sets the flag: @@ -873,6 +872,12 @@ kern_ptrace(struct thread *td, int req, pid_t pid, else p-p_flag = ~P_FOLLOWFORK; break; +case PT_FOLLOW_EXEC: +if (data) +p-p_flag |= P_FOLLOWEXEC; +else +p-p_flag = ~P_FOLLOWEXEC; +break; The SIGHUP I mentioned is due to the fact that the parent exits immediately. I guess that's not a particularly well written program. On 02/06/2012 01:19 PM, Dmitry Mikulin wrote: 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. OK. 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. You're right, the debugger needs to handle exec() events implicitly when it starts up executables. The problem is that there is OS-independent machinery in gdb which handles statup fork-exec sequence differently from when the debuggee itself does an exec(). Basically in the event handling code I need to be able to distinguish app startup by gdb from an exec done by the app. Other OS-es have flags like PL_FLAG_EXEC set on demand: they have an equivalent of PT_FOLLOW_EXEC. I attached a modified patch that solves the problem. It tries to separate the always-on TDB_EXEC from the on-demand TDB_FOLLOWEXEC without changing existing functionality. Let me know if it's acceptable. Another issue I'm investigating is that after the switch-over to the child gdb gets a SIGHUP when it continues the child. I think it has to do with the re-parenting/orphan business. I'll let you know what I find, but if you have an idea what might be causing it, please let me know. Thanks. Dmitry. ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: [ptrace] please review follow fork/exec changes
On 2012/1/26 7:48, Dmitry Mikulin wrote: snip The debugger needs to intercept fork() in both parent and child so it can detach from the old process and attach to the new one. Maybe it'll make more sense in the context of gdb changes. Should I send them too? Don't think Marcel included that patch... Does the orphan list change intended to not lost the child after fork ? But the child shall be traced, so debugger would get the SIGTRAP on the attach on fork returning to usermode. I remember that I explicitely tested this when adding followfork changes. Yes, the debugger gets SIGTRAPs. The problem arises when the real parent of the forked process has the code to collect termination status. Since attaching to a process changes the parent/child relationships, we need to keep track of the children lost due to re-parenting so we can properly attribute their exit status to the real parent. I recall that someone brought a topic in the list said that this should be fixed, debugging a process should not change parent-child relation, instead a new link list data structure should be added to struct proc to trace debugged process, this will make code clean with a small memory overhead. Regards, David Xu ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
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
@@ -889,7 +889,8 @@ 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
@@ -1035,7 +1035,9 @@ 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
@@ -660,6 +660,7 @@ 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;
@@ -873,6 +874,12 @@ 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:
@@ -936,7 +943,8 @@ 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? */
Re: [ptrace] please review follow fork/exec changes
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(). 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? Thanks. Dmitry. ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: [ptrace] please review follow fork/exec changes
On Mon, Jan 30, 2012 at 10:26:25AM -0800, Dmitry Mikulin wrote:
On 01/28/2012 11:48 PM, Kostik Belousov wrote:
On Fri, Jan 27, 2012 at 10:12:13AM -0800, Dmitry Mikulin wrote:
Attached are 4 separate patches for each somewhat independent portion of
the kernel work related to the follow-fork implementation.
Ok, as I said, I think that vfork-fork.patch is just wrong.
Gdb needs to be able to read/write process memory between the time the
child is forked and exec is called (in the vfork case). Without the change
it causes a kernel panic when gdb tries to read/write process memory. Since
my understanding of the kernel is a bit limited, it was the best I could do
at the time. I will send more details about the panic once I get a working
fbsd system again. Maybe there's a better way to deal with the panic.
Please provide more details, I am looking forward for the panic
message and backtrace.
Lets postpone discussion of the orphan.patch for later.
OK.
The follow-fork.patch and follow-exec.patch make me wonder, and I
already expressed my doubts. IMO, all features, except one bug, are
already presented in the stock src.
More, suggested follow-{fork,exec} patches break the SCE/SCX tracers
notification of fork and exec events, since TDB_FORK and TBD_EXEC flags
are consumed before syscall returns (I also said this previously).
Namely, if the process is being debugged, the successfull [f]execve()
causes unconditional stop even. This makes PT_FOLLOW_EXEC unneccessary.
Existing PT_FOLLOW_FORK implementation indeed has a bug, which was not
revealed by my testing during the development, because I only tested
SCE/SCX scenario. Namely, if PT_FOLLOW_FORK is requested, but the next
stop is not SCX, then follow-fork notification is not sent. After this
nit is fixed, PT_FOLLOW_FORK caller gets stops for the child creation.
Child is put into stop state as needed to not loose it.
I think this will fix only a part of the problem, the one that relates to
PT_CONTINUE.
I still need the change that forces a stop in both child and parent on
fork(). Without my changes the notification is generated in the child but
not in the parent. I need to be able to have both processes stopped in gdb
in order to clean up and detach from the parent, and initialize and attach
to the child. The main reason I need both processes stopped is that gdb has
to be able to read/write into both processes address space and registers.
Ideally I would like to have a single event generated for fork() at a point
where both child and parent are stopped and available for ptrace read/write
requests. Do you think it's possible?
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.
pgpDwYkpcgypf.pgp
Description: PGP signature
Re: [ptrace] please review follow fork/exec changes
On 01/28/2012 11:48 PM, Kostik Belousov wrote:
On Fri, Jan 27, 2012 at 10:12:13AM -0800, Dmitry Mikulin wrote:
Attached are 4 separate patches for each somewhat independent portion of
the kernel work related to the follow-fork implementation.
Ok, as I said, I think that vfork-fork.patch is just wrong.
Gdb needs to be able to read/write process memory between the time the child is
forked and exec is called (in the vfork case). Without the change it causes a
kernel panic when gdb tries to read/write process memory. Since my
understanding of the kernel is a bit limited, it was the best I could do at the
time. I will send more details about the panic once I get a working fbsd system
again. Maybe there's a better way to deal with the panic.
Lets postpone discussion of the orphan.patch for later.
OK.
The follow-fork.patch and follow-exec.patch make me wonder, and I
already expressed my doubts. IMO, all features, except one bug, are
already presented in the stock src.
More, suggested follow-{fork,exec} patches break the SCE/SCX tracers
notification of fork and exec events, since TDB_FORK and TBD_EXEC flags
are consumed before syscall returns (I also said this previously).
Namely, if the process is being debugged, the successfull [f]execve()
causes unconditional stop even. This makes PT_FOLLOW_EXEC unneccessary.
Existing PT_FOLLOW_FORK implementation indeed has a bug, which was not
revealed by my testing during the development, because I only tested
SCE/SCX scenario. Namely, if PT_FOLLOW_FORK is requested, but the next
stop is not SCX, then follow-fork notification is not sent. After this
nit is fixed, PT_FOLLOW_FORK caller gets stops for the child creation.
Child is put into stop state as needed to not loose it.
I think this will fix only a part of the problem, the one that relates to
PT_CONTINUE.
I still need the change that forces a stop in both child and parent on fork().
Without my changes the notification is generated in the child but not in the
parent. I need to be able to have both processes stopped in gdb in order to
clean up and detach from the parent, and initialize and attach to the child.
The main reason I need both processes stopped is that gdb has to be able to
read/write into both processes address space and registers. Ideally I would
like to have a single event generated for fork() at a point where both child
and parent are stopped and available for ptrace read/write requests. Do you
think it's possible?
I updated the test program I use to test this functionality, see
http://people.freebsd.org/~kib/misc/scescx.c
The default or -s flag causes it to use SCE/SCX tracing, while -c flag
switches it to use PT_CONTINUE tracing, which should be the kind of loop
performed by normal debuggers. You can see the exec/fork events and
child auto-attach illustrated with this test.
Can you, please, provide the test-case which illustrates the omissions
in the existing API (with the patch below applied), if any ?
diff --git a/sys/kern/subr_syscall.c b/sys/kern/subr_syscall.c
index bba4479..75328f6 100644
--- a/sys/kern/subr_syscall.c
+++ b/sys/kern/subr_syscall.c
@@ -212,7 +212,8 @@ syscallret(struct thread *td, int error, struct
syscall_args *sa __unused)
* executes. If debugger requested tracing of syscall
* returns, do it now too.
*/
- if (traced ((td-td_dbgflags TDB_EXEC) != 0 ||
+ if (traced
+ ((td-td_dbgflags (TDB_FORK | TDB_EXEC)) != 0 ||
(p-p_stops S_PT_SCX) != 0))
ptracestop(td, SIGTRAP);
td-td_dbgflags= ~(TDB_SCX | TDB_EXEC | TDB_FORK);
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: [ptrace] please review follow fork/exec changes
Gdb needs to be able to read/write process memory between the time the
child is forked and exec is called (in the vfork case). Without the change
it causes a kernel panic when gdb tries to read/write process memory. Since
my understanding of the kernel is a bit limited, it was the best I could do
at the time. I will send more details about the panic once I get a working
fbsd system again. Maybe there's a better way to deal with the panic.
Please provide more details, I am looking forward for the panic
message and backtrace.
As soon as I get my FreeBSD box fixed, hopefully tomorrow.
Lets postpone discussion of the orphan.patch for later.
OK.
The follow-fork.patch and follow-exec.patch make me wonder, and I
already expressed my doubts. IMO, all features, except one bug, are
already presented in the stock src.
More, suggested follow-{fork,exec} patches break the SCE/SCX tracers
notification of fork and exec events, since TDB_FORK and TBD_EXEC flags
are consumed before syscall returns (I also said this previously).
Namely, if the process is being debugged, the successfull [f]execve()
causes unconditional stop even. This makes PT_FOLLOW_EXEC unneccessary.
Existing PT_FOLLOW_FORK implementation indeed has a bug, which was not
revealed by my testing during the development, because I only tested
SCE/SCX scenario. Namely, if PT_FOLLOW_FORK is requested, but the next
stop is not SCX, then follow-fork notification is not sent. After this
nit is fixed, PT_FOLLOW_FORK caller gets stops for the child creation.
Child is put into stop state as needed to not loose it.
I think this will fix only a part of the problem, the one that relates to
PT_CONTINUE.
I still need the change that forces a stop in both child and parent on
fork(). Without my changes the notification is generated in the child but
not in the parent. I need to be able to have both processes stopped in gdb
in order to clean up and detach from the parent, and initialize and attach
to the child. The main reason I need both processes stopped is that gdb has
to be able to read/write into both processes address space and registers.
Ideally I would like to have a single event generated for fork() at a point
where both child and parent are stopped and available for ptrace read/write
requests. Do you think it's possible?
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.
OK, I see. I need to verify that it works with gdb and possibly tweak it to
match the kernel.
You still get a separate stop from the child, but I do not see how is it
harmful.
It's not harmful as long as gdb can tell that those stops are generated by the
fork().
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
Re: [ptrace] please review follow fork/exec changes
On Fri, Jan 27, 2012 at 10:12:13AM -0800, Dmitry Mikulin wrote:
Attached are 4 separate patches for each somewhat independent portion of
the kernel work related to the follow-fork implementation.
Ok, as I said, I think that vfork-fork.patch is just wrong.
Lets postpone discussion of the orphan.patch for later.
The follow-fork.patch and follow-exec.patch make me wonder, and I
already expressed my doubts. IMO, all features, except one bug, are
already presented in the stock src.
More, suggested follow-{fork,exec} patches break the SCE/SCX tracers
notification of fork and exec events, since TDB_FORK and TBD_EXEC flags
are consumed before syscall returns (I also said this previously).
Namely, if the process is being debugged, the successfull [f]execve()
causes unconditional stop even. This makes PT_FOLLOW_EXEC unneccessary.
Existing PT_FOLLOW_FORK implementation indeed has a bug, which was not
revealed by my testing during the development, because I only tested
SCE/SCX scenario. Namely, if PT_FOLLOW_FORK is requested, but the next
stop is not SCX, then follow-fork notification is not sent. After this
nit is fixed, PT_FOLLOW_FORK caller gets stops for the child creation.
Child is put into stop state as needed to not loose it.
I updated the test program I use to test this functionality, see
http://people.freebsd.org/~kib/misc/scescx.c
The default or -s flag causes it to use SCE/SCX tracing, while -c flag
switches it to use PT_CONTINUE tracing, which should be the kind of loop
performed by normal debuggers. You can see the exec/fork events and
child auto-attach illustrated with this test.
Can you, please, provide the test-case which illustrates the omissions
in the existing API (with the patch below applied), if any ?
diff --git a/sys/kern/subr_syscall.c b/sys/kern/subr_syscall.c
index bba4479..75328f6 100644
--- a/sys/kern/subr_syscall.c
+++ b/sys/kern/subr_syscall.c
@@ -212,7 +212,8 @@ syscallret(struct thread *td, int error, struct
syscall_args *sa __unused)
* executes. If debugger requested tracing of syscall
* returns, do it now too.
*/
- if (traced ((td-td_dbgflags TDB_EXEC) != 0 ||
+ if (traced
+ ((td-td_dbgflags (TDB_FORK | TDB_EXEC)) != 0 ||
(p-p_stops S_PT_SCX) != 0))
ptracestop(td, SIGTRAP);
td-td_dbgflags = ~(TDB_SCX | TDB_EXEC | TDB_FORK);
pgp116s22XAVu.pgp
Description: PGP signature
Re: [ptrace] please review follow fork/exec changes
Attached are 4 separate patches for each somewhat independent portion of the
kernel work related to the follow-fork implementation.
On 01/26/2012 04:23 AM, Kostik Belousov wrote:
On Wed, Jan 25, 2012 at 03:48:04PM -0800, Dmitry Mikulin wrote:
Thanks for taking a look at it. I'll try to explain the changes the best I
can: the work was done nearly 6 months ago...
I would certainly appreciate some more words describing the changes.
What is the goal of introducing the PT_FOLLOW_EXEC ? To not force
the debugger to filter all syscall exits to see exec events ?
PT_FOLLOW_EXEC was added to trace the exec() family of calls. When it's
enabled, the kernel will generate a trap to give debugger a chance to clean
up old process info and initialize its state with the new binary.
It was more a question, why PT_FLAG_EXEC is not enough.
Why did you moved the stopevent/ptracestop for exec events from
syscallret() to kern_execve() ? If moving, why the handling of TDB_EXEC
is not removed from syscallret() ? I do not think that TDB_EXEC can be
seen there after the patch. The same question about TDB_FORK.
The reason I moved the event notifications from syscallret() is because the
debugger is interested successful completion of either fork or exec, not
just the fact that they have been called. If, say, a call to fork() failed,
as far as debugger is concerned, fork() might as well had never been
called. Having a ptracestop in syscallret triggered a trap on every return
from fork without telling the debugger whether a new process had been
created or not. Same goes for exec().
PT_FLAG_EXEC is only set if an exec-kind of syscall succeeded. The same
is true for PT_FLAG_FORKED, the flag is set only if a new child was
successfully created.
Looking at the code now I think I should have removed TDB_EXEC/TDB_FORK
processing from syscallret(). I'll do that and verify that it works as
expected.
If possible, I would greatly prefer to have fork changes separated.
You mean separate fork changes into a separate patch from exec changes?
Yes. Even more, it seems that fork changes should be separated into
bug fixes and new functionality.
I doubt that disallowing RFMEM while tracing is the right change. It may
be to fix some (undescribed) bug, but RFMEM is documented behaviour used
not only for vfork(2), and the change just breaks rfork(2) for debugged
processes.
Even vfork() should not be broken, since I believe there are programs
that rely on the vfork() model, in particular, C shell. It will be
broken if vfork() is substituted by fork(). The fact that it breaks only
under debugger will make it esp. confusing.
I need to dig up the details since I can't recall the exact reason for
forcing fork() in cases of user calls to vfork() under gdb. I believe it
had to do with when child process memory was available for writing in case
of vfork() and it wasn't early enough to complete the switch over from
parent to child in gdb. After consulting with our internal kernel experts
we decided that doing fork() instead of vfork() under gdb is a low impact
change.
Why do we need to have TDB_FORK set for td2 ?
The debugger needs to intercept fork() in both parent and child so it can
detach from the old process and attach to the new one. Maybe it'll make
more sense in the context of gdb changes. Should I send them too? Don't
think Marcel included that patch...
No, I think the kernel changes are enough for now.
Does the orphan list change intended to not lost the child after fork ?
But the child shall be traced, so debugger would get the SIGTRAP on
the attach on fork returning to usermode. I remember that I explicitely
tested this when adding followfork changes.
Yes, the debugger gets SIGTRAPs. The problem arises when the real parent of
the forked process has the code to collect termination status. Since
attaching to a process changes the parent/child relationships, we need to
keep track of the children lost due to re-parenting so we can properly
attribute their exit status to the real parent.
I do not quite understand it. From the description it sounds as the problem
that is not related to follow fork changes at all, and shall be presented
regardless of the follow fork. If yes, I think this shall be separated
into a standalone patch.
Index: sys/kern/kern_exec.c
===
--- sys/kern/kern_exec.c (revision 230617)
+++ sys/kern/kern_exec.c (working copy)
@@ -55,6 +55,7 @@
#include sys/priv.h
#include sys/proc.h
#include sys/pioctl.h
+#include sys/ptrace.h
#include sys/namei.h
#include sys/resourcevar.h
#include sys/sdt.h
@@ -888,16 +889,22 @@
free(imgp-freepath, M_TEMP);
if (error == 0) {
+ if ((p-p_flag (P_TRACED | P_FOLLOWEXEC)) ==
+ (P_TRACED | P_FOLLOWEXEC)) {
+ PROC_LOCK(p);
+ td-td_dbgflags |= TDB_EXEC;
+ PROC_UNLOCK(p);
+ }
+ /*
+ * Stop the process here if its stop event mask has
+ * the S_EXEC bit set.
+ */
+ STOPEVENT(p,
Re: [ptrace] please review follow fork/exec changes
On Wed, Jan 25, 2012 at 03:48:04PM -0800, Dmitry Mikulin wrote: Thanks for taking a look at it. I'll try to explain the changes the best I can: the work was done nearly 6 months ago... I would certainly appreciate some more words describing the changes. What is the goal of introducing the PT_FOLLOW_EXEC ? To not force the debugger to filter all syscall exits to see exec events ? PT_FOLLOW_EXEC was added to trace the exec() family of calls. When it's enabled, the kernel will generate a trap to give debugger a chance to clean up old process info and initialize its state with the new binary. It was more a question, why PT_FLAG_EXEC is not enough. Why did you moved the stopevent/ptracestop for exec events from syscallret() to kern_execve() ? If moving, why the handling of TDB_EXEC is not removed from syscallret() ? I do not think that TDB_EXEC can be seen there after the patch. The same question about TDB_FORK. The reason I moved the event notifications from syscallret() is because the debugger is interested successful completion of either fork or exec, not just the fact that they have been called. If, say, a call to fork() failed, as far as debugger is concerned, fork() might as well had never been called. Having a ptracestop in syscallret triggered a trap on every return from fork without telling the debugger whether a new process had been created or not. Same goes for exec(). PT_FLAG_EXEC is only set if an exec-kind of syscall succeeded. The same is true for PT_FLAG_FORKED, the flag is set only if a new child was successfully created. Looking at the code now I think I should have removed TDB_EXEC/TDB_FORK processing from syscallret(). I'll do that and verify that it works as expected. If possible, I would greatly prefer to have fork changes separated. You mean separate fork changes into a separate patch from exec changes? Yes. Even more, it seems that fork changes should be separated into bug fixes and new functionality. I doubt that disallowing RFMEM while tracing is the right change. It may be to fix some (undescribed) bug, but RFMEM is documented behaviour used not only for vfork(2), and the change just breaks rfork(2) for debugged processes. Even vfork() should not be broken, since I believe there are programs that rely on the vfork() model, in particular, C shell. It will be broken if vfork() is substituted by fork(). The fact that it breaks only under debugger will make it esp. confusing. I need to dig up the details since I can't recall the exact reason for forcing fork() in cases of user calls to vfork() under gdb. I believe it had to do with when child process memory was available for writing in case of vfork() and it wasn't early enough to complete the switch over from parent to child in gdb. After consulting with our internal kernel experts we decided that doing fork() instead of vfork() under gdb is a low impact change. Why do we need to have TDB_FORK set for td2 ? The debugger needs to intercept fork() in both parent and child so it can detach from the old process and attach to the new one. Maybe it'll make more sense in the context of gdb changes. Should I send them too? Don't think Marcel included that patch... No, I think the kernel changes are enough for now. Does the orphan list change intended to not lost the child after fork ? But the child shall be traced, so debugger would get the SIGTRAP on the attach on fork returning to usermode. I remember that I explicitely tested this when adding followfork changes. Yes, the debugger gets SIGTRAPs. The problem arises when the real parent of the forked process has the code to collect termination status. Since attaching to a process changes the parent/child relationships, we need to keep track of the children lost due to re-parenting so we can properly attribute their exit status to the real parent. I do not quite understand it. From the description it sounds as the problem that is not related to follow fork changes at all, and shall be presented regardless of the follow fork. If yes, I think this shall be separated into a standalone patch. pgphUacrPo7lj.pgp Description: PGP signature
Re: [ptrace] please review follow fork/exec changes
On 01/26/2012 04:23 AM, Kostik Belousov wrote: On Wed, Jan 25, 2012 at 03:48:04PM -0800, Dmitry Mikulin wrote: Thanks for taking a look at it. I'll try to explain the changes the best I can: the work was done nearly 6 months ago... I would certainly appreciate some more words describing the changes. What is the goal of introducing the PT_FOLLOW_EXEC ? To not force the debugger to filter all syscall exits to see exec events ? PT_FOLLOW_EXEC was added to trace the exec() family of calls. When it's enabled, the kernel will generate a trap to give debugger a chance to clean up old process info and initialize its state with the new binary. It was more a question, why PT_FLAG_EXEC is not enough. I don't see it in the code (sp?) If you mean PL_FLAG_FORKED, than it's used to return lwpinfo, and PT_FOLLOW_EXEC is a ptrace request. Why did you moved the stopevent/ptracestop for exec events from syscallret() to kern_execve() ? If moving, why the handling of TDB_EXEC is not removed from syscallret() ? I do not think that TDB_EXEC can be seen there after the patch. The same question about TDB_FORK. The reason I moved the event notifications from syscallret() is because the debugger is interested successful completion of either fork or exec, not just the fact that they have been called. If, say, a call to fork() failed, as far as debugger is concerned, fork() might as well had never been called. Having a ptracestop in syscallret triggered a trap on every return from fork without telling the debugger whether a new process had been created or not. Same goes for exec(). PT_FLAG_EXEC is only set if an exec-kind of syscall succeeded. The same is true for PT_FLAG_FORKED, the flag is set only if a new child was successfully created. I found the test cases you sent me for your fork/exec tracing changes and now I see why I needed to make some of the changes. Before my patches tracing of fork/exec is done via PT_TO_SCE/PT_TO_SCX ptrace calls. Their semantics did not fit well into what gdb needs to do. They operate as a variant of PT_CONTINUE. What gdb needs is a process-wide setting that instructs the kernel to generate SIGTRAPs in fork/exec when the process is being controlled via the regular ptrace(PT_CONTINUE)/wait() sequence. Looking at the code now I think I should have removed TDB_EXEC/TDB_FORK processing from syscallret(). I'll do that and verify that it works as expected. If possible, I would greatly prefer to have fork changes separated. You mean separate fork changes into a separate patch from exec changes? Yes. Even more, it seems that fork changes should be separated into bug fixes and new functionality. OK, that's doable. I doubt that disallowing RFMEM while tracing is the right change. It may be to fix some (undescribed) bug, but RFMEM is documented behaviour used not only for vfork(2), and the change just breaks rfork(2) for debugged processes. Even vfork() should not be broken, since I believe there are programs that rely on the vfork() model, in particular, C shell. It will be broken if vfork() is substituted by fork(). The fact that it breaks only under debugger will make it esp. confusing. I need to dig up the details since I can't recall the exact reason for forcing fork() in cases of user calls to vfork() under gdb. I believe it had to do with when child process memory was available for writing in case of vfork() and it wasn't early enough to complete the switch over from parent to child in gdb. After consulting with our internal kernel experts we decided that doing fork() instead of vfork() under gdb is a low impact change. Why do we need to have TDB_FORK set for td2 ? The debugger needs to intercept fork() in both parent and child so it can detach from the old process and attach to the new one. Maybe it'll make more sense in the context of gdb changes. Should I send them too? Don't think Marcel included that patch... No, I think the kernel changes are enough for now. Does the orphan list change intended to not lost the child after fork ? But the child shall be traced, so debugger would get the SIGTRAP on the attach on fork returning to usermode. I remember that I explicitely tested this when adding followfork changes. Yes, the debugger gets SIGTRAPs. The problem arises when the real parent of the forked process has the code to collect termination status. Since attaching to a process changes the parent/child relationships, we need to keep track of the children lost due to re-parenting so we can properly attribute their exit status to the real parent. I do not quite understand it. From the description it sounds as the problem that is not related to follow fork changes at all, and shall be presented regardless of the follow fork. If yes, I think this shall be separated into a standalone patch. You're right, it's not related. It just happened to prevent me from finishing the work. I'll break up the changes into multiple patches and re-send.
Re: [ptrace] please review follow fork/exec changes
Thanks for taking a look at it. I'll try to explain the changes the best I can: the work was done nearly 6 months ago... I would certainly appreciate some more words describing the changes. What is the goal of introducing the PT_FOLLOW_EXEC ? To not force the debugger to filter all syscall exits to see exec events ? PT_FOLLOW_EXEC was added to trace the exec() family of calls. When it's enabled, the kernel will generate a trap to give debugger a chance to clean up old process info and initialize its state with the new binary. Why did you moved the stopevent/ptracestop for exec events from syscallret() to kern_execve() ? If moving, why the handling of TDB_EXEC is not removed from syscallret() ? I do not think that TDB_EXEC can be seen there after the patch. The same question about TDB_FORK. The reason I moved the event notifications from syscallret() is because the debugger is interested successful completion of either fork or exec, not just the fact that they have been called. If, say, a call to fork() failed, as far as debugger is concerned, fork() might as well had never been called. Having a ptracestop in syscallret triggered a trap on every return from fork without telling the debugger whether a new process had been created or not. Same goes for exec(). Looking at the code now I think I should have removed TDB_EXEC/TDB_FORK processing from syscallret(). I'll do that and verify that it works as expected. If possible, I would greatly prefer to have fork changes separated. You mean separate fork changes into a separate patch from exec changes? I doubt that disallowing RFMEM while tracing is the right change. It may be to fix some (undescribed) bug, but RFMEM is documented behaviour used not only for vfork(2), and the change just breaks rfork(2) for debugged processes. Even vfork() should not be broken, since I believe there are programs that rely on the vfork() model, in particular, C shell. It will be broken if vfork() is substituted by fork(). The fact that it breaks only under debugger will make it esp. confusing. I need to dig up the details since I can't recall the exact reason for forcing fork() in cases of user calls to vfork() under gdb. I believe it had to do with when child process memory was available for writing in case of vfork() and it wasn't early enough to complete the switch over from parent to child in gdb. After consulting with our internal kernel experts we decided that doing fork() instead of vfork() under gdb is a low impact change. Why do we need to have TDB_FORK set for td2 ? The debugger needs to intercept fork() in both parent and child so it can detach from the old process and attach to the new one. Maybe it'll make more sense in the context of gdb changes. Should I send them too? Don't think Marcel included that patch... Does the orphan list change intended to not lost the child after fork ? But the child shall be traced, so debugger would get the SIGTRAP on the attach on fork returning to usermode. I remember that I explicitely tested this when adding followfork changes. Yes, the debugger gets SIGTRAPs. The problem arises when the real parent of the forked process has the code to collect termination status. Since attaching to a process changes the parent/child relationships, we need to keep track of the children lost due to re-parenting so we can properly attribute their exit status to the real parent. ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org
[ptrace] please review follow fork/exec changes
All,
Please review the attached changes (done by Dmitry -- CC'd) to add support for
PT_FOLLOW_EXEC and cleanup PT_FOLLOW_FORK.
I'll commit this when there are no comments/objections.
Thanks,
--
Marcel Moolenaar
marc...@juniper.net
Index: sys/kern/kern_exit.c
===
--- sys/kern/kern_exit.c (revision 225189)
+++ sys/kern/kern_exit.c (working copy)
@@ -680,7 +680,7 @@ 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)
@@ -859,7 +861,7 @@ loop:
nfound++;
PROC_SLOCK(p);
if (p-p_state == PRS_ZOMBIE) {
- proc_reap(td, p, status, options, rusage);
+ proc_reap(td, p, status, options, rusage, 1);
return (0);
}
if ((p-p_flag P_STOPPED_SIG)
@@ -893,16 +895,47 @@ loop:
if (status)
*status = SIGCONT;
+ }
+ PROC_UNLOCK(p);
+ }
+ LIST_FOREACH(p, q-p_orphans, p_orphan) {
+ PROC_LOCK(p);
+ if (pid != WAIT_ANY
+ p-p_pid != pid p-p_pgid != -pid) {
+ PROC_UNLOCK(p);
+ continue;
+ }
+ if (p_canwait(td, p)) {
+ PROC_UNLOCK(p);
+ continue;
+ }
+
+ /*
+ * 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_SLOCK(p);
+ if (p-p_state == PRS_ZOMBIE) {
+ proc_reap(td, p, status, options, rusage, 0);
return (0);
}
+ PROC_SUNLOCK(p);
PROC_UNLOCK(p);
}
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);
@@ -932,6 +965,7 @@ proc_reparent(struct proc *child, struct proc *par
#ifdef RACCT
int locked;
#endif
+ struct proc *p;
sx_assert(proctree_lock, SX_XLOCKED);
PROC_LOCK_ASSERT(child, MA_OWNED);
@@ -952,5 +986,17 @@ 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);
+ PROC_UNLOCK(child);
+ PROC_LOCK(child);
+ }
child-p_pptr = parent;
}
Index: sys/kern/kern_exec.c
===
--- sys/kern/kern_exec.c (revision 225189)
+++ sys/kern/kern_exec.c (working copy)
@@ -55,6 +55,7 @@ __FBSDID($FreeBSD$);
#include sys/priv.h
#include sys/proc.h
#include sys/pioctl.h
+#include sys/ptrace.h
#include sys/namei.h
#include sys/resourcevar.h
#include sys/sdt.h
@@ -895,16 +896,22 @@ exec_fail_dealloc:
free(imgp-freepath, M_TEMP);
if (error == 0) {
+ if ((p-p_flag (P_TRACED | P_FOLLOWEXEC)) ==
+ (P_TRACED | P_FOLLOWEXEC)) {
+ PROC_LOCK(p);
+ td-td_dbgflags |= TDB_EXEC;
+ PROC_UNLOCK(p);
+ }
+ /*
+ * Stop the process here if its stop event mask has
+ * the S_EXEC bit set.
+ */
+ STOPEVENT(p, S_EXEC, 0);
+ PTRACESTOP_SC(p, td, S_PT_EXEC);
PROC_LOCK(p);
- td-td_dbgflags |= TDB_EXEC;
+ td-td_dbgflags = ~TDB_EXEC;
PROC_UNLOCK(p);
-
- /*
- * Stop the process here if its stop event mask has
- * the S_EXEC bit set.
- */
- STOPEVENT(p, S_EXEC, 0);
- goto done2;
+ goto done2;
}
exec_fail:
Index: sys/kern/kern_fork.c
===
--- sys/kern/kern_fork.c (revision 225189)
+++ sys/kern/kern_fork.c (working copy)
@@ -59,6 +59,7 @@ __FBSDID($FreeBSD$);
#include sys/proc.h
#include sys/procdesc.h
#include sys/pioctl.h
+#include sys/ptrace.h
#include sys/racct.h
#include
Re: [ptrace] please review follow fork/exec changes
On Tue, Jan 24, 2012 at 01:36:55PM -0800, Marcel Moolenaar wrote: All, Please review the attached changes (done by Dmitry -- CC'd) to add support for PT_FOLLOW_EXEC and cleanup PT_FOLLOW_FORK. I'll commit this when there are no comments/objections. Thanks, I would certainly appreciate some more words describing the changes. What is the goal of introducing the PT_FOLLOW_EXEC ? To not force the debugger to filter all syscall exits to see exec events ? Why did you moved the stopevent/ptracestop for exec events from syscallret() to kern_execve() ? If moving, why the handling of TDB_EXEC is not removed from syscallret() ? I do not think that TDB_EXEC can be seen there after the patch. The same question about TDB_FORK. If possible, I would greatly prefer to have fork changes separated. I doubt that disallowing RFMEM while tracing is the right change. It may be to fix some (undescribed) bug, but RFMEM is documented behaviour used not only for vfork(2), and the change just breaks rfork(2) for debugged processes. Even vfork() should not be broken, since I believe there are programs that rely on the vfork() model, in particular, C shell. It will be broken if vfork() is substituted by fork(). The fact that it breaks only under debugger will make it esp. confusing. Why do we need to have TDB_FORK set for td2 ? Does the orphan list change intended to not lost the child after fork ? But the child shall be traced, so debugger would get the SIGTRAP on the attach on fork returning to usermode. I remember that I explicitely tested this when adding followfork changes. pgp3FXLhsTAQ8.pgp Description: PGP signature
