linux-kernel - Re: [PATCH v7 08/16] rust: add devres abstraction

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] [day] [month] [year] [list]

Message-ID: <Z4dYsoKrbP4_Nz_v@pollux>
Date: Wed, 15 Jan 2025 07:41:54 +0100
From: Danilo Krummrich <dakr@...nel.org>
To: Gary Guo <gary@...yguo.net>, gregkh@...uxfoundation.org
Cc: rafael@...nel.org, bhelgaas@...gle.com, ojeda@...nel.org,
	alex.gaynor@...il.com, boqun.feng@...il.com,
	bjorn3_gh@...tonmail.com, benno.lossin@...ton.me, tmgross@...ch.edu,
	a.hindborg@...sung.com, aliceryhl@...gle.com, airlied@...il.com,
	fujita.tomonori@...il.com, lina@...hilina.net, pstanner@...hat.com,
	ajanulgu@...hat.com, lyude@...hat.com, robh@...nel.org,
	daniel.almeida@...labora.com, saravanak@...gle.com,
	dirk.behme@...bosch.com, j@...nau.net, fabien.parent@...aro.org,
	chrisi.schrefl@...il.com, paulmck@...nel.org,
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-pci@...r.kernel.org, devicetree@...r.kernel.org,
	rcu@...r.kernel.org
Subject: Re: [PATCH v7 08/16] rust: add devres abstraction

On Tue, Jan 14, 2025 at 05:33:45PM +0100, Danilo Krummrich wrote:
> > > +impl<T> Drop for Devres<T> {
> > > +    fn drop(&mut self) {
> > > +        // Revoke the data, such that it gets dropped already and the actual resource is freed.
> > > +        //
> > > +        // `DevresInner` has to stay alive until the devres callback has been called. This is
> > > +        // necessary since we don't know when `Devres` is dropped and calling
> > > +        // `devm_remove_action()` instead could race with `devres_release_all()`.
> > 
> > IIUC, the outcome of that race is the `WARN` if
> > devres_release_all takes the spinlock first and has already remvoed the
> > action?
> > 
> > Could you do a custom devres_release here that mimick
> > `devm_remove_action` but omit the `WARN`? This way it allows the memory
> > behind DevresInner to be freed early without keeping it allocated until
> > the end of device lifetime.
> 
> Better late than never, I now remember what's the *actual* race I was preventing
> here:
> 
>   | Task 0                               | Task 1
> --|----------------------------------------------------------------------------
> 1 | Devres::drop() {                     | devres_release_all() {
> 2 |    DevresInner::remove_action() {    |    spin_lock_irqsave();
> 3 |                                      |    remove_nodes(&todo);
> 4 |                                      |    spin_unlock_irqrestore();
> 5 |       devm_remove_action_nowarn();   |
> 6 |       let _ = Arc::from_raw();       |
> 7 |    }                                 |
> 8 | }                                    |
> 9 |                                      |    release_nodes(&todo);
> 10|                                      | }
> 
> So, if devres_release_all() wins the race it stores the devres action within the
> temporary todo list. Which means that in 9 we enter
> DevresInner::devres_callback() even though DevresInner has been freed already.
> 
> Unfortunately, this race can happen with [1], but not with this original version
> of Devres.

Well, actually this can't happen, since obviously we know whether Devres:drop
removed it or whether it will be released by devres_release_all() and we only
free DevresInner conditionally.

So, the code in [1] is fine; I somehow managed to confuse myself.

Sorry for the noise.

- Danilo

> 
> I see two ways to fix it:
> 
> 1) Just revert [1] and stick with the original version.
> 
> 2) Use devm_release_action() instead of devm_remove_action() and don't drop the
>    reference in DevresInner::remove_action() (6). This way the reference is
>    always dropped from the callback.
> 
> With 2) we still have an unnecessary call to revoke() if Devres is dropped
> before the device is detached from the driver, but that's still better than
> keeping DevresInner alive until the device is detached from the driver, which
> the original version did. Hence, I'll go ahead and prepare a patch for this,
> unless there are any concerns.
> 
> - Danilo
> 
> [1] https://lore.kernel.org/rust-for-linux/20250107122609.8135-2-dakr@kernel.org/