add kernel sync primitives - #684
Conversation
|
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
Pull request overview
This PR adds a new wdk::sync module providing safe, RAII-based wrappers around common kernel-mode shared/exclusive synchronization primitives for WDM/KMDF, along with WDM sample usage and test coverage via wdk-sys test stubs.
Changes:
- Introduces
wdk::sync::{RwLock, PushLock, RwSpinLock}with guard types and IRQL/critical-region handling. - Updates
wdk-systest stubs and WDM config tests to enable exercising these primitives in unit tests. - Extends the sample WDM driver to demonstrate the new locks and improves
UNICODE_STRINGhandling.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/config-wdm/tests/rw_lock_tests.rs | Adds WDM config tests covering guard access and get_mut for the new sync wrappers. |
| tests/config-wdm/Cargo.toml | Adds dev-deps on wdk/wdk-sys(test-stubs) and a nightly feature passthrough. |
| tests/config-wdm/Cargo.lock | Updates lockfile to include the new dependency graph (lockfile v4). |
| examples/sample-wdm-driver/src/lib.rs | Demonstrates RwLock, PushLock, and RwSpinLock; refines registry path parsing. |
| crates/wdk/src/sync/rw_spin_lock.rs | Implements RwSpinLock<T> backed by EX_SPIN_LOCK with IRQL-restoring guards. |
| crates/wdk/src/sync/rw_lock.rs | Implements RwLock<T> backed by heap-allocated ERESOURCE with read/write guards and try-acquire. |
| crates/wdk/src/sync/push_lock.rs | Implements PushLock<T> backed by EX_PUSH_LOCK with critical-region-managed guards. |
| crates/wdk/src/sync/mod.rs | Adds module-level docs and re-exports for the new sync primitives. |
| crates/wdk/src/lib.rs | Exposes wdk::sync (WDM/KMDF only) and enables alloc at crate root for those targets. |
| crates/wdk-sys/src/test_stubs.rs | Adds stubs for the kernel routines needed to link/run tests for the new wrappers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
62f09d1 to
6bbf7bc
Compare
6bbf7bc to
66679d3
Compare
|
Addressed the Copilot review feedback and folded the cleanup into the existing feature commit.
|
|
Addressed the latest review feedback in f2f74d4.
I could not run cargo fmt/test/clippy locally because the available environment does not have a Rust/Cargo toolchain installed; CI should provide the executable verification. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: J.K Lee <47030112+ntoskrnl7@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
crates/wdk/src/sync/rw_spin_lock.rs:97
- The safety contract for
read_at_dpc_levelsays the caller must already be atDISPATCH_LEVEL, but it should also require that IRQL remains atDISPATCH_LEVELuntil the guard is dropped (sinceDropusesExReleaseSpinLock*FromDpcLevel). Please document this explicitly to prevent safe-looking misuse where IRQL is lowered before the guard is dropped.
This issue also appears on line 134 of the same file.
/// Lock this `RwSpinLock` with shared read access at `DISPATCH_LEVEL`.
///
/// # Safety
///
/// The caller must already be running at `DISPATCH_LEVEL`. The returned
/// guard will not restore IRQL when it is dropped.
#[must_use]
pub unsafe fn read_at_dpc_level(&self) -> RwSpinLockReadGuard<'_, T> {
crates/wdk/src/sync/rw_spin_lock.rs:141
- Similarly to
read_at_dpc_level, the safety docs forwrite_at_dpc_levelshould state that the caller must remain atDISPATCH_LEVELfor the entire lifetime of the returned guard (until it is dropped), because theDroppath releases the lock using the*FromDpcLevelroutine.
/// Lock this `RwSpinLock` with exclusive write access at `DISPATCH_LEVEL`.
///
/// # Safety
///
/// The caller must already be running at `DISPATCH_LEVEL`. The returned
/// guard will not restore IRQL when it is dropped.
#[must_use]
pub unsafe fn write_at_dpc_level(&self) -> RwSpinLockWriteGuard<'_, T> {
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: J.K Lee <47030112+ntoskrnl7@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The DPC-level tests do not verify safety-critical acquire/release routing or saved-IRQL propagation.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tests/config-wdm/tests/sync_tests.rs:134
- The spin-lock stubs are all no-ops, so this test only validates value access; it still passes if a guard uses the wrong release routine or fails to propagate the saved IRQL. Since mismatching the normal and DPC-level release paths is safety-critical, make the stubs record each acquire/release variant (and old IRQL) and assert the matching release when both guards are dropped.
- Files reviewed: 11/12 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Repurposing the legacy no-op alloc feature can break existing allocator-free kernel drivers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 11/12 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
The new unsafe kernel synchronization abstractions and IRQL-sensitive behavior warrant final human review despite comprehensive stub-based tests.
Review details
- Files reviewed: 11/12 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The safe RwSpinLock restoring APIs can violate kernel IRQL ordering requirements.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
tests/config-wdm/tests/sync_tests.rs:100
- This test cannot detect missing or mismatched push-lock operations: all four
ExAcquire/ExReleasePushLock*Exstubs are no-ops (crates/wdk-sys/src/test_stubs.rs:261-276), so it would still pass ifPushLock::read/writenever acquired or released the kernel lock. Please instrument the stubs and assert the shared/exclusive acquire-release sequences, as theRwSpinLocktests do; ideally include the required critical-region enter/leave pairing too.
fn push_lock_read_and_write_guards_access_value() {
let lock = PushLock::new(1_u32);
assert_eq!(*lock.read(), 1);
- Files reviewed: 11/12 changed files
- Comments generated: 1
- Review effort level: Balanced
Summary
Adds safe Rust synchronization wrappers for common kernel-mode shared/exclusive locks:
wdk::sync::RwLock<T>backed byERESOURCEwdk::sync::PushLock<T>backed byEX_PUSH_LOCKwdk::sync::RwSpinLock<T>backed byEX_SPIN_LOCKThe wrappers provide RAII read/write guards, document IRQL constraints, and expose
get_mutfor exclusive mutable access without locking.Details
RwLockallocates the backingERESOURCEseparately so the kernel object has a stable address for its lifetime.PushLockwraps the modernExAcquirePushLock*ExAPIs.RwSpinLockincludes normal acquire methods that restore IRQL on drop plus*_at_dpc_levelmethods for callers already atDISPATCH_LEVEL.The WDM sample driver now demonstrates all three wrappers. WDM config tests cover guard access and
get_mut, with matchingwdk-systest stubs for the kernel routines used by these wrappers.Validation
cargo +1.91.0 check -p wdkcargo +1.91.0 test --manifest-path tests\config-wdm\Cargo.tomlcargo +1.91.0 build --manifest-path examples\sample-wdm-driver\Cargo.tomlcargo +1.91.0 clippy -p wdk --all-targets -- -D warningscargo +1.91.0 clippy --manifest-path tests\config-wdm\Cargo.toml --all-targets -- -D warningscargo +1.91.0 clippy --manifest-path examples\sample-wdm-driver\Cargo.toml --all-targets -- -D warnings -A clippy::multiple-crate-versionsThe sample driver clippy command allows
multiple-crate-versionsbecause the current dependency graph includeswindows-systhrough bothfs4and build-time dependencies.