binder: fix use-before-init of spinlock

[nbdd0121]

This causes BUG: spinlock bad magic.

[ksquirrel]

Review of 44d85c4b0ee2:

• ✔️ Commit 44d85c4: Looks fine!

[alex]

So, this looks correct to me.... but I'd love to think about how we could solve this better at the API level, to catch this at compile time.

[nbdd0121]

Well, I guess #290 is related...

[TheSven73]

Well, I guess #290 is related...

So is #290 (comment) :)

Why am I worried about this whole Pinning + unsafe business? This is the second instance that surfaces this month in our own code. If we are likely to make mistakes of this kind, what chance do driver writers less familiar with Rust have? Will this be an improvement to C?

(This by no means implies criticism of @wedsonaf whose judgement and code I respect greatly)

[wedsonaf]

Well, I guess #290 is related...

So is #290 (comment) :)

So is #145 (comment) :)

Why am I worried about this whole Pinning + unsafe business? This is the second instance that surfaces this month in our own code. If we are likely to make mistakes of this kind, what chance do driver writers less familiar with Rust have? Will this be an improvement to C?

Before Sven and Gary joined us, we had a discussion about this in one of the public meetings when I started creating these abstractions that required pinning. Broadly speaking we had two options: do what Rust does internally in its std library, which is to ignore the problem, and just assume that things behind Arc and Box are pinned (for example, they have immovable mutexes, which are wrapped in a Box to implement Mutex -- this all came before Pin, so I guess it's justified this way), or we could use Pin with all its wrinkles. We chose the second option because then we'd get some help from the compiler.

As I said in some other comment, my intent is try to talk to the language folks and improve this in the language (as I started to do in the comment above). Having cases like these I think strengthen our case that we need better language support for this.

(This by no means implies criticism of @wedsonaf whose judgement and code I respect greatly)

Thanks Sven, that's very kind.

This is one of the reasons why we need languages like Rust, they can help us catch bugs. (It didn't this time, but it should.)

[wedsonaf]

BTW, thanks @nbdd0121 for spotting and fixing this!

Does the benchmark work for you after this fix?

[nbdd0121]

BTW, thanks @nbdd0121 for spotting and fixing this!

Does the benchmark work for you after this fix?

Sadly still not.

[TheSven73]

As I said in some other comment, my intent is try to talk to the language folks and improve this in the language (as I started to do in the comment above). Having cases like these I think strengthen our case that we need better language support for this.

💯

I wonder if we can make an argument for allowing driver writers to choose which Mutex to use? E.g.

• BoxedMutex contains a struct mutex that lives on the heap. As a result, no Pinning or unsafe blocks are required to use it
• PinnedMutex means a driver writer is responsible for Pining. It's their funeral when it's not done correctly
• both structs implement the Mutex trait, so they can be used interchangably.

Once the language support is up to scratch, we may drop support for BoxedMutex.

[nbdd0121]

I wonder if we can make an argument for allowing driver writers to choose which Mutex to use? E.g.

• BoxedMutex contains a struct mutex that lives on the heap. As a result, no Pinning or unsafe blocks are required to use it
• PinnedMutex means a driver writer is responsible for Pining. It's their funeral when it's not done correctly
• both structs implement the Mutex trait, so they can be used interchangably.

Once the language support is up to scratch, we may drop support for BoxedMutex.

Can't we just add a new method (or macro due to lockdep) that returns Pin<Box<Mutex<T>>> today?

[TheSven73]

We could. Two possible issues here:

• With Pin<Box<Mutex<T>>> the T also lives on the heap. With BoxedMutex only the struct mutex has to live on the heap
• I'm not sure why we need to bother API users with Pin<> wrappers? That smells of an implementation detail to me. A private Box field works just as well, no?

Edit: in the client drivers, we frequently see Pin<...Pin<...>...> types of patterns appear. I am not sure why this is necessary... If a structure needs Pinning, should that not be an implementation detail, invisible to driver writers?

[wedsonaf]

BTW, thanks @nbdd0121 for spotting and fixing this!
Does the benchmark work for you after this fix?

Sadly still not.

Which arch are you testing this on?

[nbdd0121]

Which arch are you testing this on?

I am testing this on 64-bit RISC-V (RV64GC).

[nbdd0121]

Edit: in the client drivers, we frequently see Pin<...Pin<...>...> types of patterns appear. I am not sure why this is necessary... If a structure needs Pinning, should that not be an implementation detail, invisible to driver writers?

In Rust, whether something needs pinning or not is not a property of type; all type T can be moved freely if there is a &mut T, even if T: !Unpin. Whether something is pinned or not is conveyed entirely using the Pin wrapper. T: Unpin merely means that guarantees of Pin is unnecessary. So Pin<Box<T>> has T pinned while Box<T> doesn't; you can do core::mem::swap(&mut a.mutex, &mut b.mutex) if both a and b are Box<T>, but compiler won't let you do it for Pin<Box<T>>. In this sense, Pin is indeed necessary.

[TheSven73]

Not if Pin<SomeType> can be eliminated by using a private member of SomeType that is on the stack, such as Box or Arc...?

[nbdd0121]

In that case you should use Pin for that private member. Or, you can leave out the Pin, but then you'll need to justify for all FFI calls why SomeType wouldn't be moved.

To be fair, the concept that everything is movable is not unique to Rust; it applies to C as well. E.g. you can write struct mutex a; struct mutex b; a = b; in C. Obviously that's a bug but we don't expect C compiler rejecting the code to enforce soundness. All the pinning shenanigans only arise because Rust tries to make it safe using type system...