[PATCH v2] rust: sync: require `T: Sync` for `LockedBy::access`

[PATCH v2] rust: sync: require `T: Sync` for `LockedBy::access`

[Alice Ryhl]

The `LockedBy::access` method only requires a shared reference to the
owner, so if we have shared access to the `LockedBy` from several
threads at once, then two threads could call `access` in parallel and
both obtain a shared reference to the inner value. Thus, require that
`T: Sync` when calling the `access` method.

An alternative is to require `T: Sync` in the `impl Sync for LockedBy`.
This patch does not choose that approach as it gives up the ability to
use `LockedBy` with `!Sync` types, which is okay as long as you only use
`access_mut`.

Cc: stable@vger.kernel.org
Fixes: 7b1f55e3a984 ("rust: sync: introduce `LockedBy`")
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
Changes in v2:
- Use a `where T: Sync` on `access` instead of changing `impl Sync for
  LockedBy`.
- Link to v1: https://lore.kernel.org/r/20240912-locked-by-sync-fix-v1-1-26433cbccbd2@google.com
---
 rust/kernel/sync/locked_by.rs | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/rust/kernel/sync/locked_by.rs b/rust/kernel/sync/locked_by.rs
index babc731bd5f6..ce2ee8d87865 100644
--- a/rust/kernel/sync/locked_by.rs
+++ b/rust/kernel/sync/locked_by.rs
@@ -83,8 +83,12 @@ pub struct LockedBy<T: ?Sized, U: ?Sized> {
 // SAFETY: `LockedBy` can be transferred across thread boundaries iff the data it protects can.
 unsafe impl<T: ?Sized + Send, U: ?Sized> Send for LockedBy<T, U> {}
 
-// SAFETY: `LockedBy` serialises the interior mutability it provides, so it is `Sync` as long as the
-// data it protects is `Send`.
+// SAFETY: If `T` is not `Sync`, then parallel shared access to this `LockedBy` allows you to use
+// `access_mut` to hand out `&mut T` on one thread at the time. The requirement that `T: Send` is
+// sufficient to allow that.
+//
+// If `T` is `Sync`, then the `access` method also becomes available, which allows you to obtain
+// several `&T` from several threads at once. However, this is okay as `T` is `Sync`.
 unsafe impl<T: ?Sized + Send, U: ?Sized> Sync for LockedBy<T, U> {}
 
 impl<T, U> LockedBy<T, U> {
@@ -118,7 +122,10 @@ impl<T: ?Sized, U> LockedBy<T, U> {
     ///
     /// Panics if `owner` is different from the data protected by the lock used in
     /// [`new`](LockedBy::new).
-    pub fn access<'a>(&'a self, owner: &'a U) -> &'a T {
+    pub fn access<'a>(&'a self, owner: &'a U) -> &'a T
+    where
+        T: Sync,
+    {
         build_assert!(
             size_of::<U>() > 0,
             "`U` cannot be a ZST because `owner` wouldn't be unique"
@@ -127,7 +134,10 @@ pub fn access<'a>(&'a self, owner: &'a U) -> &'a T {
             panic!("mismatched owners");
         }
 
-        // SAFETY: `owner` is evidence that the owner is locked.
+        // SAFETY: `owner` is evidence that there are only shared references to the owner for the
+        // duration of 'a, so it's not possible to use `Self::access_mut` to obtain a mutable
+        // reference to the inner value that aliases with this shared reference. The type is `Sync`
+        // so there are no other requirements.
         unsafe { &*self.data.get() }
     }
 

---
base-commit: 93dc3be19450447a3a7090bd1dfb9f3daac3e8d2
change-id: 20240912-locked-by-sync-fix-07193df52f98

Best regards,
-- 
Alice Ryhl <aliceryhl@google.com>

Re: [PATCH v2] rust: sync: require `T: Sync` for `LockedBy::access`

[Gary Guo]

On Sun, 15 Sep 2024 14:41:28 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> The `LockedBy::access` method only requires a shared reference to the
> owner, so if we have shared access to the `LockedBy` from several
> threads at once, then two threads could call `access` in parallel and
> both obtain a shared reference to the inner value. Thus, require that
> `T: Sync` when calling the `access` method.
> 
> An alternative is to require `T: Sync` in the `impl Sync for LockedBy`.
> This patch does not choose that approach as it gives up the ability to
> use `LockedBy` with `!Sync` types, which is okay as long as you only use
> `access_mut`.
> 
> Cc: stable@vger.kernel.org
> Fixes: 7b1f55e3a984 ("rust: sync: introduce `LockedBy`")
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

Reviewed-by: Gary Guo <gary@garyguo.net>

You probably also want to have a Suggested-by to credit Boqun for
suggesting the current implementation.

Best,
Gary

> ---
> Changes in v2:
> - Use a `where T: Sync` on `access` instead of changing `impl Sync for
>   LockedBy`.
> - Link to v1: https://lore.kernel.org/r/20240912-locked-by-sync-fix-v1-1-26433cbccbd2@google.com
> ---
>  rust/kernel/sync/locked_by.rs | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)

Re: [PATCH v2] rust: sync: require `T: Sync` for `LockedBy::access`

[Alice Ryhl]

On Sun, Sep 15, 2024 at 5:36 PM Gary Guo <gary@garyguo.net> wrote:
>
> On Sun, 15 Sep 2024 14:41:28 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
>
> > The `LockedBy::access` method only requires a shared reference to the
> > owner, so if we have shared access to the `LockedBy` from several
> > threads at once, then two threads could call `access` in parallel and
> > both obtain a shared reference to the inner value. Thus, require that
> > `T: Sync` when calling the `access` method.
> >
> > An alternative is to require `T: Sync` in the `impl Sync for LockedBy`.
> > This patch does not choose that approach as it gives up the ability to
> > use `LockedBy` with `!Sync` types, which is okay as long as you only use
> > `access_mut`.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: 7b1f55e3a984 ("rust: sync: introduce `LockedBy`")
> > Signed-off-by: Alice Ryhl <aliceryhl@google.com>
>
> Reviewed-by: Gary Guo <gary@garyguo.net>
>
> You probably also want to have a Suggested-by to credit Boqun for
> suggesting the current implementation.

That's fine. Miguel can add that when he picks this.

Alice

Re: [PATCH v2] rust: sync: require `T: Sync` for `LockedBy::access`

[Miguel Ojeda]

On Sun, Sep 15, 2024 at 4:41 PM Alice Ryhl <aliceryhl@google.com> wrote:
>
> The `LockedBy::access` method only requires a shared reference to the
> owner, so if we have shared access to the `LockedBy` from several
> threads at once, then two threads could call `access` in parallel and
> both obtain a shared reference to the inner value. Thus, require that
> `T: Sync` when calling the `access` method.
>
> An alternative is to require `T: Sync` in the `impl Sync for LockedBy`.
> This patch does not choose that approach as it gives up the ability to
> use `LockedBy` with `!Sync` types, which is okay as long as you only use
> `access_mut`.
>
> Cc: stable@vger.kernel.org
> Fixes: 7b1f55e3a984 ("rust: sync: introduce `LockedBy`")
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

Applied to `rust-fixes` with Boqun's Suggested-by -- thanks!

Cheers,
Miguel