Re: [TECH TOPIC] Rust for Linux

[Wedson Almeida Filho <wedsonaf@google.com>]

On Mon, Jul 19, 2021 at 10:37:52PM +0300, Laurent Pinchart wrote:
> On Mon, Jul 19, 2021 at 07:06:36PM +0100, Wedson Almeida Filho wrote:
> > On Mon, Jul 19, 2021 at 06:02:06PM +0200, Vegard Nossum wrote:
> > > On 7/19/21 3:15 PM, Wedson Almeida Filho wrote:
> > > > On Mon, Jul 19, 2021 at 01:24:49PM +0100, Wedson Almeida Filho wrote:
> > > >> On Fri, Jul 09, 2021 at 12:13:25AM +0200, Linus Walleij wrote:
> > > >>> I have seen that QEMU has a piece of code for the Arm PrimeCell
> > > >>> PL061 GPIO block which corresponds to drivers/gpio/gpio-pl061.c
> > > >>> Note that this hardware apart from being used in all Arm reference
> > > >>> designs is used on ARMv4T systems that are not supported by
> > > >>> LLVM but only GCC, which might complicate things.
> > > >>
> > > >> Here is a working PL061 driver in Rust (converted form the C one):
> > > >> https://raw.githubusercontent.com/wedsonaf/linux/pl061/drivers/gpio/gpio_pl061_rust.rs
> > > > 
> > > > I'm also attaching an html rending of the C and Rust versions side by side where
> > > > I try to line the definitions up to make it easier to contrast the two
> > > > implementations.
> > > 
> > > This is really cool :-) As a Rust noob, I have a few questions:
> > > 
> > > 1. I'm curious about some of the writeb() vs. try_writeb() calls:
> > > 
> > > fn direction_output(data: &Ref<DeviceData>, offset: u32, value: bool) ->
> > > Result {
> > >         let woffset = bit(offset + 2).into();
> > >         let _guard = data.lock();
> > >         let pl061 = data.resources().ok_or(Error::ENXIO)?;
> > >         pl061.base.try_writeb((value as u8) << offset, woffset)?;
> > >         let mut gpiodir = pl061.base.readb(GPIODIR);
> > >         gpiodir |= bit(offset);
> > >         pl061.base.writeb(gpiodir, GPIODIR);
> > > 
> > >         // gpio value is set again, because pl061 doesn't allow to set
> > > value of a gpio pin before
> > >         // configuring it in OUT mode.
> > >         pl061.base.try_writeb((value as u8) << offset, woffset)?;
> > >         Ok(())
> > >     }
> > > 
> > > Here you have try_writeb() (and error return) where there was just a
> > > writeb() without any error handling in the C version. Is this what
> > > Miguel was answering a bit down the thread where the address is computed
> > > ((value as u8) << offset) so it _needs_ to use the try_() version?
> > 
> > The `writeb` variant only works when we know at compile-time that the offset is
> > within bounds (the compiler will reject the code otherwise). When the value is
> > computed at runtime we use a `try` version that checks before performing the
> > write. We need this to guarantee memory safety.
> > 
> > > If offset can be anything but a "correct" value here, should there be a
> > > check for that somewhere else and then the computed value can be
> > > subsequently treated as safe (i.e. there's a second try_writeb() in the
> > > function that now presumably does the runtime check a second time,
> > > redundantly)?
> > 
> > Oh, that's a neat idea. We can certainly implement something like this:
> > 
> > let woffset = pl061.base.vet_offsetb(bit(offset + 2))?;
> > 
> > Then woffset would be passed to writeb variants that are guaranteed to succeed.
> > (Rust helps us ensure that woffset cannot change without checks, which would be
> > harder to do in C.)
> > 
> > > 2. In many places you have the C code:
> > > 
> > > struct pl061 *pl061 = dev_get_drvdata(dev);
> > > 
> > > with the equivalent Rust code as:
> > > 
> > > let pl061 = data.resources().ok_or(Error::ENXIO)?;
> > > 
> > > Why doesn't the C code need to check for errors here? Or put
> > > differently, why can the Rust version fail?
> > 
> > There are two aspecs worth noting here:
> > 1. In C there is cast from void * to struct pl061 * without really knowing if
> > the stored pointer is of the right type. For example, if I simply change the
> > struct type to say `struct mutex` in the code above, it will still compile,
> > though it will be clearly wrong. In Rust we prevent this by not exposing drvdata
> > directly to drivers, and using type-specialised functions to set/get drvdata, so
> > it *knows* that the type is right. So in this sense Rust is better because it
> > offers type guarantees without additional runtime cost. (In Rust, if you change
> > the type of the function to say `&Mutex`, it won't compile.
> > 
> > 2. The extra check we have here is because of a feature that the C code doesn't
> > have: revocable resources. If we didn't want to have this, we could do say
> > `data.base.writeb(...)` directly, but then we could have situations where `base`
> > is used after the device was removed. By having these checks we guarantee that
> > anyone can hold a reference to device state, but they can no longer use hw
> > resources after the device is removed.
> 
> If the driver reached a code path with an I/O write after .remove()
> returns, the game is likely over already. It would be more interesting
> to see how we could prevent that from happening in the first place.
> Checking individual I/O writes at runtime will not only add additional
> CPU costs, but will also produce code paths that are not well tested.

You may be conflating checking offsets in individual writes/reads with accessing
hw resources. Note that these are different things.

> It
> feels that we're inventing a problem just to be able to showcase the
> solution :-)

Thanks for taking a look. I beg to differ though, as this solves (on the Rust
side) a problem you described the other day on this very thread. The solution is
different from what you propose though :)

- The internal data structures of drivers are refcounted. Drivers then share
  this internal representation with other subsystems (e.g., cdev).
- On `remove`, the registrations with other subsystems are removed (so no
  additional sharing of internal data should happen), but existing calls and
  references to internal data structures continue to exist. This part is
  important: we don't "revoke" the references, but we do revoke the hw resources
  part of the internal state.
- Attempts to access hardware resources freed during `remove` *must* be
  prevented, that's where the calls to `resources()` are relevant -- if a
  subsystem calls into the driver with one of the references it held on to, they
  won't be able to access the (already released) hw resources.

We have this problem specifically in gpio: as Linus explained, they created an
indirection via a pointer which is checked in most entry points, but there is no
synchronisation that guarantees that the pointer will remain valid during a
call, and nothing forces uses of the pointer to be checked (so as Linus points
out, they may need more checks).

For Rust drivers, if the registration with other subsystems were done by doing
references to driver data in Rust, this extra "protection" (that has race
conditions that, timed correctly, lead to use-after-free vulnerabilities) would
be obviated; all would be handled safely on the Rust side (e.g., all accesses
must be checked, there is no way to get to resources without a check, and use of
the resources is guarded by a guard that uses RCU read-side lock).

Do you still think we don't have a problem?