Accept live thread TIDs in priority syscalls#232
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
e2836c0 to
4e48ed6
Compare
| emu_nice = val; | ||
| } | ||
|
|
||
| static bool prio_process_target_alive(int who) |
There was a problem hiding this comment.
prio_process_target_alive() is a near-verbatim copy of sched_pid_alive() in src/syscall/sys.c:284: accept who==0, accept the process pid, else defer to thread_tid_alive(). The only difference is guest_pid vs proc_get_pid(), which resolve to the same value. Hoist one shared helper and call it from both sites.
| if (which != 0) | ||
| return -LINUX_EINVAL; | ||
| if (who != 0 && who != (int) guest_pid) | ||
| if (!prio_process_target_alive(who)) |
There was a problem hiding this comment.
setpriority writes the single process-global emu_nice regardless of which live TID is targeted, so setting nice on one thread changes the priority getpriority reports for every thread and for self. Linux gives each task its own nice. Widening the accepted who to any live TID makes this shared-state model observably wrong for the first time.
Either model per-TID nice, or keep the single-nice model but document it and reject non-self TIDs rather than silently accepting a TID and writing the global.
Note the new test can't catch this: it sets and reads back the same TID (tests/test-credentials.c:359-361), so a global and a per-thread implementation both yield 15.
| "setpriority/getpriority(live tid) mismatch"); | ||
| release_priority_child(); | ||
|
|
||
| TEST("getpriority dead thread TID"); |
There was a problem hiding this comment.
TEST("getpriority dead thread TID") is nested inside the else of the clone-failure check. If spawn_priority_child() fails, this sub-test's label and pass/fail count vanish entirely, skewing the totals. Hoist it to its own top-level TEST(...) block after the live-TID block.
| * CLONE_SYSVSEM | CLONE_SETTLS | CLONE_PARENT_SETTID | | ||
| * CLONE_CHILD_CLEARTID | CLONE_DETACHED. | ||
| */ | ||
| unsigned long flags = 0x7d0f00; |
There was a problem hiding this comment.
Clone flags 0x7d0f00 include CLONE_SETTLS (0x80000) but the tls argument is 0, so the TLS base is deliberately set to 0. The child only touches globals and raw syscalls, so it's harmless, but the flag is pointless and contradicts the comment. Drop CLONE_SETTLS (0x7c0f00) or pass a real tls pointer.
|
|
||
| TEST("getpriority dead thread TID"); | ||
| long dead_prio = 0; | ||
| for (int i = 0; i < 1000; i++) { |
There was a problem hiding this comment.
The dead-thread poll is the right shape (elfuse's thread_deactivate() legitimately lags the CLONE_CHILD_CLEARTID futex, so the retry is required), but the hard 1000-iteration sched_yield cap can expire under load and produce a spurious FAIL. Consider looping unbounded with a yield, or a time bound, instead of a fixed count.
4e48ed6 to
28bb50b
Compare
Linux treats thread IDs as valid PRIO_PROCESS targets because each thread is a task. elfuse only accepted who 0 and the guest process ID, so getpriority(PRIO_PROCESS, tid) returned ESRCH for live guest threads. Add a shared process/TID liveness helper and use it from both scheduler queries and getpriority. Keep setpriority self-only while elfuse stores nice as one process-wide value. This avoids silently accepting a live TID and mutating the nice value reported for every task. Add regression coverage for live thread getpriority, non-self setpriority rejection, and dead thread ESRCH behavior. Fix sysprog21#231
28bb50b to
41fa2d2
Compare
|
Fixed the review comments. Changed:
|
2 participants
Linux treats thread IDs as valid PRIO_PROCESS targets because each thread is a task. elfuse only accepted who 0 and the guest process ID, so getpriority(PRIO_PROCESS, tid) returned ESRCH for live guest threads.
Share PRIO_PROCESS target validation between getpriority and setpriority, accepting live guest TIDs from the thread table.
Add regression coverage for live thread getpriority, live thread setpriority, and dead thread ESRCH behavior.
Fix #231
Summary by cubic
Accept live guest TIDs as PRIO_PROCESS targets in getpriority, matching Linux and avoiding ESRCH for running threads. Add a shared
proc_pid_aliveused by getpriority and sched_* queries; keep setpriority self-only while nice is process-wide; add tests for live TID get, non-self set ESRCH, and dead TID ESRCH. Fixes #231.Written for commit 41fa2d2. Summary will update on new commits.