All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Alexandre Courbot" <acourbot@nvidia.com>
To: "Alice Ryhl" <aliceryhl@google.com>, "Lyude Paul" <lyude@redhat.com>
Cc: "Boqun Feng" <boqun@kernel.org>, "Gary Guo" <gary@garyguo.net>,
	"Lyude Paul" <lyude@redhat.com>,
	"Daniel Almeida" <daniel.almeida@collabora.com>,
	"Onur Özkan" <work@onurozkan.dev>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Carlos Llamas" <cmllamas@google.com>,
	"Luis Chamberlain" <mcgrof@kernel.org>,
	"Petr Pavlu" <petr.pavlu@suse.com>,
	"Daniel Gomez" <da.gomez@kernel.org>,
	"Sami Tolvanen" <samitolvanen@google.com>,
	"Aaron Tomlin" <atomlin@atomlin.com>,
	"Miguel Ojeda" <ojeda@kernel.org>,
	"Björn Roy Baron" <bjorn3_gh@protonmail.com>,
	"Benno Lossin" <lossin@kernel.org>,
	"Andreas Hindborg" <a.hindborg@kernel.org>,
	"Trevor Gross" <tmgross@umich.edu>,
	"Danilo Krummrich" <dakr@kernel.org>,
	"Tamir Duberstein" <tamird@kernel.org>,
	linux-modules@vger.kernel.org, linux-kernel@vger.kernel.org,
	rust-for-linux@vger.kernel.org
Subject: Re: [PATCH 2/3] rust: sync: add SetOnce::try_get_or_populate()
Date: Thu, 30 Jul 2026 23:48:15 +0900	[thread overview]
Message-ID: <DKBZLF7RMQE8.2KQJKLGIE0X6N@nvidia.com> (raw)
In-Reply-To: <amsVU-1o_zNOIOts@google.com>

On Thu Jul 30, 2026 at 6:11 PM JST, Alice Ryhl wrote:
> On Wed, Jul 29, 2026 at 11:05:55PM +0900, Alexandre Courbot wrote:
>> On Wed Jul 22, 2026 at 6:16 PM JST, Alice Ryhl wrote:
>> > +    where
>> > +        B: lock::Backend,
>> > +        F: FnOnce() -> Result<T, E>,
>> > +    {
>> > +        if let Some(value) = self.as_ref() {
>> > +            return Ok(value);
>> > +        }
>> > +
>> > +        let mut to_insert = f()?;
>> 
>> This means that `f` can run more than once for a given `SetOnce`, which
>> can lead to problems depending on `f`'s' side-effects.
>> 
>> In the GEM shmem case, we would create a second `SGTableMap`, and since
>> `SGTableMap` assumes it is the sole owner, the last instance to drop
>> would create a use-after-free.
>> 
>> Now this sounds more like a problem with `SGTableMap`, but if we cannot
>> avoid calling `f` at least twice then I think it would help if this was
>> documented.
>
> Hmm ... this is the behavior I want in Binder. Actually creating the
> value is an allocation, but my lock is a spinlock so I cannot invoke f()
> under the lock. This means that each caller will make their own
> allocation, and then we throw away any extras if there are concurrent
> callers.
>
> If GEM shmem wants f() invoked under the lock, then that's just a
> different operation than the one Binder wants.
>
> Do you think we should add both?

Possibly... but this would introduce a potential footgun that sleeps
while holding a spinlock, unless we limit the run-f-under-lock version
to work only with mutexes.

Maybe the proper solution is to fix `SGTableMap` so it supports multiple
instantiations. It is, after all, a bit footgunny on its own.

In any case, the current versions should warn users about the fact that
`f` can be called more than once imho.

  reply	other threads:[~2026-07-30 14:48 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-07-22  9:16 [PATCH 0/3] rust: sync: add SetOnce::try_get_or_populate() Alice Ryhl
2026-07-22  9:16 ` [PATCH 1/3] rust: sync: return `Result<&T, T>` from `SetOnce::populate()` Alice Ryhl
2026-07-22  9:23   ` sashiko-bot
2026-07-29 14:09   ` Alexandre Courbot
2026-07-22  9:16 ` [PATCH 2/3] rust: sync: add SetOnce::try_get_or_populate() Alice Ryhl
2026-07-22  9:27   ` sashiko-bot
2026-07-29 14:05   ` Alexandre Courbot
2026-07-29 14:23     ` Boqun Feng
2026-07-30  9:11     ` Alice Ryhl
2026-07-30 14:48       ` Alexandre Courbot [this message]
2026-07-22  9:16 ` [PATCH 3/3] rust_binder: use SetOnce::try_get_or_populate() Alice Ryhl
2026-07-29 14:12 ` [PATCH 0/3] rust: sync: add SetOnce::try_get_or_populate() Alexandre Courbot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DKBZLF7RMQE8.2KQJKLGIE0X6N@nvidia.com \
    --to=acourbot@nvidia.com \
    --cc=a.hindborg@kernel.org \
    --cc=aliceryhl@google.com \
    --cc=atomlin@atomlin.com \
    --cc=bjorn3_gh@protonmail.com \
    --cc=boqun@kernel.org \
    --cc=cmllamas@google.com \
    --cc=da.gomez@kernel.org \
    --cc=dakr@kernel.org \
    --cc=daniel.almeida@collabora.com \
    --cc=gary@garyguo.net \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-modules@vger.kernel.org \
    --cc=lossin@kernel.org \
    --cc=lyude@redhat.com \
    --cc=mcgrof@kernel.org \
    --cc=ojeda@kernel.org \
    --cc=petr.pavlu@suse.com \
    --cc=rust-for-linux@vger.kernel.org \
    --cc=samitolvanen@google.com \
    --cc=tamird@kernel.org \
    --cc=tmgross@umich.edu \
    --cc=work@onurozkan.dev \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.