Uh oh!
There was an error while loading. Please reload this page.
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
Uh oh!
There was an error while loading. Please reload this page.
e0bc761 to
a266b20Compared9c382f to
fbdee92Comparefslongjin
commented
Aug 29, 2026
@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. |
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 👍 / 👎.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| } | ||
| fn get_by_index(&self, id: usize) -> Result<&KernelSemSet, SystemError> { | ||
| let idx = id & IpcIdAllocator::IPC_ID_IDX_MASK; |
There was a problem hiding this comment.
当 SEM_STAT 或 SEM_STAT_ANY 收到大于 IPC_ID_IDX_MASK 的索引时，这里静默截掉高位；只要低 15 位对应现有集合，诸如 0x8000 的无效索引就会错误返回索引 0 的对象，而不是 Linux 的 EINVAL。应先验证范围，再直接按原索引查表。
AGENTS.md reference: AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
| if changed { | ||
| break; |
There was a problem hiding this comment.
当队首有 B 个仍不可执行的等待者、其后有 W 个依次可执行且改变值的等待者时，这个 break 会在每次完成后从队首重新扫描；每次模拟还会分配 HashMap，并且全过程持有命名空间级自旋锁，形成 O(B×W) 的模拟和分配。等待者数量不受 SEMOPM 限制，非特权进程可借此造成整个 IPC 命名空间长时间停顿，应仅重新调度受值变化影响的条目或使用索引化等待队列。
AGENTS.md reference: AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
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.
Uh oh!
There was an error while loading. Please reload this page.
| .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.
Uh oh!
There was an error while loading. Please reload this page.
| } | ||
| fn get_by_index(&self, id: usize) -> Result<&KernelSemSet, SystemError> { | ||
| let idx = id & IpcIdAllocator::IPC_ID_IDX_MASK; |
There was a problem hiding this comment.
[P1] SEM_STAT and SEM_STAT_ANY take a direct IPC table index, not an encoded semaphore ID. Masking the argument makes an invalid index such as 0x8000 + n alias slot n and return a valid set, whereas Linux passes the original integer to the IDR lookup and returns EINVAL. Please reject indices above IPC_ID_IDX_MASK and look up the unmodified index; add coverage for both commands with high-bit indices.
| /// 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.
Related
Summary
semget,semctl,semop, andsemtimedop.Scope
SEM_UNDOis out of scope and currently returnsENOSYS.Acceptance
ENOSYS.semopandsemtimedopshare consistent operation semantics.IPC_RMID.Testing