Page MenuHomeFreeBSD
Differential D20361

procdesc: fix reparenting when the debugger is attached
ClosedPublic
Actions

Authored by oshogbo on May 22 2019, 7:42 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 23, 7:06 PM
Unknown Object (File)
Fri, Nov 15, 10:44 PM
Unknown Object (File)
Fri, Nov 8, 5:31 AM
Unknown Object (File)
Oct 18 2024, 12:36 PM
Unknown Object (File)
Oct 4 2024, 11:30 PM
Unknown Object (File)
Oct 2 2024, 8:58 PM
Unknown Object (File)
Oct 2 2024, 6:00 PM
Unknown Object (File)
Oct 2 2024, 3:04 PM
View All 98 Files
Subscribers
imp
pho

Details

Reviewers
emaste
markj
kib
jhb
rwatson
Commits
rS350612: procdesc: fix reparenting when the debugger is attached
Summary

The process is reparented to the debugger while it is attached.

  B          B
 /   ---->   |
A          A D

Every time when the process is reparented, it is added to the orphan list of
the previous parent:

A->orphan = B
D->orphan = NULL

When the A process will close the process descriptor to the B process,
the B process will be reparented to the init process.

   B            B - init
   |   ---->
A  D           A   D
`

A->orphan = B
D->orphan = B

In this scenario, the B process is in the orphan list of A and D.

When the last process descriptor is closed instead of reparenting
it to the reaper let it stay with the debugger process and set
our previews parent to the reaper.

Add test case for this situation.
Notice that without this patch the kernel will crash with this test case:
panic: orphan 0xfffff8000e990530 of 0xfffff8000e990000 has unexpected oppid 1

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 24479
Build 23282: arc lint + arc unit

Event Timeline

oshogbo created this revision.May 22 2019, 7:42 PM
Herald added a subscriber: emaste. · View Herald TranscriptMay 22 2019, 7:42 PM
Harbormaster completed remote builds in B24405: Diff 57716.May 22 2019, 7:42 PM
oshogbo edited the summary of this revision. (Show Details)May 22 2019, 7:42 PM
oshogbo edited the summary of this revision. (Show Details)
oshogbo edited the summary of this revision. (Show Details)
oshogbo edited the summary of this revision. (Show Details)May 22 2019, 7:45 PM
oshogbo added reviewers: emaste, markj.
oshogbo removed a subscriber: emaste.
oshogbo added a reviewer: kib.May 22 2019, 9:08 PM
markj added inline comments.May 22 2019, 9:21 PM
sys/kern/sys_procdesc.c
419–425

You forgot to remove the old call.

425

I think this is necessary but it's not obvious to me that it's sufficient. Looking at proc_reparent(), it also calls sigqueue_take(), but I don't think we need to do that in the traced case. Is there any other state that might need to be updated?

oshogbo updated this revision to Diff 57731.May 22 2019, 9:32 PM

Address markj suggestion.

Harbormaster completed remote builds in B24413: Diff 57731.May 22 2019, 9:32 PM
oshogbo added inline comments.May 22 2019, 9:32 PM
sys/kern/sys_procdesc.c
419–425

Aghr. Screw up while rebasing. Thanks!

oshogbo added a reviewer: jhb.May 22 2019, 9:33 PM
oshogbo added a reviewer: rwatson.May 22 2019, 9:36 PM
kib added inline comments.May 22 2019, 9:46 PM
sys/kern/sys_procdesc.c
425

I am not sure how would it work. The process must be on the orphans' list for its old parent, otherwise premature exit of the debugger or even detach would see unexpected state.

markj added inline comments.May 22 2019, 9:51 PM
sys/kern/sys_procdesc.c
425

The parent has disassociated itself from the process by closing the process descriptor. Your comment makes me think that p should be added to the orphan list of the reaper. I noted however that the code in exit1() that raises SIGKILL to children of an exiting debugger does not do this.

kib added inline comments.May 22 2019, 10:10 PM
sys/kern/sys_procdesc.c
425

Yes, inserting into the reapper' orphan list is the most obvious option.

In exit1() case, when debugger exits, the traced children are reparented back, there is no need to do anything about new orphanage.

oshogbo updated this revision to Diff 57852.May 24 2019, 6:37 PM

Add a process to the orphan list as suggested by @kib and @markj.

Harbormaster completed remote builds in B24479: Diff 57852.May 24 2019, 6:37 PM
kib added inline comments.May 25 2019, 2:39 PM
sys/kern/kern_exit.c
1361

Can you explain why do you require PROC_LOCK there ?

oshogbo updated this revision to Diff 57897.May 25 2019, 5:47 PM

Fix locking.

Harbormaster completed remote builds in B24507: Diff 57897.May 25 2019, 5:47 PM
oshogbo added inline comments.May 25 2019, 5:47 PM
sys/kern/kern_exit.c
1361

p_flag is protected by PROC_LOCK.
But I guess the proctree_lock should be enough.

kib added a comment.May 25 2019, 6:43 PM

P_TRACED is always set or cleared with proctree_lock exclusively locked, except at the process exit. After that, the process lock in exit1() is dropped and we acquire the proctree_lock. PRS_ZOMBIE is set much later on the process.

So I wonder is it possible that your patched code would see non-traced process which is not yet safe to reparent. Or rather, I do not see anything that prevents this situation.

I suspect that we should clear P_TRACED under proctree_lock in exit1() as well.

markj added a comment.May 27 2019, 2:48 PM
In D20361#440566, @kib wrote:

So I wonder is it possible that your patched code would see non-traced process which is not yet safe to reparent. Or rather, I do not see anything that prevents this situation.

What exactly makes it unsafe? The procdesc close may race with a PT_DETACH, but PT_DETACH should not modify the process tree when p_oppid and p_ptr refer to the same process.

sys/kern/kern_exit.c
171

IMO it should be renamed to proc_clear_orphan() now.

kib added a comment.May 27 2019, 3:08 PM
In D20361#440984, @markj wrote:
In D20361#440566, @kib wrote:

So I wonder is it possible that your patched code would see non-traced process which is not yet safe to reparent. Or rather, I do not see anything that prevents this situation.

What exactly makes it unsafe? The procdesc close may race with a PT_DETACH, but PT_DETACH should not modify the process tree when p_oppid and p_ptr refer to the same process.

ptrace(PT_DETACH) is safe, but exit1() is not IMO. It clears P_TRACED very early, which would make procdesc_close() not calling proc_clear_orphan().

markj added a comment.May 27 2019, 3:21 PM
In D20361#441005, @kib wrote:
In D20361#440984, @markj wrote:
In D20361#440566, @kib wrote:

So I wonder is it possible that your patched code would see non-traced process which is not yet safe to reparent. Or rather, I do not see anything that prevents this situation.

What exactly makes it unsafe? The procdesc close may race with a PT_DETACH, but PT_DETACH should not modify the process tree when p_oppid and p_ptr refer to the same process.

ptrace(PT_DETACH) is safe, but exit1() is not IMO. It clears P_TRACED very early, which would make procdesc_close() not calling proc_clear_orphan().

Sorry, I don't follow. If procdesc_close() does not observe P_TRACED, it will simply reparent to the reaper, and proc_reparent() clears the orphan state.

markj added a comment.May 27 2019, 7:49 PM
In D20361#441018, @markj wrote:
In D20361#441005, @kib wrote:

ptrace(PT_DETACH) is safe, but exit1() is not IMO. It clears P_TRACED very early, which would make procdesc_close() not calling proc_clear_orphan().

Sorry, I don't follow. If procdesc_close() does not observe P_TRACED, it will simply reparent to the reaper, and proc_reparent() clears the orphan state.

kib pointed out that the debugger will not be able to collect the exit status in this case. I now agree that we should postpone clearing P_TRACED until the proctree lock is acquired in exit1(). It would probably be worth testing such a patch separately; I would ask Peter to run stress2, since it contains a number of tests for ptrace.

oshogbo updated this revision to Diff 58049.May 29 2019, 5:20 PM

Address @kib suggestion.

Harbormaster completed remote builds in B24592: Diff 58049.May 29 2019, 5:20 PM
kib accepted this revision.May 29 2019, 5:42 PM

As Mark noted, move of P_TRACED cleanup should be a separate commit which makes sense to ask pho to test. Also I recommend to split introduction of proc_add_orphan() into another preliminary commit, since it will make merges easier.

This revision is now accepted and ready to land.May 29 2019, 5:42 PM
markj accepted this revision.May 30 2019, 3:45 PM
markj added inline comments.
sys/kern/kern_exit.c
171

Do you disagree with my comment above?

pho added a subscriber: pho.Jul 30 2019, 11:59 AM

I got this after a 5 hour test on i386, followed by a shutdown:

panic: vm_fault_hold: fault on nofault entry, addr: 0x83326000
cpuid = 0
time = 1564486041
KDB: stack backtrace:
db_trace_self_wrapper(e6666d,1bdcda4,0,23a17534,bd2eb1,...) at db_trace_self_wrapper+0x2a/frame 0x23a17508
kdb_backtrace(e,0,0,2b06100,2db5000,...) at kdb_backtrace+0x2e/frame 0x23a17568
vpanic(163709e,23a175b0,23a175b0,23a17680,12c2044,...) at vpanic+0x121/frame 0x23a17590
panic(163709e,158dad9,83326000,4895800,7000004,...) at panic+0x14/frame 0x23a175a4
vm_fault_hold(2db5000,83326000,2,0,0) at vm_fault_hold+0x2474/frame 0x23a17680
vm_fault(2db5000,83326000,2,0) at vm_fault+0x4a/frame 0x23a176a8
trap_pfault(8332680c) at trap_pfault+0xf3/frame 0x23a176f4
trap(23a177bc,8,28,28,83df8640,...) at trap+0x3c0/frame 0x23a177b0
calltrap() at 0xffc0316d/frame 0x23a177b0
--- trap 0xc, eip = 0xf7f255, esp = 0x23a177fc, ebp = 0x23a17814 ---
funsetownlst(27e5ea50) at funsetownlst+0xa5/frame 0x23a17814
pgdelete(27e5ea6c,23c00a80,1651842,36,23a178f8,...) at pgdelete+0x60/frame 0x23a17834
leavepgrp(298d46a8,0,1651842,396,44c,...) at leavepgrp+0xd5/frame 0x23a1784c
proc_reap(23c00a80,298d46a8,23a17ba4,36,1edeb50,...) at proc_reap+0x714/frame 0x23a178f8
proc_to_reap(23c00a80,298d46a8,7,0,0,...) at proc_to_reap+0x718/frame 0x23a17998
kern_wait6(23c00a80,7,0,0,23a17ba4,...) at kern_wait6+0x37a/frame 0x23a17a78
kern_wait(23c00a80,ffffffff,23a17ba4,36,0,...) at kern_wait+0x119/frame 0x23a17b80
sys_wait4(23c00a80,23c00d0c) at sys_wait4+0x68/frame 0x23a17c08
syscall(23a17ce8,3b,3b,3b,0,...) at syscall+0x304/frame 0x23a17cdc
Xint0x80_syscall() at 0xffc033b7/frame 0x23a17cdc

https://people.freebsd.org/~pho/stress/log/oshogbo001.txt

pho added a comment.Jul 30 2019, 7:03 PM

Just a follow up. I'm currently testing your patch on amd64 where I have not yet seen any problems.

pho added a comment.Aug 1 2019, 3:19 AM

I completed a full stress2 test on amd64 without seeing any problems.

oshogbo added a comment.Aug 2 2019, 9:10 AM

pho@ Thank you!

Closed by commit rS350612: procdesc: fix reparenting when the debugger is attached (authored by oshogbo). · Explain WhyAug 5 2019, 8:15 PM
This revision was automatically updated to reflect the committed changes.
oshogbo added a commit: rS350612: procdesc: fix reparenting when the debugger is attached.
Herald added a subscriber: imp. · View Herald TranscriptAug 5 2019, 8:15 PM

Revision Contents
Changeset List

PathSize
sys/
kern/
33 lines
8 lines
sys/
2 lines
tests/
sys/
kern/
56 lines

Diff 57852

View Options

sys/kern/kern_exit.c

Loading...
View Options

sys/kern/sys_procdesc.c

Loading...
View Options

sys/sys/proc.h

Loading...
View Options

tests/sys/kern/ptrace_test.c

Loading...