The `impl Sync for LockedBy` implementation has insufficient trait bounds, as it only requires `T: Send`. However, `T: Sync` is also required for soundness because the `LockedBy::access` method could be used to provide shared access to the inner value from several threads in parallel.
Cc: stable@vger.kernel.org Fixes: 7b1f55e3a984 ("rust: sync: introduce `LockedBy`") Signed-off-by: Alice Ryhl aliceryhl@google.com --- rust/kernel/sync/locked_by.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/rust/kernel/sync/locked_by.rs b/rust/kernel/sync/locked_by.rs index babc731bd5f6..153ba4edcb03 100644 --- a/rust/kernel/sync/locked_by.rs +++ b/rust/kernel/sync/locked_by.rs @@ -83,9 +83,10 @@ 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`. -unsafe impl<T: ?Sized + Send, U: ?Sized> Sync for LockedBy<T, U> {} +// SAFETY: Shared access to the `LockedBy` can provide both `&mut T` references in a synchronized +// manner, or `&T` access in an unsynchronized manner. The `Send` trait is sufficient for the first +// case, and `Sync` is sufficient for the second case. +unsafe impl<T: ?Sized + Send + Sync, U: ?Sized> Sync for LockedBy<T, U> {}
impl<T, U> LockedBy<T, U> { /// Constructs a new instance of [`LockedBy`]. @@ -127,7 +128,7 @@ 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. unsafe { &*self.data.get() } }
--- base-commit: 93dc3be19450447a3a7090bd1dfb9f3daac3e8d2 change-id: 20240912-locked-by-sync-fix-07193df52f98
Best regards,
On Thu, Sep 12, 2024 at 02:20:06PM +0000, Alice Ryhl wrote:
The `impl Sync for LockedBy` implementation has insufficient trait bounds, as it only requires `T: Send`. However, `T: Sync` is also required for soundness because the `LockedBy::access` method could be used to provide shared access to the inner value from several threads in parallel.
Cc: stable@vger.kernel.org Fixes: 7b1f55e3a984 ("rust: sync: introduce `LockedBy`") Signed-off-by: Alice Ryhl aliceryhl@google.com
So I was pondering this forever, because we don't yet have read locks and for exclusive locks Send is enough. But since Arc<T> allows us to build really funny read locks already we need to require Sync for LockedBy, unlike Lock.
We could split access and access_mut up with a newtype so that Sync is only required when needed, but that's not too hard to sneak in when we actually need it.
Reviewed-by: Simona Vetter simona.vetter@ffwll.ch
rust/kernel/sync/locked_by.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/rust/kernel/sync/locked_by.rs b/rust/kernel/sync/locked_by.rs index babc731bd5f6..153ba4edcb03 100644 --- a/rust/kernel/sync/locked_by.rs +++ b/rust/kernel/sync/locked_by.rs @@ -83,9 +83,10 @@ 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`. -unsafe impl<T: ?Sized + Send, U: ?Sized> Sync for LockedBy<T, U> {} +// SAFETY: Shared access to the `LockedBy` can provide both `&mut T` references in a synchronized +// manner, or `&T` access in an unsynchronized manner. The `Send` trait is sufficient for the first +// case, and `Sync` is sufficient for the second case. +unsafe impl<T: ?Sized + Send + Sync, U: ?Sized> Sync for LockedBy<T, U> {} impl<T, U> LockedBy<T, U> { /// Constructs a new instance of [`LockedBy`]. @@ -127,7 +128,7 @@ 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. unsafe { &*self.data.get() }
base-commit: 93dc3be19450447a3a7090bd1dfb9f3daac3e8d2 change-id: 20240912-locked-by-sync-fix-07193df52f98
Best regards,
Alice Ryhl aliceryhl@google.com
On Fri, Sep 13, 2024 at 08:45:16PM +0200, Simona Vetter wrote:
On Thu, Sep 12, 2024 at 02:20:06PM +0000, Alice Ryhl wrote:
The `impl Sync for LockedBy` implementation has insufficient trait bounds, as it only requires `T: Send`. However, `T: Sync` is also required for soundness because the `LockedBy::access` method could be used to provide shared access to the inner value from several threads in parallel.
Cc: stable@vger.kernel.org Fixes: 7b1f55e3a984 ("rust: sync: introduce `LockedBy`") Signed-off-by: Alice Ryhl aliceryhl@google.com
So I was pondering this forever, because we don't yet have read locks and for exclusive locks Send is enough. But since Arc<T> allows us to build really funny read locks already we need to require Sync for LockedBy, unlike Lock.
We could split access and access_mut up with a newtype so that Sync is only required when needed, but that's not too hard to sneak in when we actually need it.
Hmm.. I think it makes more sense to make `access()` requires `where T: Sync` instead of the current fix? I.e. I propose we do:
impl<T, U> LockedBy<T, U> { pub fn access<'a>(&'a self, owner: &'a U) -> &'a T where T: Sync { ... } }
The current fix in this patch disallows the case where a user has a `Foo: !Sync`, but want to have multiple `&LockedBy<Foo, X>` in different threads (they would use `access_mut()` to gain unique accesses), which seems to me is a valid use case.
The where-clause fix disallows the case where a user has a `Foo: !Sync`, a `&LockedBy<Foo, X>` and a `&X`, and is trying to get a `&Foo` with `access()`, this doesn't seems to be a common usage, but maybe I'm missing something?
Thoughts?
Regards, Boqun
Reviewed-by: Simona Vetter simona.vetter@ffwll.ch
rust/kernel/sync/locked_by.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/rust/kernel/sync/locked_by.rs b/rust/kernel/sync/locked_by.rs index babc731bd5f6..153ba4edcb03 100644 --- a/rust/kernel/sync/locked_by.rs +++ b/rust/kernel/sync/locked_by.rs @@ -83,9 +83,10 @@ 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`. -unsafe impl<T: ?Sized + Send, U: ?Sized> Sync for LockedBy<T, U> {} +// SAFETY: Shared access to the `LockedBy` can provide both `&mut T` references in a synchronized +// manner, or `&T` access in an unsynchronized manner. The `Send` trait is sufficient for the first +// case, and `Sync` is sufficient for the second case. +unsafe impl<T: ?Sized + Send + Sync, U: ?Sized> Sync for LockedBy<T, U> {} impl<T, U> LockedBy<T, U> { /// Constructs a new instance of [`LockedBy`]. @@ -127,7 +128,7 @@ 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. unsafe { &*self.data.get() }
base-commit: 93dc3be19450447a3a7090bd1dfb9f3daac3e8d2 change-id: 20240912-locked-by-sync-fix-07193df52f98
Best regards,
Alice Ryhl aliceryhl@google.com
-- Simona Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Fri, 13 Sep 2024 23:28:37 -0700 Boqun Feng boqun.feng@gmail.com wrote:
Hmm.. I think it makes more sense to make `access()` requires `where T: Sync` instead of the current fix? I.e. I propose we do:
impl<T, U> LockedBy<T, U> { pub fn access<'a>(&'a self, owner: &'a U) -> &'a T where T: Sync { ... } }
The current fix in this patch disallows the case where a user has a `Foo: !Sync`, but want to have multiple `&LockedBy<Foo, X>` in different threads (they would use `access_mut()` to gain unique accesses), which seems to me is a valid use case.
The where-clause fix disallows the case where a user has a `Foo: !Sync`, a `&LockedBy<Foo, X>` and a `&X`, and is trying to get a `&Foo` with `access()`, this doesn't seems to be a common usage, but maybe I'm missing something?
+1 on this. Our `LockedBy` type only works with `Lock` -- which provides mutual exclusion rather than `RwLock`-like semantics, so I think it should be perfectly valid for people to want to use `LockedBy` for `Send + !Sync` types and only use `access_mut`. So placing `Sync` bound on `access` sounds better.
There's even a way to not requiring `Sync` bound at all, which is to ensure that the owner itself is a `!Sync` type:
impl<T, U> LockedBy<T, U> { pub fn access<'a, B: Backend>(&'a self, owner: &'a Guard<U, B>) -> &'a T { ... } }
Because there's no way for `Guard<U, B>` to be sent across threads, we can also deduce that all caller of `access` must be from a single thread and thus the `Sync` bound is unnecessary.
Best, Gary
Thoughts?
Regards, Boqun
On Sun, Sep 15, 2024 at 3:49 PM Gary Guo gary@garyguo.net wrote:
On Fri, 13 Sep 2024 23:28:37 -0700 Boqun Feng boqun.feng@gmail.com wrote:
Hmm.. I think it makes more sense to make `access()` requires `where T: Sync` instead of the current fix? I.e. I propose we do:
impl<T, U> LockedBy<T, U> { pub fn access<'a>(&'a self, owner: &'a U) -> &'a T where T: Sync { ... } }The current fix in this patch disallows the case where a user has a `Foo: !Sync`, but want to have multiple `&LockedBy<Foo, X>` in different threads (they would use `access_mut()` to gain unique accesses), which seems to me is a valid use case.
The where-clause fix disallows the case where a user has a `Foo: !Sync`, a `&LockedBy<Foo, X>` and a `&X`, and is trying to get a `&Foo` with `access()`, this doesn't seems to be a common usage, but maybe I'm missing something?
+1 on this. Our `LockedBy` type only works with `Lock` -- which provides mutual exclusion rather than `RwLock`-like semantics, so I think it should be perfectly valid for people to want to use `LockedBy` for `Send + !Sync` types and only use `access_mut`. So placing `Sync` bound on `access` sounds better.
I will add the `where` bound to `access`.
There's even a way to not requiring `Sync` bound at all, which is to ensure that the owner itself is a `!Sync` type:
impl<T, U> LockedBy<T, U> { pub fn access<'a, B: Backend>(&'a self, owner: &'a Guard<U, B>) -> &'a T { ... } }Because there's no way for `Guard<U, B>` to be sent across threads, we can also deduce that all caller of `access` must be from a single thread and thus the `Sync` bound is unnecessary.
Isn't Guard Sync? Either way, it's inconvenient to make Guard part of the interface. That prevents you from using it from within `&self`/`&mut self` methods on the owner.
Alice
On Sun, 15 Sep 2024 16:11:57 +0200 Alice Ryhl aliceryhl@google.com wrote:
On Sun, Sep 15, 2024 at 3:49 PM Gary Guo gary@garyguo.net wrote:
On Fri, 13 Sep 2024 23:28:37 -0700 Boqun Feng boqun.feng@gmail.com wrote:
Hmm.. I think it makes more sense to make `access()` requires `where T: Sync` instead of the current fix? I.e. I propose we do:
impl<T, U> LockedBy<T, U> { pub fn access<'a>(&'a self, owner: &'a U) -> &'a T where T: Sync { ... } }The current fix in this patch disallows the case where a user has a `Foo: !Sync`, but want to have multiple `&LockedBy<Foo, X>` in different threads (they would use `access_mut()` to gain unique accesses), which seems to me is a valid use case.
The where-clause fix disallows the case where a user has a `Foo: !Sync`, a `&LockedBy<Foo, X>` and a `&X`, and is trying to get a `&Foo` with `access()`, this doesn't seems to be a common usage, but maybe I'm missing something?
+1 on this. Our `LockedBy` type only works with `Lock` -- which provides mutual exclusion rather than `RwLock`-like semantics, so I think it should be perfectly valid for people to want to use `LockedBy` for `Send + !Sync` types and only use `access_mut`. So placing `Sync` bound on `access` sounds better.
I will add the `where` bound to `access`.
There's even a way to not requiring `Sync` bound at all, which is to ensure that the owner itself is a `!Sync` type:
impl<T, U> LockedBy<T, U> { pub fn access<'a, B: Backend>(&'a self, owner: &'a Guard<U, B>) -> &'a T { ... } }Because there's no way for `Guard<U, B>` to be sent across threads, we can also deduce that all caller of `access` must be from a single thread and thus the `Sync` bound is unnecessary.
Isn't Guard Sync? Either way, it's inconvenient to make Guard part of the interface. That prevents you from using it from within `&self`/`&mut self` methods on the owner.
I stand corrected. It's not `Send` but is indeed `Sync`. Let's go with a bound on `access`.
- Gary
Alice
On Sun, Sep 15, 2024 at 04:11:57PM +0200, Alice Ryhl wrote:
On Sun, Sep 15, 2024 at 3:49 PM Gary Guo gary@garyguo.net wrote:
On Fri, 13 Sep 2024 23:28:37 -0700 Boqun Feng boqun.feng@gmail.com wrote:
Hmm.. I think it makes more sense to make `access()` requires `where T: Sync` instead of the current fix? I.e. I propose we do:
impl<T, U> LockedBy<T, U> { pub fn access<'a>(&'a self, owner: &'a U) -> &'a T where T: Sync { ... } }The current fix in this patch disallows the case where a user has a `Foo: !Sync`, but want to have multiple `&LockedBy<Foo, X>` in different threads (they would use `access_mut()` to gain unique accesses), which seems to me is a valid use case.
The where-clause fix disallows the case where a user has a `Foo: !Sync`, a `&LockedBy<Foo, X>` and a `&X`, and is trying to get a `&Foo` with `access()`, this doesn't seems to be a common usage, but maybe I'm missing something?
+1 on this. Our `LockedBy` type only works with `Lock` -- which provides mutual exclusion rather than `RwLock`-like semantics, so I think it should be perfectly valid for people to want to use `LockedBy` for `Send + !Sync` types and only use `access_mut`. So placing `Sync` bound on `access` sounds better.
I will add the `where` bound to `access`.
Yeah I considered but it felt a bit icky to put constraints on the functions. But I didn't come up with a real use-case that would be prevented, so I think it's fine.
Even the use-case below where a shared references only gives you the guarantee something is valid you likely have additional locks to protected the data if it's mutable.
There's even a way to not requiring `Sync` bound at all, which is to ensure that the owner itself is a `!Sync` type:
impl<T, U> LockedBy<T, U> { pub fn access<'a, B: Backend>(&'a self, owner: &'a Guard<U, B>) -> &'a T { ... } }Because there's no way for `Guard<U, B>` to be sent across threads, we can also deduce that all caller of `access` must be from a single thread and thus the `Sync` bound is unnecessary.
Isn't Guard Sync? Either way, it's inconvenient to make Guard part of the interface. That prevents you from using it from within `&self`/`&mut self` methods on the owner.
I think there's also plenty of patterns where just having reference is enoug to guarantee access and exclusive ownership gives exclusive access. E.g. in drm we have some objects that are generally attached to a File, but get independently destroyed. But some of the fields/values are only valid as long as the corresponding File is still around. Lockedby as-is can perfectly encode these kind of rules.
So I don't think tying LockedBy to Guard, or even a specific Lock type is a good idea. -Sima
linux-stable-mirror@lists.linaro.org
-
Alice Ryhl -
Boqun Feng -
Gary Guo -
Simona Vetter