Safe initialization of pinned structs

[nbdd0121]

The kernel has a lot of data structures that cannot be freely moved, e.g. Mutex and CondVar. Currently what we do is to first unsafely create instance of them, pin them, and then unsafely initialize them after having them pinned (currently the code does not require unsafe to initialize Mutex, which I believe is a bug, as it could be initialized while locked, which is definitely unsound).

For example, this snippet (related #286) from miscdev needs 3 unsafes (5 if we count both inits):

linux/samples/rust/rust_miscdev.rs

Lines 35 to 59 in 1a73789

	struct SharedState {
	state_changed: CondVar,
	inner: Mutex<SharedStateInner>,
	}
	impl SharedState {
	fn try_new() -> Result<Pin<Arc<Self>>> {
	// SAFETY: `state` is pinning `Arc`, which implements `Unpin`.
	let state = unsafe {
	Pin::new_unchecked(Arc::try_new(Self {
	// SAFETY: `condvar_init!` is called below.
	state_changed: CondVar::new(),
	// SAFETY: `mutex_init!` is called below.
	inner: Mutex::new(SharedStateInner { token_count: 0 }),
	})?)
	};
	// SAFETY: `state_changed` is pinned behind `Arc`.
	let state_changed = unsafe { Pin::new_unchecked(&state.state_changed) };
	kernel::condvar_init!(state_changed, "SharedState::state_changed");
	// SAFETY: `inner` is pinned behind `Arc`.
	let inner = unsafe { Pin::new_unchecked(&state.inner) };
	kernel::mutex_init!(inner, "SharedState::inner");
	Ok(state)
	}
	}

And this is a very common pattern. In the current state, essentially any use of CondVar, Mutex, Spinlock or any pinned types require a lot of unsafe to initialize. Of course we can have these types Box-ed internally but that's many unnecessary allocations.

So I spent the past few days designing and implementing a safe way to pin-initialized struct, pin-init (doc, repo). This design allows safe initialization and use of pthread mutex without any boxing: https://github.com/nbdd0121/pin-init/blob/trunk/examples/pthread_mutex.rs.

Kernel CondVar & Mutex could be implemented in a similar way. With pin-init, the above snippet could be written like this:

#[pin_init]
struct SharedState {
    #[pin]
    state_changed: CondVar,
    #[pin]
    inner: Mutex<SharedStateInner>,
}

impl SharedState {
    fn try_new() -> Result<Pin<Arc<Self>>> {
        try_new_arc(init_pin!(Self {
            state_changed: |s| kernel::condvar_new!(s, "SharedState::state_changed"),
            inner: |s| kernel::mutex_new!(s, "SharedState::inner", SharedStateInner { token_count: 0 }),
        }))
    }
}

Approaches like this make code more readable and safer; but it requires procedural macro (and parsing capability of syn) so it might not be easy to integrate at current stage.

[ojeda]

Related rust-lang/rust#85579 by @alex.

[nbdd0121]

Actually not quite so related :) pin-init ensures that an object is pinned even before is constructed -- with Arc::try_pin, you only get rid of one unsafe, but the other 4 remains.

[ojeda]

Related != equivalent :) Both improve/simplify the Result<Pin<Arc<Self>>> initialization. I was not implying we need to go with one or the other.

[TheSven73]

Currently what we do is to first unsafely create instance of them, pin them, and then unsafely initialize them after having them pinned (currently the code does not require unsafe to initialize Mutex, which I believe is a bug, as it could be initialized while locked, which is definitely unsound).

I've got to admit that, coming to this project from scratch, I was scratching my head a little bit when I saw these abstractions. I'm sure there's a reason why they are the way they are, I'm hoping to learn why.

Maybe #289 indicates that even we couldn't use this safely inside a simple driver? If we could not, what hope do driver developers new to Rust have?

@nbdd0121 this looks like a creative solution. Possibly only a partial solution though? Because people could still make structs with #[pin] annotations, then actually forget to wrap them inside a pinned pointer. And then the mutex or condvar data could still move out, and the code could still mushroom cloud?

I remember having an interesting conversation with @ojeda where he mentioned that in Rust, a little bit of inefficiency is acceptable if it allows us to improve safety. I wonder if we could make a Mutex or CondVar constructor that's entirely safe, but slightly less efficient? Something like this?

fn new_mutex(data: T) -> Result<Pin<Box<Mutex<T>>>>

Then driver authors wouldn't need any unsafe blocks. They also wouldn't have to pin their own structures, nor think deeply about pinning. They could also not move the Mutex out of the Pin<Box> accidentally, because Mutex is !Unpin.

Penalty is one extra allocation per mutex-protected structure. May be worth it in many circumstances?

Could be you folks have been here before, and the new guy is just going around in circles :)

[nbdd0121]

I've got to admit that, coming to this project from scratch, I was scratching my head a little bit when I saw these abstractions. I'm sure there's a reason why they are the way they are, I'm hoping to learn why.

It's indeed weird! The fundamentally problem is that currently in Rust you don't have a way to create a type pinned -- you always need to create it and then pin it. E.g. Box::pin or Arc::pin requires you to have T first and gives you a Pin<Box<T>>, but for types like CondVar and Mutex, they must be pinned as soon as they are initialized, which means it'll be unsafe to create CondVar or Mutex.

Maybe #289 indicates that even we couldn't use this safely inside a simple driver? If we could not, what hope do driver developers new to Rust have?

That's a major problem. unsafe code can be hard to get right, and that's why I hope to have a safe abstraction.

@nbdd0121 this looks like a creative solution. Possibly only a partial solution though? Because people could still make structs with #[pin] annotations, then actually forget to wrap them inside a pinned pointer. And then the mutex or condvar data could still move out, and the code could still mushroom cloud?

No! The main merit of pin-init is that it is completely safe without user writing extra unsafe stuff. If you look at the signature of new_arc:

pub fn new_arc<T, E, F>(f: F) -> Result<Pin<Arc<T>>, E> 
where
    F: for<'a> FnOnce(PinInit<'a, T>) -> PinInitResult<'a, T, E>,

you can see that the return type is already pinned. In fact, the whole design about PinInit and PinInitResult is to ensure that the user can never obtain T or &mut T. If the user cannot obtain CondVar, then they have no way to construct SharedState safely themselves, and can only use init_pin! to safely create a Pin<SomePointer<T>>.

I remember having an interesting conversation with @ojeda where he mentioned that in Rust, a little bit of inefficiency is acceptable if it allows us to improve safety. I wonder if we could make a Mutex or CondVar constructor that's entirely safe, but slightly less efficient? Something like this?

fn new_mutex(data: T) -> Result<Pin<Box<Mutex<T>>>>

Then driver authors wouldn't need any unsafe blocks. They also wouldn't have to pin their own structures, nor think deeply about pinning. They could also not move the Mutex out of the Pin<Box> accidentally, because Mutex is !Unpin.

Penalty is one extra allocation per mutex-protected structure. May be worth it in many circumstances?

I don't think kernel people will like it; an allocation required for creating each !Unpin type is one drawback, but the major issue is the cache penalty due to the indirection it brings each time such types are used.

With pin-init you can see use box if you don't want to pin the outer struct:

impl<T> Mutex<T> {
    fn new(this: PinInit<'a, Self>, data: T) -> PinInitResult<'a, Self, kernel::Error> { /* ... */ }
}

try_new_box(|s| Mutex::new(s, data)) // Result<Pin<Box<Mutex<T>>>>
try_new_arc(|s| Mutex::new(s, data)) // Result<Pin<Arc<Mutex<T>>>>

[ojeda]

Yeah, extra allocations are quite bad, and more than just a little bit of inefficiency. :(

[TheSven73]

Yeah I agree with you guys on this one. Gary's solution looks like the way forward. I'm still learning when it comes to pinning...

[alex]

@TheSven73 if you haven't seen it yet, https://fasterthanli.me/articles/pin-and-suffering is the best resource I've read

[TheSven73]

Thank you @alex! I really need to understand pinning properly - the next stage of the hwrng Rust driver will depend on it.

[TheSven73]

@nbdd0121 How does your pin-init solution relate to pin-project ?

[nbdd0121]

pin-project addresses how you can idiomatically and safely use pinned structs, and pin-init addresses how you can idiomatically and safely create them. I've designed pin-init so that it can be used together with pin-project.