Development #166
Development #166
Conversation
WalkthroughAdds secure process creation APIs and a public CreateSecureProcess dispatcher, makes MLFQ CreateSecureProcess public, and updates scheduler call sites. Extends compositor/window API and Window struct with owner_pid and updates CreateWindow signature. Introduces a GUI shell process (VFShell) and compositor-integrated console printing with ConsoleSetWindowPrint. Implements a hybrid kernel heap path, expands the Magazine allocator with stats, validation, and control APIs, and adds magazine headers. Miscellaneous changes: cursor bitmap, palette default/color update, POST timing instrumentation, CMake/build option removals, and Shell current_dir visibility change. Poem
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
include/Scheduler.h (1)
5-11: Remove unconditional MLFQ include to fix non-MLFQ builds.-#include "MLFQ.h" #if defined(VF_CONFIG_SCHED_MLFQ) #include "MLFQ.h" #elif defined(VF_CONFIG_SCHED_EEVDF) #include "EEVDF.h" #endifkernel/etc/Console.c (1)
127-135: Use validated window pointer at call sites to prevent UAF.- if (console_window) WindowClearText(&g_compositor_ctx, console_window); + if (ConsoleWindowChecked()) WindowClearText(&g_compositor_ctx, ConsoleWindowChecked()); @@ - if (console_window) WindowScrollUp(&g_compositor_ctx, console_window); + if (ConsoleWindowChecked()) WindowScrollUp(&g_compositor_ctx, ConsoleWindowChecked()); @@ - if (console_window) WindowPrintString(&g_compositor_ctx, console_window, str); + if (ConsoleWindowChecked()) WindowPrintString(&g_compositor_ctx, ConsoleWindowChecked(), str);Optionally, expose a compositor callback to clear the binding on window destruction.
Also applies to: 137-151, 198-211
kernel/sched/EEVDF.c (2)
885-895: Add privilege‑escalation guard before allocating resources.Prevent non‑system creators from spawning system processes.
uint64_t flags = rust_spinlock_lock_irqsave(eevdf_lock); +EEVDFProcessControlBlock* creator = EEVDFGetCurrentProcess(); +if (priv == EEVDF_PROC_PRIV_SYSTEM && creator && creator->privilege_level != EEVDF_PROC_PRIV_SYSTEM) { + rust_spinlock_unlock_irqrestore(eevdf_lock, flags); + EEVDFASTerminate(creator->pid, "Unauthorized system process creation"); + return 0; +} if (UNLIKELY(process_count >= EEVDF_MAX_PROCESSES)) {
885-896: Reframe alignment concern; current flag values don't truncate in uint8_t.The review identifies a valid consistency concern but uses inaccurate framing. Current flag values (max 11 from PROC_FLAG_CORE bitwise OR) fit comfortably within uint8_t range (0–255), so no truncation occurs. However, MLFQ's equivalent function uses
uint32_t initial_flags, indicating an intentional API design choice for extensibility and consistency. If adopting uint32_t for the parameter, theflagsfield inEEVDFSecurityToken(line 70 in EEVDF.h) should also change touint32_tto avoid type mismatches. Update EEVDF.h declaration, function definition, callers, and the token struct field consistently.vfcompositor/Compositor.c (3)
61-65: Fix potential NULL dereference in fork arg check.
FastStrCmp(is_fork, "fork")is called before verifyingis_forkis non‑NULL.Apply:
- if (FastStrCmp(is_fork, "fork") == 0 && is_fork) fork = true; + if (is_fork && FastStrCmp(is_fork, "fork") == 0) fork = true;
443-458: Help window content: avoid overpainting and fix typo.
WindowDrawRect(..., 0, 25, ..., TERMINAL_TEXT)fills most of the client area after printing help text; it likely overwrites text and then gets cleared on next refresh. Remove or draw before printing.- Typo: “ATL + ” → “ALT + ”.
Apply:
- WindowPrintString(&g_compositor_ctx, w, "CTRL + <ESC>: Quit VFCompositor\n"); - WindowPrintString(&g_compositor_ctx, w, "ATL + <TAB>: Switches between window\n"); - WindowDrawRect(w, 0, 25, w->rect.width, w->rect.height - 25, TERMINAL_TEXT); + WindowPrintString(&g_compositor_ctx, w, "CTRL + <ESC>: Quit VFCompositor\n"); + WindowPrintString(&g_compositor_ctx, w, "ALT + <TAB>: Switches between window\n"); + // (Optional) If you want a content area background, draw it before printing: + // WindowDrawRect(w, 0, 25, w->rect.width, w->rect.height - 25, SOME_BG_COLOR);
568-619: Owner PID spoofing risk; enforce ownership and validate window size.
- Any caller can pass arbitrary
owner_pid, enabling window close → kill of another process. Enforce: non‑system callers cannot setowner_pidto anything but their own PID; allowowner_pid=0only for system callers.- Validate and clamp
width/heightto screen bounds to avoid overflow/large allocations.Apply:
-Window* CreateWindow(CompositorContext* ctx, int x, int y, int width, int height, const char* title, uint32_t owner_pid) { +Window* CreateWindow(CompositorContext* ctx, int x, int y, int width, int height, const char* title, uint32_t owner_pid) { Window* window = (Window*)KernelMemoryAlloc(sizeof(Window)); if (!window) return NULL; + // Sanitize geometry + if (width <= 0 || height <= 0 || !ctx || !ctx->g_vbe_info) { KernelFree(window); return NULL; } + if (width > (int)ctx->g_vbe_info->width) width = (int)ctx->g_vbe_info->width; + if (height > (int)ctx->g_vbe_info->height) height = (int)ctx->g_vbe_info->height; + window->rect.x = x; window->rect.y = y; window->rect.width = width; window->rect.height = height; @@ - window->minimized = false; - window->owner_pid = owner_pid; // Store the owner PID + window->minimized = false; + // Enforce ownership: non‑system callers cannot spoof owner_pid; allow 0 only for system + CurrentProcessControlBlock* self = GetCurrentProcess(); + if (self && self->privilege_level != PROC_PRIV_SYSTEM) { + window->owner_pid = self->pid; + } else { + window->owner_pid = owner_pid; // system may set 0 for compositor/system windows + } @@ - size_t buffer_size = width * height * sizeof(uint32_t); + size_t buffer_size = (size_t)width * (size_t)height * sizeof(uint32_t);Optional: add a thin wrapper to reduce swappable XYWH risk:
// Convenience wrapper Window* CreateWindowRect(CompositorContext* ctx, Rect r, const char* title, uint32_t owner_pid) { return CreateWindow(ctx, r.x, r.y, r.width, r.height, title, owner_pid); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
cmake/source.cmake(3 hunks)include/Scheduler.h(2 hunks)include/Window.h(1 hunks)kernel/core/Pallete.h(2 hunks)kernel/etc/Console.c(5 hunks)kernel/etc/Console.h(1 hunks)kernel/etc/Shell.c(2 hunks)kernel/sched/EEVDF.c(6 hunks)kernel/sched/EEVDF.h(1 hunks)kernel/sched/MLFQ.c(1 hunks)kernel/sched/MLFQ.h(1 hunks)vfcompositor/Compositor.c(12 hunks)vfcompositor/Compositor.h(1 hunks)vfcompositor/app/GUIShell.c(1 hunks)vfcompositor/app/GUIShell.h(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-10T06:03:22.076Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:03:22.076Z
Learning: In VoidFrame kernel's VBEConsoleClear() function, the color is hardcoded to 0x08 (gray on black) instead of using the current console color state, causing color washing issues. The function should use console.color or the current color state to maintain color consistency across screen clears.
Applied to files:
kernel/etc/Console.c
📚 Learning: 2025-08-10T06:31:43.779Z
Learnt from: assembler-0
PR: assembler-0/VoidFrame#62
File: kernel/core/Panic.c:69-79
Timestamp: 2025-08-10T06:31:43.779Z
Learning: In VoidFrame kernel's VBEConsolePutChar() function, the color_buffer must be updated with the current attribute byte whenever a character is stored in the buffer. Without this, characters inherit stale colors from VBEConsoleClear() causing all text to appear in the wrong color (gray/0x08) after refresh operations.
Applied to files:
kernel/etc/Console.c
🧬 Code graph analysis (8)
vfcompositor/app/GUIShell.h (1)
vfcompositor/app/GUIShell.c (1)
VFShellProcess(15-47)
kernel/sched/EEVDF.h (1)
kernel/sched/EEVDF.c (1)
EEVDFCreateSecureProcess(885-995)
vfcompositor/app/GUIShell.c (4)
kernel/etc/Console.c (1)
ConsoleSetWindowPrint(81-83)vfcompositor/Compositor.c (3)
CreateWindow(568-619)WindowFill(669-676)WindowPrintString(272-279)drivers/input/Keyboard.c (2)
HasInput(8-16)GetChar(21-29)kernel/etc/Shell.c (1)
ExecuteCommand(1257-1291)
vfcompositor/Compositor.h (1)
vfcompositor/Compositor.c (1)
CreateWindow(568-619)
vfcompositor/Compositor.c (3)
kernel/sched/MLFQ.c (1)
CreateSecureProcess(1014-1165)drivers/PS2.c (1)
PS2_CalcCombo(48-58)vfcompositor/app/GUIShell.c (1)
VFShellProcess(15-47)
kernel/etc/Console.c (1)
vfcompositor/Compositor.c (3)
WindowClearText(282-295)WindowScrollUp(190-205)WindowPrintString(272-279)
include/Scheduler.h (2)
kernel/sched/MLFQ.c (1)
CreateSecureProcess(1014-1165)kernel/sched/EEVDF.c (1)
EEVDFCreateSecureProcess(885-995)
kernel/sched/EEVDF.c (1)
kernel/etc/Shell.c (1)
ShellProcess(1297-1325)
🪛 Clang (14.0.6)
vfcompositor/app/GUIShell.c
[error] 1-1: 'Compositor.h' file not found
(clang-diagnostic-error)
[warning] 8-8: variable 'command_buffer' is non-const and globally accessible, consider making it const
(cppcoreguidelines-avoid-non-const-global-variables)
[warning] 8-8: 256 is a magic number; consider replacing it with a named constant
(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
[warning] 9-9: variable 'cmd_pos' is non-const and globally accessible, consider making it const
(cppcoreguidelines-avoid-non-const-global-variables)
[warning] 10-10: variable 'current_dir' is non-const and globally accessible, consider making it const
(cppcoreguidelines-avoid-non-const-global-variables)
[warning] 10-10: 256 is a magic number; consider replacing it with a named constant
(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
[warning] 12-12: variable 'g_compositor_ctx' is non-const and globally accessible, consider making it const
(cppcoreguidelines-avoid-non-const-global-variables)
[warning] 13-13: parameter name 'w' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 17-17: statement should be inside braces
(readability-braces-around-statements)
[warning] 24-24: variable name 'c' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 24-24: narrowing conversion from 'int' to signed type 'char' is implementation-defined
(bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions)
[warning] 38-38: 255 is a magic number; consider replacing it with a named constant
(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
vfcompositor/Compositor.c
[warning] 130-130: loop variable name 'y' is too short, expected at least 2 characters
(readability-identifier-length)
[warning] 131-131: loop variable name 'x' is too short, expected at least 2 characters
(readability-identifier-length)
[note] 472-472: +4, including nesting penalty of 3, nesting level increased to 4
(clang)
[note] 477-477: +1, nesting level increased to 4
(clang)
[note] 478-478: +5, including nesting penalty of 4, nesting level increased to 5
(clang)
[note] 479-479: +1, nesting level increased to 4
(clang)
[note] 481-481: +1, nesting level increased to 4
(clang)
[note] 482-482: +5, including nesting penalty of 4, nesting level increased to 5
(clang)
[note] 483-483: +1, nesting level increased to 4
(clang)
[warning] 472-472: repeated branch in conditional chain
(bugprone-branch-clone)
[note] 477-477: end of the original
(clang)
[note] 479-479: clone 1 starts here
(clang)
[warning] 477-477: repeated branch in conditional chain
(bugprone-branch-clone)
[note] 479-479: end of the original
(clang)
[note] 481-481: clone 1 starts here
(clang)
[note] 483-483: clone 2 starts here
(clang)
[warning] 568-568: 4 adjacent parameters of 'CreateWindow' of similar type ('int') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 568-568: the first parameter in the range is 'x'
(clang)
[note] 568-568: the last parameter in the range is 'height'
(clang)
[warning] 568-568: parameter 'owner_pid' is unused
(misc-unused-parameters)
kernel/etc/Console.c
[warning] 78-78: variable 'g_compositor_ctx' is non-const and globally accessible, consider making it const
(cppcoreguidelines-avoid-non-const-global-variables)
[warning] 79-79: variable 'console_window' is non-const and globally accessible, consider making it const
(cppcoreguidelines-avoid-non-const-global-variables)
[warning] 79-79: variable 'console_window' provides global access to a non-const object; consider making the pointed-to data 'const'
(cppcoreguidelines-avoid-non-const-global-variables)
[warning] 81-81: parameter name 'w' is too short, expected at least 3 characters
(readability-identifier-length)
include/Scheduler.h
[error] 5-5: 'MLFQ.h' file not found
(clang-diagnostic-error)
[warning] 43-43: 2 adjacent parameters of 'CreateSecureProcess' of similar type ('int') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 43-43: the first parameter in the range is 'priv'
(clang)
[note] 43-43: the last parameter in the range is 'flag'
(clang)
kernel/sched/EEVDF.c
[warning] 885-885: 2 adjacent parameters of 'EEVDFCreateSecureProcess' of similar type ('int') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 885-885: the first parameter in the range is 'priv'
(clang)
[note] 885-885: the last parameter in the range is 'flag'
(clang)
[warning] 885-885: parameter 'priv' is unused
(misc-unused-parameters)
[warning] 885-885: parameter 'flag' is unused
(misc-unused-parameters)
kernel/sched/MLFQ.c
[warning] 1014-1014: 2 adjacent parameters of 'CreateSecureProcess' of similar type ('int') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 1014-1014: the first parameter in the range is 'privilege'
(clang)
[note] 1014-1014: the last parameter in the range is 'initial_flags'
(clang)
[warning] 1014-1014: parameter 'privilege' is unused
(misc-unused-parameters)
[warning] 1014-1014: parameter 'initial_flags' is unused
(misc-unused-parameters)
kernel/etc/Shell.c
[warning] 46-46: variable 'current_dir' is non-const and globally accessible, consider making it const
(cppcoreguidelines-avoid-non-const-global-variables)
[warning] 46-46: 256 is a magic number; consider replacing it with a named constant
(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
🔇 Additional comments (13)
kernel/core/Pallete.h (1)
12-12: LGTM! Theme configuration updated.The palette changes update the accent color for "Deep Space" theme and switch the default palette from Tokyo Night to Deep Space. These are purely cosmetic changes with no functional impact.
Also applies to: 104-104
kernel/sched/EEVDF.h (1)
183-183: LGTM! Secure process creation API added.The new
EEVDFCreateSecureProcessfunction declaration extends the EEVDF scheduler with secure process creation capabilities, accepting privilege level and security flag parameters. This aligns with the PR's objective to enable process-owned windows and privilege management.vfcompositor/app/GUIShell.h (1)
1-6: LGTM! GUI shell header is clean.The header correctly declares the
VFShellProcessfunction entry point for the GUI shell component, with proper include guards.include/Window.h (1)
34-34: LGTM! Window ownership tracking field added.The
owner_pidfield enables tracking which process created each window, supporting the PR's window lifecycle management objectives. The placement at the end of the struct minimizes impact on existing code.kernel/sched/MLFQ.c (1)
1014-1165: Static analysis warnings are false positives.The static analysis tool incorrectly flags
privilegeandinitial_flagsas unused. These parameters are actually used at:
- Line 1023: privilege check
- Line 1042: privilege escalation check
- Line 1097: privilege assignment
- Line 1132: token privilege initialization
- Line 1133: token flags initialization
These warnings can be safely ignored.
vfcompositor/Compositor.h (1)
69-69: All CreateWindow call sites verified as updated.The verification confirms all 4 call sites in the codebase have been properly updated to include the new
owner_pidparameter:
GUIShell.c:16passesGetCurrentProcess()->pidCompositor.c:443, 473, 780each pass0cmake/source.cmake (2)
181-183: LGTM: include paths for vfcompositor.
23-26: No duplicate object entries found—code is correct as-is.Verification confirms that
vfcompositor/Compositor.candvfcompositor/app/GUIShell.cappear exactly once in the codebase (lines 24-25 incmake/source.cmake). Line 215 appends the${VFC_SOURCES}variable toC_SOURCES, which is standard CMake aggregation and does not create duplicate file entries. No other.cmakeorCMakeLists.txtfiles reference these sources.kernel/etc/Console.c (1)
127-135: Color consistency reminder for VBE clears.Re-verify VBEConsoleClear() respects the current console color to avoid gray-on-black wash. Based on learnings.
kernel/sched/EEVDF.c (1)
865-879: LGTM: switch to secure shell creation.vfcompositor/Compositor.c (3)
780-787: Start menu window owner_pid=0 justification.LGTM since compositor is system; closing should not kill a process. Keep this pattern restricted to system‑created UI.
Consider scanning for non‑system code passing
owner_pid=0.
16-16: Include for GUIShell.h.OK; required for
VFShellProcess.
568-569: All CreateWindow callsites are safe.Verification confirms that all four callsites pass either
0(for system/compositor windows) orGetCurrentProcess()->pid(for client application windows). No arbitrary PIDs are passed from non-system sources.
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cmake/source.cmake (1)
56-66: Conditionally compile Magazine.c by heap backend.Avoid compiling C backend when building pure RUST heap to cut image size and prevent accidental link-time conflicts.
-set(MM_SOURCES +set(MM_SOURCES mm/PMem.c mm/MemOps.c mm/VMem.c mm/StackGuard.c mm/MemPool.c mm/trace/StackTrace.c mm/security/Cerberus.c - mm/dynamic/c/Magazine.c mm/PageFaultHandler.c ) + +# Only include C magazine when C or HYBRID heap is enabled +if(VF_CONFIG_HEAP_LANG STREQUAL "C" OR VF_CONFIG_HEAP_LANG STREQUAL "HYBRID") + list(APPEND MM_SOURCES mm/dynamic/c/Magazine.c) +endif()mm/dynamic/c/Magazine.c (2)
87-90: Bound CPU index to avoid OOB per‑CPU arrays.lapic IDs aren’t guaranteed < MAX_CPU_CORES or contiguous. Modulo to stay in‑bounds.
-static inline uint32_t GetCpuId(void) { - // In a real scenario, this might need to be more robust, but for now, lapic_get_id is fine. - return lapic_get_id(); -} +static inline uint32_t GetCpuId(void) { + // Map LAPIC ID into [0, MAX_CPU_CORES) + return lapic_get_id() % MAX_CPU_CORES; +}
564-591: HYBRID: reallocate must route foreign (Rust) blocks to rust_krealloc.Current code assumes a Magazine header for unknown blocks and will read past-start of Rust blocks.
- Slab* slab = FindSlabForPointer(ptr); + uint64_t lflags = rust_spinlock_lock_irqsave(depot.lock); + Slab* slab = FindSlabForPointer(ptr); + rust_spinlock_unlock_irqrestore(depot.lock, lflags); if (slab) { old_size = slab->block_size; // For small allocations, size is block_size - } else { - // Assume large allocation - LargeBlockHeader* header = (LargeBlockHeader*)ptr - 1; - old_size = header->size; - } + } else { +#ifdef VF_CONFIG_HEAP_HYBRID + LargeBlockHeader* header = (LargeBlockHeader*)ptr - 1; + if (!header || header->magic != LARGE_BLOCK_MAGIC) { + // Foreign (Rust) block: delegate to Rust + return rust_krealloc(ptr, size); + } + old_size = header->size; +#else + LargeBlockHeader* header = (LargeBlockHeader*)ptr - 1; + old_size = header->size; +#endif + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
cmake/cache.cmake(1 hunks)cmake/features.cmake(1 hunks)cmake/source.cmake(4 hunks)kernel/etc/POST.c(3 hunks)mm/KernelHeap.h(2 hunks)mm/dynamic/c/Magazine.c(15 hunks)mm/dynamic/c/Magazine.h(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
mm/dynamic/c/Magazine.h (1)
mm/dynamic/c/Magazine.c (4)
MagazinePrintStats(628-647)MagazineSetValidationLevel(617-621)MagazineFlushCaches(601-615)MagazineSetPerfMode(623-626)
kernel/etc/POST.c (2)
kernel/atomic/rust/src/spinlock.rs (1)
rdtsc(13-15)kernel/etc/Console.c (1)
PrintKernelF(272-279)
mm/dynamic/c/Magazine.c (5)
mm/dynamic/rust/src/rust_heap.rs (2)
lapic_get_id(77-77)rust_kfree(122-147)mm/MemOps.c (1)
FastMemset(34-49)kernel/atomic/rust/src/ffi.rs (2)
rust_spinlock_lock_irqsave(210-221)rust_spinlock_unlock_irqrestore(224-231)mm/dynamic/rust/src/ffi.rs (2)
VMemFree(4-4)PrintKernel(12-12)kernel/etc/Console.c (1)
PrintKernelF(272-279)
🪛 Clang (14.0.6)
kernel/etc/POST.c
[warning] 26-26: 1000 is a magic number; consider replacing it with a named constant
(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
[warning] 28-28: variable 'ptr' is not initialized
(cppcoreguidelines-init-variables)
[warning] 35-35: statement should be inside braces
(readability-braces-around-statements)
[warning] 35-35: integer to pointer cast pessimizes optimization opportunities
(performance-no-int-to-ptr)
[warning] 35-35: 128 is a magic number; consider replacing it with a named constant
(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
[warning] 40-40: statement should be inside braces
(readability-braces-around-statements)
[warning] 46-46: integer to pointer cast pessimizes optimization opportunities
(performance-no-int-to-ptr)
[warning] 46-46: 64 is a magic number; consider replacing it with a named constant
(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
[warning] 46-46: 256 is a magic number; consider replacing it with a named constant
(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
mm/dynamic/c/Magazine.c
[warning] 30-30: 64 is a magic number; consider replacing it with a named constant
(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
[warning] 30-30: variable 'heap_stats_per_cpu' is non-const and globally accessible, consider making it const
(cppcoreguidelines-avoid-non-const-global-variables)
[warning] 33-33: variable 'g_validation_level' is non-const and globally accessible, consider making it const
(cppcoreguidelines-avoid-non-const-global-variables)
[warning] 34-34: variable 'g_stats_enabled' is non-const and globally accessible, consider making it const
(cppcoreguidelines-avoid-non-const-global-variables)
[warning] 36-36: 2 adjacent parameters of 'StatsAdd' of similar type ('int') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 36-36: the first parameter in the range is 'cpu'
(clang)
[note] 36-36: the last parameter in the range is 'sc'
(clang)
[warning] 36-36: parameter 'cpu' is unused
(misc-unused-parameters)
[warning] 36-36: parameter name 'sc' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 36-36: parameter 'ctr' is unused
(misc-unused-parameters)
[warning] 38-38: statement should be inside braces
(readability-braces-around-statements)
[warning] 43-43: parameter name 'sc' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 232-232: parameter 'slab' is unused
(misc-unused-parameters)
[warning] 233-233: statement should be inside braces
(readability-braces-around-statements)
[warning] 235-235: if with identical then and else branches
(bugprone-branch-clone)
[note] 239-239: else branch starts here
(clang)
[warning] 257-257: 0xDD is a magic number; consider replacing it with a named constant
(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
[warning] 269-269: 0xDD is a magic number; consider replacing it with a named constant
(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
[warning] 271-271: statement should be inside braces
(readability-braces-around-statements)
[warning] 596-596: 8 adjacent parameters of 'PrintOneStatLine' of similar type ('int') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 596-596: the first parameter in the range is 'sc_idx'
(clang)
[note] 596-596: the last parameter in the range is 'f'
(clang)
[warning] 596-596: parameter name 'a' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 596-596: parameter name 'b' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 596-596: parameter name 'c' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 596-596: parameter name 'd' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 596-596: parameter name 'e' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 596-596: parameter name 'f' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 618-618: statement should be inside braces
(readability-braces-around-statements)
[warning] 619-619: statement should be inside braces
(readability-braces-around-statements)
🔇 Additional comments (6)
cmake/cache.cmake (1)
7-8: LGTM: HYBRID default and choices exposed.mm/dynamic/c/Magazine.h (1)
124-127: Public API surface looks consistent with Magazine.c.No issues; names and signatures match usage.
cmake/source.cmake (2)
134-184: Compositor sources: OK to add; consider gating by feature.If you intend optional builds, wrap ${VFC_SOURCES} and include dirs behind a VF_CONFIG_ENABLE_VFCOMPOSITOR option.
200-212: LGTM: source aggregation includes VFC_SOURCES at the end.mm/dynamic/c/Magazine.c (2)
289-304: Free fast path: OK with IRQ off; keep.With alloc fix, fast paths are symmetric.
596-601: Minor: avoid magic numbers for poison/scribble.Define named constants for 0xCD/0xDD and reuse.
- FastMemset(ret, 0xCD, size_classes[sc_idx]); + #define KHEAP_POISON_ALLOC 0xCD + FastMemset(ret, KHEAP_POISON_ALLOC, size_classes[sc_idx]);And similarly in PoisonOnFreeSmall: use KHEAP_POISON_FREE 0xDD.
Likely an incorrect or invalid review comment.
cmake/features.cmake
Outdated
| elseif(VF_CONFIG_HEAP_LANG STREQUAL "HYBRID") | ||
| add_compile_definitions(VF_CONFIG_HEAP_HYBRID) | ||
| else() | ||
| message(FATAL_ERROR "Invalid heap language: ${VF_CONFIG_HEAP_LANG}. Did you pass: -DVF_CONFIG_HEAP_LANG=<RUST/C>?") | ||
| endif() |
There was a problem hiding this comment.
Include HYBRID in the error hint.
The failure message omits the new HYBRID option.
Apply:
- message(FATAL_ERROR "Invalid heap language: ${VF_CONFIG_HEAP_LANG}. Did you pass: -DVF_CONFIG_HEAP_LANG=<RUST/C>?")
+ message(FATAL_ERROR "Invalid heap language: ${VF_CONFIG_HEAP_LANG}. Did you pass: -DVF_CONFIG_HEAP_LANG=<RUST/C/HYBRID>?")📝 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.
| elseif(VF_CONFIG_HEAP_LANG STREQUAL "HYBRID") | |
| add_compile_definitions(VF_CONFIG_HEAP_HYBRID) | |
| else() | |
| message(FATAL_ERROR "Invalid heap language: ${VF_CONFIG_HEAP_LANG}. Did you pass: -DVF_CONFIG_HEAP_LANG=<RUST/C>?") | |
| endif() | |
| elseif(VF_CONFIG_HEAP_LANG STREQUAL "HYBRID") | |
| add_compile_definitions(VF_CONFIG_HEAP_HYBRID) | |
| else() | |
| message(FATAL_ERROR "Invalid heap language: ${VF_CONFIG_HEAP_LANG}. Did you pass: -DVF_CONFIG_HEAP_LANG=<RUST/C/HYBRID>?") | |
| endif() |
🤖 Prompt for AI Agents
In cmake/features.cmake around lines 59 to 63, the fatal error message for an
invalid VF_CONFIG_HEAP_LANG omits the new "HYBRID" option; update the message to
include HYBRID in the hint (e.g., change the hint list to <RUST/C/HYBRID>) so
users see all valid choices.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
mm/dynamic/c/Magazine.c (4)
489-525: Critical: MagazineReallocate missing HYBRID delegation.
MagazineFreecorrectly delegates unknown blocks torust_kfreein HYBRID mode (line 325), butMagazineReallocatedoes not. At line 505-507, it blindly casts toLargeBlockHeaderwithout checking the magic. Reallocating arust_kmallocpointer will read foreign memory and corrupt the heap.Add HYBRID delegation before the LargeBlockHeader logic:
MagazineBlockHeader* small_header = (MagazineBlockHeader*)ptr - 1; if (small_header && small_header->magic == MAGAZINE_BLOCK_MAGIC) { old_size = size_classes[small_header->sc_idx]; } else { LargeBlockHeader* large_header = (LargeBlockHeader*)ptr - 1; if (large_header && large_header->magic == LARGE_BLOCK_MAGIC) { old_size = large_header->size; } else { +#ifdef VF_CONFIG_HEAP_HYBRID + // Unknown block - delegate to Rust + return rust_krealloc(ptr, size); +#else // In a hybrid system, realloc should be handled by a dispatcher // that knows which allocator owns the pointer. // Since we can't know the size, we can't safely reallocate. PANIC("MagazineReallocate: unknown pointer type"); +#endif } }
36-41: Cleanup StatsAdd: remove volatile cast and add braces.The volatile cast is unnecessary and the early return lacks braces for clarity.
Apply this diff per the past review:
-static inline void StatsAdd(uint32_t cpu, int sc, volatile uint64_t* ctr) { - // Best-effort increment; tearing is acceptable for stats. - if (!g_stats_enabled) return; - (void)cpu; (void)sc; - (*((uint64_t*)ctr))++; -} +static inline void StatsAdd(uint32_t cpu, int sc, uint64_t* ctr) { + if (!g_stats_enabled) { return; } + (void)cpu; (void)sc; + ++(*ctr); +}
43-50: Remove misleading for(;;) loop.The infinite loop immediately returns, serving no purpose and confusing readers.
Apply this diff:
-static void StatsSlabAllocated(int sc) { - for (;;) { - // Use CPU id defensively; OK if called under depot lock from any CPU. - uint32_t cpu = lapic_get_id(); - heap_stats_per_cpu[cpu].slabs_allocated[sc]++; - return; - } -} +static void StatsSlabAllocated(int sc) { + uint32_t cpu = lapic_get_id(); + heap_stats_per_cpu[cpu].slabs_allocated[sc]++; +}
191-207: Critical: Alloc fast path races with ISRs.The fast path reads and modifies
mag->countandmag->blockswithout disabling interrupts. If an ISR callsMagazineFree(which disables IRQs at line 270), the magazine state can be corrupted.Wrap the fast-path access in an IRQ-disabled critical section:
if (mag && mag->count > 0) { + irq_flags_t aflags = save_irq_flags(); + cli(); + // Re-read after disabling IRQs + mag = cache->active_magazines[sc_idx]; + if (mag && mag->count > 0) { + mag->count--; + void* raw_block = mag->blocks[mag->count]; + StatsAdd(cpu_id, sc_idx, &heap_stats_per_cpu[cpu_id].alloc_fast_hits[sc_idx]); + + MagazineBlockHeader* header = (MagazineBlockHeader*)raw_block; + header->magic = MAGAZINE_BLOCK_MAGIC; + header->sc_idx = sc_idx; + void* user_ptr = (void*)(header + 1); + + if (g_validation_level == KHEAP_VALIDATION_FULL) { + FastMemset(user_ptr, 0xCD, size_classes[sc_idx]); + } + restore_irq_flags(aflags); + return user_ptr; + } + restore_irq_flags(aflags); + } - mag->count--; - StatsAdd(cpu_id, sc_idx, &heap_stats_per_cpu[cpu_id].alloc_fast_hits[sc_idx]); - - void* raw_block = mag->blocks[mag->count]; - MagazineBlockHeader* header = (MagazineBlockHeader*)raw_block; - header->magic = MAGAZINE_BLOCK_MAGIC; - header->sc_idx = sc_idx; - - void* user_ptr = (void*)(header + 1); - - if (g_validation_level == KHEAP_VALIDATION_FULL) { - FastMemset(user_ptr, 0xCD, size_classes[sc_idx]); - } - return user_ptr; - }mm/KernelHeap.h (2)
53-58: HYBRID routing: depends on Magazine.c realloc fix.The
KernelReallocatemacro routes all pointers toMagazineReallocate. This is only safe after applying the fix I suggested in Magazine.c (lines 489-525) to delegate unknown blocks torust_kreallocin HYBRID mode.After applying the Magazine.c fix, verify realloc behavior with:
#!/bin/bash # Check that MagazineReallocate has HYBRID delegation for unknown blocks rg -n "MagazineReallocate" mm/dynamic/c/Magazine.c -A 30 | rg -C 3 "rust_krealloc|VF_CONFIG_HEAP_HYBRID"
48-51: Consider increasing HYBRID_SMALL_THRESHOLD to 1024.Magazine handles allocations up to 1024 bytes. A threshold of 512 leaves half of Magazine's capacity unused, sending 513-1024 byte allocations to the Rust backend unnecessarily. Since Magazine is optimized for per-CPU lock-free operation, maximizing its utilization improves performance.
Apply this diff:
// Threshold for routing to Rust small-object allocator #ifndef HYBRID_SMALL_THRESHOLD -#define HYBRID_SMALL_THRESHOLD 512 +#define HYBRID_SMALL_THRESHOLD 1024 #endifmm/dynamic/c/Magazine.h (1)
53-56: ABI change: LargeBlockHeader layout modified.Adding the
magicfield beforesizechanges the structure layout. Existing large blocks allocated before this change will have their size field misinterpreted as magic, causing corruption.Ensure this is a breaking change that requires:
- Heap reinitialization (no migration of old blocks)
- Static asserts to lock the ABI going forward
Apply this diff to add ABI guardrails as suggested in past reviews:
+#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112L +_Static_assert(sizeof(LargeBlockHeader) == sizeof(uint64_t) + sizeof(size_t), + "LargeBlockHeader size unexpected"); +_Static_assert(_Alignof(LargeBlockHeader) >= _Alignof(uint64_t) && + _Alignof(LargeBlockHeader) >= _Alignof(size_t), + "LargeBlockHeader alignment unexpected"); +#endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
mm/KernelHeap.h(2 hunks)mm/dynamic/c/Magazine.c(12 hunks)mm/dynamic/c/Magazine.h(2 hunks)mm/dynamic/rust/src/backend.rs(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
mm/dynamic/c/Magazine.h (1)
mm/dynamic/c/Magazine.c (4)
MagazinePrintStats(562-581)MagazineSetValidationLevel(551-555)MagazineFlushCaches(535-549)MagazineSetPerfMode(557-560)
mm/dynamic/c/Magazine.c (5)
mm/dynamic/rust/src/rust_heap.rs (2)
lapic_get_id(77-77)rust_kfree(122-147)mm/MemOps.c (2)
FastMemset(34-49)FastMemcpy(51-70)kernel/atomic/rust/src/ffi.rs (2)
rust_spinlock_lock_irqsave(210-221)rust_spinlock_unlock_irqrestore(224-231)mm/dynamic/rust/src/ffi.rs (2)
VMemFree(4-4)PrintKernel(12-12)kernel/etc/Console.c (1)
PrintKernelF(272-279)
🪛 Clang (14.0.6)
mm/dynamic/c/Magazine.c
[warning] 30-30: 64 is a magic number; consider replacing it with a named constant
(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
[warning] 30-30: variable 'heap_stats_per_cpu' is non-const and globally accessible, consider making it const
(cppcoreguidelines-avoid-non-const-global-variables)
[warning] 33-33: variable 'g_validation_level' is non-const and globally accessible, consider making it const
(cppcoreguidelines-avoid-non-const-global-variables)
[warning] 34-34: variable 'g_stats_enabled' is non-const and globally accessible, consider making it const
(cppcoreguidelines-avoid-non-const-global-variables)
[warning] 36-36: 2 adjacent parameters of 'StatsAdd' of similar type ('int') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 36-36: the first parameter in the range is 'cpu'
(clang)
[note] 36-36: the last parameter in the range is 'sc'
(clang)
[warning] 36-36: parameter 'cpu' is unused
(misc-unused-parameters)
[warning] 36-36: parameter name 'sc' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 36-36: parameter 'ctr' is unused
(misc-unused-parameters)
[warning] 38-38: statement should be inside braces
(readability-braces-around-statements)
[warning] 43-43: parameter name 'sc' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 245-245: parameter 'size' is unused
(misc-unused-parameters)
[warning] 246-246: statement should be inside braces
(readability-braces-around-statements)
[warning] 248-248: if with identical then and else branches
(bugprone-branch-clone)
[note] 252-252: else branch starts here
(clang)
[warning] 273-273: variable 'cache' is not initialized
(cppcoreguidelines-init-variables)
[warning] 317-317: 0xDD is a magic number; consider replacing it with a named constant
(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
[warning] 424-424: variable 'block' is not initialized
(cppcoreguidelines-init-variables)
[warning] 516-516: variable 'new_ptr' is not initialized
(cppcoreguidelines-init-variables)
[warning] 530-530: 8 adjacent parameters of 'PrintOneStatLine' of similar type ('int') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 530-530: the first parameter in the range is 'sc_idx'
(clang)
[note] 530-530: the last parameter in the range is 'f'
(clang)
[warning] 530-530: parameter name 'a' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 530-530: parameter name 'b' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 530-530: parameter name 'c' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 530-530: parameter name 'd' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 530-530: parameter name 'e' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 530-530: parameter name 'f' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 552-552: statement should be inside braces
(readability-braces-around-statements)
[warning] 553-553: statement should be inside braces
(readability-braces-around-statements)
⏰ 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 (10)
mm/dynamic/rust/src/backend.rs (1)
1-538: LGTM! Whitespace formatting improvements.The changes are purely cosmetic—adding blank lines to improve code readability without altering any logic or behavior.
mm/dynamic/c/Magazine.h (3)
44-45: LGTM! Magic constants for block identification.The magic constants use distinct 64-bit values that clearly identify block types, enabling safe runtime dispatch in HYBRID mode.
131-134: LGTM! New public APIs for heap management.The new functions provide essential observability and control: stats printing, validation levels, cache flushing, and performance tuning. These align with the implementations in Magazine.c.
47-50: Code handles struct padding correctly through standard C practices; however, manual verification on target architecture is recommended.The
MagazineBlockHeaderstruct will include compiler-inserted padding aftersc_idxto align the struct to 8 bytes, resulting in a total size of 16 bytes (8 bytes magic + 1 byte sc_idx + 7 bytes padding).The codebase correctly accounts for this:
- Line 411 uses
sizeof(MagazineBlockHeader)when calculating chunk sizes, which automatically includes padding- Lines 263, 500 use typed pointer arithmetic
(MagazineBlockHeader*)ptr - 1, which automatically subtractssizeof(MagazineBlockHeader)(the full size including padding)The concern about padding affecting "user pointer offset calculation" is valid in principle, but the implementation relies on standard C semantics where pointer arithmetic and
sizeof()are layout-aware. This should work correctly on any standard C compiler and target architecture.However, verification requires compiling and testing on your actual target architecture(s) to confirm the struct layout matches assumptions and that the fast path (line 263) correctly retrieves the header from user pointers.
mm/dynamic/c/Magazine.c (4)
535-549: LGTM! Cache flush implementation is correct.The function properly acquires the depot lock with IRQs disabled, iterates all per-CPU caches, returns magazines to depot, and clears the active pointers.
551-560: LGTM! Validation and performance mode controls.Both functions provide simple runtime configuration with appropriate bounds checking.
562-581: LGTM! Stats aggregation and printing.The implementation correctly aggregates per-CPU stats across all size classes and formats them clearly for debugging.
313-330: No issues identified; validation in Rust heap protects against Magazine block mishandling.The script output confirms that
rust_kfree_backendvalidates the block's magic value before freeing (lines 388-390): if(*block).magic != HEAP_MAGIC_ALLOC, the function returns early without processing the block. Since Magazine blocks useLARGE_BLOCK_MAGIC, a corrupted Magazine-owned block accidentally passed torust_kfreewould fail this check and be safely rejected—preventing any double-free or use-after-free scenario. The code is safe as-is.mm/KernelHeap.h (2)
20-24: LGTM! C backend now exposes Magazine features.The updated macros properly route to the new Magazine functions for stats, validation, cache flushing, and performance mode control.
59-63: LGTM! HYBRID control functions properly coordinate both backends.The macros correctly invoke both Magazine and Rust heap operations for stats, validation, cache flushing, and performance mode, ensuring consistent behavior across the hybrid system.
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmake/features.cmake (1)
47-53: Optionally set a default scheduler to avoid hard failure when unset.Provide a default (e.g., MLFQ) and constrain cache values for better DX; keep fatal for invalid.
-if(VF_SCHEDULER STREQUAL "MLFQ") +if(NOT DEFINED VF_SCHEDULER) + set(VF_SCHEDULER "MLFQ" CACHE STRING "Scheduler" FORCE) + set_property(CACHE VF_SCHEDULER PROPERTY STRINGS "MLFQ;EEVDF") +elseif(VF_SCHEDULER STREQUAL "MLFQ") add_compile_definitions(VF_CONFIG_SCHED_MLFQ) elseif(VF_SCHEDULER STREQUAL "EEVDF") add_compile_definitions(VF_CONFIG_SCHED_EEVDF) else() message(FATAL_ERROR "Invalid scheduler: ${VF_SCHEDULER}. Did you pass: -DVF_SCHEDULER=<MLFQ/EEVDF>?") endif()mm/dynamic/c/Magazine.c (1)
175-177: Bound CPU index when accessing per‑CPU arrays.Use modulo to prevent OOB if LAPIC IDs are sparse/non‑contiguous.
-uint32_t cpu_id = GetCpuId(); +uint32_t cpu_id = GetCpuId() % MAX_CPU_CORES; @@ -uint32_t cpu_id = GetCpuId(); +uint32_t cpu_id = GetCpuId() % MAX_CPU_CORES; @@ -uint32_t cpu = lapic_get_id(); +uint32_t cpu = lapic_get_id() % MAX_CPU_CORES;Also applies to: 260-263, 46-49
♻️ Duplicate comments (5)
kernel/etc/Console.h (1)
77-77: Uncomment the declaration to fix compilation/linking issues.The
ConsoleSetWindowPrintdeclaration remains commented out while the function is implemented in Console.c and called in GUIShell.c. This will cause linking errors.Apply this diff to uncomment the declaration:
-// void ConsoleSetWindowPrint(Window* w); +void ConsoleSetWindowPrint(Window* w);mm/dynamic/c/Magazine.c (4)
36-41: StatsAdd: drop volatile cast; add braces.-static inline void StatsAdd(uint32_t cpu, int sc, volatile uint64_t* ctr) { - // Best-effort increment; tearing is acceptable for stats. - if (!g_stats_enabled) return; - (void)cpu; (void)sc; - (*((uint64_t*)ctr))++; -} +static inline void StatsAdd(uint32_t cpu, int sc_idx, uint64_t* ctr) { + if (!g_stats_enabled) { return; } + (void)cpu; (void)sc_idx; + ++(*ctr); +}
43-50: Remove dead for(;;) in StatsSlabAllocated; bound CPU id.-static void StatsSlabAllocated(int sc) { - for (;;) { - // Use CPU id defensively; OK if called under depot lock from any CPU. - uint32_t cpu = lapic_get_id(); - heap_stats_per_cpu[cpu].slabs_allocated[sc]++; - return; - } -} +static void StatsSlabAllocated(int sc_idx) { + uint32_t cpu = GetCpuId() % MAX_CPU_CORES; + heap_stats_per_cpu[cpu].slabs_allocated[sc_idx]++; +}
179-195: IRQ race: alloc fast‑path can interleave with free fast‑path.- if (mag && mag->count > 0) { - mag->count--; - StatsAdd(cpu_id, sc_idx, &heap_stats_per_cpu[cpu_id].alloc_fast_hits[sc_idx]); - void* raw_block = mag->blocks[mag->count]; - MagazineBlockHeader* header = (MagazineBlockHeader*)raw_block; - header->magic = MAGAZINE_BLOCK_MAGIC; - header->sc_idx = sc_idx; - void* user_ptr = (void*)(header + 1); - if (g_validation_level == KHEAP_VALIDATION_FULL) { - FastMemset(user_ptr, 0xCD, size_classes[sc_idx]); - } - return user_ptr; - } + if (mag) { + irq_flags_t aflags = save_irq_flags(); + cli(); + mag = cache->active_magazines[sc_idx]; // re-read under IRQ-off + if (mag && mag->count > 0) { + mag->count--; + void* raw_block = mag->blocks[mag->count]; + MagazineBlockHeader* header = (MagazineBlockHeader*)raw_block; + header->magic = MAGAZINE_BLOCK_MAGIC; + header->sc_idx = sc_idx; + void* user_ptr = (void*)(header + 1); + if (g_validation_level == KHEAP_VALIDATION_FULL) { + FastMemset(user_ptr, KHEAP_POISON_ALLOC, size_classes[sc_idx]); + } + StatsAdd(cpu_id, sc_idx, &heap_stats_per_cpu[cpu_id].alloc_fast_hits[sc_idx]); + restore_irq_flags(aflags); + return user_ptr; + } + restore_irq_flags(aflags); + }
213-228: Same IRQ race in slow‑path after installing new magazine.- cache->active_magazines[sc_idx] = mag; - mag->count--; - void* raw_block = mag->blocks[mag->count]; - MagazineBlockHeader* header = (MagazineBlockHeader*)raw_block; - header->magic = MAGAZINE_BLOCK_MAGIC; - header->sc_idx = sc_idx; - void* user_ptr = (void*)(header + 1); - if (g_validation_level == KHEAP_VALIDATION_FULL) { - FastMemset(user_ptr, 0xCD, size_classes[sc_idx]); - } - return user_ptr; + cache->active_magazines[sc_idx] = mag; + irq_flags_t aflags = save_irq_flags(); + cli(); + mag = cache->active_magazines[sc_idx]; + mag->count--; + void* raw_block = mag->blocks[mag->count]; + MagazineBlockHeader* header = (MagazineBlockHeader*)raw_block; + header->magic = MAGAZINE_BLOCK_MAGIC; + header->sc_idx = sc_idx; + void* user_ptr = (void*)(header + 1); + if (g_validation_level == KHEAP_VALIDATION_FULL) { + FastMemset(user_ptr, KHEAP_POISON_ALLOC, size_classes[sc_idx]); + } + restore_irq_flags(aflags); + return user_ptr;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/main.yaml(0 hunks)CMakeLists.txt(1 hunks)README.md(1 hunks)cmake/cache.cmake(0 hunks)cmake/features.cmake(2 hunks)docs/DEVELOPMENT.md(3 hunks)kernel/core/InitRD.c(0 hunks)kernel/etc/Console.h(2 hunks)mm/KernelHeap.h(1 hunks)mm/dynamic/c/Magazine.c(12 hunks)
💤 Files with no reviewable changes (3)
- .github/workflows/main.yaml
- cmake/cache.cmake
- kernel/core/InitRD.c
🧰 Additional context used
🧬 Code graph analysis (2)
mm/KernelHeap.h (5)
mm/dynamic/rust/src/backend.rs (1)
VMemAlloc(108-108)mm/dynamic/rust/src/ffi.rs (2)
VMemAlloc(3-3)VMemFree(4-4)mm/VMem.c (2)
VMemAlloc(465-503)VMemFree(505-529)mm/dynamic/c/Magazine.c (1)
MagazineFree(245-318)mm/dynamic/rust/src/rust_heap.rs (1)
rust_kfree(122-147)
mm/dynamic/c/Magazine.c (5)
mm/dynamic/rust/src/rust_heap.rs (2)
lapic_get_id(77-77)rust_kfree(122-147)mm/MemOps.c (2)
FastMemset(34-49)FastMemcpy(51-70)kernel/atomic/rust/src/ffi.rs (2)
rust_spinlock_lock_irqsave(210-221)rust_spinlock_unlock_irqrestore(224-231)mm/dynamic/rust/src/ffi.rs (2)
VMemFree(4-4)PrintKernel(12-12)kernel/etc/Console.c (1)
PrintKernelF(272-279)
🪛 Clang (14.0.6)
mm/KernelHeap.h
[warning] 23-23: parameter 'size' is unused
(misc-unused-parameters)
[warning] 25-25: variable 'raw_mem' is not initialized
(cppcoreguidelines-init-variables)
mm/dynamic/c/Magazine.c
[warning] 30-30: 64 is a magic number; consider replacing it with a named constant
(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
[warning] 30-30: variable 'heap_stats_per_cpu' is non-const and globally accessible, consider making it const
(cppcoreguidelines-avoid-non-const-global-variables)
[warning] 33-33: variable 'g_validation_level' is non-const and globally accessible, consider making it const
(cppcoreguidelines-avoid-non-const-global-variables)
[warning] 34-34: variable 'g_stats_enabled' is non-const and globally accessible, consider making it const
(cppcoreguidelines-avoid-non-const-global-variables)
[warning] 36-36: 2 adjacent parameters of 'StatsAdd' of similar type ('int') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 36-36: the first parameter in the range is 'cpu'
(clang)
[note] 36-36: the last parameter in the range is 'sc'
(clang)
[warning] 36-36: parameter 'cpu' is unused
(misc-unused-parameters)
[warning] 36-36: parameter name 'sc' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 36-36: parameter 'ctr' is unused
(misc-unused-parameters)
[warning] 38-38: statement should be inside braces
(readability-braces-around-statements)
[warning] 43-43: parameter name 'sc' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 233-233: parameter 'size' is unused
(misc-unused-parameters)
[warning] 234-234: statement should be inside braces
(readability-braces-around-statements)
[warning] 236-236: if with identical then and else branches
(bugprone-branch-clone)
[note] 240-240: else branch starts here
(clang)
[warning] 261-261: variable 'cache' is not initialized
(cppcoreguidelines-init-variables)
[warning] 305-305: 0xDD is a magic number; consider replacing it with a named constant
(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers)
[warning] 412-412: variable 'block' is not initialized
(cppcoreguidelines-init-variables)
[warning] 504-504: variable 'new_ptr' is not initialized
(cppcoreguidelines-init-variables)
[warning] 518-518: 8 adjacent parameters of 'PrintOneStatLine' of similar type ('int') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 518-518: the first parameter in the range is 'sc_idx'
(clang)
[note] 518-518: the last parameter in the range is 'f'
(clang)
[warning] 518-518: parameter name 'a' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 518-518: parameter name 'b' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 518-518: parameter name 'c' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 518-518: parameter name 'd' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 518-518: parameter name 'e' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 518-518: parameter name 'f' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 540-540: statement should be inside braces
(readability-braces-around-statements)
[warning] 541-541: statement should be inside braces
(readability-braces-around-statements)
🔇 Additional comments (3)
cmake/features.cmake (2)
36-37: No functional impact.Comment toggling looks fine.
1-28: VF_CONFIG_ENABLE_OPIC is not a typo; no changes needed.OPIC stands for Open Programmable Interrupt Controller, a deprecated technology from the mid-1990s. This is distinct from IOAPIC (I/O Advanced PIC), which is listed separately in the project's README as a supported feature for modern systems.
The OPIC driver is intended for compatibility with specific legacy hardware like the AMD ÉlanSC520, and the naming is consistent throughout: VF_CONFIG_ENABLE_OPIC guards the OpicInstall() call in kernel/core/Kernel.c, and the corresponding driver implementation is complete. The ifdef mechanism correctly enables/disables the code path—there is no risk of silent disabling.
mm/KernelHeap.h (1)
75-77: Bound CPU id when flushing per‑CPU Rust caches.LAPIC IDs can be sparse; modulo to avoid OOB inside Rust.
-#define KernelHeapFlushCaches() do { MagazineFlushCaches(); rust_heap_flush_cpu(lapic_get_id()); } while (0) +#define KernelHeapFlushCaches() do { MagazineFlushCaches(); rust_heap_flush_cpu(lapic_get_id() % MAX_CPU_CORES); } while (0)
| #define KernelAllocate(num, size) KernelMemoryAlloc((num)*(size)) // Simplified; assumes no overflow | ||
|
|
||
| #define KernelReallocate(ptr, size) MagazineReallocate((ptr), (size)) // Realloc remains complex, handled by Magazine for now |
There was a problem hiding this comment.
KernelAllocate: guard multiplication overflow.
Current macro assumes no overflow.
// Add helper:
static inline void* KernelAllocateImpl(size_t num, size_t size) {
if (num > 0 && size > SIZE_MAX / num) { return NULL; }
return KernelMemoryAllocImpl(num * size);
}-#define KernelAllocate(num, size) KernelMemoryAlloc((num)*(size)) // Simplified; assumes no overflow
+#define KernelAllocate(num, size) KernelAllocateImpl((num), (size))🤖 Prompt for AI Agents
In mm/KernelHeap.h around lines 67 to 69, the KernelAllocate macro multiplies
num*size without overflow protection; replace it with a safe inline
implementation that checks if num > 0 and size > SIZE_MAX / num, returns NULL on
overflow, otherwise calls the underlying allocator (e.g., KernelMemoryAllocImpl)
with the product; update the macro to call this inline helper (or remove the
macro and expose KernelAllocateImpl) so all allocations are guarded against
multiplication overflow.
| #define KernelReallocate(ptr, size) MagazineReallocate((ptr), (size)) // Realloc remains complex, handled by Magazine for now | ||
|
|
||
| #define KernelFree(ptr) HybridFree(ptr) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Realloc across HYBRID: align macro with new delegation.
Once MagazineReallocate delegates unknown ptrs to rust in HYBRID, keep macro; otherwise introduce a unified inline that routes appropriately. Verify rust_krealloc is declared in KernelHeapRust.h.
🤖 Prompt for AI Agents
In mm/KernelHeap.h around lines 69 to 71, the KernelReallocate macro currently
aliases MagazineReallocate but the comment indicates MagazineReallocate may
delegate unknown pointers to Rust in HYBRID; replace or align the macro with
that delegation by introducing a unified inline function that checks ownership
and routes reallocs to MagazineReallocate or rust_krealloc as appropriate (or
keep the macro only if MagazineReallocate already performs delegation), and
ensure rust_krealloc is declared in KernelHeapRust.h; update includes/forward
declarations so KernelHeap.h can call rust_krealloc when HYBRID is enabled.
| cmake .. -DCMAKE_BUILD_TYPE=Release \ | ||
| -DCMAKE_TOOLCHAIN_FILE=../cmake/toolchain/<linux/windows/macos>-x64.cmake \ | ||
| -G Ninja \ | ||
| -DVF_SCHEDULER=<MLFQ/EEVDF> \ | ||
| -DVF_CONFIG_HEAP_LANG=<C/RUST> | ||
| -DVF_SCHEDULER=<MLFQ/EEVDF> | ||
| ccmake . # Optinal, tune as needed |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Docs: reflect default build type and expose HYBRID heap option.
- CMake now defaults to Release; you can drop the explicit -DCMAKE_BUILD_TYPE in the quickstart.
- Consider documenting the optional HYBRID path (e.g., -DVF_CONFIG_HEAP_HYBRID=ON) since code gates on it.
🤖 Prompt for AI Agents
In README.md around lines 62 to 66, the quickstart still shows an explicit
-DCMAKE_BUILD_TYPE=Release and doesn't mention the optional HYBRID heap flag;
update the instructions to remove the explicit -DCMAKE_BUILD_TYPE since CMake
now defaults to Release, and add a short note showing the optional HYBRID heap
toggle (e.g., -DVF_CONFIG_HEAP_HYBRID=ON) with brief placement guidance so users
know the code paths are gated by that option.
VFC again
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores