feat(ipc/sem): add semaphore support for dragonOS - #2172
feat(ipc/sem): add semaphore support for dragonOS#2172mistcoversmyeyes wants to merge 7 commits into
Conversation
e0bc761 to
a266b20
Compare
d9c382f to
fbdee92
Compare
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbdee927ce
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let set = self | ||
| .get_by_semid_checked_mut(token.id) | ||
| .map_err(|_| SystemError::EIDRM)?; |
There was a problem hiding this comment.
当 prepare_setall() 检查权限后复制用户数组时，集合所有者可并发执行 IPC_SET 撤销调用者的写权限；这里重新加锁后只验证 ID 和长度，仍会提交全部新值。应在持锁修改 semval 前按当前权限再次执行写权限检查，避免权限撤销后的 TOCTOU 写入。
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
fslongjin
left a comment
fslongjin
left a comment
There was a problem hiding this comment.
Request changes: this PR establishes a useful base for System V semaphore support, but it does not yet satisfy the Linux 6.6 compatibility and concurrency-safety contract stated in #2142.
The blocking issues are:
- IPC_SET permission updates can partially commit on an error, changing the owner even though the syscall returns EINVAL; the shared helper also affects SHM.
- SEM_UNDO is rejected with ENOSYS and the new test codifies that incompatibility, while Linux maintains per-process/shared undo state and replays it at process exit.
- A single namespace-wide spinlock protects the registry and every semaphore set, so unrelated sets are serialized; the lock also covers allocation-heavy queue simulation and scheduler wakeups.
- User-controlled semaphore-set allocation is infallible and can reach the kernel panic allocation handler instead of returning ENOMEM.
- SEM_STAT and SEM_STAT_ANY mask their direct table index, causing out-of-range indices to alias valid objects.
The basic syscall wiring, atomic multi-operation simulation, timeout/removal paths, and test breadth are valuable. However, the issues above are architectural or user-visible Linux semantic mismatches rather than optional refinements. Please address them, add the corresponding regression tests, and rerun the guest suite. The current Integration Test check also reports 5666 passed, 1 failed, and 180 skipped; I am not attributing that failure to this PR without further evidence, but the PR description should not claim that Integration Test passed while the check remains red.
| .iter() | ||
| .any(|op| (op.sem_flg as u32) & SemFlags::SEM_UNDO.bits() != 0) | ||
| { | ||
| return Err(SystemError::ENOSYS); |
There was a problem hiding this comment.
[P1] Rejecting every SEM_UNDO operation with ENOSYS is not Linux-compatible System V semaphore behavior. Linux 6.6 maintains sem_undo/semadj state, shares the undo list for CLONE_SYSVSEM, clears adjustments on SETVAL/SETALL/IPC_RMID, and replays them from exit_sem() when a task exits. This is essential crash-recovery behavior: without it, a lock holder exiting can leave peers blocked indefinitely. Please implement the full lifecycle before treating #2142 as complete; the new test should verify Linux behavior instead of expecting ENOSYS.
There was a problem hiding this comment.
Confirmed. Full SEM_UNDO lifecycle support is required for Linux 6.6 compatibility, including shared undo state for CLONE_SYSVSEM, semadj updates and limits, cleanup on SETVAL/SETALL/IPC_RMID, and replay on task exit. I am implementing this now and will replace the current ENOSYS test with lifecycle coverage. Keeping this thread open until the implementation and tests are complete.
| /// SysV SHM manager (phase one: per-namespace SHM only) | ||
| pub shm: SpinLock<ShmManager>, | ||
| /// SysV semaphore manager | ||
| pub sem: SpinLock<SemManager>, |
There was a problem hiding this comment.
[P1] A namespace-wide spinlock is too broad for semaphore-set state. Every operation on every set, including update_queue(), is serialized here; queue simulation allocates a HashMap, may rescan waiters quadratically, and calls Waker::wake() while this lock is held. A user can therefore stall unrelated semaphore sets in the same namespace. Please keep the manager lock limited to ID/key/quota lookup, store stable Arc<KernelSemSet> objects with per-set locking, use a non-allocating operation fast path, and collect wakeups for execution after releasing the set lock, following Linux's registry/array locking and wake_q separation.
|
While reviewing the queue-rescan concern, I found a separate Linux 6.6 compatibility issue in the current unified waiter queue. Linux classifies queued requests into const operations (the entire operation group has no altering operation) and alter operations when they are enqueued: ipc/sem.c#L2127-L2153. Its update path handles wait-for-zero requests before scanning pending alter requests: ipc/sem.c#L1029-L1039. After a queued alter operation succeeds, Linux immediately checks wait-for-zero requests made satisfiable by that change before continuing with further alter operations: ipc/sem.c#L974-L992. The current DragonOS implementation stores both classes in one Proposed minimal design for this PR:
This fixes the observable ordering difference without attempting to eliminate the irreducible worst-case rescan for complex multi-operation groups. |
2 participants
Related
Summary
semget,semctl,semop, andsemtimedop.Scope
SEM_UNDOis out of scope and currently returnsENOSYS.Acceptance
ENOSYS.semopandsemtimedopshare consistent operation semantics.IPC_RMID.Testing