Details
- Reviewers
jhb markj kevans - Commits
- rG6403a140243d: tools/test/ptrace: update scescx to do remote getpid(2) on each SCX event
rGa98613f23892: ptrace(2): document PT_SC_REMOTE
rG140ceb5d956b: ptrace(2): add PT_SC_REMOTE remote syscall request
rG0e07241c372d: ptrace(2): explain how to select specific thread to operate on
rGf0592b3c8dd8: Add a thread debugging flag TDB_BOUNDARY
rGe6feeae2f915: sys: rename td_coredump to td_remotereq
rGf081a291a17d: compat32: move struct ptrace_sc_ret32 definition from .c to .h
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
What's the motivation for adding this feature?
| sys/kern/kern_sig.c | ||
|---|---|---|
| 2680 ↗ | (On Diff #113767) | Should we try to respect trap_enotcap somehow? |
| 2683 ↗ | (On Diff #113767) | Why not try to update td_cowgen here? |
| 2723 ↗ | (On Diff #113767) | The duplication between this function and syscallenter() isn't ideal. I wonder if the latter can be broken up into multiple inline functions, and some of them reused here. |
| 2756 ↗ | (On Diff #113767) | Extra newline. |
| sys/kern/kern_sig.c | ||
|---|---|---|
| 2680 ↗ | (On Diff #113767) | I thought about it, and decided not to do. The main reason is that I do not additionally add a recursion to generate new signal from ptracestop() (it is there anyway, but I do not want to add more). Logical reason is that the code is requested by the debugger already, so it should be capable to see the ECAPMODE by itself, without additional help. If you still consider it useful to generate SIGTRAP there, please explain. |
| 2723 ↗ | (On Diff #113767) | I considered it very briefly, but the functions appear to be too small and ad-hoc, IMO. This can be reconsidered, but for me it does not look compelling. |
There are some programs doing exactly that, by hacking a place in the .text and injecting their code. I remember that reptyr (ports/sysutils/reptyr) does that. Also some time ago I listen to a talk describing the same hack. Since we already has the hardest part done, the ability to request a code to be executed in the remote context (for coredumps), I finally decided to add this feature in a regular and hopefully more convenient way.
This is the primary reason why Kyle is added as a reviewer.
| lib/libc/sys/ptrace.2 | ||
|---|---|---|
| 996 ↗ | (On Diff #113767) | Could we add some context about which thread is chosen? |
Hmm, GDB implements this sort of thing by changing registers to point to a block of code inserted into the address space.
Exactly. This is what I am trying to provide a (much) more convenient interface for.
+1; iirc this kills off the only MD part of reptyr, which was mainly the registers that it swaps out for syscall args. It doesn't go as far as a whole block of code, though, iirc- just proceed to syscall entry, swap out syscall args with its own, then rewind %Ip to execute victim's syscall. Still uglier than it needs to be...
Well, except that GDB does this for all sorts of things that is more general than just invoking a system call. I can't see it ever making use of this as it is too limited. That said, there's lots of this that GDB does more of on Linux than on FreeBSD, and it might be that using this to call mmap(MAP_ANON...) to get the initial set of scratch space to which compiled code can be written might be useful. Also for things like displaced stepping that is not implemented on FreeBSD.
Hmm, well, seems on Linux the displaced stepping thing just reuses a block of instructions at the entry point assuming they won't get re-used once the program has started execution.
| lib/libc/sys/ptrace.2 | ||
|---|---|---|
| 130 ↗ | (On Diff #113863) | |
| 132 ↗ | (On Diff #113863) | |
| 135 ↗ | (On Diff #113863) | |
| 136 ↗ | (On Diff #113863) | |
| 138 ↗ | (On Diff #113863) | |
| 140 ↗ | (On Diff #113863) | (I think "legitimate" is redundant.) |
| 142 ↗ | (On Diff #113863) | |
| 143 ↗ | (On Diff #113863) | |
| 1004 ↗ | (On Diff #113863) | |
| 1010 ↗ | (On Diff #113863) | |
| 1014 ↗ | (On Diff #113863) | |
| 1016 ↗ | (On Diff #113863) | |
| 1049 ↗ | (On Diff #113863) | Maybe note specifically that the process can deadlock. In this situation, the only way out is to kill the process. |
| sys/kern/kern_sig.c | ||
| 2673 ↗ | (On Diff #113863) | |
| 2751 ↗ | (On Diff #113863) | As a matter of naming, we have _remotereq, _syscallrq and _coredump. IMHO it'd be nicer to be more consistent and just use req as the suffix everywhere, also for TDB_ flags. rq is somewhat cryptic to me. |
| 2758 ↗ | (On Diff #113863) | Should we assert that the flag is still set here? |
| 2680 ↗ | (On Diff #113767) | It's a corner case, not very important. Maybe this behaviour is actually preferable. I would perhaps add a comment noting that trap_enotcap (and the corresponding procctl() flag) are ignored in this mode. |
| 2723 ↗ | (On Diff #113767) | The amount of duplication is still worrisome to me. I still don't have a concrete suggestion to resolve it though. |
| sys/kern/sys_process.c | ||
| 1646 ↗ | (On Diff #113863) | Why exactly is this assertion true? What if two threads in the tracing process both issue a remote syscall request? |
| sys/sys/ptrace.h | ||
| 200 ↗ | (On Diff #113863) | Should we use register_t instead of u_long to represent parameters? That would be consistent with struct syscall_args, and the memcpy in the PT_SC_REMOTE effectively assumes that the types are compatible already. |
| lib/libc/sys/ptrace.2 | ||
|---|---|---|
| 1057 ↗ | (On Diff #114205) | |