Page MenuHomeFreeBSD

Move parent fork dbgwait to syscallret() to avoid hang with RFSTOPPED with PT_FOLLOW_FORK.
Needs ReviewPublic

Authored by bdrewery on Oct 6 2015, 6:29 PM.

Details

Reviewers
kib
jhb
Summary

Using fork1() with RFSTOPPED while being debugged with ptrace(PT_FOLLOW_FORK)
will cause the child to be forked off, never scheduled to attach debugger, and
the parent waiting for the child to attach at the end of do_fork() forever.

This should fix the hang with Linux fork(2) wrappers, which use RFSTOPPED, when
using ptrace(PT_FOLLOW_FORK). It also is much safer for other code that
may be using RFSTOPPED, such as vendor ABI modules, by removing the
possibility of the do_fork() code waiting forever for an unscheduled child.

I like the idea of the fork return hooks and still want to see that
implemented, but prefer this approach for this problem as otherwise the bug
is still present for other code to run into.

Test Plan

Tested with truss -f on native binaries, and Linux. Both work.

% truss -f /compat/linux/bin/bash -c "id & id &"
P78
% truss -f bash -c "id & id &"
...
% truss -f sh -c "id & id &"

The Kyua tests as of r288949 work as well.

/usr/tests/sys/kern # kyua test ptrace_test
ptrace_test:ptracefollow_fork_both_attached -> passed [0.010s]
ptrace_test:ptrace
follow_fork_both_attached_unrelated_debugger -> passed [0.010s]
ptrace_test:ptracefollow_fork_child_detached -> passed [0.009s]
ptrace_test:ptrace
follow_fork_child_detached_unrelated_debugger -> passed [0.010s]
ptrace_test:ptracefollow_fork_parent_detached -> passed [0.010s]
ptrace_test:ptrace
follow_fork_parent_detached_unrelated_debugger -> passed [0.010s]
ptrace_test:ptracegetppid -> passed [0.009s]
ptrace_test:ptrace
new_child_pl_syscall_code_fork -> passed [0.009s]
ptrace_test:ptracenew_child_pl_syscall_code_thread -> passed [0.009s]
ptrace_test:ptrace
new_child_pl_syscall_code_vfork -> passed [0.009s]
ptrace_test:ptraceparent_sees_exit_after_child_debugger -> passed [0.015s]
ptrace_test:ptrace
parent_sees_exit_after_unrelated_debugger -> passed [0.015s]
ptrace_test:ptraceparent_wait_after_attach -> passed [0.009s]
ptrace_test:ptrace
parent_wait_after_trace_me -> passed [0.009s]

Results file id is usr_tests_sys_kern.20151006-222504-676851
Results saved to /root/.kyua/store/results.usr_tests_sys_kern.20151006-222504-676851.db

14/14 passed (0 failed)

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 701
Build 701: arc lint + arc unit

Event Timeline

bdrewery updated this revision to Diff 9192.Oct 6 2015, 6:29 PM
bdrewery retitled this revision from to Move parent fork dbgwait to syscallret()..
bdrewery updated this object.
bdrewery edited the test plan for this revision. (Show Details)
bdrewery added reviewers: kib, jhb.
bdrewery retitled this revision from Move parent fork dbgwait to syscallret(). to Move parent fork dbgwait to syscallret() to avoid hang with RFSTOPPED with PT_FOLLOW_FORK..Oct 6 2015, 7:43 PM
bdrewery updated this object.
bdrewery edited the test plan for this revision. (Show Details)

I am guessing that the problem could be recreated from user land with rfork(2), without Linux code involvement. I am testing that idea now and will add a test case if so.

I am guessing that the problem could be recreated from user land with rfork(2), without Linux code involvement. I am testing that idea now and will add a test case if so.

That was a silly thought :). Not possible of course to use RFSTOPPED from user land.

jhb edited edge metadata.Oct 6 2015, 9:03 PM

Userland cannot send RFSTOPPED via rfork(). It is an internal-only flag (part of the RFKERNEL_ONLY mask) and attempts to pass it via rfork() result in EINVAL.

In D3829#79027, @jhb wrote:

Userland cannot send RFSTOPPED via rfork(). It is an internal-only flag (part of the RFKERNEL_ONLY mask) and attempts to pass it via rfork() result in EINVAL.

Yeah I realized that and then found the code proving that. It would be stupid to expose it to user land.

jhb added a comment.Oct 6 2015, 9:20 PM

I think this looks fine as far as implementing this design. The only open question is if we want to go this route to fix the issue or not. Looking at the code in linux_fork.c, it would actually be a bit of a PITA to pass in the required state to sv_fork_return(), especially in the linux_clone_proc() case. You would need to malloc a copy of the args and hand that off to the new thread so that sv_fork_return() could see it. Also, you would need to keep the parent "around" for linux_proc_init() (which needs threads from both the parent and child processes) or handle the race of it exiting before the child runs, etc. Leaving RFSTOPPED in place and moving the wait to syscallret() might actually be the simplest solution.

bdrewery edited the test plan for this revision. (Show Details)Oct 6 2015, 10:28 PM
bdrewery edited edge metadata.