[PATCH 1/2] rust: sync: require `Send` and `Sync` for `Backend::State`

[PATCH 1/2] rust: sync: require `Send` and `Sync` for `Backend::State`

[Benno Lossin]

`Lock<T, B>` implements `Send` and `Sync` when `T` is `Send` or `Sync`
respectively. Since this does not depend on `B`, creating a `Lock` that
is `Send` and `Sync`, but with a `!Sync` or `!Send` state is possible.
This is a soundness issue, thus add the bounds to the respective impls.

Signed-off-by: Benno Lossin <benno.lossin@proton.me>
---
 rust/kernel/sync/lock.rs | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
index f6c34ca4d819..e73ac9d97b29 100644
--- a/rust/kernel/sync/lock.rs
+++ b/rust/kernel/sync/lock.rs
@@ -97,12 +97,23 @@ pub struct Lock<T: ?Sized, B: Backend> {
     pub(crate) data: UnsafeCell<T>,
 }
 
-// SAFETY: `Lock` can be transferred across thread boundaries iff the data it protects can.
-unsafe impl<T: ?Sized + Send, B: Backend> Send for Lock<T, B> {}
+// SAFETY: `Lock` can be transferred across thread boundaries iff the data it protects and the
+// backend state can.
+unsafe impl<T: ?Sized, B: Backend> Send for Lock<T, B>
+where
+    T: Send,
+    B::State: Send,
+{
+}
 
 // SAFETY: `Lock` serialises the interior mutability it provides, so it is `Sync` as long as the
-// data it protects is `Send`.
-unsafe impl<T: ?Sized + Send, B: Backend> Sync for Lock<T, B> {}
+// data it protects is `Send` and the backend state can be shared.
+unsafe impl<T: ?Sized + Send, B: Backend> Sync for Lock<T, B>
+where
+    T: Send,
+    B::State: Sync,
+{
+}
 
 impl<T, B: Backend> Lock<T, B> {
     /// Constructs a new lock initialiser.

base-commit: a335e95914046c6bed45c0d17cabcd483682cf5e
-- 
2.46.0

[PATCH 2/2] rust: sync: require `Sync` for `Backend::GuardState`

[Benno Lossin]

`Guard<T, B>` implements `Sync` when `T` is `Sync`. Since this does not
depend on `B`, creating a `Guard` that is `Sync`, but with `!Sync` state
is possible. This is a soundness issue, thus add the bounds to the
respective impls.

Signed-off-by: Benno Lossin <benno.lossin@proton.me>
---
 rust/kernel/sync/lock.rs | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
index e73ac9d97b29..336ad2f0ec06 100644
--- a/rust/kernel/sync/lock.rs
+++ b/rust/kernel/sync/lock.rs
@@ -153,8 +153,13 @@ pub struct Guard<'a, T: ?Sized, B: Backend> {
     _not_send: PhantomData<*mut ()>,
 }
 
-// SAFETY: `Guard` is sync when the data protected by the lock is also sync.
-unsafe impl<T: Sync + ?Sized, B: Backend> Sync for Guard<'_, T, B> {}
+// SAFETY: `Guard` is sync when the data protected by the lock and the guard state is also sync.
+unsafe impl<T: ?Sized, B: Backend> Sync for Guard<'_, T, B>
+where
+    T: Sync,
+    B::GuardState: Sync,
+{
+}
 
 impl<T: ?Sized, B: Backend> Guard<'_, T, B> {
     pub(crate) fn do_unlocked<U>(&mut self, cb: impl FnOnce() -> U) -> U {
-- 
2.46.0

Re: [PATCH 2/2] rust: sync: require `Sync` for `Backend::GuardState`

[Alice Ryhl]

On Tue, Sep 3, 2024 at 11:17 AM Benno Lossin <benno.lossin@proton.me> wrote:
>
> `Guard<T, B>` implements `Sync` when `T` is `Sync`. Since this does not
> depend on `B`, creating a `Guard` that is `Sync`, but with `!Sync` state
> is possible. This is a soundness issue, thus add the bounds to the
> respective impls.
>
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>

Right now, a `&Guard<T, B>` has exactly the same powers as &T, as the
only thing you can do on the guard with only a shared reference is
deref to a &T. So the bounds are correct as they are, unless new APIs
are added (which seems unlikely?). But the safety comment could
certainly be improved.

Alice

Re: [PATCH 2/2] rust: sync: require `Sync` for `Backend::GuardState`

[Benno Lossin]

On 03.09.24 11:32, Alice Ryhl wrote:
> On Tue, Sep 3, 2024 at 11:17 AM Benno Lossin <benno.lossin@proton.me> wrote:
>>
>> `Guard<T, B>` implements `Sync` when `T` is `Sync`. Since this does not
>> depend on `B`, creating a `Guard` that is `Sync`, but with `!Sync` state
>> is possible. This is a soundness issue, thus add the bounds to the
>> respective impls.
>>
>> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
> 
> Right now, a `&Guard<T, B>` has exactly the same powers as &T, as the
> only thing you can do on the guard with only a shared reference is
> deref to a &T. So the bounds are correct as they are, unless new APIs
> are added (which seems unlikely?). 

Right, but I thought it was strange not to require that. Since that
would be the default behavior of the `Sync` auto-trait. And the only
reason why we have to implement `Sync` is because we want it to be
`!Send` with the `PhantomData<*mut ()>`.

All of our locks currently use `()` as the guard state, so we don't lose
anything.

Maybe it might make sense to instead have a marker type that is `!Send`
but `Sync` that can be used here instead, since then we could avoid the
`unsafe impl Sync`.

> But the safety comment could certainly be improved.

That's why I stumbled on this :)

---
Cheers,
Benno

Re: [PATCH 2/2] rust: sync: require `Sync` for `Backend::GuardState`

[Alice Ryhl]

On Tue, Sep 3, 2024 at 12:06 PM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On 03.09.24 11:32, Alice Ryhl wrote:
> > On Tue, Sep 3, 2024 at 11:17 AM Benno Lossin <benno.lossin@proton.me> wrote:
> >>
> >> `Guard<T, B>` implements `Sync` when `T` is `Sync`. Since this does not
> >> depend on `B`, creating a `Guard` that is `Sync`, but with `!Sync` state
> >> is possible. This is a soundness issue, thus add the bounds to the
> >> respective impls.
> >>
> >> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
> >
> > Right now, a `&Guard<T, B>` has exactly the same powers as &T, as the
> > only thing you can do on the guard with only a shared reference is
> > deref to a &T. So the bounds are correct as they are, unless new APIs
> > are added (which seems unlikely?).
>
> Right, but I thought it was strange not to require that. Since that
> would be the default behavior of the `Sync` auto-trait. And the only
> reason why we have to implement `Sync` is because we want it to be
> `!Send` with the `PhantomData<*mut ()>`.
>
> All of our locks currently use `()` as the guard state, so we don't lose
> anything.
>
> Maybe it might make sense to instead have a marker type that is `!Send`
> but `Sync` that can be used here instead, since then we could avoid the
> `unsafe impl Sync`.

I think it's actually quite reasonable to `unsafe impl Sync` for lock
guards. The lock guard in std is also rather special in regards to its
Sync implementation, since it also has a situation where Send deals
with special details of the guard, but Sync does not.

Alice

Re: [PATCH 1/2] rust: sync: require `Send` and `Sync` for `Backend::State`

[Alice Ryhl]

On Tue, Sep 3, 2024 at 11:17 AM Benno Lossin <benno.lossin@proton.me> wrote:
>
> `Lock<T, B>` implements `Send` and `Sync` when `T` is `Send` or `Sync`
> respectively. Since this does not depend on `B`, creating a `Lock` that
> is `Send` and `Sync`, but with a `!Sync` or `!Send` state is possible.
> This is a soundness issue, thus add the bounds to the respective impls.
>
> Signed-off-by: Benno Lossin <benno.lossin@proton.me>

Currently, B::State is set directly to `bindings::spinlock_t` or
`bindings::mutex` and these types are pretty unlikely to be Send/Sync
on all kernel configurations. If you're going to make this change, you
will need to change these types.

Considering that B::State is already stored in Opaque meaning that we
don't run its destructor either, it's not really treated as a normal
field right now. Perhaps it would be better to change the safety
requirements of the `Backend` trait to impose restrictions on the
thread safety of B::State?

Alice

Re: [PATCH 1/2] rust: sync: require `Send` and `Sync` for `Backend::State`

[Benno Lossin]

On 03.09.24 11:30, Alice Ryhl wrote:
> On Tue, Sep 3, 2024 at 11:17 AM Benno Lossin <benno.lossin@proton.me> wrote:
>>
>> `Lock<T, B>` implements `Send` and `Sync` when `T` is `Send` or `Sync`
>> respectively. Since this does not depend on `B`, creating a `Lock` that
>> is `Send` and `Sync`, but with a `!Sync` or `!Send` state is possible.
>> This is a soundness issue, thus add the bounds to the respective impls.
>>
>> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
> 
> Currently, B::State is set directly to `bindings::spinlock_t` or
> `bindings::mutex` and these types are pretty unlikely to be Send/Sync
> on all kernel configurations. If you're going to make this change, you
> will need to change these types.

Oh yeah you are correct. I did try to compile it, but maybe I missed
some config options, since it didn't complain?

> Considering that B::State is already stored in Opaque meaning that we
> don't run its destructor either, it's not really treated as a normal
> field right now. Perhaps it would be better to change the safety
> requirements of the `Backend` trait to impose restrictions on the
> thread safety of B::State?

Yes that sounds like a better idea.

---
Cheers,
Benno

Re: [PATCH 1/2] rust: sync: require `Send` and `Sync` for `Backend::State`

[Alice Ryhl]

On Tue, Sep 3, 2024 at 11:58 AM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On 03.09.24 11:30, Alice Ryhl wrote:
> > On Tue, Sep 3, 2024 at 11:17 AM Benno Lossin <benno.lossin@proton.me> wrote:
> >>
> >> `Lock<T, B>` implements `Send` and `Sync` when `T` is `Send` or `Sync`
> >> respectively. Since this does not depend on `B`, creating a `Lock` that
> >> is `Send` and `Sync`, but with a `!Sync` or `!Send` state is possible.
> >> This is a soundness issue, thus add the bounds to the respective impls.
> >>
> >> Signed-off-by: Benno Lossin <benno.lossin@proton.me>
> >
> > Currently, B::State is set directly to `bindings::spinlock_t` or
> > `bindings::mutex` and these types are pretty unlikely to be Send/Sync
> > on all kernel configurations. If you're going to make this change, you
> > will need to change these types.
>
> Oh yeah you are correct. I did try to compile it, but maybe I missed
> some config options, since it didn't complain?

My guess is that it only happens with lockdep enabled.

Alice