Re: [PATCH v5 05/20] rust: io: generalize `MmioRaw` to pointer to arbitrary type

[sashiko-bot@kernel.org]

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 <gary@garyguo.net>

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/20260626-io_projection-v5-0-d0961471ae50@garyguo.net?part=5