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.