[PATCH] rust: sync: fix incorrect Sync bounds for LockedBy

[PATCH] rust: sync: fix incorrect Sync bounds for LockedBy

[Alice Ryhl]

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,
-- 
Alice Ryhl <aliceryhl@google.com>

Re: [PATCH] rust: sync: fix incorrect Sync bounds for LockedBy

[Simona Vetter]

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>
> 

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] rust: sync: fix incorrect Sync bounds for LockedBy

[Boqun Feng]

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

Re: [PATCH] rust: sync: fix incorrect Sync bounds for LockedBy

[Gary Guo]

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

Re: [PATCH] rust: sync: fix incorrect Sync bounds for LockedBy

[Alice Ryhl]

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

Re: [PATCH] rust: sync: fix incorrect Sync bounds for LockedBy

[Gary Guo]

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

Re: [PATCH] rust: sync: fix incorrect Sync bounds for LockedBy

[Simona Vetter]

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
-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch