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: <04E0C753-9D96-4D99-992E-63E36C0F1904@collabora.com>
Date: Wed, 6 Nov 2024 20:31:44 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Danilo Krummrich <dakr@...nel.org>,
 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,
 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

Hi,

> On 28 Oct 2024, at 12:43, Alice Ryhl <aliceryhl@...gle.com> wrote:
> 
> 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?

Please let’s not add a lifetime here. Drivers will usually have this mapped
at probe time and it will usually remain mapped until remove(), so I think this
works fine the way it is.

> 
>> 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