linux-kernel - Re: [PATCH v6 07/16] rust: add `io::{Io, IoRaw}` base types

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Z2LGWFXx8mjGvH8-@pollux>
Date: Wed, 18 Dec 2024 13:55:52 +0100
From: Danilo Krummrich <dakr@...nel.org>
To: Fabien Parent <fabien.parent@...aro.org>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, bhelgaas@...gle.com,
	ojeda@...nel.org, alex.gaynor@...il.com, boqun.feng@...il.com,
	gary@...yguo.net, bjorn3_gh@...tonmail.com, benno.lossin@...ton.me,
	tmgross@...ch.edu, a.hindborg@...sung.com, aliceryhl@...gle.com,
	airlied@...il.com, fujita.tomonori@...il.com, lina@...hilina.net,
	pstanner@...hat.com, ajanulgu@...hat.com, lyude@...hat.com,
	robh@...nel.org, daniel.almeida@...labora.com, saravanak@...gle.com,
	dirk.behme@...bosch.com, j@...nau.net, chrisi.schrefl@...il.com,
	paulmck@...nel.org, rust-for-linux@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
	devicetree@...r.kernel.org, rcu@...r.kernel.org
Subject: Re: [PATCH v6 07/16] rust: add `io::{Io, IoRaw}` base types

On Tue, Dec 17, 2024 at 12:10:38PM -0800, Fabien Parent wrote:
> Hi Danilo,
> 
> > +/// ```no_run
> > +/// # use kernel::{bindings, io::{Io, IoRaw}};
> > +/// # use core::ops::Deref;
> > +///
> > +/// // See also [`pci::Bar`] for a real example.
> > +/// struct IoMem<const SIZE: usize>(IoRaw<SIZE>);
> > +///
> > +/// impl<const SIZE: usize> IoMem<SIZE> {
> > +///     /// # Safety
> > +///     ///
> > +///     /// [`paddr`, `paddr` + `SIZE`) must be a valid MMIO region that is mappable into the CPUs
> > +///     /// virtual address space.
> > +///     unsafe fn new(paddr: usize) -> Result<Self>{
> > +///         // SAFETY: By the safety requirements of this function [`paddr`, `paddr` + `SIZE`) is
> > +///         // valid for `ioremap`.
> > +///         let addr = unsafe { bindings::ioremap(paddr as _, SIZE.try_into().unwrap()) };
> 
> This line generates a warning when building the doctests on arm64:
> 
> warning: useless conversion to the same type: usize
>     --> rust/doctests_kernel_generated.rs:3601:59
>      |
> 3601 |         let addr = unsafe { bindings::ioremap(paddr as _,
> SIZE.try_into().unwrap()) };
>      |                                                           ^^^^^^^^^^^^^^^
>      |
>      = help: consider removing .try_into()
>      = help: for further information visit
> https://rust-lang.github.io/rust-clippy/master/index.html#useless_conversion
> 
> Same things happens as well in devres.rs

I think that's because arch specific ioremap() implementations sometimes use
unsigned long and sometimes size_t.

I think we can just change this line to

`let addr = unsafe { bindings::ioremap(paddr as _, SIZE as _) };`

instead.

- Danilo

> 
> > +///         if addr.is_null() {
> > +///             return Err(ENOMEM);
> > +///         }
> > +///
> > +///         Ok(IoMem(IoRaw::new(addr as _, SIZE)?))
> > +///     }
> > +/// }
> > +///
> > +/// impl<const SIZE: usize> Drop for IoMem<SIZE> {
> > +///     fn drop(&mut self) {
> > +///         // SAFETY: `self.0.addr()` is guaranteed to be properly mapped by `Self::new`.
> > +///         unsafe { bindings::iounmap(self.0.addr() as _); };
> > +///     }
> > +/// }
> > +///
> > +/// impl<const SIZE: usize> Deref for IoMem<SIZE> {
> > +///    type Target = Io<SIZE>;
> > +///
> > +///    fn deref(&self) -> &Self::Target {
> > +///         // SAFETY: The memory range stored in `self` has been properly mapped in `Self::new`.
> > +///         unsafe { Io::from_raw(&self.0) }
> > +///    }
> > +/// }
> > +///
> > +///# fn no_run() -> Result<(), Error> {
> > +/// // SAFETY: Invalid usage for example purposes.
> > +/// let iomem = unsafe { IoMem::<{ core::mem::size_of::<u32>() }>::new(0xBAAAAAAD)? };
> > +/// iomem.writel(0x42, 0x0);
> > +/// assert!(iomem.try_writel(0x42, 0x0).is_ok());
> > +/// assert!(iomem.try_writel(0x42, 0x4).is_err());
> > +/// # Ok(())
> > +/// # }
> > +/// ```