Page MenuHomeFreeBSD
Differential D3536

Export current system call code and argument count for SCE and SCX events.
ClosedPublic
Actions

Authored by jhb on Aug 31 2015, 4:44 AM.
Tags
None
Referenced Files
F105545283: D3536.id8402.diff
Tue, Dec 17, 11:58 AM
Unknown Object (File)
Sat, Nov 23, 9:05 AM
Unknown Object (File)
Oct 19 2024, 3:39 AM
Unknown Object (File)
Sep 17 2024, 4:39 AM
Unknown Object (File)
Sep 8 2024, 12:06 AM
Unknown Object (File)
Sep 7 2024, 7:51 AM
Unknown Object (File)
Sep 4 2024, 11:58 AM
Unknown Object (File)
Sep 3 2024, 7:35 AM
View All Files
Subscribers
emaste
imp

Details

Reviewers
kib
Commits
rS287386: Export current system call code and argument count for system call entry
Summary

Export current system call code and argument count for system call entry
and exit events. procfs stop events for system call tracing report these
values (argument count for system call entry and code for system call exit),
but ptrace() does not provide this information. (Note that while the system
call code can be determined in an ABI-specific manner during system call
entry, it is not generally available during system call exit.)

The values are exported via new fields at the end of struct ptrace_lwpinfo
exported via PT_LWPINFO.

Test Plan
  • I have updated truss to use these values, in particular the argument count on entry restores behavior lost when truss was converted from procfs to ptrace.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jhb updated this revision to Diff 8359.Aug 31 2015, 4:44 AM
jhb retitled this revision from to Export current system call code and argument count for SCE and SCX events..
jhb updated this object.
jhb edited the test plan for this revision. (Show Details)
jhb added a reviewer: kib.
Herald added subscribers: emaste, imp. · View Herald TranscriptAug 31 2015, 4:44 AM
kib edited edge metadata.Aug 31 2015, 7:36 AM

IMO you cannot keep the pointer to the syscall_args in struct thread. I would add td_syscall_code and _narg to the struct thread, for direct use.

That said, at least syscall_code is available in registers. although in the MD way.

sys/kern/subr_syscall.c
71 ↗(On Diff #8359)

You cannot do it this way. struct syscall_args is typically allocated on the thread stack, which means that it could be swapped out. I do not remember any reason why stopped thread cannot be swapped.

161 ↗(On Diff #8359)

Why not keeping the (analog of) td_dbgsa for the whole syscall processing duration ? Setting pl_syscall_* should be directed by TDB_SCE/TDB_SCX, then you do not need to temporary NULLify the data.

sys/kern/sys_process.c
1218 ↗(On Diff #8359)

As I said above, I would change the condition to td2->td_dbgflags & (TDB_SCE|TDB_SCX) != 0

jhb marked 3 inline comments as done.Aug 31 2015, 7:36 PM

As noted in the commit log, while you can obtain the system call code for system call entry events, you cannot obtain it for system call exit (and newborn processes and threads return from a system call that a tracer would not have a matching system call entry event for. I suspect this is why procfs returned the code for SCX stops.)

jhb updated this revision to Diff 8371.Aug 31 2015, 7:37 PM
jhb edited edge metadata.
  • Store the system call code and narg directly in struct thread.
jhb added inline comments.Aug 31 2015, 7:42 PM
sys/kern/subr_syscall.c
71 ↗(On Diff #8371)

So one slightly annoying thing here is that sa->code and sa->narg aren't valid yet when we set TDB_SCE. For now I've deferred setting the td_dbg_sc_* fields until we call ptracestop(). If we want these fields to always be valid when TDB_SCE is set, then we either need to defer setting TDB_SCE or move this section after the call to sv_fetch_syscall_args so that sa->code and sa->narg are valid. I would probably prefer the latter, but I'm not sure of the implications of deferring the set of TDB_SCE until after the call to fetch_syscall_args, especially in regards to TDB_USERWR.

kib accepted this revision.Sep 1 2015, 11:23 AM
kib edited edge metadata.

BTW, why not take a step further and not provide the syscall arguments as well ?

sys/kern/subr_syscall.c
71 ↗(On Diff #8371)

But TDB_ flags are not really usable until the debugee is at the stop point. That is, I do not see much difference between having TDB_SCE set where it is now, and moving it to the ptracestop() location. Except possibly one less PROC_LOCK().

sys/sys/ptrace.h
117 ↗(On Diff #8371)

Make this unsigned as well ?

This revision is now accepted and ready to land.Sep 1 2015, 11:23 AM
jhb marked an inline comment as done.Sep 1 2015, 5:42 PM
In D3536#72717, @kib wrote:

BTW, why not take a step further and not provide the syscall arguments as well ?

When I had asked you about that previously via e-mail I found your argument convincing that it would be putting too much into the kernel. It would mean embedding syscall_args directly in struct thread so that the full register array is present as well.

sys/kern/subr_syscall.c
71 ↗(On Diff #8371)

After talking more on IRC, we realized that moving TDB_SCE down would be a bug because it would not properly report PL_FLAG_SCE if a stop inside of a system call triggered but the debugger had not enabled system call stops, so I will leave TDB_SCE where it is.

sys/kern/sys_process.c
99 ↗(On Diff #8371)

Unrelated: should this be pid_t instead of int? Right now that would be a no-op since pid_t is int32_t?

sys/sys/ptrace.h
117 ↗(On Diff #8371)

Done. Note that it is signed in 'struct syscall_args' so I was just copying from there. However, unsigned is probably better.

jhb updated this revision to Diff 8396.Sep 1 2015, 5:46 PM
jhb edited edge metadata.
jhb marked an inline comment as done.
  • Leave TDB_SCE where it is.
  • Make narg unsigned.
This revision now requires review to proceed.Sep 1 2015, 5:46 PM
kib added a comment.Sep 1 2015, 6:02 PM

What about the fix for TDB_USERWR bug ? Is it planned for other change ?

sys/kern/subr_syscall.c
174 ↗(On Diff #8396)

Is this now stray change ?

sys/kern/sys_process.c
99–101 ↗(On Diff #8396)

int is explicit type, this is probably a reason why I used it there (and in ptrace_lwpinfo).
I do not object against changing it.

jhb marked an inline comment as done.Sep 1 2015, 7:28 PM
In D3536#72841, @kib wrote:

What about the fix for TDB_USERWR bug ? Is it planned for other change ?

Yes, I will handle TDB_USERWR separately. I ran into that trying to debug why I'm getting a seemingly spurious SIGHUP testing fork following in gdb (per bug 201149) and noticed the odd output in the kdump.

sys/kern/subr_syscall.c
174 ↗(On Diff #8396)

Yes, though it is actually used, so it is more correct. I can do it separately.

jhb updated this revision to Diff 8402.Sep 1 2015, 7:28 PM
jhb marked an inline comment as done.
jhb edited edge metadata.
  • Cleanup.
kib accepted this revision.Sep 1 2015, 7:47 PM
kib edited edge metadata.
kib added inline comments.
sys/kern/subr_syscall.c
173 ↗(On Diff #8402)

I am fine with either. It is more logical to make a separate commit.

This revision is now accepted and ready to land.Sep 1 2015, 7:47 PM
Closed by commit rS287386: Export current system call code and argument count for system call entry (authored by jhb). · Explain WhySep 1 2015, 10:25 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
head/
sys/
kern/
6 lines
11 lines
sys/
4 lines
2 lines

Diff 8410

View Options

head/sys/kern/subr_syscall.c

Loading...
View Options

head/sys/kern/sys_process.c

Loading...
View Options

head/sys/sys/proc.h

Loading...
View Options

head/sys/sys/ptrace.h

Loading...