Skip to content

RFC 0009: Rust std on vibix#847

Merged
dburkart merged 4 commits intomainfrom
rfc/0009-std-on-vibix
May 5, 2026
Merged

RFC 0009: Rust std on vibix#847
dburkart merged 4 commits intomainfrom
rfc/0009-std-on-vibix

Conversation

@dburkart
Copy link
Copy Markdown
Owner

@dburkart dburkart commented May 5, 2026

Summary

RFC 0009 proposes a hybrid staged approach to bring Rust's std library to vibix: a vibix_abi runtime crate for std's PAL, a vibix_libc C-ABI shim for libc/nix crate ecosystem compatibility, a custom x86_64-unknown-vibix target spec, and the ~20 missing syscalls needed to close the gap.

This PR contains a design document only — no kernel code changes. The RFC will be reviewed by the scoped archetype panel (security researcher, OS engineer, user space staff engineer, academic, plus toolchain-engineer), revised until all blocking findings are addressed, then merged. Implementation issues will be filed from the roadmap section after merge.

RFC sections

  • Abstract
  • Motivation
  • Background
  • Design (with sub-sections: Overview, Key Data Structures, Algorithms and Protocols, Kernel–Userspace Interface)
  • Security Considerations
  • Performance Considerations
  • Alternatives Considered
  • Open Questions
  • Implementation Roadmap

Test plan

  • Every scoped archetype reviewer posts a review comment.
  • All blocking findings are addressed within the scoped defense-cycle cap (3 cycles).
  • RFC frontmatter updated to status: Accepted before merge.

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 5, 2026

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 921faabb-d556-4b94-9c83-19c1cc92c9ba

📥 Commits

Reviewing files that changed from the base of the PR and between bcfd607 and abf18e0.

📒 Files selected for processing (1)
  • docs/RFC/0009-std-on-vibix.md

📝 Walkthrough

Walkthrough

Adds RFC 0009: a staged design to bring Rust's std to vibix via a vibix_abi no_std PAL, a vibix_libc C-ABI shim, a custom x86_64 target spec, phased syscall additions (including threading/futex), security/performance constraints, and an implementation roadmap.

Changes

RFC 0009: Rust std on vibix

Layer / File(s) Summary
RFC Header & Motivation
docs/RFC/0009-std-on-vibix.md
Adds RFC metadata, abstract, motivation, and prerequisites (TLS, mmap, VFS, auxv).
Background & Missing Syscalls
docs/RFC/0009-std-on-vibix.md
Describes Rust std PAL model, prior art, lists current syscall surface and missing syscalls required for std and threading.
Architecture & Crate Design
docs/RFC/0009-std-on-vibix.md
Defines hybrid staged architecture: vibix_abi (#![no_std]) as PAL ABI and vibix_libc C shim; introduces x86_64-unknown-vibix.json target spec.
Errno & TLS Handling
docs/RFC/0009-std-on-vibix.md
Specifies per-thread errno via Rust TLS (#[thread_local]) and exported __errno_location.
Phased Implementation Roadmap
docs/RFC/0009-std-on-vibix.md
Presents Phase 1–5 deliverables: minimal println!, fs/process wiring, threading/futex design, libc symbol surface, and dynamic linking phase.
Threading/Futex Kernel Constraints
docs/RFC/0009-std-on-vibix.md
Specifies clone flag whitelist with -EINVAL for others, fd-table locking guidance, and futex atomicity/lost-wakeup invariants.
Dynamic Linking Phase
docs/RFC/0009-std-on-vibix.md
Describes building vibix_libc as shared object and a minimal Rust ELF dynamic linker with specific relocation handling and RELRO.
Kernel–Userspace ABI & Syscall Tables
docs/RFC/0009-std-on-vibix.md
Lists syscall implementations per phase, kernel ABI expectations, and readv/writev iovec validation/atomicity notes.
Security & Performance Considerations
docs/RFC/0009-std-on-vibix.md
Documents pointer validation, getrandom semantics, clone-flag behavior, relocation hardening, syscall/TLS/allocator/futex performance notes.
Alternatives, Open Questions & Checklist
docs/RFC/0009-std-on-vibix.md
Lists considered alternatives, open questions, and a phase-by-phase implementation checklist.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant App as "Rust app (uses std)"
  participant PAL as "vibix_abi (std PAL)"
  participant Lib as "vibix_libc (C shim)"
  participant Kernel as "vibix Kernel (syscalls)"
  rect rgba(135,206,250,0.5)
    App->>PAL: call std API (println!, spawn, alloc)
    PAL->>Lib: use libc-compatible shims / errno pointer
    PAL->>Kernel: invoke syscalls (readv/writev/getrandom/clone/futex/clock_gettime...)
    Kernel-->>PAL: return result/errors (Linux ABI numbers)
    PAL->>App: translate results, set thread-local errno
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

Possibly related PRs

Poem

In my burrow I map syscalls and scheme,
Threads stitched with TLS, a std-filled dream.
A libc bridge and linker to bind,
Vibix and Rust, hand-in-hand combined,
I hop—std grows bright beneath moonbeam. 🐇✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and specifically describes the main change: an RFC document proposing a staged approach to bring Rust std to vibix.
Description check ✅ Passed The description is directly related to the changeset, detailing the RFC's purpose, structure, components, and review process for the Rust std on vibix implementation proposal.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch rfc/0009-std-on-vibix

Comment @coderabbitai help to get the list of available commands and usage tips.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@dburkart
Copy link
Copy Markdown
Owner Author

dburkart commented May 5, 2026

Review: Security Researcher

Summary: The RFC is architecturally sound — it layers userspace ABI crates at ring 3 on top of existing kernel syscall infrastructure with SMEP/SMAP enforcement and check_user_range validation. However, two of the newly proposed syscall families (writev and clone/futex) introduce attack surface that needs explicit specification of validation invariants before implementation.

Blocking findings:

  • [B1] writev iovec array is kernel-parsed user memory without specified double-fetch mitigation — writev(20) requires the kernel to read an array of iovec structs from userspace (base pointer + length pairs), then iterate over each to copy the payload. Each iovec::iov_base is itself a user pointer that must be validated. The RFC does not specify how the iovec array will be copied and validated. The correct pattern (copy the entire iovec array into a kernel buffer via copy_from_user first, then validate each element) must be mandated; without it a TOCTOU race lets a concurrent thread mutate an iovec entry between validation and use. Existing syscalls in syscall.rs only handle single flat buffers — this is a new shape of user-memory access that needs an explicit design note.

  • [B2] clone syscall flag validation unspecified — clone(56) accepts a flags word that controls which resources are shared (fd table, address space, signal handlers, etc.). Unvalidated or under-validated flags are a classic privilege-escalation vector (e.g., CLONE_FS sharing root directory, CLONE_FILES without proper refcounting). The RFC lists "shared fd table locking, per-thread TLS, shared signal disposition" but does not specify which CLONE_* flags will be accepted versus rejected. The implementation MUST whitelist only the flags needed for pthreads-style threading and reject all others with -EINVAL. This should be specified in the RFC.

Advisory findings:

  • [A1] getrandom fallback behavior on RDRAND/RDSEED failure — The existing csprng.rs has a deterministic_at_random_fallback path. If getrandom(318) is wired to this, userspace calling getrandom could silently receive non-random bytes on QEMU configurations without -cpu max. The syscall should return -ENOSYS rather than deterministic output, or the RFC should specify that the deterministic fallback is test-only and gated behind a build feature.

  • [A2] futex address validation — futex(202) operates on a user-supplied virtual address used as a wait-queue key. The kernel must ensure: (a) the address is in the user half (check_user_range), (b) it is 4-byte aligned (unaligned futex ops are undefined on x86 and a DoS vector via #AC exceptions), and (c) the physical backing is stable for the duration of the wait (i.e., the page cannot be munmapped while a waiter is queued). Worth specifying the locking protocol between futex wait-queues and munmap.

  • [A3] uname kernel info leak — uname(63) fills a user-provided utsname struct. If the kernel-side struct is stack-allocated and not fully zeroed before copy_to_user, padding bytes may leak kernel stack contents (KASLR pointers, old syscall args). Ensure the struct is zero-initialized before populating named fields.

  • [A4] copy_to_user/copy_from_user lack panic safety on page fault — The current byte-at-a-time volatile copy loop in uaccess.rs will take a #PF if the user page is not mapped or swapped out. The RFC should clarify that the #PF handler returns -EFAULT to the syscall rather than panicking, especially since new syscalls like writev and pread64/pwrite64 operate on larger buffers where partial-page mappings are more likely.

  • [A5] Dynamic linker in Phase 5 needs W^X enforcement — ld-vibix.so performing JUMP_SLOT relocations requires a writable-then-executable GOT/PLT region. The RFC mentions PT_GNU_RELRO but should explicitly require that the kernel enforces W^X: no page should be simultaneously writable and executable, and mprotect should reject PROT_WRITE|PROT_EXEC unless a specific process flag is set.

Verdict: CHANGES REQUESTED

@dburkart
Copy link
Copy Markdown
Owner Author

dburkart commented May 5, 2026

Review: Academic

Summary: The RFC presents a well-structured, phased approach to bringing Rust std to vibix that correctly synthesizes the Hermit ABI-crate and Redox libc-shim patterns. The design is grounded in established prior art and makes defensible engineering trade-offs. No published result contradicts the stated safety or liveness claims.

Blocking findings:
None.

Advisory findings:

  • [A1] Futex correctness gap — The RFC specifies futex WAIT/WAKE with "per-address wait queue" but omits the critical atomicity invariant: the kernel must check the futex word value and enqueue the calling thread atomically with respect to concurrent WAKE operations, or missed wakeups violate liveness. Drepper (2003), "Futexes Are Tricky," catalogs the subtle failure modes. The RFC should reference this and state that the value-check-and-enqueue is performed under the wait-queue lock, which is the standard solution (Linux kernel commit history, Franke et al. 2002, "Fuss, Futexes and Furwocks").
  • [A2] TLS model vs. dynamic linking tension — The target spec hardcodes "tls-model": "initial-exec", which allocates TLS statically at program startup. In Phase 5, dynamically loaded shared objects that declare #[thread_local] variables will fail at link time or silently corrupt memory because the static TLS block has no room for them. Drepper (2003), "ELF Handling For Thread-Local Storage," documents this constraint: initial-exec is incompatible with dlopen-loaded TLS. The Phase 5 design should either (a) switch the target to global-dynamic TLS for shared objects, (b) implement a static TLS surplus reservation (as glibc does via DTV_SURPLUS), or (c) explicitly document that dynamically loaded libraries cannot use thread-local storage.
  • [A3] Dynamic linker relocation coverage — Only three relocation types are listed (R_X86_64_RELATIVE, GLOB_DAT, JUMP_SLOT). Notably absent is R_X86_64_TPOFF64, which is required for initial-exec TLS relocations in shared objects. If vibix_libc.so itself uses #[thread_local] (e.g., the ERRNO cell), the linker must process TPOFF64 relocations to compute the correct %fs-relative offsets. Also missing: R_X86_64_64 (absolute symbol references) and R_X86_64_DTPMOD64/DTPOFF64 if general-dynamic TLS is ever introduced. The RFC should enumerate which relocation types are needed based on what vibix_libc.so actually emits.
  • [A4] Kernel safety claim is slightly overstated — The claim "vibix_libc runs at ring 3, cannot compromise kernel" is true as a hardware-isolation property but elides the standard confused-deputy concern: a buggy libc can construct adversarial syscall arguments that exercise kernel validation paths. The RFC correctly requires uaccess::check_user_range, but the claim as stated could be read as "no kernel bugs can be triggered from userspace." Consider softening to "cannot bypass hardware privilege separation" or adding a note that kernel-side validation remains the actual security boundary.
  • [A5] Missing formal/testing methodology for syscall compatibility — Reusing Linux syscall numbers and calling conventions creates an implicit compatibility contract. The RFC does not describe how conformance will be validated beyond pjdfstest. For the Linux-compatible syscall subset, consider referencing the Linux Test Project (LTP) syscall test suite or strace-based differential testing (as Asterinas uses) to establish a regression baseline. This is particularly important for clone(2) and futex(2), which have notoriously complex edge-case semantics.
  • [A6] No discussion of fork-safety in multithreaded context — Phase 2 introduces fork(2) and Phase 3 introduces clone(2)/threads, but the RFC does not discuss the well-known POSIX fork-in-multithreaded-process problem (only the calling thread survives in the child; mutexes held by other threads become permanently locked). This is a known source of deadlocks documented extensively in the POSIX rationale and in Chen et al. (2004). The RFC should state whether vibix will support pthread_atfork or recommend vfork/posix_spawn for the multithreaded case.

Verdict: LGTM

@dburkart
Copy link
Copy Markdown
Owner Author

dburkart commented May 5, 2026

Review: User Space Staff Engineer

Summary: Well-structured RFC with a sound phased approach. The hybrid vibix_abi + vibix_libc design correctly balances time-to-first-println against ecosystem compatibility. Two interface gaps need resolution before implementation can proceed safely.

Blocking findings:

  • [B1] readv missing from Phase 1 syscall list — writev(20) is listed but readv(19) is not. Rust std uses readv for Read::read_vectored (the default BufReader implementation calls it). Without readv, vectored reads silently fall back to a non-vectored path only if the PAL reports is_read_vectored() == false, but the libc/nix crate path calls readv directly and will get ENOSYS. Since vibix_libc targets nix 0.29 compatibility (Phase 4), and nix exposes readv/writev as a pair, omitting readv breaks the interface contract. Add readv(19) alongside writev(20) in Phase 1.
  • [B2] writev/readv atomicity requirement undocumented — POSIX requires that a single writev call be atomic with respect to other writes on the same file description (i.e., the iovec segments are written as one contiguous operation, not interleaved). The RFC lists writev with no notes on atomicity. The kernel implementation must hold the file position lock across all segments; if it iterates and calls write per-segment, concurrent writers can interleave. Document the atomicity guarantee in the Kernel-Userspace Interface table and note it as an implementation constraint.

Advisory findings:

  • [A1] clock_gettime minimum clock ID set — The RFC specifies CLOCK_MONOTONIC and CLOCK_REALTIME. Rust std on Linux also probes CLOCK_MONOTONIC_COARSE and CLOCK_MONOTONIC_RAW in some configurations. Recommend documenting that unsupported clock IDs return -EINVAL (which is POSIX-correct) and listing the exact IDs that will be supported so the PAL can avoid probing unsupported ones.
  • [A2] waitpid in vibix_libc vs wait4 in kernel — The vibix_libc symbol list includes waitpid but the kernel only exposes wait4(61). This is fine if waitpid is implemented as wait4(pid, status, options, NULL) in the shim, which is the standard approach. Worth a one-line note in the vibix_libc design to make this delegation explicit.
  • [A3] getrandom flags — The security section mentions blocking or returning -EAGAIN, which implies GRND_NONBLOCK flag support. The interface table should specify which flags are supported (GRND_NONBLOCK, GRND_RANDOM, GRND_INSECURE) and what the default blocking behavior is. Rust std calls getrandom(buf, len, GRND_NONBLOCK) and falls back on EAGAIN, so GRND_NONBLOCK is required at minimum.
  • [A4] O_CLOEXEC behavior under clone with shared fd table — Phase 3 introduces clone with CLONE_FILES (shared fd table). The interaction between O_CLOEXEC and execve in a shared-fd-table process group is subtle: execve must close O_CLOEXEC fds even when the fd table is shared with other threads (and those threads must see the fd disappear). This is worth a note in the Security Considerations section for Phase 3.
  • [A5] Phase 2 process exit status encoding — The existing wait4 encodes wstatus = (exit_code & 0xFF) << 8, which matches the W_EXITSTATUS macro for normal exits. The RFC should note that signal-death encoding (wstatus = signo & 0x7F) is also needed for Command::output() to correctly distinguish normal exit from signal termination. This may already be handled, but it is not documented in the RFC.

Verdict: CHANGES REQUESTED

@dburkart
Copy link
Copy Markdown
Owner Author

dburkart commented May 5, 2026

Review: Toolchain Engineer

Summary: The target spec is mostly sound for the current static-linking phase, but contains one contradiction that will cause silent ABI mismatches (PIE flags with static relocation model) and a linker-flavor/linker pairing that will fail at link time. Several Phase 5 design points need attention before dynamic linking lands.

Blocking findings:

  • [B1] position-independent-executables + static-position-independent-executables vs relocation-model: "static" — These are contradictory. When position-independent-executables is true, rustc/LLVM emits a PIE executable (ET_DYN with a base of 0, GOT-relative data references, PLT-mediated calls). But relocation-model: "static" tells the code generator to use absolute addressing with no GOT/PLT. The result is a binary that the linker marks as PIE (so the kernel loader may apply ASLR or rebase) but whose code uses hardcoded absolute addresses that will break if loaded anywhere other than the link-time base. For the current static-only phase, set all three to the non-PIE configuration: "position-independent-executables": false, "static-position-independent-executables": false, "relocation-model": "static". The existing userspace linker scripts already hardcode a load address at 0x400000, confirming PIE is not in play today.
  • [B2] linker-flavor: "gnu-lld-cc" with linker: "rust-lld" — The gnu-lld-cc flavor tells rustc to invoke the linker as a C compiler driver (i.e., pass -Wl, prefixed args). But rust-lld is a raw linker, not a compiler driver. Either use "linker-flavor": "gnu-lld" with "linker": "rust-lld", or use "linker-flavor": "gnu-lld-cc" with a GCC/Clang driver as the linker. With the current pairing, every pre-link-args entry will be passed with -Wl, wrapping that rust-lld does not understand, and the link will fail.

Advisory findings:

  • [A1] disable-redzone: true is unnecessary for userspace — The red zone is a legitimate System V ABI optimization for leaf functions in user mode. Only the kernel needs it disabled (because interrupts clobber below RSP). The existing kernel target already passes -C no-redzone=yes via .cargo/config.toml. Keeping it in the userspace target spec wastes 128 bytes of stack per leaf call and pessimizes code generation for no benefit.
  • [A2] -Z build-std with forked std — -Z build-std rebuilds std from the rust-src rustup component, not from a local checkout. To use a forked std you need to either (a) point the RUST_SRC_PATH env var at your fork, (b) vendor the modified sysroot source under a path and use --sysroot, or (c) use [patch.crates-io] in the workspace Cargo.toml to redirect the std dependency to a local path. The RFC should specify which mechanism is used, because the naive -Z build-std invocation will silently ignore the fork and build upstream std.
  • [A3] Phase 5 will require a second target spec (or target-level overrides) for shared objects — relocation-model: "static" cannot produce correct .so files. Shared libraries require "relocation-model": "pic". When Phase 5 lands, vibix_libc.so and ld-vibix.so will need to be compiled with a PIC-capable target variant. Plan for this now (e.g., x86_64-unknown-vibix-pic.json or a cargo profile that overrides relocation-model).
  • [A4] tls-model: "initial-exec" is correct for static linking and Phase 5 eager binding — initial-exec uses a GOT entry resolved at load time, which is compatible with the static TLS block the kernel already allocates from PT_TLS. However, once dlopen() support arrives (beyond Phase 5), initial-exec TLS will not work for dynamically loaded libraries (it requires a fixed-offset TLS slot known at link time). This is fine to defer, but worth documenting as a known Phase 5+ limitation.
  • [A5] crt-objects-fallback: "false" means no crt1.o/crti.o/crtn.o — This is correct for the current _start-entry no_std binaries. Once the RFC ships a forked std with a proper PAL entry point, you will need either a minimal crt1.o (calling __libc_start_main or equivalent) or a Rust-side #[lang = "start"] shim. The target spec value is fine for now but will need revisiting alongside the std PAL work.
  • [A6] __errno_location with #[thread_local] + Cell — The Cell<i32> wrapping is correct for single-threaded access within a thread, and initial-exec TLS ensures the GOT slot is resolved before any code runs. One subtlety: ensure ERRNO has #[used] or is otherwise referenced so the linker does not dead-strip it from the .tdata/.tbss section. The #[no_mangle] __errno_location export should anchor it, but verify in the final link map.
  • [A7] Missing R_X86_64_64 and R_X86_64_PC32 in the dynamic linker relocation list — The Phase 5 dynamic linker lists only RELATIVE, GLOB_DAT, and JUMP_SLOT. For Rust-generated code (especially with initial-exec TLS), you will likely also encounter R_X86_64_TPOFF64 (TLS offset relocs) and potentially R_X86_64_64 in data sections. Audit the actual relocation types emitted by a test .so build before finalizing the relocation handler.

Verdict: CHANGES REQUESTED

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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.

Inline comments:
In `@docs/RFC/0009-std-on-vibix.md`:
- Line 168: Several fenced code blocks in the RFC are unlabeled (three
occurrences flagged) which triggers MD040; locate each triple-backtick fence
with no language identifier in the document and replace the opening fence (```)
with a language-specific fence such as ```text, ```json, or ```rust as
appropriate for the snippet content (e.g., plain output -> text, JSON payloads
-> json, code samples -> rust), ensuring every fenced code block has an explicit
language tag.
- Line 230: The example target JSON contains the field "crt-objects-fallback"
set as a string; change it from the quoted string "false" to a boolean false
(unquoted) so the JSON uses a proper boolean value for crt-objects-fallback and
will parse correctly when copied.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ada1b56e-1d59-4278-a05f-4fb6a1fd0d86

📥 Commits

Reviewing files that changed from the base of the PR and between 630cf1c and 735a3a9.

📒 Files selected for processing (1)
  • docs/RFC/0009-std-on-vibix.md

ABI-crate model (fast path to working std) with a Redox-style libc
shim (ecosystem compatibility for `nix`/`libc` crates):

```
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add language identifiers to fenced code blocks (MD040).

Line 168, Line 247, and Line 264 use unlabeled fences. Add explicit languages (text, json, rust, etc.) to satisfy markdownlint and keep renderers/tooling consistent.

Also applies to: 247-247, 264-264

🧰 Tools
🪛 markdownlint-cli2 (0.22.1)

[warning] 168-168: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 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 `@docs/RFC/0009-std-on-vibix.md` at line 168, Several fenced code blocks in the
RFC are unlabeled (three occurrences flagged) which triggers MD040; locate each
triple-backtick fence with no language identifier in the document and replace
the opening fence (```) with a language-specific fence such as ```text, ```json,
or ```rust as appropriate for the snippet content (e.g., plain output -> text,
JSON payloads -> json, code samples -> rust), ensuring every fenced code block
has an explicit language tag.

"relocation-model": "static",
"executables": true,
"max-atomic-width": 64,
"crt-objects-fallback": "false"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify whether the RFC contains string-typed booleans for target JSON fields.
# Expected: no matches for quoted true/false values in the JSON snippet.

rg -n '"(crt-objects-fallback|has-thread-local|position-independent-executables|static-position-independent-executables|executables)"\s*:\s*"(true|false)"' docs/RFC/0009-std-on-vibix.md

Repository: dburkart/vibix

Length of output: 96


Fix boolean value in target JSON example on line 230.

The crt-objects-fallback field should use a boolean value, not a string. Line 230 currently has "crt-objects-fallback": "false", which will cause target parsing issues if copied directly.

Fix
-  "crt-objects-fallback": "false"
+  "crt-objects-fallback": false
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
"crt-objects-fallback": "false"
"crt-objects-fallback": false
🤖 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 `@docs/RFC/0009-std-on-vibix.md` at line 230, The example target JSON contains
the field "crt-objects-fallback" set as a string; change it from the quoted
string "false" to a boolean false (unquoted) so the JSON uses a proper boolean
value for crt-objects-fallback and will parse correctly when copied.

@dburkart
Copy link
Copy Markdown
Owner Author

dburkart commented May 5, 2026

Review: OS Engineer

Summary: The RFC is well-structured and builds on solid existing infrastructure (per-task kernel stacks, Arc-wrapped fd tables, CoW fork). However, the clone(CLONE_FILES) path introduces a shared spin::Mutex<FileDescTable> between threads that can deadlock on the single-CPU kernel, and the futex design lacks enough detail to evaluate correctness of the atomic check-and-park sequence.

Blocking findings:

  • [B1] spin::Mutex on shared fd table is a single-CPU deadlock — Today each fork child gets its own Arc<Mutex<FileDescTable>> so the spin::Mutex is uncontended. clone(CLONE_FILES) makes sibling threads share the same Arc<spin::Mutex<FileDescTable>>. Interrupts are re-enabled at the top of syscall_dispatch (sti), so the timer ISR can preempt a thread inside the fd-table critical section (e.g. current_fd_table().lock().get(fd) in the READ/WRITE paths). If the scheduler switches to a co-thread that also calls into a fd-table path, the second thread spins forever on the spin::Mutex while the holder is parked in the ready queue and can never be scheduled. On a single-CPU system this is a hard deadlock, not just priority inversion. Fix: either (a) convert fd_table to IrqLock so the critical section is non-preemptible, or (b) convert to BlockingMutex so the contending thread parks instead of spinning. Option (a) is preferable given the short critical sections.

  • [B2] futex check-and-park atomicity unspecifiedFUTEX_WAIT must atomically verify *uaddr == expected and enqueue the caller before any concurrent FUTEX_WAKE can observe the waiter. The RFC proposes "per-address wait queue" but does not specify how the value check and the enqueue are made atomic. If the kernel reads the futex word, finds it equal to expected, then a preemption (or yield) fires before the task is enqueued, a concurrent FUTEX_WAKE will find zero waiters and the wakeup is lost. The implementation must hold whatever lock protects the per-address bucket across both the copy_from_user value check and the waitqueue enqueue — analogous to Linux's hb->lock in futex_wait_queue(). Please specify this in the RFC so reviewers can evaluate the design before implementation.

Advisory findings:

  • [A1] clone(CLONE_VM) vs. fork() address-space contractfork_current_task calls fork_address_space(), which takes a write lock on the parent's AddressSpace and makes pages CoW. For clone(CLONE_VM) the child must instead Arc::clone the parent's address_space (sharing, not copying). The RFC should clarify which code path clone() will take and whether fork_current_task will be extended or a new clone_current_task will be introduced. Mixing CoW remap into a shared-address-space thread would corrupt the sibling's mappings.

  • [A2] Global SYSCALL_SCRATCH_RSP / SYSCALL_KERNEL_RSP are per-CPU hazards — These are safe today because SFMASK clears IF during the SYSCALL entry preamble and the kernel is single-CPU. The RFC's Phase 3 stays single-CPU, so this is fine now, but the target spec and ABI crate are foundational — once SMP lands, every clone(CLONE_THREAD) thread could enter SYSCALL simultaneously on different CPUs, and the global scratch slot becomes a data race (UB). Consider adding a // SAFETY: single-CPU invariant annotation to both statics now, and flagging per-CPU storage as a hard prerequisite for SMP in the RFC.

  • [A3] errno via #[thread_local] Cell<i32> requires clone to allocate fresh TLS — The RFC's clone(CLONE_SETTLS) flag handles this, but the current TLS deep-copy in fork_current_task copies the parent's TLS block verbatim into the child. For threads (shared address space), the child needs a separate TLS allocation at a new VA with a fresh fs_base, not a deep copy of the parent's region. The arch_prctl(ARCH_SET_FS) and tls_region_start/pages bookkeeping on the Task struct is ready for this, but the allocation of a new TLS block (as opposed to a deep copy) is not yet wired. The RFC should note this as a distinct subtask.

  • [A4] Lock ordering with shared fd tables across credential changescurrent_fd_table() acquires SCHED, clones the Arc, drops SCHED, then the caller locks the fd-table mutex. replace_current_credentials() acquires SCHED, then the per-task credentials rwlock. If a future path ever needs to hold both the fd-table lock and the credential lock (e.g. fchown checking credentials while holding an open-file reference), the ordering between fd-table mutex and credentials rwlock must be documented. Not a bug today, but shared fd tables increase the surface area for lock-order violations.

  • [A5] SYSCALL_RESTART_PENDING is a global atomic flag — With clone(CLONE_THREAD), two threads could independently trigger restart paths. The flag is consumed in the per-task SYSRET asm sequence, but it is a single global — a restart on thread A could be consumed by thread B's SYSRET on context switch. On single-CPU this is serialized by the scheduler, but the flag's per-syscall-entry lifetime assumption (set in signal delivery, consumed in the same task's return path) should be explicitly validated against the preemption window between sti and sysretq. Consider making it per-task state on the kernel stack frame.

Verdict: CHANGES REQUESTED

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@dburkart
Copy link
Copy Markdown
Owner Author

dburkart commented May 5, 2026

Defense Cycle 1 — Addressing Blocking Findings

Security Researcher

[B1] writev iovec TOCTOU — Addressed. Added explicit "readv/writev implementation constraints" section specifying: (1) the iovec array must be copy_from_user'd into a kernel-side buffer before validating any iov_base/iov_len pair, preventing TOCTOU races from concurrent userspace mutation; (2) POSIX atomicity requires holding the file position lock across all segments.

[B2] clone flag whitelist — Addressed. Added explicit whitelist of accepted CLONE_* flags (CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND | CLONE_THREAD | CLONE_SETTLS | CLONE_PARENT_SETTID | CLONE_CHILD_CLEARTID | CLONE_SYSVSEM). All other flags rejected with -EINVAL. Updated both the Design section and Security Considerations.

OS Engineer

[B1] spin::Mutex fd table deadlock — Addressed. Specified that FileDescTable's lock must be converted to IrqLock (disable interrupts for the critical section) when CLONE_FILES makes threads share the same table. This prevents the preemption-under-spinlock deadlock on single-CPU. Updated Design §Phase 3 and Security Considerations.

[B2] futex check-and-park atomicity — Addressed. Added explicit atomicity invariant: the per-bucket lock must be held across both the copy_from_user value check and the waitqueue enqueue, analogous to Linux's hb->lock. Specified the bucket lock should be IrqLock for the same preemption safety reason.

User Space Staff Engineer

[B1] readv missing — Addressed. Added readv(19) alongside writev(20) in the Phase 1 syscall table, the "Missing for std" table, and the Implementation Roadmap.

[B2] writev/readv atomicity — Addressed. Added explicit atomicity constraint in the new "readv/writev implementation constraints" section (same fix as Security Researcher B1 above).

Toolchain Engineer

[B1] PIE vs static contradiction — Addressed. Changed target spec to "position-independent-executables": false, "static-position-independent-executables": false to match "relocation-model": "static". Added key-choices note explaining non-PIE matches the fixed load address (0x400000).

[B2] linker-flavor mismatch — Addressed. Changed "linker-flavor": "gnu-lld-cc" to "linker-flavor": "gnu-lld" and updated pre-link-args key accordingly. Added key-choices note explaining raw linker invocation avoids -Wl, wrapping.

@dburkart
Copy link
Copy Markdown
Owner Author

dburkart commented May 5, 2026

Review: Security Researcher

Summary: Defense cycle 1 satisfactorily addressed both blocking findings. The iovec TOCTOU mitigation (copy-then-validate) and POSIX atomicity constraint are clearly specified, and the clone flag whitelist is explicit and documented in both the design section and Security Considerations.

Blocking findings:

  • None.

Advisory findings:

  • [A1] iovec count cap. The RFC does not specify a maximum iovcnt value. Linux caps at UIO_MAXIOV (1024) and returns -EINVAL above that. Without a cap, a malicious userspace could force a large kernel-side allocation via the copy-from-user buffer. Recommend documenting a UIO_MAXIOV limit (1024 matches Linux) and rejecting larger counts with -EINVAL.
  • [A2] futex bucket lock granularity. The RFC correctly requires holding the per-bucket lock across the value check and enqueue (good), but does not specify the bucket count or hash function. A single global lock would serialize all futex operations across unrelated addresses. Recommend documenting that the implementation should use a hash table of at least 256 buckets keyed on the physical (or virtual) address, consistent with the Linux approach.
  • [A3] getrandom blocking vs. early boot. The RFC says getrandom should "block or return -EAGAIN if entropy is insufficient" but does not specify which behavior applies for flags=0 vs. GRND_NONBLOCK. Linux blocks on flags=0 and returns -EAGAIN on GRND_NONBLOCK. Recommend aligning with Linux semantics explicitly so the std HashMap seeding path (which passes flags=0) does not silently get weak entropy.

Verdict: LGTM

@dburkart
Copy link
Copy Markdown
Owner Author

dburkart commented May 5, 2026

Review: Toolchain Engineer

Summary: Both original blocking findings (B1: PIE/static contradiction, B2: linker-flavor mismatch) have been correctly resolved. The target spec is now internally consistent and will produce working builds.

Blocking findings:

  • None.

Advisory findings:

  • [A1] "crt-objects-fallback": "false" — the value is a string "false", not a JSON boolean. Rustc currently accepts both, but the canonical form in upstream target specs is the boolean false. Low-risk; worth fixing when the JSON is first committed to avoid a future deprecation warning.
  • [A2] The "executables": true key was deprecated in rustc nightly (Represent Result<usize, Box<T>> as ScalarPair(i64, ptr) rust-lang/rust#121668, March 2024) and is now a no-op. Removing it will silence a compiler warning. Non-blocking since the warning is harmless.

Verdict: LGTM

@dburkart
Copy link
Copy Markdown
Owner Author

dburkart commented May 5, 2026

Review: User Space Staff Engineer

Summary: Both blocking findings from defense cycle 1 have been addressed. readv(19) now appears alongside writev(20) in the Phase 1 syscall table, the "Missing for std" table, and the Implementation Roadmap. The new "readv/writev implementation constraints" section explicitly documents the POSIX atomicity requirement (hold file position lock across all iovec segments) and the TOCTOU mitigation (copy_from_user the entire iovec array before validation).

Blocking findings:

  • None.

Advisory findings:

  • [A1] The readv/writev atomicity section correctly specifies holding the file position lock across segments, but does not mention the UIO_MAXIOV (1024) limit on iovcnt that Linux enforces. Exceeding this should return EINVAL. Worth specifying to avoid a divergence footgun when porting software that relies on this limit.
  • [A2] The vibix_libc stdio.rs module description lists writev and read but not readv. Minor alignment issue with the updated syscall table — consider adding readv to that file list for consistency.

Verdict: LGTM

@dburkart
Copy link
Copy Markdown
Owner Author

dburkart commented May 5, 2026

Review: OS Engineer

Summary: Both original blocking findings (B1: spin::Mutex deadlock under CLONE_FILES, B2: futex check-and-park atomicity) have been addressed with correct and well-specified mitigations. The RFC is ready to proceed.

Blocking findings:
None.

Advisory findings:

  • [A1] IrqLock granularity for fd table — The RFC correctly mandates IrqLock for the shared fd table, but the conversion should be scoped carefully. If IrqLock is applied unconditionally (even for single-threaded processes with their own fd table), every file descriptor operation pays the interrupt-disable cost unnecessarily. Consider keeping spin::Mutex for unshared fd tables and only promoting to IrqLock when CLONE_FILES actually creates sharing, or document that the uniform IrqLock cost is accepted for simplicity. Either choice is fine, but the implementation should be intentional about it.
  • [A2] Futex bucket count and hash function — The RFC specifies per-bucket IrqLock but does not specify the number of buckets or the hash function mapping addresses to buckets. A poor choice (e.g., too few buckets or a trivial hash) creates false contention between unrelated futexes. Linux uses a per-node array of 256 buckets with jhash2 over the (key, page, offset) tuple. The implementation should document its bucket sizing rationale, even if a simpler scheme (e.g., 64 buckets, address-based hash) is appropriate for vibix at this stage.
  • [A3] IrqLock hold time during copy_from_user in futex path — The futex bucket lock is held across copy_from_user, which reads from user memory. If the user page is not present (demand-paged or swapped), the resulting page fault handler runs with interrupts disabled. Verify that the page fault path does not need to acquire any lock that the futex path might also hold, and that the fault handler can complete with IRQs off. If not, the copy_from_user should be split: copy the value into a kernel-side variable before acquiring the bucket lock, then re-verify under the lock (double-check pattern, as Linux does with get_futex_value_locked).

Verdict: LGTM

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants