-
Notifications
You must be signed in to change notification settings - Fork 1
Development #152
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Development #152
Conversation
Caution Review failedThe pull request is closed. WalkthroughThe build system switches to Corrosion-based Rust integration, adding Corrosion sources and presets, FindRust and generator modules, and a rust_utils helper. Top-level CMake gains metadata and crate imports with a cargo target. Legacy Rust build logic for spinlocks is removed. Minor C and header adjustments and a Rust dependency bump are included. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant CMake as CMake (Project)
participant Cor as Corrosion.cmake
participant Find as FindRust.cmake
participant Cargo as Cargo
participant Crate as Rust Crates (heap/atomic)
participant CTargets as C/C++ Targets
Dev->>CMake: Configure/Generate
CMake->>Cor: include(Corrosion)
Cor->>Find: find_package(Rust)
Find-->>Cor: Rust toolchain, host/target triples
Cor->>Cargo: cargo metadata/build (per manifest)
Cargo-->>Cor: Artifacts (staticlib/cdylib), metadata
Cor-->>CMake: Imported targets for crates
CMake->>CTargets: target_link_libraries(..., crate_targets)
Note over CMake,Cor: New flow replaces custom rust_* targets
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (9)
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 |
There was a problem hiding this 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
mm/rust/Cargo.lock
is excluded by!**/*.lock
📒 Files selected for processing (9)
CMakeLists.txt
(7 hunks)cmake/rust_utils.cmake
(1 hunks)cmake/variable.cmake
(1 hunks)fs/FAT/FAT1x.c
(1 hunks)include/Io.h
(0 hunks)kernel/atomic/rust/CMakeLists.txt
(0 hunks)kernel/etc/Shell.c
(0 hunks)kernel/ipc/Ipc.c
(2 hunks)mm/rust/Cargo.toml
(1 hunks)
💤 Files with no reviewable changes (3)
- include/Io.h
- kernel/atomic/rust/CMakeLists.txt
- kernel/etc/Shell.c
🧰 Additional context used
🧬 Code graph analysis (1)
kernel/ipc/Ipc.c (1)
kernel/atomic/rust/src/ffi.rs (2)
rust_spinlock_free
(39-50)rust_spinlock_unlock
(60-64)
🔇 Additional comments (7)
fs/FAT/FAT1x.c (1)
17-17
: LGTM! Appropriate logging level adjustment.The change from
PrintKernelError
toPrintKernel
is correct for these detection failure paths. SinceFat1xDetect
probes whether a block device contains a FAT1x filesystem, returning 0 (not detected) is expected behavior when the device contains a different filesystem. Using error-level logging would create noise during normal filesystem detection.Also applies to: 23-23
cmake/variable.cmake (1)
16-16
: LGTM! Proper 64-bit literal prevents overflow.The ULL suffix ensures 64-bit arithmetic throughout the multiplication chain, preventing potential integer overflow in intermediate calculations.
cmake/rust_utils.cmake (1)
6-9
: LGTM! Clean helper for Rust library linking.The hyphen-to-underscore conversion correctly bridges Rust crate naming conventions (hyphens) to CMake target naming (underscores).
CMakeLists.txt (3)
5-10
: LGTM! Metadata additions improve project documentation.Adding homepage URL and description to the project declaration provides useful context for the build system and developers.
77-94
: LGTM! Corrosion integration streamlines Rust build workflow.The manifest-based Corrosion approach with
NO_STD
flags is appropriate for kernel-level Rust components and replaces the previous bespoke Rust subdirectory build system.
154-155
: LGTM! Consistent use of the new Rust linking helper.The
link_rust_library
calls correctly use the new helper function to link Rust components with proper name conversion.mm/rust/Cargo.toml (1)
10-10
: Approve spin crate bump to 0.10.0. Usingdefault-features = false
with only["mutex","spin_mutex"]
remains no_std-compatible (thestd
feature is opt-in), 0.10.0 is the max stable release, and no security advisories affect ≥0.9.8.
kernel/ipc/Ipc.c
Outdated
target->state = PROC_READY; | ||
} | ||
rust_spinlock_unlock(queue->lock); | ||
rust_spinlock_free(queue->lock); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical: Use-after-free bug in lock lifecycle management.
rust_spinlock_free(queue->lock)
marks the lock slot as unused, but queue->lock
still points to the freed slot. Any subsequent access to queue->lock
(including other IPC operations on the same queue) will access freed/potentially reused memory.
The lock should be freed only when the queue itself is destroyed, not after every operation. If per-operation lifecycle is intended, you must set queue->lock = NULL
after freeing. However, examining the code reveals additional issues:
- In
IpcReceiveMessage
(lines 100-141), the lock is freed inside the loop (lines 132, 138), but line 101 checksif (!queue->lock)
before reallocating. Since the pointer is not NULL after free, subsequent loop iterations will attempt to lock freed memory. IpcReceiveMessageType
(lines 143-182) has no free call, creating inconsistent lock lifecycle management.IpcHasMessageType
andIpcFlushQueue
also don't free locks.
Apply this diff to fix the immediate use-after-free by removing premature free calls:
}
rust_spinlock_unlock(queue->lock);
- rust_spinlock_free(queue->lock);
return IPC_SUCCESS;
}
Then apply similar fixes to lines 132 and 138. The lock should be freed only when the message queue or process is destroyed, not after each IPC operation.
📝 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.
rust_spinlock_free(queue->lock); | |
} | |
rust_spinlock_unlock(queue->lock); | |
return IPC_SUCCESS; | |
} |
🤖 Prompt for AI Agents
In kernel/ipc/Ipc.c around line 92 (and similarly for the calls at lines ~132
and ~138, and in the IpcReceiveMessage block at lines 100-141 and
IpcReceiveMessageType at 143-182), the code calls
rust_spinlock_free(queue->lock) while leaving queue->lock pointing at freed
memory, causing a use-after-free on subsequent operations; remove the premature
rust_spinlock_free calls from per-operation paths (lines 92, ~132, ~138) so the
lock is only freed when the message queue or process is destroyed, and ensure
the lock lifecycle is handled consistently across IpcReceiveMessage,
IpcReceiveMessageType, IpcHasMessageType and IpcFlushQueue (either free only on
queue teardown or, if you insist on per-operation freeing, set queue->lock =
NULL immediately after free and update all callers to reallocate when NULL),
updating comments and tests to reflect the chosen lifecycle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (1)
corrosion/RELEASES.md (1)
3-27
: Normalize heading levels for markdownlint
#
→###
jumps violate MD001 and make the new lint job fail. Please switch intermediate headings to##
(e.g.## Breaking Changes
,## New features
)—and mirror that pattern for the other sections you added here—so the heading hierarchy increments by one level.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
mm/rust/Cargo.lock
is excluded by!**/*.lock
📒 Files selected for processing (18)
CMakeLists.txt
(7 hunks)cmake/rust_utils.cmake
(1 hunks)cmake/variable.cmake
(1 hunks)corrosion/.gitignore
(1 hunks)corrosion/CMakeLists.txt
(1 hunks)corrosion/CMakePresets.json
(1 hunks)corrosion/LICENSE
(1 hunks)corrosion/README.md
(1 hunks)corrosion/RELEASES.md
(1 hunks)corrosion/cmake/CorrosionConfig.cmake.in
(1 hunks)corrosion/cmake/CorrosionGenerator.cmake
(1 hunks)corrosion/cmake/FindRust.cmake
(1 hunks)fs/FAT/FAT1x.c
(1 hunks)include/Io.h
(0 hunks)kernel/atomic/rust/CMakeLists.txt
(0 hunks)kernel/etc/Shell.c
(0 hunks)kernel/ipc/Ipc.c
(2 hunks)mm/rust/Cargo.toml
(1 hunks)
💤 Files with no reviewable changes (3)
- include/Io.h
- kernel/atomic/rust/CMakeLists.txt
- kernel/etc/Shell.c
🧰 Additional context used
🧬 Code graph analysis (1)
kernel/ipc/Ipc.c (1)
kernel/atomic/rust/src/ffi.rs (2)
rust_spinlock_free
(39-50)rust_spinlock_unlock
(60-64)
🪛 markdownlint-cli2 (0.18.1)
corrosion/RELEASES.md
3-3: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
36-36: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
44-44: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
54-54: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
87-87: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
100-100: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
109-109: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
118-118: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
125-125: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
132-132: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
141-141: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
150-150: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
164-164: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
304-304: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🔇 Additional comments (4)
mm/rust/Cargo.toml (1)
10-10
: Confirm spin 0.10 feature compatibility
spin
jumped from 0.9 → 0.10. Because pre-1.0 crates treat minor bumps as breaking, please double-check the release notes/build to ensure themutex
/spin_mutex
feature flags and the APIs we rely on stayed intact.cmake/variable.cmake (1)
16-16
: Correct fix for potential integer overflow.Adding the
ULL
suffix ensures the entire calculation128*1024*1024*1024
is performed in unsigned long long (64-bit) arithmetic, preventing overflow on 32-bit systems where the intermediate multiplication would otherwise happen in 32-bit signed int before assignment.cmake/rust_utils.cmake (1)
5-9
: LGTM! Proper normalization for Rust library names.The hyphen-to-underscore conversion correctly handles the naming convention mismatch between Cargo crate names (with hyphens) and the generated library file names (with underscores). The private linkage is appropriate for internal Rust dependencies.
corrosion/CMakeLists.txt (1)
1-86
: LGTM! Well-structured Corrosion build configuration.The CMakeLists.txt properly handles:
- Version 0.99.99 as a pre-release marker with clear documentation
- Conditional test building and language enablement based on PROJECT_IS_TOP_LEVEL
- CORROSION_INSTALL_ONLY option for subdirectory usage without installation
- Proper installation rules with GNUInstallDirs and CMakePackageConfigHelpers
- SameMajorVersion compatibility policy and ARCH_INDEPENDENT flag are appropriate
if(LOCKED) | ||
set(cargo_locked "--locked") | ||
endif() | ||
if(FROZEN) | ||
set(cargo_frozen "--frozen") | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix LOCKED/FROZEN option handling
cmake_parse_arguments(... CM ...)
populates CM_LOCKED
/ CM_FROZEN
, not bare LOCKED
/ FROZEN
. As written the conditions never fire, so --locked/--frozen
are silently ignored. Please switch the checks to if(CM_LOCKED)
/ if(CM_FROZEN)
(and update the assignments accordingly) so the options work.
- set(cargo_locked "")
- set(cargo_frozen "")
- if(LOCKED)
+ set(cargo_locked "")
+ set(cargo_frozen "")
+ if(CM_LOCKED)
set(cargo_locked "--locked")
endif()
- if(FROZEN)
+ if(CM_FROZEN)
set(cargo_frozen "--frozen")
endif()
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In corrosion/cmake/CorrosionGenerator.cmake around lines 18 to 23, the code
checks LOCKED/FROZEN but cmake_parse_arguments populated CM_LOCKED and
CM_FROZEN, so the conditions never trigger; update the conditions to
if(CM_LOCKED) and if(CM_FROZEN) and set the corresponding variables (e.g.,
set(cargo_locked "--locked") and set(cargo_frozen "--frozen")) inside those
branches so the flags are applied correctly.
function(_findrust_version_ok ACTUAL_VERSION OUT_IS_OK) | ||
if(DEFINED Rust_FIND_VERSION_RANGE) | ||
if(Rust_FIND_VERSION_RANGE_MAX STREQUAL "INCLUDE") | ||
set(COMPARSION_OPERATOR "VERSION_LESS_EQUAL") | ||
elseif(Rust_FIND_VERSION_RANGE_MAX STREQUAL "EXCLUDE") | ||
set(COMPARSION_OPERATOR "VERSION_LESS") | ||
else() | ||
message(FATAL_ERROR "Unexpected value in `<PackageName>_FIND_VERSION_RANGE_MAX`: " | ||
"`${Rust_FIND_VERSION_RANGE_MAX}`.") | ||
endif() | ||
if(("${ACTUAL_VERSION}" VERSION_GREATER_EQUAL "${Rust_FIND_VERSION_RANGE_MIN}") | ||
AND | ||
( "${ACTUAL_VERSION}" ${COMPARSION_OPERATOR} "${Rust_FIND_VERSION_RANGE_MAX}" ) | ||
) | ||
set("${OUT_IS_OK}" TRUE PARENT_SCOPE) | ||
else() | ||
set("${OUT_IS_OK}" FALSE PARENT_SCOPE) | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dereference ACTUAL_VERSION
before the range comparison
When Rust_FIND_VERSION_RANGE
is set, ACTUAL_VERSION
is the name of the variable holding the toolchain version (e.g. _TOOLCHAIN_default_VERSION
). The quoted comparison therefore ends up comparing the literal string _TOOLCHAIN_default_VERSION
against the requested range, which always fails. Please resolve the name to its value before doing the VERSION_*
tests (e.g. set(actual_version "${${ACTUAL_VERSION}}")
and use actual_version
in the checks).
- if(DEFINED Rust_FIND_VERSION_RANGE)
+ if(DEFINED Rust_FIND_VERSION_RANGE)
if(Rust_FIND_VERSION_RANGE_MAX STREQUAL "INCLUDE")
set(COMPARSION_OPERATOR "VERSION_LESS_EQUAL")
elseif(Rust_FIND_VERSION_RANGE_MAX STREQUAL "EXCLUDE")
set(COMPARSION_OPERATOR "VERSION_LESS")
else()
message(FATAL_ERROR "Unexpected value in `<PackageName>_FIND_VERSION_RANGE_MAX`: "
"`${Rust_FIND_VERSION_RANGE_MAX}`.")
endif()
- if(("${ACTUAL_VERSION}" VERSION_GREATER_EQUAL "${Rust_FIND_VERSION_RANGE_MIN}")
- AND
- ( "${ACTUAL_VERSION}" ${COMPARSION_OPERATOR} "${Rust_FIND_VERSION_RANGE_MAX}" )
+ set(actual_version "${${ACTUAL_VERSION}}")
+ if(("${actual_version}" VERSION_GREATER_EQUAL "${Rust_FIND_VERSION_RANGE_MIN}")
+ AND
+ ( "${actual_version}" ${COMPARSION_OPERATOR} "${Rust_FIND_VERSION_RANGE_MAX}" )
)
set("${OUT_IS_OK}" TRUE PARENT_SCOPE)
else()
set("${OUT_IS_OK}" FALSE PARENT_SCOPE)
endif()
📝 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.
function(_findrust_version_ok ACTUAL_VERSION OUT_IS_OK) | |
if(DEFINED Rust_FIND_VERSION_RANGE) | |
if(Rust_FIND_VERSION_RANGE_MAX STREQUAL "INCLUDE") | |
set(COMPARSION_OPERATOR "VERSION_LESS_EQUAL") | |
elseif(Rust_FIND_VERSION_RANGE_MAX STREQUAL "EXCLUDE") | |
set(COMPARSION_OPERATOR "VERSION_LESS") | |
else() | |
message(FATAL_ERROR "Unexpected value in `<PackageName>_FIND_VERSION_RANGE_MAX`: " | |
"`${Rust_FIND_VERSION_RANGE_MAX}`.") | |
endif() | |
if(("${ACTUAL_VERSION}" VERSION_GREATER_EQUAL "${Rust_FIND_VERSION_RANGE_MIN}") | |
AND | |
( "${ACTUAL_VERSION}" ${COMPARSION_OPERATOR} "${Rust_FIND_VERSION_RANGE_MAX}" ) | |
) | |
set("${OUT_IS_OK}" TRUE PARENT_SCOPE) | |
else() | |
set("${OUT_IS_OK}" FALSE PARENT_SCOPE) | |
endif() | |
function(_findrust_version_ok ACTUAL_VERSION OUT_IS_OK) | |
if(DEFINED Rust_FIND_VERSION_RANGE) | |
if(Rust_FIND_VERSION_RANGE_MAX STREQUAL "INCLUDE") | |
set(COMPARSION_OPERATOR "VERSION_LESS_EQUAL") | |
elseif(Rust_FIND_VERSION_RANGE_MAX STREQUAL "EXCLUDE") | |
set(COMPARSION_OPERATOR "VERSION_LESS") | |
else() | |
message(FATAL_ERROR "Unexpected value in `<PackageName>_FIND_VERSION_RANGE_MAX`: " | |
"`${Rust_FIND_VERSION_RANGE_MAX}`.") | |
endif() | |
# resolve the variable name in ACTUAL_VERSION to its value before comparing | |
set(actual_version "${${ACTUAL_VERSION}}") | |
if(("${actual_version}" VERSION_GREATER_EQUAL "${Rust_FIND_VERSION_RANGE_MIN}") | |
AND | |
("${actual_version}" ${COMPARSION_OPERATOR} "${Rust_FIND_VERSION_RANGE_MAX}") | |
) | |
set("${OUT_IS_OK}" TRUE PARENT_SCOPE) | |
else() | |
set("${OUT_IS_OK}" FALSE PARENT_SCOPE) | |
endif() | |
elseif(DEFINED Rust_FIND_VERSION) | |
… | |
endif() | |
endfunction() |
🤖 Prompt for AI Agents
In corrosion/cmake/FindRust.cmake around lines 38 to 55, ACTUAL_VERSION is
currently the name of a variable that holds the real version string so the
VERSION_* comparisons are comparing the variable name instead of its value; fix
this by dereferencing ACTUAL_VERSION into a local variable (e.g.
set(actual_version "${${ACTUAL_VERSION}}")), then use that local actual_version
(properly quoted) in the VERSION_GREATER_EQUAL and the ${COMPARSION_OPERATOR}
VERSION comparisons; leave the OUT_IS_OK PARENT_SCOPE logic as-is but ensure you
reference actual_version in the conditional expressions.
kernel/ipc/Ipc.c
Outdated
rust_spinlock_free(queue->lock); | ||
return IPC_SUCCESS; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not free queue->lock
after every unlock.
queue->lock
stays non-null, so later calls skip reallocation but keep using storage that was just returned to the Rust spinlock pool. This turns every subsequent lock/unlock into a use-after-free and eventually a double-free once another queue reuses that slot. Remove these rust_spinlock_free
calls (or reset queue->lock
and reacquire on next use).
- rust_spinlock_unlock(queue->lock);
- rust_spinlock_free(queue->lock);
+ rust_spinlock_unlock(queue->lock);
- rust_spinlock_unlock(queue->lock);
- rust_spinlock_free(queue->lock);
+ rust_spinlock_unlock(queue->lock);
- rust_spinlock_unlock(queue->lock);
- rust_spinlock_free(queue->lock);
+ rust_spinlock_unlock(queue->lock);
Also applies to: 133-134, 138-139
🤖 Prompt for AI Agents
kernel/ipc/Ipc.c lines 92-93 (and similarly 133-134, 138-139): the code is
freeing queue->lock on every unlock which leaves queue->lock non-NULL pointing
to freed storage, causing use-after-free/double-free; remove the
rust_spinlock_free(...) calls at these locations so the spinlock remains valid
across unlock/lock cycles, and instead ensure the spinlock is only freed when
the queue is destroyed (or if you prefer freeing here, set queue->lock = NULL
after free and reallocate on next use) — implement the simpler fix by deleting
the per-unlock rust_spinlock_free calls and verify destruction code frees the
spinlock exactly once.
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores