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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240915152558.68ff5675.gary@garyguo.net>
Date: Sun, 15 Sep 2024 15:25:58 +0100
From: Gary Guo <gary@...yguo.net>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Boqun Feng <boqun.feng@...il.com>, Miguel Ojeda <ojeda@...nel.org>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>, Benno Lossin
 <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...sung.com>, Trevor
 Gross <tmgross@...ch.edu>, Martin Rodriguez Reboredo <yakoyoku@...il.com>,
 rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
 stable@...r.kernel.org
Subject: Re: [PATCH] rust: sync: fix incorrect Sync bounds for LockedBy

On Sun, 15 Sep 2024 16:11:57 +0200
Alice Ryhl <aliceryhl@...gle.com> wrote:

> On Sun, Sep 15, 2024 at 3:49 PM Gary Guo <gary@...yguo.net> wrote:
> >
> > On Fri, 13 Sep 2024 23:28:37 -0700
> > Boqun Feng <boqun.feng@...il.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