Message-ID: <Zpq8b0I2kYeAh2qm@pollux>
Date: Fri, 19 Jul 2024 21:20:15 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Daniel Almeida <daniel.almeida@...labora.com>
Cc: Benno Lossin <benno.lossin@...ton.me>, Jonathan Corbet <corbet@....net>,
	Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Wedson Almeida Filho <wedsonaf@...il.com>,
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Andreas Hindborg <a.hindborg@...sung.com>,
	Alice Ryhl <aliceryhl@...gle.com>, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [RFC PATCH 1/5] doc: rust: create safety standard

On Fri, Jul 19, 2024 at 03:09:09PM -0300, Daniel Almeida wrote:
> Hi Danilo,
> 
> 
> > 
> > We can easily build abstractions that ensure that the address a driver is trying
> > to access is mapped properly, such that you can't have accidential out-of-bound
> > accesses.
> > 
> > Those can be implemented by the corresponding subsystem / bus that the resource
> > originates from.
> > 
> > In fact, we already have abstractions for that on the way, a generic I/O
> > abstraction [1] as base implementation and a specific abstraction for PCI bars
> > [2].
> > 
> > Of course, if the MMIO region comes from let's say the device tree, we still
> > have to assume that the information in the DT is correct, but the driver does
> > not need unsafe code for this.
> > 
> > [1] https://lore.kernel.org/rust-for-linux/20240618234025.15036-8-dakr@redhat.com/
> > [2] https://lore.kernel.org/rust-for-linux/20240618234025.15036-11-dakr@redhat.com/
> > 
> 
> Thanks for pointing that out. So from this:
> 
> +impl<const SIZE: usize> Io<SIZE> {
> + ///
> + ///
> + /// # Safety
> + ///
> + /// Callers must ensure that `addr` is the start of a valid I/O mapped memory region of size
> + /// `maxsize`.
> + pub unsafe fn new(addr: usize, maxsize: usize) -> Result<Self> {
> + if maxsize < SIZE {
> + return Err(EINVAL);
> + }
> +
> + Ok(Self { addr, maxsize })
> + }
> 
> It looks like one can do this:
> 
> let io = unsafe { Io::new(<some address>, <some size>)? }; 
> let value = io.readb(<some offset>)?;
> 
> Where <some address> has already been mapped for <some size> at an earlier point?

Yes, but (at least for full Rust drivers) this shouldn't be called by the driver
directly, but the corresponding subsystem / bus should provide a `Devres`
wrapped `Io` instance, like the PCI abstraction in [2] does.

Example:

```
// Get a `Devres` managed PCI bar mapping
let bar: Devres<pci::Bar> = pdev.iomap_region(0);

let reg = bar.try_readl(0x42)?;
```
You can also let the driver assert that the requested resource must have a
minimum size:

```
// Only succeeds if the PCI bar has at least a size of 0x1000
let bar = pdev.iomap_region_size::<0x1000>(0);

// Note: `readl` does not need to return a `Result`, since the boundary checks
// can be done on compile time due to the driver specified minimal mapping size.
let reg = bar.readl(0x42);
```

> 
> That’s fine, as I said, if an abstraction makes sense, I have nothing
> against it. My point is more that we shouldn’t enact a blanket ban on
> 'unsafe' in drivers because corner cases do exist. But it’s good to know that this
> particular example I gave does not apply.
> 
> 
> >> 
> >> If a driver is written partially in Rust, and partially in C, and it gets a
> >> pointer to some kcalloc’d memory in C, should It be forbidden to use unsafe
> >> in order to build a slice from that pointer? How can you possibly design a
> >> general abstraction for something that is, essentially, a driver-internal API?
> > 
> > That sounds perfectly valid to me.
> > 
> 
> 
> — Daniel
> 
> 

Powered by blists - more mailing lists