Repository navigation
fix(kernel): preserve user-return work and retire stopped TLB targets - #2511
Conversation
|
The original concern about enabling interrupts inside a caller-owned atomic section is valid. After rebasing locally and an independent subagent review, I am skipping this PR in its present form because it changes the shootdown completion contract without a safe, validated replacement:
The historical integration job also stalled at ProcessVMTest.WriteToZombie for 300 seconds; this needs reproduction when validating a revised shootdown protocol. The independent review did not establish an ordinary IPI-reentrancy UAF, so I am not treating that as a confirmed finding. No local rebase was pushed and this PR was not merged. The old uprobe Dunitest assertion is already fixed by #2509. |
fslongjin
left a comment
fslongjin
left a comment
There was a problem hiding this comment.
Changes requested. Please split the original IRQ-context/shootdown problem into an independently tracked issue and a focused PR that implements the complete root-cause fix across the initiator, callers, receiver, and CPU lifecycle. Establish and enforce the caller's interrupt/locking contract, preserve synchronous invalidation and resource-lifetime guarantees, and validate concurrent initiators and CPU shutdown races.
The current implementation stops waiting for CPUs that are merely Dying and constructs diagnostic strings from independently published pointer/length values. Both require correction. Diagnostic instrumentation should be a separate change with a coherent, memory-safe snapshot and a safe reporting context. Please provide before/after reproduction and adversarial tests; recording a contract violation alone does not complete the underlying bug fix.
…initiators Remove the temporary IRQ-enable/restore hack from the synchronous TLB shootdown path. The initiator now runs with interrupts enabled (and preemption disabled by the global flush lock), matching Linux native_flush_tlb_multi(); the contract is documented and debug-asserted. Callers that initiate shootdowns while writing user memory are fixed: - exit_to_user_mode_loop: run Rseq resume with interrupts enabled. Its copy_to_user can fault on a COW page and must be able to shoot down; with IRQs disabled two initiators can deadlock on each other's IPI. - Defer CLONE_CHILD_SETTID from the context-switch hook (IRQs disabled) to the return-to-user boundary, clearing the work flag after use. CPU liveness: a Dying CPU may still execute and access page tables, so it is a valid shootdown target; only a Dead target (which can never acknowledge or touch memory again) ends the wait. Refs: DragonOS-Community#2511, DragonOS-Community#2546
|
已按 review 意见收敛/修正，本 PR 现在是“完整根因修复 + 契约校验”： 1. 发起方契约
2. 定位并修复所有已知的 IRQ-off 发起方
3. CPU 生命周期
4. 拆分
CI 已重新触发；本地用 CI 同款镜像跑 |
Signed-off-by: longjin <longjin@dragonos.org>
c59aa23 to
a28c1dd
Compare
fslongjin
changed the title
|
@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. |
Signed-off-by: longjin <longjin@dragonos.org>
![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: a28c1dd948
ℹ️ 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".
| normal/rlimit_data | ||
| normal/fork_mapping_holes | ||
|
|
||
| normal/user_return_work |
There was a problem hiding this comment.
在未启用 sched_selftest 的默认 x86_64 内核上，MigrationNotificationSurvivesRealResumeHandler 会因调试入口不存在而执行 GTEST_SKIP()；但 runner/src/main.rs 中的 enforce_no_skip() 会将名单内二进制的任何 skip 改判为失败。因此，通过默认安装的 run_tests.sh 运行时，即使其余四项全部通过，整个 normal/user_return_work 仍会失败。应将特性专用 fixture 拆成独立二进制，仅在启用该特性的 CI 中强制禁止跳过，同时保留普通用例的默认 no-skip 约束。
Useful? React with 👍 / 👎.
Faultable child-TID and rseq writes currently run in IRQ-off switch/return paths. Moving rseq work to an interruptible context also requires consuming its notification before processing: clearing it afterward can discard a migration notification published while updating the user CPU ID.
Publish child-TID work during fork and perform its best-effort write before signals/ptrace at the first user-return boundary. Remove the thread-lock check from every context switch. Consume NEED_RSEQ before enabling IRQs, and let notifications generated during the handler request another pass.
For x86 shootdowns, include Dying targets until their teardown finishes. After acquiring an irreversible Dead state, clear the CSD stack pointer before publishing completion. Keep master's IRQ-off inbound-service and lock-free claim protocol. Normal acknowledgement polling gains no state read; terminal checks reuse the existing 1024-spin slow path. Non-x86 target policy is unchanged.
Validation:
Performance: remove the per-switch thread lock and keep test probes out of production. Four-vCPU ABBA samples show no observed regression (pipe round trip 18,070.5 → 17,932 ns; fork/wait 524,324 → 512,827 ns). Two-vCPU runs include slower candidate medians and substantial variability under concurrent host workloads, including after host-CPU pinning; these results do not establish a reliable performance change.
The notification fixture exercises a real handler with deterministic migration-notification publication, not an actual CPU migration. The private CSD fixture verifies retirement/reuse, not a real CPU shutdown race. Terminal cancellation depends on Dead CPUs remaining non-restartable.