B6: T-024 — task_create_from_image (LoadedImage → runnable Task capability)#38
Merged
Conversation
…sk capability)
The deferred B4 §3 / B6-step-3 bridge: turns a LoadedImage into a runnable
CapHandle{CapObject::Task(...)}. It resolves the loaded address space from
loaded.as_cap (lookup + CapKind::AddressSpace kind-check), mints a Task kernel
object bound to it (create_task), and installs a root CapKind::Task capability
naming it (insert_root), returning the CapHandle.
- Mints, does not schedule. Running the task is the B6 wire-up's job (it calls
Scheduler::add_user_task with the image's entry_va/stack_top_va — the EL0
entry context lives in the cooperative context per ADR-0037, not on the Task
object, so task_create_from_image reads only loaded.as_cap), and not before
the T-021 carry-forward gates (gate #1 per-task user-VA translation, gate #3
current-task cap table) close. Dormant in v1: no BSP caller, so the QEMU
trace is byte-stable.
- Rollback: if insert_root hits a full cap-table (CapsExhausted), the just-
created task object is destroyed (arena slot freed) so no orphaned task
leaks. Pinned by a host test (slot 0 reused after the failed create).
- TaskCreateError: InvalidAddressSpaceCap(CapError) (stale / wrong-kind
as_cap), TaskArenaFull, CapTableExhausted(CapError). Re-exported from
crate::obj.
- v1 cap-rights model: the Task cap is kind-only (CapKind::Task names/authorizes
the task object), mirroring the CapKind::AddressSpace model; task_rights
govern only cap management. No Task-operation right; no new ADR. No new unsafe.
Composes ADR-0029 (loader) + ADR-0028 (AS cap) + ADR-0016 (kernel object) +
ADR-0037 (EL0 context). New host tests: mint + AS-binding + id; wrong-kind
as_cap; stale as_cap; rollback-on-cap-table-exhausted.
Gates: cargo fmt; host + kernel clippy -D warnings; 347 host tests (+4);
kernel build; QEMU smoke byte-stable + fault-clean (dormant); cargo +nightly
miri test --workspace --exclude tyrne-bsp-qemu-virt clean (0 UB).
Security-relevant (cap minting + AS-cap resolution + rollback); flagged for
explicit security review per the task's Definition of done.
Refs: T-024, ADR-0029, ADR-0028, ADR-0037
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…oc drift, ObjError-map comment
Folds in an adversarial multi-agent review of T-024 (4 lenses × per-finding
verification): 6 findings, all confirmed Low/Nit, no code/security/correctness
defect (the borrow scoping, rollback, error mapping, kind-only model, and the
4 host tests were verified correct). Fixed:
- CC-001 (Low) — obj/mod.rs module-doc was stale: it omitted the new
task_create_from_image / TaskCreateError surface and still asserted the
loader does NOT mint a runnable CapHandle{CapObject::Task(...)} (now false).
Updated the surface list + the load_image/task_create_from_image split note
+ the Status heading.
- DOC-1 (Low) — current.md retained a "Next to open: task_create_from_image"
sentence (historical T-022 prose) that contradicted the same bullet's T-024
lead; removed (the T-024 lead's own "Next:" supersedes it).
- DOC-2 (Nit) — dropped the redundant explicit `[crate::mm::cap_map]` target
on the fn doc's `[cap_map]` link (rustdoc redundant-target warning).
- T024-C-N1 (Nit) — added a comment pinning that create_task only returns
ObjError::ArenaFull, so the map_err(|_| TaskArenaFull) is exhaustive by
contract (not a lossy catch-all).
Deferred (tracked in the T-024 task doc, no code change):
- SEC-T024-01 (Nit) — the minted Task cap stores an AddressSpaceHandle whose
liveness is not coupled to the AS cap; dormant + out of scope (mints, does
not schedule) + a pre-existing v1 object-lifecycle gap (ADR-0016). Recorded
as a carry-forward precondition for the B6 wire-up / the successor lifecycle
ADR (a Task must not outlive / alias a reused slot of its bound AS).
- CC-002 (Nit) — the inline AS-cap resolve duplicates the module-private
resolve_address_space_cap; reuse is not cleanly available (error-taxonomy
coupling). Tracked as a future generic CapabilityTable::lookup_kind helper.
Gates: cargo fmt; host + kernel clippy -D warnings; 347 host tests; the
DOC-2 rustdoc warning is gone (kernel binary unchanged → prior byte-stable
QEMU smoke + Miri 0 UB hold).
Refs: T-024
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Reviewer's GuideImplements task_create_from_image as the bridge from a LoadedImage to a runnable Task capability, adds an error taxonomy and tests around it, and updates documentation/roadmap to reflect the new T-024 task and API surface. Sequence diagram for task_create_from_image minting a Task capabilitysequenceDiagram
participant Caller
participant CapabilityTable
participant TaskArena
participant TaskLoader
Caller->>TaskLoader: task_create_from_image(loaded, table, task_arena, id, task_rights)
rect rgb(230,230,230)
Note over TaskLoader,CapabilityTable: Step 1 — resolve loaded.as_cap to AddressSpaceHandle
TaskLoader->>CapabilityTable: lookup(loaded.as_cap)
alt [CapError::InvalidHandle or CapError::WrongKind]
CapabilityTable-->>TaskLoader: Err(CapError)
TaskLoader-->>Caller: Err(TaskCreateError::InvalidAddressSpaceCap)
else [CapObject::AddressSpace(handle)]
CapabilityTable-->>TaskLoader: Ok(Capability)
TaskLoader->>TaskLoader: extract AddressSpaceHandle
end
end
rect rgb(230,230,230)
Note over TaskLoader,TaskArena: Step 2 — mint Task object in arena
TaskLoader->>TaskArena: create_task(task_arena, Task::new(id, as_handle))
alt [arena full]
TaskArena-->>TaskLoader: Err(ObjError::ArenaFull)
TaskLoader-->>Caller: Err(TaskCreateError::TaskArenaFull)
else [task created]
TaskArena-->>TaskLoader: Ok(task_handle)
end
end
rect rgb(230,230,230)
Note over TaskLoader,CapabilityTable: Step 3 — install root Task capability
TaskLoader->>CapabilityTable: insert_root(Capability::new(task_rights, CapObject::Task(task_handle)))
alt [success]
CapabilityTable-->>TaskLoader: Ok(task_cap: CapHandle)
TaskLoader-->>Caller: Ok(task_cap)
else [CapError::CapsExhausted]
CapabilityTable-->>TaskLoader: Err(CapError::CapsExhausted)
TaskLoader->>TaskArena: destroy_task(task_arena, task_handle)
TaskArena-->>TaskLoader: Ok(())
TaskLoader-->>Caller: Err(TaskCreateError::CapTableExhausted)
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR implements the task_create_from_image bridge that converts a successfully loaded address space (LoadedImage) into a minted CapObject::Task capability; it updates docs/roadmap/module re-exports, adds TaskCreateError and the public API, implements minting/insert/rollback logic, and adds unit tests. ChangesTask creation feature
Sequence Diagram(s)sequenceDiagram
participant Caller
participant CapabilityTable
participant TaskArena
Caller->>CapabilityTable: resolve loaded.as_cap
CapabilityTable-->>Caller: CapHandle / error
Caller->>Caller: verify CapKind == AddressSpace
Caller->>TaskArena: create_task(bound to AS)
TaskArena-->>Caller: TaskId / TaskArenaFull
Caller->>CapabilityTable: insert CapKind::Task root cap
CapabilityTable-->>Caller: success / CapTableExhausted
alt CapTableExhausted
Caller->>TaskArena: destroy_task(TaskId)
TaskArena-->>Caller: destroyed
end
Caller-->>Caller: return Result<CapHandle, TaskCreateError>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the task_create_from_image bridge (T-024), which transitions a LoadedImage into a runnable CapObject::Task capability. It resolves the loaded address space capability, mints a Task kernel object, and inserts a root CapKind::Task capability into the capability table, with a rollback mechanism to prevent orphaned task slots if the capability table is full. The reviewer suggested masking the task_rights to only allow capability management rights (DUPLICATE, DERIVE, REVOKE, TRANSFER) when minting the root Task capability to adhere to the principle of least privilege.
| // 3 — install a root Task capability. On a full cap-table, roll the task | ||
| // object back so no orphaned arena slot leaks (the handle was just | ||
| // created, so `destroy_task` succeeds; the recovered `Task` is dropped). | ||
| let cap = Capability::new(task_rights, CapObject::Task(task_handle)); |
There was a problem hiding this comment.
Since the Task capability in v1 is kind-only and does not carry any task-operation rights, any rights other than capability management rights (DUPLICATE, DERIVE, REVOKE, TRANSFER) are meaningless and could violate the principle of least privilege if granted. It is safer to mask task_rights to ensure only capability management rights are allowed when minting the root Task capability.
let allowed_rights = task_rights & (CapRights::DUPLICATE | CapRights::DERIVE | CapRights::REVOKE | CapRights::TRANSFER);
let cap = Capability::new(allowed_rights, CapObject::Task(task_handle));![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
task_create_from_image,TaskCreateError::CapTableExhaustedcurrently assumesinsert_rootonly returnsCapsExhausted; if that contract ever widens, this mapping becomes misleading, so consider either matching explicitly onCapError::CapsExhausted(and handling others separately) or adding a defensive assertion to keep the assumption checked. - When rolling back after a failed
insert_root, thedestroy_taskresult is discarded; given the comment that it "succeeds", it would be safer todebug_assert!(destroy_task(..).is_ok())(or at least log failures) to catch violations of that assumption early.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `task_create_from_image`, `TaskCreateError::CapTableExhausted` currently assumes `insert_root` only returns `CapsExhausted`; if that contract ever widens, this mapping becomes misleading, so consider either matching explicitly on `CapError::CapsExhausted` (and handling others separately) or adding a defensive assertion to keep the assumption checked.
- When rolling back after a failed `insert_root`, the `destroy_task` result is discarded; given the comment that it "succeeds", it would be safer to `debug_assert!(destroy_task(..).is_ok())` (or at least log failures) to catch violations of that assumption early.
## Individual Comments
### Comment 1
<location path="kernel/src/obj/task_loader.rs" line_range="944-948" />
<code_context>
+ // object back so no orphaned arena slot leaks (the handle was just
+ // created, so `destroy_task` succeeds; the recovered `Task` is dropped).
+ let cap = Capability::new(task_rights, CapObject::Task(task_handle));
+ match table.insert_root(cap) {
+ Ok(task_cap) => Ok(task_cap),
+ Err(e) => {
+ let _ = destroy_task(task_arena, task_handle);
+ Err(TaskCreateError::CapTableExhausted(e))
+ }
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Align `CapTableExhausted` semantics with the documented `CapsExhausted`-only expectation.
This branch currently wraps any `CapError` from `insert_root` in `CapTableExhausted`, which contradicts the documented “wraps `CapError::CapsExhausted`” contract and could mislead callers if other variants are ever returned. Either enforce the contract with `debug_assert!(matches!(e, CapError::CapsExhausted))`, or match on `CapError` explicitly and route non-`CapsExhausted` variants to a different `TaskCreateError` (or mark them unreachable) to keep the error taxonomy accurate.
Suggested implementation:
```rust
let cap = Capability::new(task_rights, CapObject::Task(task_handle));
match table.insert_root(cap) {
Ok(task_cap) => Ok(task_cap),
Err(e) => {
let _ = destroy_task(task_arena, task_handle);
Err(TaskCreateError::CapTableExhausted(e))
}
}
```
```rust
let cap = Capability::new(task_rights, CapObject::Task(task_handle));
match table.insert_root(cap) {
Ok(task_cap) => Ok(task_cap),
Err(e) => {
debug_assert!(matches!(e, CapError::CapsExhausted));
let _ = destroy_task(task_arena, task_handle);
Err(TaskCreateError::CapTableExhausted(e))
}
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
kernel/src/obj/task_loader.rs (1)
1088-1212: ⚡ Quick winAdd a dedicated
TaskArenaFulltest case.
TaskCreateError::TaskArenaFullis part of the public taxonomy but currently unpinned by tests. Adding one focused test will lock this branch against regressions.Proposed test addition
@@ #[test] fn task_create_from_image_rolls_back_task_on_cap_table_exhausted() { @@ } + + #[test] + fn task_create_from_image_returns_task_arena_full() { + let (mut table, as_cap, _mmu, _arena, _pmm, _b) = fixture(16); + let expected_ash = match table.lookup(as_cap).unwrap().object() { + CapObject::AddressSpace(h) => h, + other => panic!("not an AS cap: {other:?}"), + }; + let loaded = loaded_image_with(as_cap); + let mut task_arena = TaskArena::default(); + + // Fill task arena completely. + while create_task(&mut task_arena, Task::new(0, expected_ash)).is_ok() {} + + assert_eq!( + task_create_from_image(&loaded, &mut table, &mut task_arena, 1, CapRights::empty()), + Err(TaskCreateError::TaskArenaFull), + ); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@kernel/src/obj/task_loader.rs` around lines 1088 - 1212, Add a new unit test that asserts task_create_from_image returns TaskCreateError::TaskArenaFull when the TaskArena is exhausted: use fixture(...) to obtain a valid as_cap, create a TaskArena via TaskArena::default(), pre-fill the arena until it reports full (e.g. by repeatedly calling create_task(Task::new(...)) or otherwise allocating until allocation fails), then call task_create_from_image(&loaded, &mut table, &mut task_arena, id, CapRights::empty()) and assert it returns Err(TaskCreateError::TaskArenaFull(...)); reference task_create_from_image, TaskArena, TaskArena::default, create_task, Task::new and TaskCreateError::TaskArenaFull to locate where to add this test alongside the existing task_create_from_image_* tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@kernel/src/obj/task_loader.rs`:
- Around line 1088-1212: Add a new unit test that asserts task_create_from_image
returns TaskCreateError::TaskArenaFull when the TaskArena is exhausted: use
fixture(...) to obtain a valid as_cap, create a TaskArena via
TaskArena::default(), pre-fill the arena until it reports full (e.g. by
repeatedly calling create_task(Task::new(...)) or otherwise allocating until
allocation fails), then call task_create_from_image(&loaded, &mut table, &mut
task_arena, id, CapRights::empty()) and assert it returns
Err(TaskCreateError::TaskArenaFull(...)); reference task_create_from_image,
TaskArena, TaskArena::default, create_task, Task::new and
TaskCreateError::TaskArenaFull to locate where to add this test alongside the
existing task_create_from_image_* tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d44d68f-6f18-4f6f-9b8d-801bfab9e444
📒 Files selected for processing (4)
docs/analysis/tasks/phase-b/T-024-task-create-from-image.mddocs/roadmap/current.mdkernel/src/obj/mod.rskernel/src/obj/task_loader.rs
…ebug_assert, rollback/rights docs Folds in a second T-024 review. Each finding verified against the code: - Add the missing TaskArenaFull test (task_create_from_image_rejects_when_ task_arena_full): fill the TaskArena via create_task until full, then task_create_from_image returns Err(TaskCreateError::TaskArenaFull). Closes the one untested error branch. - CapTableExhausted assumed insert_root only returns CapsExhausted: that holds today (cap/table.rs), so a debug_assert!(matches!(e, CapsExhausted)) now pins the assumption — a future widening of insert_root's contract is caught in debug rather than silently mislabelled. - Rollback assertion: the reviewer's literal `debug_assert!(destroy_task(..). is_ok())` would be a RELEASE BUG — debug_assert! strips its argument in release, so destroy_task would not be called and the task slot would leak. Kept the unconditional `let _ = destroy_task(..)` rollback and documented why it is deliberately NOT debug_assert-wrapped (and that destroy_task cannot fail for a just-created handle: it deallocates a live generation-matched slot with no reachability check). Skipped (with reason): masking task_rights to cap-management bits. Not a security fix — in the kind-only Task-cap model, object-operation rights (SEND/RECV/NOTIFY/CONSOLE_WRITE) are INERT on a Task cap (no operation gates on them; a Task cap fails the CapKind check on, e.g., console_write regardless of CONSOLE_WRITE). Silent masking diverges from the cap model's convention (callers choose rights at mint; operations gate on the relevant right; no other mint masks), would surprise callers, and is future-fragile (it would silently drop a future Task-operation right). A uniform Task-cap rights policy, if ever wanted, is an ADR-level decision, not a per-site mask. Instead the fn doc now guides callers to pass only cap-management rights. Gates: cargo fmt; host + kernel clippy -D warnings; 348 host tests (+1); QEMU smoke byte-stable + fault-clean (dormant); Miri 0 UB. Refs: T-024 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
B6 step 3 (the deferred B4 §3 bridge):
task_create_from_imageturns aLoadedImageinto a runnableCapHandle{CapObject::Task(...)}. It mints, does not schedule — running the task is the later B6 wire-up.What lands
task_create_from_image(loaded, table, task_arena, id, task_rights) -> Result<CapHandle, TaskCreateError>(kernel/src/obj/task_loader.rs): (1) resolveloaded.as_cap→AddressSpaceHandle(lookup +CapKind::AddressSpacekind-check); (2)create_task(Task::new(id, ash)); (3)Capability::new(task_rights, CapObject::Task(handle))→insert_root. Rollback on a full cap-table (destroy_task→ no orphaned arena slot).TaskCreateError(InvalidAddressSpaceCap/TaskArenaFull/CapTableExhausted), re-exported fromcrate::obj.CapKind::AddressSpacemodel — no Task-operation right;task_rightsgovern only cap management. No new ADR (composes ADR-0029/0028/0016/0037). No newunsafe.as_cap; staleas_cap; rollback-on-cap-table-exhausted (slot-0 reuse proves no leak).Dormant
No BSP caller yet — the QEMU trace is byte-stable. Running a real EL0 task (via
add_user_taskwith the image's entry/stack) is the wire-up, gated behind the T-021 carry-forward gates (gate #1 per-task user-VA translation; gate #3 current-task cap table).Review
An adversarial multi-agent review (4 lenses × per-finding verification) found 6 findings, all Low/Nit, no code/security/correctness defect (borrow scoping, rollback, error mapping, kind-only model, and the tests verified correct). 4 fixed in
ad13d12(module-doc surface, a stale current.md line, a redundant rustdoc link target, anObjError-map clarifying comment); 2 deferred-with-tracking (the AS-handle/Task lifecycle coupling — a carry-forward precondition for the wire-up / a successor lifecycle ADR; and a future genericlookup_kindcap helper). Security-relevant (cap minting); T-024's formal code security review remains a DoD item before the wire-up runs a real EL0 task.Gates
cargo fmt· host + kernel clippy-D warnings· 347 host tests (+4) · kernel build · QEMU smoke byte-stable + fault-clean (dormant) ·cargo +nightly miri test --workspace --exclude tyrne-bsp-qemu-virt0 UB.🤖 Generated with Claude Code
Summary by Sourcery
Introduce a kernel helper that mints a runnable Task capability from a loaded userspace image, along with documentation and roadmap updates describing this new bridge in the Phase B plan.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit