Re: [TECH TOPIC] Rust for Linux

Wedson Almeida Filho
2021-07-21T04:39:38Z

On Tue, Jul 20, 2021 at 06:33:38PM -0700, Andy Lutomirski wrote:
> On Tue, Jul 20, 2021 at 4:54 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi Wedson,
> >
> > On Mon, Jul 19, 2021 at 10:09:46PM +0100, Wedson Almeida Filho wrote:
> > > 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.
> >
> > Yes, it's the data.resources().ok_or() that I was talking about, not the
> > I/O writes, sorry.
> >
> > > > 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).
> >
> > Refcounting the driver-specific structure is good, that matches what I
> > proposed (it's of course implemented differently in C and rust, but
> > that's expected).
> >
> > > - 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.
> >
> > No issue here either. The handling of the internal data structure (the
> > "non-revoke" part to be precise) matches my proposal too I believe.
> > Revoking the I/O memory is of course rust-specific.
> >
> > > - 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.
> >
> > That's where our opinions differ. Yes, those accesses must be prevented,
> > but I don't think the right way to do so is to check if the I/O memory
> > resource is still valid. We should instead prevent reaching driver code
> > paths that make those I/O accesses, by waiting for all calls in progress
> > to return, and preventing new calls from being made. This is a more
> > generic solution in the sense that it doesn't prevent accessing I/O
> > memory only, but avoids any operation that is not supposed to take
> > place. My reasoning is that drivers will be written with the assumption
> > that, for instance, nobody will try to set the GPIO direction once
> > .remove() returns. Even if the direction_output() function correctly
> > checks if the I/O memory is available and returns an error if it isn't,
> > it may also contain other logic that will not work correctly after
> > .remove() as the developer will not have considered that case. This
> > uncorrect logic may or may not lead to bugs, and some categories of bugs
> > may be prevented by rust (such as accessing I/O memory after .remove()),
> > but I don't think that's relevant. The subsystem, with minimal help from
> > the driver's implementation of the .remove() function if necessary,
> > should prevent operations from being called when they shouldn't, and
> > especially when the driver's author will not expect them to be called.
> > That way we'll address whole classes of issues in one go. And if we do
> > so, checking if I/O memory access has been revoked isn't required
> > anymore, as we guarantee if isn't.
> >
> > True, this won't prevent I/O memory from being accessed after .remove()
> > in other contexts, for instance in a timer handler that the driver would
> > have registered and forgotten to cancel in .remove(). And maybe the I/O
> > memory revoking mechanism runtime overhead may be a reasonable price to
> > pay for avoiding this, I don't know. I however believe that regardless
> > of whether I/O memory is revoked or not, implementing a mechanism in the
> > subsytem to avoid erroneous conditions from happening in the first place
> > is where we'll get the largest benefit with a (hopefully) reasonable
> > effort.
>
> Preventing functions from being called, when those functions are
> scattered all over the place (sysfs, etc) may be hard. Preventing
> access to a resource seems much more tractable. That being said,
> preventing access to the I/O resource in particular seems a bit odd to
> me. It's really the whole device that's gone after it has been
> removed. So maybe the whole device (for some reasonable definition of
> "whole device") could get revoked?

(Some details that I omitted from the original email for brevity.)

I actually split device state into three categories:
1. Registrations: state needed by other subsystems (e.g., `cdev`, `gpio_chip`)
2. Resources: part of the device state that is revoked when .remove() completes.
3. General: state available to anyone who holds a reference to the state.

The above is expressed in the following line on the driver:
type DeviceData = device::Data<PL061Registrations, PL061Resources, IrqDisableSpinLock<PL061Data>>;

Developers are free to put whatever they want in each of the categories. What
happens automatically when a device is about to be removed is:
1. .remove() callback is called, if one exists.
2. Registrations are dropped (e.g., subsystems are called to unregister previous
successful registrations)
3. Resources are revoked then dropped (e.g., io mem is unmapped).

[This happens automatically in Rust, even if a driver doesn't implement
.remove()]

The reason I split registration & resources is because I wanted to guarantee
that the latter are only revoked after registrations are gone. (Because we may
need the resources while unregistration is in progress.)

The reason there is a split between a revocable and non-revocable part is that
we could have things like links of intrusive data structures that code holding
on to references may need to have access to even after .remove().