6.6-stable review patch. If anyone has any objections, please let me know.
------------------
From: Alice Ryhl aliceryhl@google.com
commit a8ee30f45d5d57467ddb7877ed6914d0eba0af7f upstream.
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 Suggested-by: Boqun Feng boqun.feng@gmail.com Reviewed-by: Gary Guo gary@garyguo.net Link: https://lore.kernel.org/r/20240915-locked-by-sync-fix-v2-1-1a8d89710392@goog... Signed-off-by: Miguel Ojeda ojeda@kernel.org Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org --- rust/kernel/sync/locked_by.rs | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)
--- a/rust/kernel/sync/locked_by.rs +++ b/rust/kernel/sync/locked_by.rs @@ -80,8 +80,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> { @@ -115,7 +119,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" @@ -124,7 +131,10 @@ impl<T: ?Sized, U> LockedBy<T, U> { 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() } }