fork: avoid endless wait with PTRACE_FORK and RFSTOPPED

[yanko.yankulov_gmail.com]

Summary

Issue was that RFSTOPPED thread can't clean TDB_STOPATFORK, so parent is stuck forever. Tirggered when trying to ptrace linux binary.

Instead of waiting for the new thread to clear TDB_STOPATFORK, tag it as traced in do_fork and let it only notify the debugger when run.

[yanko.yankulov_gmail.com]

Removed the now outdated comment in do_fork

[kib]

I was not able to imagine a case which is broken by this simplification. Also, the ptrace_test works with the patch.

I added John to review.

sys/kern/kern_fork.c

763 ↗

(On Diff #43921)

Style:

if (has_ptrace_fork) {
...
}

I am not sure about usefulness of __prefict_false() there. I agree that has_ptrace_fork is almost always false, but it is not clear if the hint makes any change.

770 ↗

(On Diff #43921)

Again style. Also compact the conditions into single if:

if ((p2->p_pptr->p_ptevents & PTRACE_FORK)  != 0 &&
    (td2->td_dbgflags & TDB_STOPATFORK) != 0) {

1085 ↗

(On Diff #43921)

if ((p->p_flag & P_TRACED) != 0) {

[yanko.yankulov_gmail.com]

address style issues

[kib]

sys/kern/kern_fork.c

396 ↗

(On Diff #43996)

bool ?

763 ↗

(On Diff #43996)

Remove spaces after '(' and before ')',

[yanko.yankulov_gmail.com]

bool & yet more style issues

[jhb]

sys/kern/kern_fork.c

763 ↗

(On Diff #44002)

I do wonder if we couldn't collapse the logic a bit so that it was all in one place instead of splitting it up. I think we can't move this logic up, but not sure if we couldn't move the earlier logic down into here?

766 ↗

(On Diff #44002)

Can you do 's/p2->p_pptr/p1/' here?

768 ↗

(On Diff #44002)

I would move the KTR_PTRACE for "attaching to forked child" here since this is now when the attach happens. (Would have to change the function name to no longer be fork_return)

[yanko.yankulov_gmail.com]

sys/kern/kern_fork.c

766 ↗

(On Diff #44002)

this is after,
_PRELE(p1);
PROC_UNLOCK(p1);

so I am not sure if p1 is still valid / parent. Don't know the locking semantics

If it is not, the proc_reparent call is probably wrong
proc_reparent(p2, p1->p_pptr);

In fact p1 is used just after the unlock, so it is probably valid, I just don't know why. And I am not sure that the use guarantees it is still p2's parent.

Any thoughts?

[kib]

sys/kern/kern_fork.c

766 ↗

(On Diff #44002)

p1 is just a different name for curproc.

And td2 still cannot execute because it is unknown to the scheduler. sched_add() is done later, so it cannot exit and be reparented. Since p1 is curproc and we are executing in its context, p1 cannot exit and p2 cannot be re-childed (silly pun) from p1.

[yanko.yankulov_gmail.com]

sys/kern/kern_fork.c

766 ↗

(On Diff #44002)

Yep, fork1 is called with the current thread or thread0 only, so yes p1 is safe.

[yanko.yankulov_gmail.com]

Address 2 of the jhb comments.

Have no idea about moving other code around.

[kib]

In D15857#336018, @yanko.yankulov_gmail.com wrote:

Address 2 of the jhb comments.

Have no idea about moving other code around.

I think John' idea was to move the block which sets the has_ptrace_fork variable to true, down to the code which acts on its true value. De-fact, eliminating the var, and removing one if().

[yanko.yankulov_gmail.com]

I think John' idea was to move the block which sets the has_ptrace_fork variable to true, down to the code which acts on its true value. De-fact, eliminating the var, and removing one if().

OK, but as far as I can tell we will need p1's PROC_LOCK or the proctree_lock in order to check the PTRACE_FORK flag. Am i missing something obvious again :) ?

[kib]

In D15857#336986, @yanko.yankulov_gmail.com wrote:

I think John' idea was to move the block which sets the has_ptrace_fork variable to true, down to the code which acts on its true value. De-fact, eliminating the var, and removing one if().

OK, but as far as I can tell we will need p1's PROC_LOCK or the proctree_lock in order to check the PTRACE_FORK flag. Am i missing something obvious again :) ?

We can read it lockless, and re-acquire the proc lock if set to re-read under the lock. This is not much different from reading under the lock, remembering the result, then drop the lock and use the result.

Also, I am unsure why do you state that p_ptevents requires proctree_lock.

[yanko.yankulov_gmail.com]

In D15857#337018, @kib wrote:

In D15857#336986, @yanko.yankulov_gmail.com wrote:

I think John' idea was to move the block which sets the has_ptrace_fork variable to true, down to the code which acts on its true value. De-fact, eliminating the var, and removing one if().

OK, but as far as I can tell we will need p1's PROC_LOCK or the proctree_lock in order to check the PTRACE_FORK flag. Am i missing something obvious again :) ?

We can read it lockless, and re-acquire the proc lock if set to re-read under the lock. This is not much different from reading under the lock, remembering the result, then drop the lock and use the result.

Right. Obvious. Will do the changes shortly.

Also, I am unsure why do you state that p_ptevents requires proctree_lock.

In kern_ptrace PT_FOLLOW_FORK request acquires proctree_lock. It seems that this is specific to PTRACE_FORK & PTRACE_LWP.

[yanko.yankulov_gmail.com]

Merge the two parts of set child as traced

[kib]

Other than two notes I put inline, the patch looks fine to me.

sys/kern/kern_fork.c

748 ↗

(On Diff #44164)

Use the same style != 0 as at line 751.

751 ↗

(On Diff #44164)

After looking at your variant of code, I do not see why the recheck is useful. You do not lock p1 there, so the race with unlocked read of p1->p_ptevents is the same as without the re-read.

[yanko.yankulov_gmail.com]

sys/kern/kern_fork.c

751 ↗

(On Diff #44164)

My reasoning:

From my last comment: p1->p_ptevents is protected for PTRACE_FORK & PTRACE_LWP by both p1's lock and proctree_lock in kern_ptrace. And all other (unlocked) modifications to p_ptevents preserve those flags.

We need to check that p1 is still traced else p1->p_pptr might not be a debugger. Both p1->p_flag P_TRACED and p1->p_ptevents PTRACE_FORK are proctected by proctree_lock, so PTRACE_FORK implies P_TRACED and that p->p_pptr is a tracer.

Thus the recheck.

[kib]

sys/kern/kern_fork.c

751 ↗

(On Diff #44164)

You changed from the declared locking model to the de-facto occuring locking model then, this cannot happen in ad-hoc mode. At least, you need to add a note to the proc.h both to the locking annotation for p_ptevents and to the PTRACE_FORK symbol itself, and perhaps add a comment there too, explaining why p1 lock is not taken, but the recheck under proctree is enough.

[yanko.yankulov_gmail.com]

sys/kern/kern_fork.c

751 ↗

(On Diff #44164)

I am a little confused. I can't figure out the difference in the locking before and after the patch, so maybe there is another thing I am missing.

The old code in fork_return:
sx_xlock(&proctree_lock);
PROC_LOCK(p)
if (p->p_pptr->p_ptevents & PTRACE_FORK) {
....

p->p_pptr lock is not taken, same as the:

new code in do_fork

+ sx_xlock(&proctree_lock);
+ PROC_LOCK(p2);
+ if ((p1->p_ptevents & PTRACE_FORK) != 0) {
+ /*

Will look into the annotation and adding a comment, but if the old code depended on the same guarantees probably a separate commit ?

[kib]

sys/kern/kern_fork.c

751 ↗

(On Diff #44164)

I am not sure where to put the blame, on the old code, on the existing annotation, or both. But it is clear that according to the rules from proc.h, the read of PTRACE_FORK in the old code should be considered unlocked:

	u_int		p_ptevents;	/* (c) ptrace() event mask. */
 *      c - locked by proc mtx

I think it is fine to update the locking annotation first. Your commit should add an explicit comment into the new code still, IMO.

[yanko.yankulov_gmail.com]

sys/kern/kern_fork.c

751 ↗

(On Diff #44164)

Thanks. My question was if I have involuntary changed something in the locking that I have totally missed.

I went through the code again and the whole p_ptevents is always protected by proctree_lock with two exceptions - cleared in exit1 and in ptracestop if process is killed. Both will not affect the code in do_fork, but what would be proper annotation - "(c + e) unless process is exiting" ?

[kib]

sys/kern/kern_fork.c

751 ↗

(On Diff #44164)

I think just c+e is enough there.

[yanko.yankulov_gmail.com]

Style & comments

[kib]

It looks fine to me, I will wait some time for John opinion.

sys/kern/kern_fork.c

752 ↗

(On Diff #44240)

End the sentence with the dot.

758 ↗

(On Diff #44240)

Please expand it, '... are protected by both process and proctree locks for modifications, so owning proctree_lock allows the race-free read'. And the dot at the end of sentence.

761 ↗

(On Diff #44240)

Remove the blank line.

1093 ↗

(On Diff #44240)

Dot.

[jhb]

sys/kern/kern_fork.c

750 ↗

(On Diff #44240)

I would put the speculative check comment above the speculative check instead of after it.

752 ↗

(On Diff #44240)

Also, s/beeing/being/

763 ↗

(On Diff #44240)

Probably don't need this blank line.

773 ↗

(On Diff #44240)

Or this blank line either.

781 ↗

(On Diff #44240)

Perhaps swap order of unlocks so it is symmetric? We tend to do symmetric unlocks.

[yanko.yankulov_gmail.com]

Address last comments.

[jhb]

Thanks