linux-kernel - Re: [PATCH v4 06/13] 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAH5fLgg4wxyar_2uPfUJ=Bcc5=SVWOoYWvoC3ieVd9ayakiopQ@mail.gmail.com>
Date: Fri, 6 Dec 2024 15:13:14 +0100
From: Alice Ryhl <aliceryhl@...gle.com>
To: Danilo Krummrich <dakr@...nel.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, 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, fabien.parent@...aro.org, chrisi.schrefl@...il.com, 
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-pci@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v4 06/13] rust: add `io::{Io, IoRaw}` base types

On Thu, Dec 5, 2024 at 3:16 PM Danilo Krummrich <dakr@...nel.org> wrote:
>
> I/O memory is typically either mapped through direct calls to ioremap()
> or subsystem / bus specific ones such as pci_iomap().
>
> Even though subsystem / bus specific functions to map I/O memory are
> based on ioremap() / iounmap() it is not desirable to re-implement them
> in Rust.
>
> Instead, implement a base type for I/O mapped memory, which generically
> provides the corresponding accessors, such as `Io::readb` or
> `Io:try_readb`.
>
> `Io` supports an optional const generic, such that a driver can indicate
> the minimal expected and required size of the mapping at compile time.
> Correspondingly, calls to the 'non-try' accessors, support compile time
> checks of the I/O memory offset to read / write, while the 'try'
> accessors, provide boundary checks on runtime.
>
> `IoRaw` is meant to be embedded into a structure (e.g. pci::Bar or
> io::IoMem) which creates the actual I/O memory mapping and initializes
> `IoRaw` accordingly.
>
> To ensure that I/O mapped memory can't out-live the device it may be
> bound to, subsystems must embed the corresponding I/O memory type (e.g.
> pci::Bar) into a `Devres` container, such that it gets revoked once the
> device is unbound.
>
> Signed-off-by: Danilo Krummrich <dakr@...nel.org>

One nit below. With that addressed:

Reviewed-by: Alice Ryhl <aliceryhl@...gle.com>

> +impl<const SIZE: usize> Io<SIZE> {
> +    /// Converts an `IoRaw` into an `Io` instance, providing the accessors to the MMIO mapping.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must ensure that `addr` is the start of a valid I/O mapped memory region of size
> +    /// `maxsize`.
> +    pub unsafe fn from_raw<'a>(raw: &IoRaw<SIZE>) -> &'a Self {

I would use this signature:

pub unsafe fn from_raw(raw: &IoRaw<SIZE>) -> &Self;

Otherwise, you're saying that the returned reference is allowed to
outlive the IoRaw instance, which wouldn't be okay.

Alice