Add API test and improve memory safety in MMIO constructors#54
Add API test and improve memory safety in MMIO constructors#54phip1611 wants to merge 2 commits intorust-osdev:mainfrom
Conversation
Having such tests is useful. Without it, it could happen that `Backend` isn't exported anymore - which would still enable to create instances via the constructors but one can't create bindings anymore. This test therefore protects against that. The test succeeds if the test compiles.
Casting an integer address to a raw pointer with `0x1000 as *mut u8`
produces a pointer with unspecified provenance in Rust's abstract
machine, which is technically unsound even if it works in practice.
The correct, stabilized API for constructing pointers to memory that
lives outside the Rust abstract machine (such as MMIO regions) is
ptr::with_exposed_provenance_mut(), introduced in Rust 1.84. Combined
with NonNull, this makes the provenance model explicit and well-defined.
let mmio_address = ptr::with_exposed_provenance_mut::<u8>(0x1000);
let mmio_address = NonNull::new(mmio_address).unwrap();
This is a breaking change: callers must now pass NonNull<u8> instead of
*mut u8. As a consequence, null-pointer validation is now enforced by
the type system rather than at runtime, so InvalidAddressError::Null has
been removed.
|
After that, I'd ship |
mkroening
left a comment
mkroening
left a comment
There was a problem hiding this comment.
The code looks good to me, but the justification is not quite correct.
The previous
0x1000 as *mut u8cast produced a pointer with unspecified provenance, which is unsound under Rust's abstract machine even if it works in practice.
This is not true. addr as *mut T is fully equivalent to ptr::with_exposed_provenance_mut(addr), according to its documentation. I think it is more about being explicit about making up a provenance.
The whole situation is a bit confusing, to be honest, because for volatile operations on memory that is outside any Rust allocation (such as NULL), the pointer does not have to be valid for the corresponding non-volatile operation and does not need provenance. For details, see the second bullet in ptr::read_volatile as well as rust-osdev/volatile#69.
2 participants
This PR makes two changes to the
uart_16550crate.First, a compile-only API test is added to guard against accidental breakage of public exports. Without it, removing
Backendfrom the public API would go undetected as long as direct constructors still compiled.Second,
new_mmio()on bothUart16550andUart16550Ttynow takesNonNull<u8>instead of*mut u8. The previous0x1000 as *mut u8cast produced a pointer with unspecified provenance, which is unsound under Rust's abstract machine even if it works in practice. The correct approach since Rust 1.84 isptr::with_exposed_provenance_mut(), which makes provenance explicit for addresses pointing outside the Rust abstract machine — exactly the MMIO use case.This is a breaking change: callers must update their pointer construction accordingly. As a side effect, null-pointer validation moves from runtime to the type system, so
InvalidAddressError::Nullhas been removed.Closes #52.