Simple Rust driver that touches real hardware: PR 5

[TheSven73]

Simple Rust driver that touches real h/w: step 5

@alex and @ojeda suggested that #254 should be split into smaller PRs, so they can be reviewed and merged individually.

Roadmap:

• platform_device
• devicetree (of) matching
• probe
• DrvData
• Pinning cleanup (you are here)
• DrvData references in PlatformDriver trait
• regmap/iomem

[wedsonaf]

Would the code be much more complicated if you had Pin<Box<...>> here?

My impression is that it wouldn't be too different (after the simplification you're applying), and the compiler would give you a little bit of help in enforcing the invariant.

[TheSven73]

Thanks for the review! I'm glad you are asking this question. I can use it as a vehicle to check and update my understanding of the whole Pinning concept.

As far as I know, using Pin<Box<bindings::platform_driver>> on its own here won't change a thing. It won't prevent the platform_driver from moving out of the Box. Why? Because platform_driver: Unpin by default. So we can always call Pin::into_inner() safely, and the inner object will simply move out without complaints.

If we really want to prevent moving out, we need to mark platform_driver as !Unpin:

impl !Unpin for bindings::platform_driver {}

But Rust doesn't currently allow that!

error[E0658]: negative trait bounds are not yet fully implemented; use marker types for now
  --> rust/kernel/platdev.rs:20:6
   |
20 | impl !Unpin for bindings::platform_driver {}

So we'd have to use PhantomPinned, which in turn requires us to use some wrapper type:

struct pinned_platform_driver {
    inner: bindings::platform_driver,
    _pin: PhantomPinned,
}
// can't move out of Pin<Box<pinned_platform_driver>>, yay !

But now we are storing a different structure, one we can't get an inner mutable reference to without using unsafe blocks. Yet the kernel's platform_register()/platform_unregister() require mutable references !

During registration, it's not so bad, because we can register first, then pin. Which kind of defeats the reason for using Pin<> in the first place.. But the unregistration function now becomes problematic: it needs a mutable reference to a bindings::platform_driver, but that structure is wrapped in a !Unpin outer wrapper, so we need a bunch of unsafe acrobatics there to get the reference.

As far as I can tell, it all becomes a huge unreadable, unsafe mess. IMHO using a "do not move out" Invariant works pretty well. But I am open to suggestions.

[ojeda]

It would not be an invariant, no? i.e. an invariant is something that is guaranteed (as long as one does not break preconditions on e.g. constructors).

[bjorn3]

It would be an invariant, but not one the compiler will help enforcing.

[TheSven73]

If we want to avoid using Pin<> here (and I think that's probably the way to go, subject to further feedback), then we'll need a way to document / enforce that no code inside platdev::Registration moves pdrv out of its Box. We should not need to enforce this externally, as pdrv is a private member field, and is never exposed externally. Except through a kernel function, but that doesn't count.

What is the best way to accomplish that? Using an invariant? Or some other way?

[TheSven73]

Thanks for the tip! I'm sorry that my confusion is so confusing :) I'd like to ask a few questions, to update my internal model.

When we register a miscdev for example, we pass a pointer to a bindings::miscdevice to the kernel on registration. I'm assuming we need two guarantees:

• the struct's memory area will continue to exist until after we unregister the miscdevice and
• the struct's memory area won't be overwritten until after we unregister the miscdevice

The easiest way is probably like this: (a)

struct Registration {
    mdev: Box<bindings::miscdevice>,
    // etc
}
impl Registration {
    fn new() -> Self {
        let mut mdev = Box::new(default());
        unsafe { bindings::misc_register(&mut *mdev) };
        Self { mdev }
    }
}

Yet the existing codebase does the following: (b)

struct Registration {
    mdev: bindings::miscdevice,
    _pin: PhantomPinned,
}
impl Registration {
    fn new() -> Pin<Box<Self>> {
        let mut r = Pin::from(Box::new(Self::default()));
        let mut this = unsafe { /* r.get_mut() and get_unchecked_mut() gymnastics */ };
        unsafe { bindings::misc_register(&mut this.mdev) };
        r
    }
}

So my question is: why does the current codebase choose (b) over (a)? What are the advantages of (b) over (a)?
According to the comments in the code,

    /// It must be pinned because the memory block that represents the registration is
    /// self-referential.

but I am not sure that's a sufficient justification, because (a) is not Pinned, yet also allows self-referential structures.

[ojeda]

Sounds correct to me. If one keeps the things that need to be pinned inside a Box, and the box remains private (i.e. others cannot mess with it), and the implementation of the abstraction takes care of never moving it, then it is OK. It is also OK to Pin the private Box to ensure it is not moved by mistake by the implementation.

Though I do not recall why Registration is pinned here, rather than leaving it a detail of the implementation. @wedsonaf?

[wedsonaf]

Option (b) isn't really what we have in the current codebase. What we have is (simplified):

impl Registration {
    fn new() -> Self {
        [...]
    }
}

Then as a helper we have:

    fn new_pinned() -> Result<Pin<Box<Self>>> {
        [...]
    } 

And the reason is that we want to have a zero-cost abstraction. If we forced everyone to use new_pinned or option (a) above, we would require everyone to have an extra allocation. Here's an example (C Binder) on how miscdev is embedded in some outer structure: https://elixir.bootlin.com/linux/latest/source/drivers/android/binder_internal.h#L36 -- it would require an extra allocation if we were to require miscdev to be always boxed; such a solution would preclude the zero-cost case, but the current implementation allows both: if you want zero-cost, you embed Registration in your outer structure and ensure that it's pinned (by pinning the outer structure and ensuring that the projection is also pinned), but if you don't care about zero-cost, just use new_pinned() and pay the cost of the extra allocation.

Regarding Sven's example, that is perfectly fine: note that pd1.set(pd2) causes the destructor of whatever was pinned in pd1 to be called before it is overwritten by pd2, that is, if what was initially pinned was a Registration, its destructor would unregister it first, then it would be overwritten with the new one.

[TheSven73]

@wedsonaf and @ojeda, thank you so much for your patience and willingness to explain, I really appreciate it !! Let’s see if I understand the underlying issues better now. I encourage you to update my understanding if something doesn’t sound quite right.

When we create an abstraction that uses a kernel C structure in Rust, e.g. via misc_register() or __mutex_init(), there are a couple of “gotchas”:

1. We must not allow our users to modify the structure, other than through the proper C APIs and/or rules.
2. We must not allow our users to move the kernel C structure around in memory.

(1) is easy. Users from outside the kernel crate can’t see private fields, so we can always make the C structure private, and ensure our public functions use it as prescribed in the kernel.

(2) is trickier! If the C structure is just a straight-forward field inside our Rust struct, then our users could move it around in memory, which breaks the C API. There are two solutions to this problem:

1. Store the C structure in a private, Boxed field of the Rust struct. Our users can’t touch it because it’s private. They also can’t move it around - moving the Rust structure in memory is fine, because it’s just holding a pointer to the Boxed C struct.
2. Store the C structure inside the Rust structure in a straightforward way. And prevent our users from moving it around using various techniques. Such as # Safety requirements, or method functions with a self type that’s Pin<&Self> or Pin<&mut Self>. All of these require our users to use unsafe blocks and make careful reasonings about pinning and safety.

All are partially flawed. Solution (1) leads to multiple allocations and indirections, which is a non-zero cost abstraction - hey that’s not what Rust is supposed to be about! Solution (2) is true zero-cost, yet requires users to write tricky unsafe code just to use what are essentially very simple abstractions. They have a good chance of getting it wrong, and go down in flames - hey that’s also not what Rust is all about!

I am not sure what the group consensus is here. Are the unsafe blocks and tricky pin projections in driver code not seen as a problem?

For platdev I will go with solution (1) for now, which is easy to use for driver writers, yet leads to multiple allocations and indirections. For a stone-cold path such as registering a platform driver (not a device), that shouldn't be a problem.

Edited to add: I'm assuming a solution along the lines of pin-project has been considered at some point, and rejected?

[ojeda]

I think your understanding is correct!

I think it is reasonable to favor the easiest solution for cold paths, although people may still complain because, in the case of allocations, they may lead to other issues outside the cold path (say, memory fragmentation with SLOB).

Then there is the problem of having to return more Results. And, in general, the overall feeling that the language "forces" us to do allocations just to have a nicer API.

I don't think what way would be best for us, but I tend to think that medium-/long-term we should invest in finding out the best ergonomic solution while keeping the zero-cost (via the pin-*-like crates, a new language feature, or something else).

[wedsonaf]

I think you'd only have to pin here.

[ksquirrel]

Review of 221d99b96378:

• ✔️ Commit dbbd1dc: Looks fine!
• ✔️ Commit 221d99b: Looks fine!

[TheSven73]

v1 -> v2:

• rebased to latest rust branch
• added comment suggested in Simple Rust driver that touches real hardware: PR 5 #381 (comment)

[TheSven73]

PTAL. Do we have a consensus that using a private Box + comment is strict enough for a C structure shared with the kernel, that must not move? See explanation in #381 (comment) as to why Pin<Box<T>> is problematic.

If not, I'll just drop this PR. But I'd like us to make a decision, so we can move on.

[nbdd0121]

A private Box + comment is sufficient for C structures that can't move. Rust std Mutex uses pthread mutex in the same way.

However I am uncertain that we want an internal allocation, because that'll make platdev::Registration API differ from miscdev::Registration.

[TheSven73]

We can reopen if there is more consensus on Pinning.