Development #146
Development #146
Conversation
WalkthroughAdds a Rust-backed kernel heap: new Rust crate and build integration, C header bridge for allocator APIs, a full Rust allocator implementation with FFI to kernel services, links the Rust static library into the kernel, and exposes a shell command to invoke the Rust allocator. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Shell as Shell (C)
participant RustHeap as Rust Heap (C ABI)
participant VMem as VMem (C)
User->>Shell: rsalloc <size>
Shell->>RustHeap: rust_kmalloc(size)
alt fast-cache hit
RustHeap-->>Shell: user_ptr
Shell-->>User: print allocated ptr
else slow-path
RustHeap->>VMem: VMemAlloc(aligned_size)
VMem-->>RustHeap: ptr or null
alt success
RustHeap-->>Shell: user_ptr
Shell-->>User: print allocated ptr
else failure
RustHeap-->>Shell: null
Shell-->>User: print allocation failure
end
end
sequenceDiagram
autonumber
participant C as Kernel (C)
participant RustHeap as Rust Heap (C ABI)
C->>RustHeap: rust_kfree(ptr)
alt valid block
RustHeap->>RustHeap: mark free / maybe stash in cache
RustHeap-->>C: return
else invalid ptr/magic
RustHeap->>C: PrintKernelError("invalid free")
RustHeap-->>C: return
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
kernel/etc/Shell.c (1)
371-386: RsAllocHandler implementation looks correct but has a type safety concern.The function correctly validates input and calls the Rust allocator. However, there's a potential issue with type conversion:
atoireturnsint(typically 32-bit)rust_kmallocexpectssize_t(64-bit on x86_64)- Casting
inttouint32_tthen tosize_tis safe but limits allocations to 4GBFor consistency with the Rust heap's
usizeparameter, consider using a safer conversion:static void RsAllocHandler(const char * args) { char* size_str = GetArg(args, 1); if (!size_str) { PrintKernel("Usage: alloc <size>\n"); KernelFree(size_str); return; } - int size = atoi(size_str); - if (size <= 0) { + // Use unsigned long to support larger allocations + char* endptr; + unsigned long size = strtoul(size_str, &endptr, 10); + if (size == 0 || *endptr != '\0') { PrintKernel("Usage: alloc <size>\n"); KernelFree(size_str); return; } - if (!rust_kmalloc((uint32_t)size)) PrintKernelErrorF("Allocation for %d bytes failed\n", size); + if (!rust_kmalloc((size_t)size)) PrintKernelErrorF("Allocation for %lu bytes failed\n", size); KernelFree(size_str); }Note: This assumes
strtoulis available in your kernel environment.mm/rust/CMakeLists.txt (1)
16-18: Consider supporting different build types.The library path is hardcoded to
release, but CMake'sCMAKE_BUILD_TYPEmight be set toDebug. This could cause build failures or use the wrong library version.Apply this diff to respect the CMake build type:
# Rust library target set(RUST_LIB_NAME "voidframe_mm") -set(RUST_LIB_PATH "${CMAKE_CURRENT_SOURCE_DIR}/target/${RUST_TARGET}/release/lib${RUST_LIB_NAME}.a") + +# Determine Rust build profile based on CMake build type +if(CMAKE_BUILD_TYPE MATCHES "Debug") + set(RUST_BUILD_PROFILE "debug") + set(CARGO_BUILD_FLAGS "") +else() + set(RUST_BUILD_PROFILE "release") + set(CARGO_BUILD_FLAGS "--release") +endif() + +set(RUST_LIB_PATH "${CMAKE_CURRENT_SOURCE_DIR}/target/${RUST_TARGET}/${RUST_BUILD_PROFILE}/lib${RUST_LIB_NAME}.a")Then update the cargo command on line 23:
- COMMAND ${CARGO_EXECUTABLE} build --release --target ${RUST_TARGET} + COMMAND ${CARGO_EXECUTABLE} build ${CARGO_BUILD_FLAGS} --target ${RUST_TARGET}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
mm/rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
CMakeLists.txt(3 hunks)kernel/etc/Shell.c(3 hunks)mm/KernelHeapRust.h(1 hunks)mm/rust/.cargo/config.toml(1 hunks)mm/rust/CMakeLists.txt(1 hunks)mm/rust/Cargo.toml(1 hunks)mm/rust/src/heap.rs(1 hunks)mm/rust/src/lib.rs(1 hunks)mm/rust/src/vmem_ffi.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
mm/rust/src/lib.rs (1)
mm/rust/src/heap.rs (7)
rust_kmalloc(161-214)rust_kfree(217-247)rust_krealloc(250-277)rust_kcalloc(280-287)core(93-93)core(97-97)core(116-116)
mm/KernelHeapRust.h (1)
mm/rust/src/heap.rs (4)
rust_kmalloc(161-214)rust_kfree(217-247)rust_krealloc(250-277)rust_kcalloc(280-287)
kernel/etc/Shell.c (5)
mm/KernelHeap.c (2)
KernelMemoryAlloc(357-433)KernelFree(480-525)kernel/etc/Console.c (2)
PrintKernelErrorF(268-275)PrintKernel(171-189)mm/rust/src/vmem_ffi.rs (1)
PrintKernel(12-12)include/stdlib.h (1)
atoi(18-44)mm/rust/src/heap.rs (1)
rust_kmalloc(161-214)
mm/rust/src/heap.rs (1)
mm/rust/src/vmem_ffi.rs (3)
VMemAlloc(3-3)VMemFree(4-4)PrintKernelError(13-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (7)
mm/rust/.cargo/config.toml (1)
1-9: LGTM!The Cargo configuration correctly targets bare-metal kernel development with appropriate compiler flags. Disabling floating-point and SIMD features (SSE, AVX, etc.) is standard practice for kernel code to avoid the overhead of saving/restoring CPU state.
CMakeLists.txt (2)
194-197: LGTM!The Rust memory management subdirectory integration is correctly configured.
256-257: No conflicts detected between C and Rust heap implementations
C allocator symbols (KernelMemoryAlloc,KernelReallocate,KernelFree,KernelCalloc) differ from Rust externs (rust_kmalloc,rust_krealloc, etc.), andkmalloc/kfreemacros inKernelHeapRust.honly redirect generic calls in Rust-linked code—no overlap or unintended routing.kernel/etc/Shell.c (1)
367-369: Good improvement: AllocHandler now checks for allocation failure.The updated code properly handles allocation failure by checking the return value and printing an error message.
mm/rust/Cargo.toml (1)
1-15: LGTM!The Cargo configuration is appropriate for a kernel static library:
- Static library output for C linking
- spin 0.9 provides lock-free synchronization suitable for kernel contexts
- Release profile optimizations (LTO, panic=abort) are standard for kernel code
Based on learnings.
mm/rust/src/vmem_ffi.rs (1)
1-16: LGTM!The FFI declarations are correctly structured for interfacing with C kernel functions. The use of
extern "C"ensures proper ABI compatibility, and the types are appropriate for kernel memory management and console operations.mm/rust/CMakeLists.txt (1)
20-30: LGTM with minor observation.The custom command correctly builds the Rust library with proper dependency tracking. The
DEPENDSclause ensures rebuilds when source files change.Minor note: Consider adding
.cargo/config.tomlto the dependencies list since it affects the build:DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/Cargo.toml ${CMAKE_CURRENT_SOURCE_DIR}/src/lib.rs ${CMAKE_CURRENT_SOURCE_DIR}/src/heap.rs ${CMAKE_CURRENT_SOURCE_DIR}/src/vmem_ffi.rs + ${CMAKE_CURRENT_SOURCE_DIR}/.cargo/config.toml
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
mm/rust/src/lib.rs (1)
19-27: Restore panic details when bridging toPanic.
panicstill dropsPanicInfo’s message/location and always passes the same literal, leaving allocator panics opaque. Please marshal the actual panic payload into a buffer (even truncated) before callingPanic.mm/rust/src/heap.rs (2)
55-62: Move theSend/Syncimpls belowHeapState’s definition.
Implementing traits on a type before it’s defined fails to compile—HeapStateisn’t in scope yet when these impls are emitted. Define the struct first, then add the unsafe impls.
272-373: Hold the heap lock while claiming a free block.
find_free_blockgrabs the mutex, returns a raw pointer, then drops the lock;rust_kmallocmarks the block as allocated only after the guard is gone. A competing thread can grab the same block, leading to double-allocation/corruption. Keep the mutex held through selection and state updates (e.g., operate on a&mut HeapStateor return while the guard lives).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/main.yaml(1 hunks)kernel/etc/Shell.c(3 hunks)mm/KernelHeapRust.h(1 hunks)mm/rust/src/heap.rs(1 hunks)mm/rust/src/lib.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
mm/rust/src/lib.rs (1)
mm/rust/src/heap.rs (13)
rust_kmalloc(315-383)rust_kfree(397-449)rust_krealloc(452-483)rust_kcalloc(486-497)core(146-146)core(150-150)core(155-155)core(170-170)core(220-220)core(239-239)core(248-248)core(253-253)core(257-257)
kernel/etc/Shell.c (4)
mm/KernelHeap.c (2)
KernelMemoryAlloc(357-433)KernelFree(480-525)kernel/etc/Console.c (2)
PrintKernelErrorF(268-275)PrintKernel(171-189)include/stdlib.h (1)
atoi(18-44)mm/rust/src/heap.rs (1)
rust_kmalloc(315-383)
mm/KernelHeapRust.h (1)
mm/rust/src/heap.rs (6)
rust_kmalloc(315-383)rust_kfree(397-449)rust_krealloc(452-483)rust_kcalloc(486-497)rust_heap_get_stats(500-526)rust_heap_validate(529-544)
| cmake ninja-build \ | ||
| curl --proto '=https' --tlsv1.3 https://sh.rustup.rs -sSf | sh |
There was a problem hiding this comment.
Remove the trailing backslash and make the rustup install non-interactive
Line 32 keeps the line-continuation \, so the shell treats the subsequent curl … | sh as part of the apt-get install command (and even pipes apt output into sh). The step fails before rustup ever runs. Drop the trailing backslash so curl executes separately, and make the rustup install non-interactive while exporting Cargo to the PATH.
- cmake ninja-build \
- curl --proto '=https' --tlsv1.3 https://sh.rustup.rs -sSf | sh
+ cmake \
+ ninja-build
+ curl --proto '=https' --tlsv1.3 https://sh.rustup.rs -sSf | sh -s -- -y --profile minimal --default-toolchain stable
+ source "$HOME/.cargo/env"
+ echo "$HOME/.cargo/bin" >> "$GITHUB_PATH"📝 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.
| cmake ninja-build \ | |
| curl --proto '=https' --tlsv1.3 https://sh.rustup.rs -sSf | sh | |
| cmake \ | |
| ninja-build | |
| curl --proto '=https' --tlsv1.3 https://sh.rustup.rs -sSf | sh -s -- -y --profile minimal --default-toolchain stable | |
| source "$HOME/.cargo/env" | |
| echo "$HOME/.cargo/bin" >> "$GITHUB_PATH" |
🤖 Prompt for AI Agents
In .github/workflows/main.yaml around lines 32-33, remove the trailing backslash
after the apt install line so the curl pipeline is not treated as part of apt,
run rustup non-interactively by invoking the installer with -s -- -y (curl
--proto '=https' --tlsv1.3 https://sh.rustup.rs -sSf | sh -s -- -y), and after
installation ensure Cargo is on PATH by sourcing $HOME/.cargo/env or adding
$HOME/.cargo/bin to the job PATH so subsequent steps can use cargo.
| if let Some(size_class) = get_size_class(aligned_size) { | ||
| let mut heap = HEAP.lock(); | ||
| let cache = &mut heap.fast_caches[size_class]; | ||
| if !cache.free_list.is_null() { | ||
| let block = cache.free_list; | ||
| if !(*block).validate() { | ||
| PrintKernelError(b"[HEAP] Corrupted block in cache\0".as_ptr()); | ||
| return ptr::null_mut(); | ||
| } | ||
|
|
||
| cache.free_list = (*block).cache_next; | ||
| cache.count -= 1; | ||
| cache.hits += 1; | ||
|
|
||
| (*block).cache_next = ptr::null_mut(); | ||
| (*block).in_cache = 0; | ||
| (*block).is_free = 0; | ||
| (*block).magic = HEAP_MAGIC_ALLOC; | ||
| (*block).checksum = (*block).compute_checksum(); | ||
|
|
||
| // Update counters | ||
| let new_total = TOTAL_ALLOCATED.fetch_add((*block).size, Ordering::Relaxed); | ||
| update_peak(new_total); | ||
| ALLOC_COUNTER.fetch_add(1, Ordering::Relaxed); | ||
|
|
||
| return (*block).to_user_ptr(); | ||
| } | ||
| cache.misses += 1; | ||
| } | ||
|
|
||
| // Slow path | ||
| let block = find_free_block(aligned_size); | ||
| let block = if block.is_null() { | ||
| create_new_block(aligned_size) | ||
| } else { | ||
| if !(*block).validate() { | ||
| PrintKernelError(b"[HEAP] Corrupted free block\0".as_ptr()); | ||
| return ptr::null_mut(); | ||
| } | ||
| (*block).is_free = 0; | ||
| (*block).magic = HEAP_MAGIC_ALLOC; | ||
| (*block).checksum = (*block).compute_checksum(); | ||
|
|
||
| // Split if too large | ||
| if (*block).size > aligned_size * 2 { | ||
| split_block(block, aligned_size); | ||
| } | ||
|
|
||
| block | ||
| }; |
There was a problem hiding this comment.
Reinitialize the canary before handing a block back to callers.
Both the fast-cache branch and the slow-path reuse branch flip flags/checksums but never rewrite the trailing canary. rust_kfree poisons the payload, so the canary is gone; the next free will hit validate_canary() and report a bogus overflow, leaving the block permanently leaked. Restore the guard byte(s) (e.g., via a helper that writes CANARY_VALUE) before returning the pointer.
| let new_total = TOTAL_ALLOCATED.fetch_add((*block).size, Ordering::Relaxed); | ||
| update_peak(new_total); | ||
| ALLOC_COUNTER.fetch_add(1, Ordering::Relaxed); | ||
|
|
||
| return (*block).to_user_ptr(); | ||
| } | ||
| cache.misses += 1; | ||
| } | ||
|
|
||
| // Slow path | ||
| let block = find_free_block(aligned_size); | ||
| let block = if block.is_null() { | ||
| create_new_block(aligned_size) | ||
| } else { | ||
| if !(*block).validate() { | ||
| PrintKernelError(b"[HEAP] Corrupted free block\0".as_ptr()); | ||
| return ptr::null_mut(); | ||
| } | ||
| (*block).is_free = 0; | ||
| (*block).magic = HEAP_MAGIC_ALLOC; | ||
| (*block).checksum = (*block).compute_checksum(); | ||
|
|
||
| // Split if too large | ||
| if (*block).size > aligned_size * 2 { | ||
| split_block(block, aligned_size); | ||
| } | ||
|
|
||
| block | ||
| }; | ||
|
|
||
| if block.is_null() { | ||
| return ptr::null_mut(); | ||
| } | ||
|
|
||
| let new_total = TOTAL_ALLOCATED.fetch_add((*block).size, Ordering::Relaxed); | ||
| update_peak(new_total); | ||
| ALLOC_COUNTER.fetch_add(1, Ordering::Relaxed); | ||
|
|
There was a problem hiding this comment.
Fix TOTAL_ALLOCATED peak accounting.
fetch_add returns the previous total, but you pass that straight into update_peak, so the peak counter never advances. Capture the return value, add the block’s size, then feed the real new total into update_peak in both places.
1 participant
Summary by CodeRabbit