Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [High] `IoMem::ioremap` dynamically rejects valid hardware resources whose size is not a multiple of 4 or whose base address is unaligned. - [Medium] The infallible 64-bit accessors (`read64`, `write64`, etc.) fail to compile on 64-bit platforms when used with `Mmio` due to a flawed static alignment assertion. - [Low] Missing `// INVARIANT:` comment when returning a validated pointer establishing the `Region` struct's invariants. Pre-existing issues: - [High] The virtual address mapped by `ioremap` is leaked if `MmioRaw::new_region` fails. -- commit 4ac7791c4c497b9e3e333241d199ec287a09b395 Author: Gary Guo <[email protected]> rust: io: generalize MmioRaw to pointer to arbitrary type Conceptually, MmioRaw is just __iomem *, so it should work for any types. Update the existing use case where it represents a region of compile-time known minimum size and run-time known actual size to use the dynamic-sized type Region<SIZE> instead. Rename maxsize method to reflect that it is the actual size (not a bound) of the region. > diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs > index 9f060dd291826..12be266d7ed7e 100644 > --- a/rust/kernel/io.rs > +++ b/rust/kernel/io.rs [ ... ] > +impl<const SIZE: usize> MmioRaw<Region<SIZE>> { > + /// Create a MmioRaw representing a I/O region with given size. > + /// > + /// The size is checked against the minimum size specified via const > generics. > + #[inline] > + pub fn new_region(addr: usize, size: usize) -> Result<Self> { > + Ok(Self { > + ptr: > Region::ptr_try_from_raw_parts_mut(core::ptr::without_provenance_mut(addr), > size)?, > + }) > + } > +} [Severity: Low] Is there a missing // INVARIANT: comment when returning a validated pointer establishing the Region struct's invariants? The Region type has a documented # Invariants section. Region::ptr_try_from_raw_parts_mut() validates these invariants and returns a pointer that establishes them, but seems to lack the mandatory comment explaining how the checks satisfy the invariants: rust/kernel/io.rs:Region::ptr_try_from_raw_parts_mut() { ... Ok(Self::ptr_from_raw_parts_mut(base, size)) ... } [ ... ] > @@ -193,7 +224,7 @@ pub fn maxsize(&self) -> usize { > /// # } > /// ``` > #[repr(transparent)] > -pub struct Mmio<const SIZE: usize = 0>(MmioRaw<SIZE>); > +pub struct Mmio<const SIZE: usize = 0>(MmioRaw<Region<SIZE>>); [Severity: Medium] Can this fail to compile on 64-bit platforms when using infallible 64-bit accessors like read64() or write64()? When U is u64 (alignment 8 on 64-bit systems) and the Target is Region<SIZE>, Region specifies #[repr(C, align(4))] which sets MIN_ALIGN to 4. This causes the assertion strictly bounding the requested type's alignment to evaluate to false: rust/kernel/io.rs:io_view_assert() { ... const_assert!(Alignment::of::<U>().as_usize() <= IO::Target::MIN_ALIGN.as_usize()); ... } [ ... ] > diff --git a/rust/kernel/io/mem.rs b/rust/kernel/io/mem.rs > index fc2a3e24f8d5d..9e15bc8fde784 100644 > --- a/rust/kernel/io/mem.rs > +++ b/rust/kernel/io/mem.rs [ ... ] > @@ -264,8 +264,7 @@ fn ioremap(dev: &'a Device<Bound>, resource: &Resource) > -> Result<Self> { > return Err(ENOMEM); > } > > - let io = MmioRaw::new(addr as usize, size)?; > - > + let io = MmioRaw::new_region(addr as usize, size)?; [Severity: High] Could this unconditionally reject valid hardware resources whose size is not a multiple of 4 or whose base address is unaligned? MmioRaw::new_region() calls Region::ptr_try_from_raw_parts_mut() which enforces overly restrictive alignment and size checks. Device drivers probing hardware with I/O regions like 1-byte RTCs or 2-byte serial ports will fail to load: rust/kernel/io.rs:Region::ptr_try_from_raw_parts_mut() { ... if size < SIZE || base.align_offset(4) != 0 || !size.is_multiple_of(4) { return Err(EINVAL); } ... } [Severity: High] This isn't a bug introduced by this patch, but does this code leak the virtual address mapped by bindings::ioremap() if MmioRaw::new_region() fails? The ? operator here immediately propagates the error. Unlike Bar::new() which explicitly cleans up with bindings::pci_iounmap() on failure, this path fails to call bindings::iounmap() before returning. > Ok(IoMem { dev, io }) > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
