linux-kernel - Re: [PATCH v3 09/16] rust: add `io::Io` base type

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: <CAH5fLggFD7pq0WCfMPYTZcFkvrXajPbxTBtkvSeh-N2isT1Ryw@mail.gmail.com>
Date: Mon, 28 Oct 2024 16:43:02 +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, 
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-pci@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v3 09/16] rust: add `io::Io` base type

On Tue, Oct 22, 2024 at 11:33 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.

And using zero works because the user then statically knows that zero
bytes are available ... ?

> `Io` 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
> `Io` accordingly.
>
> To ensure that I/O mapped memory can't out-live the device it may be
> bound to, subsystems should embedd the corresponding I/O memory type
> (e.g. pci::Bar) into a `Devres` container, such that it gets revoked
> once the device is unbound.

I wonder if `Io` should be a reference type instead. That is:

struct Io<'a, const SIZE: usize> {
    addr: usize,
    maxsize: usize,
    _lifetime: PhantomData<&'a ()>,
}

and then the constructor requires "addr must be valid I/O mapped
memory for maxsize bytes for the duration of 'a". And instead of
embedding it in another struct, the other struct creates an `Io` on
each access?

> Co-developed-by: Philipp Stanner <pstanner@...hat.com>
> Signed-off-by: Philipp Stanner <pstanner@...hat.com>
> Signed-off-by: Danilo Krummrich <dakr@...nel.org>

[...]

> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> new file mode 100644
> index 000000000000..750af938f83e
> --- /dev/null
> +++ b/rust/kernel/io.rs
> @@ -0,0 +1,234 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Memory-mapped IO.
> +//!
> +//! C header: [`include/asm-generic/io.h`](srctree/include/asm-generic/io.h)
> +
> +use crate::error::{code::EINVAL, Result};
> +use crate::{bindings, build_assert};
> +
> +/// IO-mapped memory, starting at the base address @addr and spanning @maxlen bytes.
> +///
> +/// The creator (usually a subsystem / bus such as PCI) is responsible for creating the
> +/// mapping, performing an additional region request etc.
> +///
> +/// # Invariant
> +///
> +/// `addr` is the start and `maxsize` the length of valid I/O mapped memory region of size
> +/// `maxsize`.

Do you not also need an invariant that `SIZE <= maxsize`?

Alice