From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from bali.collaboradmins.com (bali.collaboradmins.com [148.251.105.195]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 95A77314B73; Fri, 6 Feb 2026 07:53:55 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=148.251.105.195 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1770364436; cv=none; b=QT3FlYADj3Qt40hPjBPzXD7zVIOTnbeHqBFSLtoE6hxm9gDNLEZwVhF245EusYHy6Vu/xu/5r+Mabwi86U5T0q8wqsh3jLoiPFqklQs0UhfQWP0LHqVWVSPVTDX4ypev5OhxT5EWpUTz8UMjj1yw4Wz/G1x0Yo9PfyOAkAL+mRI= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1770364436; c=relaxed/simple; bh=m4cWgc4LDTHareEHz+5x97wcVU4nLN++KhsOx8f9moE=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=mn+U8J7JIc4WU+cMfrmkoHR/LV8cIrmUQtI/tozFJzxWSzoZxCfq9gRDpQ0Xy669wob743Eo8GdPmpxN+cGgNI3IYXZCm/2uuUn/OdpJqxaRTSagpj0j8LVkDn+WoyBqZyby6znQSU4ZlrhfS9cWwMbMnzqgskeXgElk7vWrFfE= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=collabora.com; spf=pass smtp.mailfrom=collabora.com; dkim=pass (2048-bit key) header.d=collabora.com header.i=@collabora.com header.b=IG0hvRYu; arc=none smtp.client-ip=148.251.105.195 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=collabora.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=collabora.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=collabora.com header.i=@collabora.com header.b="IG0hvRYu" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1770364427; bh=m4cWgc4LDTHareEHz+5x97wcVU4nLN++KhsOx8f9moE=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=IG0hvRYuypLCsp5pSJ0MxEFpRT/GNK9i61V+0v2ZrhG56EjbzdihbbLRlVqvQ9qCt 9VeZRzTaPYkZtkgeqXypDe2qcTGS585KnWnEC5EWhwOsCLsC/B+mJAbvIOwe6g8YMT mLNG1ZlFNz1LJsqF4F6vHumcMKK1Ica+WrqoziVxWHCLmmgRtC0aX2gbnwKVB2nJIK V/QWvJcvv8n/QggPW3aS2LnmJW4bWHz67hQy+sF2E/QtT6v8ef/r8vNtjHcDC+Q1dK rd1esEeaH65v+eoKkbHNRK+SQ1UGaIhbGH8ePTPnssehUsrRmBOlVoeAFcnuJEp4Qw u6EnuVuIjewXA== Received: from fedora (unknown [IPv6:2a01:e0a:2c:6930:d919:a6e:5ea1:8a9f]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (prime256v1) server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: bbrezillon) by bali.collaboradmins.com (Postfix) with ESMTPSA id 1E0BB17E1301; Fri, 6 Feb 2026 08:53:47 +0100 (CET) Date: Fri, 6 Feb 2026 08:53:42 +0100 From: Boris Brezillon To: Danilo Krummrich Cc: gregkh@linuxfoundation.org, rafael@kernel.org, ojeda@kernel.org, boqun.feng@gmail.com, gary@garyguo.net, bjorn3_gh@protonmail.com, lossin@kernel.org, a.hindborg@kernel.org, aliceryhl@google.com, tmgross@umich.edu, driver-core@lists.linux.dev, rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org, Markus Probst Subject: Re: [PATCH] rust: devres: fix race condition due to nesting Message-ID: <20260206085342.609ab3a1@fedora> In-Reply-To: <20260205222529.91465-1-dakr@kernel.org> References: <20260205222529.91465-1-dakr@kernel.org> Organization: Collabora X-Mailer: Claws Mail 4.3.1 (GTK 3.24.51; x86_64-redhat-linux-gnu) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit On Thu, 5 Feb 2026 23:25:15 +0100 Danilo Krummrich wrote: > Commit f5d3ef25d238 ("rust: devres: get rid of Devres' inner Arc") did > attempt to optimize away the internal reference count of Devres. > > However, without an internal reference count, we can't support cases > where Devres is indirectly nested, resulting into a deadlock. > > Such indirect nesting easily happens in the following way: > > A registration object (which is guarded by devres) hold a reference > count of an object that holds a device resource guarded by devres > itself. > > For instance a drm::Registration holds a reference of a drm::Device. The > drm::Device itself holds a device resource in its private data. > > When the drm::Registration is dropped by devres, and it happens that it > did hold the last reference count of the drm::Device, it also drops the > device resource, which is guarded by devres itself. > > Thus, resulting into a deadlock in the Devres destructor of the device > resource, as in the following backtrace. > > sysrq: Show Blocked State > task:rmmod state:D stack:0 pid:1331 tgid:1331 ppid:1330 task_flags:0x400100 flags:0x00000010 > Call trace: > __switch_to+0x190/0x294 (T) > __schedule+0x878/0xf10 > schedule+0x4c/0xcc > schedule_timeout+0x44/0x118 > wait_for_common+0xc0/0x18c > wait_for_completion+0x18/0x24 > _RINvNtCs4gKlGRWyJ5S_4core3ptr13drop_in_placeINtNtNtCsgzhNYVB7wSz_6kernel4sync3arc3ArcINtNtBN_6devres6DevresmEEECsRdyc7Hyps3_15rust_driver_pci+0x68/0xe8 [rust_driver_pci] > _RINvNvNtCsgzhNYVB7wSz_6kernel6devres16register_foreign8callbackINtNtCs4gKlGRWyJ5S_4core3pin3PinINtNtNtB6_5alloc4kbox3BoxINtNtNtB6_4sync3arc3ArcINtB4_6DevresmEENtNtB1A_9allocator7KmallocEEECsRdyc7Hyps3_15rust_driver_pci+0x34/0xc8 [rust_driver_pci] > devm_action_release+0x14/0x20 > devres_release_all+0xb8/0x118 > device_release_driver_internal+0x1c4/0x28c > driver_detach+0x94/0xd4 > bus_remove_driver+0xdc/0x11c > driver_unregister+0x34/0x58 > pci_unregister_driver+0x20/0x80 > __arm64_sys_delete_module+0x1d8/0x254 > invoke_syscall+0x40/0xcc > el0_svc_common+0x8c/0xd8 > do_el0_svc+0x1c/0x28 > el0_svc+0x54/0x1d4 > el0t_64_sync_handler+0x84/0x12c > el0t_64_sync+0x198/0x19c > > In order to fix this, re-introduce the internal reference count. > > Reported-by: Boris Brezillon > Closes: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/.E2.9C.94.20Deadlock.20caused.20by.20nested.20Devres/with/571242651 > Reported-by: Markus Probst > Closes: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/.E2.9C.94.20Devres.20inside.20Devres.20stuck.20on.20cleanup/with/571239721 > Reported-by: Alice Ryhl > Closes: https://gitlab.freedesktop.org/panfrost/linux/-/merge_requests/56#note_3282757 > Fixes: f5d3ef25d238 ("rust: devres: get rid of Devres' inner Arc") > Signed-off-by: Danilo Krummrich Tested-by: Boris Brezillon > --- > rust/kernel/devres.rs | 148 +++++++++++------------------------------- > 1 file changed, 39 insertions(+), 109 deletions(-) > > diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs > index cdc49677022a..ff66f561740a 100644 > --- a/rust/kernel/devres.rs > +++ b/rust/kernel/devres.rs > @@ -21,30 +21,11 @@ > sync::{ > aref::ARef, > rcu, > - Completion, // > - }, > - types::{ > - ForeignOwnable, > - Opaque, > - ScopeGuard, // > + Arc, // > }, > + types::ForeignOwnable, > }; > > -use pin_init::Wrapper; > - > -/// [`Devres`] inner data accessed from [`Devres::callback`]. > -#[pin_data] > -struct Inner { > - #[pin] > - data: Revocable, > - /// Tracks whether [`Devres::callback`] has been completed. > - #[pin] > - devm: Completion, > - /// Tracks whether revoking [`Self::data`] has been completed. > - #[pin] > - revoke: Completion, > -} > - > /// This abstraction is meant to be used by subsystems to containerize [`Device`] bound resources to > /// manage their lifetime. > /// > @@ -121,18 +102,13 @@ struct Inner { > /// # fn no_run(dev: &Device) -> Result<(), Error> { > /// // SAFETY: Invalid usage for example purposes. > /// let iomem = unsafe { IoMem::<{ core::mem::size_of::() }>::new(0xBAAAAAAD)? }; > -/// let devres = KBox::pin_init(Devres::new(dev, iomem), GFP_KERNEL)?; > +/// let devres = Devres::new(dev, iomem)?; > /// > /// let res = devres.try_access().ok_or(ENXIO)?; > /// res.write8(0x42, 0x0); > /// # Ok(()) > /// # } > /// ``` > -/// > -/// # Invariants > -/// > -/// `Self::inner` is guaranteed to be initialized and is always accessed read-only. > -#[pin_data(PinnedDrop)] > pub struct Devres { > dev: ARef, > /// Pointer to [`Self::devres_callback`]. > @@ -140,14 +116,7 @@ pub struct Devres { > /// Has to be stored, since Rust does not guarantee to always return the same address for a > /// function. However, the C API uses the address as a key. > callback: unsafe extern "C" fn(*mut c_void), > - /// Contains all the fields shared with [`Self::callback`]. > - // TODO: Replace with `UnsafePinned`, once available. > - // > - // Subsequently, the `drop_in_place()` in `Devres::drop` and `Devres::new` as well as the > - // explicit `Send` and `Sync' impls can be removed. > - #[pin] > - inner: Opaque>, > - _add_action: (), > + data: Arc>, > } > > impl Devres { > @@ -155,74 +124,47 @@ impl Devres { > /// > /// The `data` encapsulated within the returned `Devres` instance' `data` will be > /// (revoked)[`Revocable`] once the device is detached. > - pub fn new<'a, E>( > - dev: &'a Device, > - data: impl PinInit + 'a, > - ) -> impl PinInit + 'a > + pub fn new(dev: &Device, data: impl PinInit) -> Result > where > - T: 'a, > Error: From, > { > - try_pin_init!(&this in Self { > - dev: dev.into(), > - callback: Self::devres_callback, > - // INVARIANT: `inner` is properly initialized. > - inner <- Opaque::pin_init(try_pin_init!(Inner { > - devm <- Completion::new(), > - revoke <- Completion::new(), > - data <- Revocable::new(data), > - })), > - // TODO: Replace with "initializer code blocks" [1] once available. > - // > - // [1] https://github.com/Rust-for-Linux/pin-init/pull/69 > - _add_action: { > - // SAFETY: `this` is a valid pointer to uninitialized memory. > - let inner = unsafe { &raw mut (*this.as_ptr()).inner }; > + let callback = Self::devres_callback; > + let data = Arc::pin_init(Revocable::new(data), GFP_KERNEL)?; > > - // SAFETY: > - // - `dev.as_raw()` is a pointer to a valid bound device. > - // - `inner` is guaranteed to be a valid for the duration of the lifetime of `Self`. > - // - `devm_add_action()` is guaranteed not to call `callback` until `this` has been > - // properly initialized, because we require `dev` (i.e. the *bound* device) to > - // live at least as long as the returned `impl PinInit`. > - to_result(unsafe { > - bindings::devm_add_action(dev.as_raw(), Some(*callback), inner.cast()) > - }).inspect_err(|_| { > - let inner = Opaque::cast_into(inner); > + // SAFETY: > + // - `dev.as_raw()` is a pointer to a valid bound device. > + // - `data` is guaranteed to be a valid for the duration of the lifetime of `Self`. > + // - `devm_add_action()` is guaranteed not to call `callback` for the entire lifetime of > + // `dev`. > + to_result(unsafe { > + bindings::devm_add_action( > + dev.as_raw(), > + Some(callback), > + Arc::as_ptr(&data).cast_mut().cast(), > + ) > + })?; > > - // SAFETY: `inner` is a valid pointer to an `Inner` and valid for both reads > - // and writes. > - unsafe { core::ptr::drop_in_place(inner) }; > - })?; > - }, > - }) > - } > + // Take additional reference count for `devm_add_action()`. > + core::mem::forget(data.clone()); > > - fn inner(&self) -> &Inner { > - // SAFETY: By the type invairants of `Self`, `inner` is properly initialized and always > - // accessed read-only. > - unsafe { &*self.inner.get() } > + Ok(Self { > + dev: dev.into(), > + callback, > + data, > + }) > } > > fn data(&self) -> &Revocable { > - &self.inner().data > + &self.data > } > > #[allow(clippy::missing_safety_doc)] > unsafe extern "C" fn devres_callback(ptr: *mut kernel::ffi::c_void) { > - // SAFETY: In `Self::new` we've passed a valid pointer to `Inner` to `devm_add_action()`, > - // hence `ptr` must be a valid pointer to `Inner`. > - let inner = unsafe { &*ptr.cast::>() }; > + // SAFETY: In `Self::new` we've passed a valid pointer of `Revocable` to > + // `devm_add_action()`, hence `ptr` must be a valid pointer to `Revocable`. > + let data = unsafe { Arc::from_raw(ptr.cast::>()) }; > > - // Ensure that `inner` can't be used anymore after we signal completion of this callback. > - let inner = ScopeGuard::new_with_data(inner, |inner| inner.devm.complete_all()); > - > - if !inner.data.revoke() { > - // If `revoke()` returns false, it means that `Devres::drop` already started revoking > - // `data` for us. Hence we have to wait until `Devres::drop` signals that it > - // completed revoking `data`. > - inner.revoke.wait_for_completion(); > - } > + data.revoke(); > } > > fn remove_action(&self) -> bool { > @@ -234,7 +176,7 @@ fn remove_action(&self) -> bool { > bindings::devm_remove_action_nowarn( > self.dev.as_raw(), > Some(self.callback), > - core::ptr::from_ref(self.inner()).cast_mut().cast(), > + core::ptr::from_ref(self.data()).cast_mut().cast(), > ) > } == 0) > } > @@ -313,31 +255,19 @@ unsafe impl Send for Devres {} > // SAFETY: `Devres` can be shared with any task, if `T: Sync`. > unsafe impl Sync for Devres {} > > -#[pinned_drop] > -impl PinnedDrop for Devres { > - fn drop(self: Pin<&mut Self>) { > +impl Drop for Devres { > + fn drop(&mut self) { > // SAFETY: When `drop` runs, it is guaranteed that nobody is accessing the revocable data > // anymore, hence it is safe not to wait for the grace period to finish. > if unsafe { self.data().revoke_nosync() } { > // We revoked `self.data` before the devres action did, hence try to remove it. > - if !self.remove_action() { > - // We could not remove the devres action, which means that it now runs concurrently, > - // hence signal that `self.data` has been revoked by us successfully. > - self.inner().revoke.complete_all(); > - > - // Wait for `Self::devres_callback` to be done using this object. > - self.inner().devm.wait_for_completion(); > + if self.remove_action() { > + // SAFETY: In `Self::new` we have taken an additional reference count of `self.data` > + // for `devm_add_action()`. Since `remove_action()` was successful, we have to drop > + // this additional reference count. > + drop(unsafe { Arc::from_raw(Arc::as_ptr(&self.data)) }); > } > - } else { > - // `Self::devres_callback` revokes `self.data` for us, hence wait for it to be done > - // using this object. > - self.inner().devm.wait_for_completion(); > } > - > - // INVARIANT: At this point it is guaranteed that `inner` can't be accessed any more. > - // > - // SAFETY: `inner` is valid for dropping. > - unsafe { core::ptr::drop_in_place(self.inner.get()) }; > } > } > > > base-commit: f55ae0bfa00e446ea751d09f468daeafc303e03f