Page MenuHomeFreeBSD
Differential D57482

exit1(9): do not deadlock if exit is called due to PT_SC_REMOTERQ
ClosedPublic
Actions

Authored by kib on Fri, Jun 5, 9:50 PM.
Tags
None
Referenced Files
F159852659: D57482.diff
Thu, Jun 18, 7:47 PM
F159846806: D57482.id179323.diff
Thu, Jun 18, 6:26 PM
F159846542: D57482.id179311.diff
Thu, Jun 18, 6:21 PM
F159846484: D57482.id179323.diff
Thu, Jun 18, 6:21 PM
F159846310: D57482.id179311.diff
Thu, Jun 18, 6:18 PM
F159771936: D57482.id179333.diff
Thu, Jun 18, 1:30 AM
F159771711: D57482.id179333.diff
Thu, Jun 18, 1:26 AM
Unknown Object (File)
Tue, Jun 16, 1:37 PM
View All 33 Files
Subscribers
imp
olce

Details

Reviewers
markj
jhb
kevans
Commits
rGa2cfe535771d: exit1(9): do not deadlock if exit is called due to PT_SC_REMOTERQ
Summary

The remote syscall is executed in the context where debugger owns a
p_lock hold on the target. Due to this, exit1() waiting for p_lock
going to zero, never happen.

Postpone the exit1() call to ast then, saving the provided rval and
signo in the struct proc. Mark the async-exiting proc with the new
p_flag P_ASYNC_EXIT.

While p_xexit can be reused, p_xsig can be only set by actual exit1(),
otherwise it breaks the ptrace mechanism. Allocate a dedicated p_asig
for it.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

kib created this revision.Fri, Jun 5, 9:50 PM
Herald added subscribers: olce, imp. · View Herald TranscriptFri, Jun 5, 9:50 PM
kib requested review of this revision.Fri, Jun 5, 9:50 PM
kib updated this revision to Diff 179311.Fri, Jun 5, 9:58 PM

Commit everything before generating diff from git.

markj added inline comments.Fri, Jun 5, 11:00 PM
sys/kern/kern_ucoredump.c
200

What about the exit1() calls from do_execve() and fork_return()? Can't they be triggered via ptrace(PT_SC_REMOTE)?

kib marked an inline comment as done.Fri, Jun 5, 11:03 PM
kib added inline comments.
sys/kern/kern_ucoredump.c
200

From fork_return it cannot, I believe.
For execve yes, and I had it changed in the previous version of the patch: I had exit1 and exit2 swapped. Then I decided to keep exit1 synchronous and forgot to update execve.

kib updated this revision to Diff 179316.Fri, Jun 5, 11:09 PM
kib marked an inline comment as done.

Plug exit call in execve

markj added inline comments.Fri, Jun 5, 11:36 PM
sys/kern/kern_exit.c
229

Is there any reason this shouldn't be spelled kern_exit(), and all external consumers updated to use it? e.g., shouldn't linux_exit() call this function as well? AFAIK it is possible to use ptrace(PT_SC_REMOTE) with foreign ABIs.

245

How can this case happen?

kib marked 2 inline comments as done.Sat, Jun 6, 12:16 AM
kib added inline comments.
sys/kern/kern_exit.c
229

I like the exit2() name, it has the same weirdness flavor as exit1(). If you insist, I will change it.

245

I believe e.g. ptrace(PT_CONTINUE, pid, 1, SIGKILL) could trigger it. Any killing signal processed by TDA_SIG before TDA_ASYNC_EXIT would get there through sigexit().

kib updated this revision to Diff 179323.Sat, Jun 6, 12:16 AM
kib marked 2 inline comments as done.

Handle linux_exit()

markj added inline comments.Sat, Jun 6, 1:27 AM
sys/kern/kern_exit.c
229

I think it's not very nice to force others to figure out whether they should use exit1() or exit2(), when there is no comment explaining the difference, nor any clue in the names (shouldn't exit1() at least call exit2()?).

If we keep both names, then one of them should be local to kern_exit.c.

kib updated this revision to Diff 179333.Sat, Jun 6, 3:50 AM
kib marked an inline comment as done.

Rename exit2() to kern_exit().
Add comment.

markj added a comment.Mon, Jun 8, 2:25 PM

I'm not sure about the exit1() call in fork_return(). Isn't it possible for the debugger to attach to the nascent process (see the earlier ptracestop() calls there) and then trigger a system call immediately?

sys/kern/kern_exec.c
1046

This annotation isn't true anymore.

sys/kern/kern_exit.c
240
kib updated this revision to Diff 179400.Mon, Jun 8, 5:26 PM
kib marked 2 inline comments as done.

kern_exit() in fork_return()
Remove unreached comment.
Fix function name in kassert.

markj accepted this revision.Mon, Jun 8, 7:50 PM
In D57482#1317457, @markj wrote:

I'm not sure about the exit1() call in fork_return(). Isn't it possible for the debugger to attach to the nascent process (see the earlier ptracestop() calls there) and then trigger a system call immediately?

I added a test for this scenario to D57485. PT_SC_REMOTE requires the target to be stopped at the user/kernel boundary so cannot be used while the new child is blocked in fork_return(). But I think using kern_exit() in fork_return() is okay anyway.

This revision is now accepted and ready to land.Mon, Jun 8, 7:50 PM
Closed by commit rGa2cfe535771d: exit1(9): do not deadlock if exit is called due to PT_SC_REMOTERQ (authored by kib). · Explain WhyMon, Jun 8, 8:15 PM
This revision was automatically updated to reflect the committed changes.
kib added a commit: rGa2cfe535771d: exit1(9): do not deadlock if exit is called due to PT_SC_REMOTERQ.

Revision Contents
Changeset List

PathSize
sys/
kern/
12 lines
49 lines
3 lines
sys/
5 lines
2 lines

Diff 179316

View Options

sys/kern/kern_exec.c

Loading...
View Options

sys/kern/kern_exit.c

Loading...
View Options

sys/kern/kern_ucoredump.c

Loading...
View Options

sys/sys/proc.h

Loading...
View Options

sys/sys/signalvar.h

Loading...