2024-12-19 17:04:10 +00:00
|
|
|
// SPDX-License-Identifier: GPL-2.0
|
|
|
|
|
|
|
|
//! Devres abstraction
|
|
|
|
//!
|
|
|
|
//! [`Devres`] represents an abstraction for the kernel devres (device resource management)
|
|
|
|
//! implementation.
|
|
|
|
|
|
|
|
use crate::{
|
|
|
|
alloc::Flags,
|
|
|
|
bindings,
|
2025-04-13 17:37:03 +00:00
|
|
|
device::{Bound, Device},
|
2024-12-19 17:04:10 +00:00
|
|
|
error::{Error, Result},
|
2025-01-07 12:25:11 +00:00
|
|
|
ffi::c_void,
|
2024-12-19 17:04:10 +00:00
|
|
|
prelude::*,
|
2025-06-11 17:48:25 +00:00
|
|
|
revocable::{Revocable, RevocableGuard},
|
|
|
|
sync::{rcu, Arc, Completion},
|
2025-01-07 12:25:11 +00:00
|
|
|
types::ARef,
|
2024-12-19 17:04:10 +00:00
|
|
|
};
|
|
|
|
|
|
|
|
#[pin_data]
|
2025-06-26 13:24:46 +00:00
|
|
|
struct DevresInner<T: Send> {
|
2025-01-07 12:25:11 +00:00
|
|
|
dev: ARef<Device>,
|
|
|
|
callback: unsafe extern "C" fn(*mut c_void),
|
2024-12-19 17:04:10 +00:00
|
|
|
#[pin]
|
|
|
|
data: Revocable<T>,
|
rust: devres: fix race in Devres::drop()
In Devres::drop() we first remove the devres action and then drop the
wrapped device resource.
The design goal is to give the owner of a Devres object control over when
the device resource is dropped, but limit the overall scope to the
corresponding device being bound to a driver.
However, there's a race that was introduced with commit 8ff656643d30
("rust: devres: remove action in `Devres::drop`"), but also has been
(partially) present from the initial version on.
In Devres::drop(), the devres action is removed successfully and
subsequently the destructor of the wrapped device resource runs.
However, there is no guarantee that the destructor of the wrapped device
resource completes before the driver core is done unbinding the
corresponding device.
If in Devres::drop(), the devres action can't be removed, it means that
the devres callback has been executed already, or is still running
concurrently. In case of the latter, either Devres::drop() wins revoking
the Revocable or the devres callback wins revoking the Revocable. If
Devres::drop() wins, we (again) have no guarantee that the destructor of
the wrapped device resource completes before the driver core is done
unbinding the corresponding device.
CPU0 CPU1
------------------------------------------------------------------------
Devres::drop() { Devres::devres_callback() {
self.data.revoke() { this.data.revoke() {
is_available.swap() == true
is_available.swap == false
}
}
// [...]
// device fully unbound
drop_in_place() {
// release device resource
}
}
}
Depending on the specific device resource, this can potentially lead to
user-after-free bugs.
In order to fix this, implement the following logic.
In the devres callback, we're always good when we get to revoke the
device resource ourselves, i.e. Revocable::revoke() returns true.
If Revocable::revoke() returns false, it means that Devres::drop(),
concurrently, already drops the device resource and we have to wait for
Devres::drop() to signal that it finished dropping the device resource.
Note that if we hit the case where we need to wait for the completion of
Devres::drop() in the devres callback, it means that we're actually
racing with a concurrent Devres::drop() call, which already started
revoking the device resource for us. This is rather unlikely and means
that the concurrent Devres::drop() already started doing our work and we
just need to wait for it to complete it for us. Hence, there should not
be any additional overhead from that.
(Actually, for now it's even better if Devres::drop() does the work for
us, since it can bypass the synchronize_rcu() call implied by
Revocable::revoke(), but this goes away anyways once I get to implement
the split devres callback approach, which allows us to first flip the
atomics of all registered Devres objects of a certain device, execute a
single synchronize_rcu() and then drop all revocable objects.)
In Devres::drop() we try to revoke the device resource. If that is *not*
successful, it means that the devres callback already did and we're good.
Otherwise, we try to remove the devres action, which, if successful,
means that we're good, since the device resource has just been revoked
by us *before* we removed the devres action successfully.
If the devres action could not be removed, it means that the devres
callback must be running concurrently, hence we signal that the device
resource has been revoked by us, using the completion.
This makes it safe to drop a Devres object from any task and at any point
of time, which is one of the design goals.
Fixes: 76c01ded724b ("rust: add devres abstraction")
Reported-by: Alice Ryhl <aliceryhl@google.com>
Closes: https://lore.kernel.org/lkml/aD64YNuqbPPZHAa5@google.com/
Reviewed-by: Benno Lossin <lossin@kernel.org>
Link: https://lore.kernel.org/r/20250612121817.1621-4-dakr@kernel.org
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
2025-06-12 12:17:15 +00:00
|
|
|
#[pin]
|
|
|
|
revoke: Completion,
|
2024-12-19 17:04:10 +00:00
|
|
|
}
|
|
|
|
|
|
|
|
/// This abstraction is meant to be used by subsystems to containerize [`Device`] bound resources to
|
|
|
|
/// manage their lifetime.
|
|
|
|
///
|
|
|
|
/// [`Device`] bound resources should be freed when either the resource goes out of scope or the
|
rust: devres: fix race in Devres::drop()
In Devres::drop() we first remove the devres action and then drop the
wrapped device resource.
The design goal is to give the owner of a Devres object control over when
the device resource is dropped, but limit the overall scope to the
corresponding device being bound to a driver.
However, there's a race that was introduced with commit 8ff656643d30
("rust: devres: remove action in `Devres::drop`"), but also has been
(partially) present from the initial version on.
In Devres::drop(), the devres action is removed successfully and
subsequently the destructor of the wrapped device resource runs.
However, there is no guarantee that the destructor of the wrapped device
resource completes before the driver core is done unbinding the
corresponding device.
If in Devres::drop(), the devres action can't be removed, it means that
the devres callback has been executed already, or is still running
concurrently. In case of the latter, either Devres::drop() wins revoking
the Revocable or the devres callback wins revoking the Revocable. If
Devres::drop() wins, we (again) have no guarantee that the destructor of
the wrapped device resource completes before the driver core is done
unbinding the corresponding device.
CPU0 CPU1
------------------------------------------------------------------------
Devres::drop() { Devres::devres_callback() {
self.data.revoke() { this.data.revoke() {
is_available.swap() == true
is_available.swap == false
}
}
// [...]
// device fully unbound
drop_in_place() {
// release device resource
}
}
}
Depending on the specific device resource, this can potentially lead to
user-after-free bugs.
In order to fix this, implement the following logic.
In the devres callback, we're always good when we get to revoke the
device resource ourselves, i.e. Revocable::revoke() returns true.
If Revocable::revoke() returns false, it means that Devres::drop(),
concurrently, already drops the device resource and we have to wait for
Devres::drop() to signal that it finished dropping the device resource.
Note that if we hit the case where we need to wait for the completion of
Devres::drop() in the devres callback, it means that we're actually
racing with a concurrent Devres::drop() call, which already started
revoking the device resource for us. This is rather unlikely and means
that the concurrent Devres::drop() already started doing our work and we
just need to wait for it to complete it for us. Hence, there should not
be any additional overhead from that.
(Actually, for now it's even better if Devres::drop() does the work for
us, since it can bypass the synchronize_rcu() call implied by
Revocable::revoke(), but this goes away anyways once I get to implement
the split devres callback approach, which allows us to first flip the
atomics of all registered Devres objects of a certain device, execute a
single synchronize_rcu() and then drop all revocable objects.)
In Devres::drop() we try to revoke the device resource. If that is *not*
successful, it means that the devres callback already did and we're good.
Otherwise, we try to remove the devres action, which, if successful,
means that we're good, since the device resource has just been revoked
by us *before* we removed the devres action successfully.
If the devres action could not be removed, it means that the devres
callback must be running concurrently, hence we signal that the device
resource has been revoked by us, using the completion.
This makes it safe to drop a Devres object from any task and at any point
of time, which is one of the design goals.
Fixes: 76c01ded724b ("rust: add devres abstraction")
Reported-by: Alice Ryhl <aliceryhl@google.com>
Closes: https://lore.kernel.org/lkml/aD64YNuqbPPZHAa5@google.com/
Reviewed-by: Benno Lossin <lossin@kernel.org>
Link: https://lore.kernel.org/r/20250612121817.1621-4-dakr@kernel.org
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
2025-06-12 12:17:15 +00:00
|
|
|
/// [`Device`] is unbound respectively, depending on what happens first. In any case, it is always
|
|
|
|
/// guaranteed that revoking the device resource is completed before the corresponding [`Device`]
|
|
|
|
/// is unbound.
|
2024-12-19 17:04:10 +00:00
|
|
|
///
|
|
|
|
/// To achieve that [`Devres`] registers a devres callback on creation, which is called once the
|
|
|
|
/// [`Device`] is unbound, revoking access to the encapsulated resource (see also [`Revocable`]).
|
|
|
|
///
|
|
|
|
/// After the [`Devres`] has been unbound it is not possible to access the encapsulated resource
|
|
|
|
/// anymore.
|
|
|
|
///
|
|
|
|
/// [`Devres`] users should make sure to simply free the corresponding backing resource in `T`'s
|
|
|
|
/// [`Drop`] implementation.
|
|
|
|
///
|
|
|
|
/// # Example
|
|
|
|
///
|
|
|
|
/// ```no_run
|
2025-04-13 17:37:03 +00:00
|
|
|
/// # use kernel::{bindings, c_str, device::{Bound, Device}, devres::Devres, io::{Io, IoRaw}};
|
2024-12-19 17:04:10 +00:00
|
|
|
/// # use core::ops::Deref;
|
|
|
|
///
|
|
|
|
/// // See also [`pci::Bar`] for a real example.
|
|
|
|
/// struct IoMem<const SIZE: usize>(IoRaw<SIZE>);
|
|
|
|
///
|
|
|
|
/// impl<const SIZE: usize> IoMem<SIZE> {
|
|
|
|
/// /// # Safety
|
|
|
|
/// ///
|
|
|
|
/// /// [`paddr`, `paddr` + `SIZE`) must be a valid MMIO region that is mappable into the CPUs
|
|
|
|
/// /// virtual address space.
|
|
|
|
/// unsafe fn new(paddr: usize) -> Result<Self>{
|
|
|
|
/// // SAFETY: By the safety requirements of this function [`paddr`, `paddr` + `SIZE`) is
|
|
|
|
/// // valid for `ioremap`.
|
|
|
|
/// let addr = unsafe { bindings::ioremap(paddr as _, SIZE as _) };
|
|
|
|
/// if addr.is_null() {
|
|
|
|
/// return Err(ENOMEM);
|
|
|
|
/// }
|
|
|
|
///
|
|
|
|
/// Ok(IoMem(IoRaw::new(addr as _, SIZE)?))
|
|
|
|
/// }
|
|
|
|
/// }
|
|
|
|
///
|
|
|
|
/// impl<const SIZE: usize> Drop for IoMem<SIZE> {
|
|
|
|
/// fn drop(&mut self) {
|
|
|
|
/// // SAFETY: `self.0.addr()` is guaranteed to be properly mapped by `Self::new`.
|
|
|
|
/// unsafe { bindings::iounmap(self.0.addr() as _); };
|
|
|
|
/// }
|
|
|
|
/// }
|
|
|
|
///
|
|
|
|
/// impl<const SIZE: usize> Deref for IoMem<SIZE> {
|
|
|
|
/// type Target = Io<SIZE>;
|
|
|
|
///
|
|
|
|
/// fn deref(&self) -> &Self::Target {
|
|
|
|
/// // SAFETY: The memory range stored in `self` has been properly mapped in `Self::new`.
|
|
|
|
/// unsafe { Io::from_raw(&self.0) }
|
|
|
|
/// }
|
|
|
|
/// }
|
2025-04-13 17:37:03 +00:00
|
|
|
/// # fn no_run(dev: &Device<Bound>) -> Result<(), Error> {
|
2024-12-19 17:04:10 +00:00
|
|
|
/// // SAFETY: Invalid usage for example purposes.
|
|
|
|
/// let iomem = unsafe { IoMem::<{ core::mem::size_of::<u32>() }>::new(0xBAAAAAAD)? };
|
2025-04-13 17:37:03 +00:00
|
|
|
/// let devres = Devres::new(dev, iomem, GFP_KERNEL)?;
|
2024-12-19 17:04:10 +00:00
|
|
|
///
|
|
|
|
/// let res = devres.try_access().ok_or(ENXIO)?;
|
2025-02-24 18:36:43 +00:00
|
|
|
/// res.write8(0x42, 0x0);
|
2024-12-19 17:04:10 +00:00
|
|
|
/// # Ok(())
|
|
|
|
/// # }
|
|
|
|
/// ```
|
2025-06-26 13:24:46 +00:00
|
|
|
pub struct Devres<T: Send>(Arc<DevresInner<T>>);
|
2024-12-19 17:04:10 +00:00
|
|
|
|
2025-06-26 13:24:46 +00:00
|
|
|
impl<T: Send> DevresInner<T> {
|
2025-04-13 17:37:03 +00:00
|
|
|
fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Arc<DevresInner<T>>> {
|
2024-12-19 17:04:10 +00:00
|
|
|
let inner = Arc::pin_init(
|
2025-06-26 20:00:39 +00:00
|
|
|
try_pin_init!( DevresInner {
|
2025-01-07 12:25:11 +00:00
|
|
|
dev: dev.into(),
|
|
|
|
callback: Self::devres_callback,
|
2024-12-19 17:04:10 +00:00
|
|
|
data <- Revocable::new(data),
|
rust: devres: fix race in Devres::drop()
In Devres::drop() we first remove the devres action and then drop the
wrapped device resource.
The design goal is to give the owner of a Devres object control over when
the device resource is dropped, but limit the overall scope to the
corresponding device being bound to a driver.
However, there's a race that was introduced with commit 8ff656643d30
("rust: devres: remove action in `Devres::drop`"), but also has been
(partially) present from the initial version on.
In Devres::drop(), the devres action is removed successfully and
subsequently the destructor of the wrapped device resource runs.
However, there is no guarantee that the destructor of the wrapped device
resource completes before the driver core is done unbinding the
corresponding device.
If in Devres::drop(), the devres action can't be removed, it means that
the devres callback has been executed already, or is still running
concurrently. In case of the latter, either Devres::drop() wins revoking
the Revocable or the devres callback wins revoking the Revocable. If
Devres::drop() wins, we (again) have no guarantee that the destructor of
the wrapped device resource completes before the driver core is done
unbinding the corresponding device.
CPU0 CPU1
------------------------------------------------------------------------
Devres::drop() { Devres::devres_callback() {
self.data.revoke() { this.data.revoke() {
is_available.swap() == true
is_available.swap == false
}
}
// [...]
// device fully unbound
drop_in_place() {
// release device resource
}
}
}
Depending on the specific device resource, this can potentially lead to
user-after-free bugs.
In order to fix this, implement the following logic.
In the devres callback, we're always good when we get to revoke the
device resource ourselves, i.e. Revocable::revoke() returns true.
If Revocable::revoke() returns false, it means that Devres::drop(),
concurrently, already drops the device resource and we have to wait for
Devres::drop() to signal that it finished dropping the device resource.
Note that if we hit the case where we need to wait for the completion of
Devres::drop() in the devres callback, it means that we're actually
racing with a concurrent Devres::drop() call, which already started
revoking the device resource for us. This is rather unlikely and means
that the concurrent Devres::drop() already started doing our work and we
just need to wait for it to complete it for us. Hence, there should not
be any additional overhead from that.
(Actually, for now it's even better if Devres::drop() does the work for
us, since it can bypass the synchronize_rcu() call implied by
Revocable::revoke(), but this goes away anyways once I get to implement
the split devres callback approach, which allows us to first flip the
atomics of all registered Devres objects of a certain device, execute a
single synchronize_rcu() and then drop all revocable objects.)
In Devres::drop() we try to revoke the device resource. If that is *not*
successful, it means that the devres callback already did and we're good.
Otherwise, we try to remove the devres action, which, if successful,
means that we're good, since the device resource has just been revoked
by us *before* we removed the devres action successfully.
If the devres action could not be removed, it means that the devres
callback must be running concurrently, hence we signal that the device
resource has been revoked by us, using the completion.
This makes it safe to drop a Devres object from any task and at any point
of time, which is one of the design goals.
Fixes: 76c01ded724b ("rust: add devres abstraction")
Reported-by: Alice Ryhl <aliceryhl@google.com>
Closes: https://lore.kernel.org/lkml/aD64YNuqbPPZHAa5@google.com/
Reviewed-by: Benno Lossin <lossin@kernel.org>
Link: https://lore.kernel.org/r/20250612121817.1621-4-dakr@kernel.org
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
2025-06-12 12:17:15 +00:00
|
|
|
revoke <- Completion::new(),
|
2024-12-19 17:04:10 +00:00
|
|
|
}),
|
|
|
|
flags,
|
|
|
|
)?;
|
|
|
|
|
|
|
|
// Convert `Arc<DevresInner>` into a raw pointer and make devres own this reference until
|
|
|
|
// `Self::devres_callback` is called.
|
|
|
|
let data = inner.clone().into_raw();
|
|
|
|
|
|
|
|
// SAFETY: `devm_add_action` guarantees to call `Self::devres_callback` once `dev` is
|
|
|
|
// detached.
|
2025-01-07 12:25:11 +00:00
|
|
|
let ret =
|
|
|
|
unsafe { bindings::devm_add_action(dev.as_raw(), Some(inner.callback), data as _) };
|
2024-12-19 17:04:10 +00:00
|
|
|
|
|
|
|
if ret != 0 {
|
|
|
|
// SAFETY: We just created another reference to `inner` in order to pass it to
|
|
|
|
// `bindings::devm_add_action`. If `bindings::devm_add_action` fails, we have to drop
|
|
|
|
// this reference accordingly.
|
|
|
|
let _ = unsafe { Arc::from_raw(data) };
|
|
|
|
return Err(Error::from_errno(ret));
|
|
|
|
}
|
|
|
|
|
|
|
|
Ok(inner)
|
|
|
|
}
|
|
|
|
|
2025-01-07 12:25:11 +00:00
|
|
|
fn as_ptr(&self) -> *const Self {
|
|
|
|
self as _
|
|
|
|
}
|
|
|
|
|
rust: devres: fix race in Devres::drop()
In Devres::drop() we first remove the devres action and then drop the
wrapped device resource.
The design goal is to give the owner of a Devres object control over when
the device resource is dropped, but limit the overall scope to the
corresponding device being bound to a driver.
However, there's a race that was introduced with commit 8ff656643d30
("rust: devres: remove action in `Devres::drop`"), but also has been
(partially) present from the initial version on.
In Devres::drop(), the devres action is removed successfully and
subsequently the destructor of the wrapped device resource runs.
However, there is no guarantee that the destructor of the wrapped device
resource completes before the driver core is done unbinding the
corresponding device.
If in Devres::drop(), the devres action can't be removed, it means that
the devres callback has been executed already, or is still running
concurrently. In case of the latter, either Devres::drop() wins revoking
the Revocable or the devres callback wins revoking the Revocable. If
Devres::drop() wins, we (again) have no guarantee that the destructor of
the wrapped device resource completes before the driver core is done
unbinding the corresponding device.
CPU0 CPU1
------------------------------------------------------------------------
Devres::drop() { Devres::devres_callback() {
self.data.revoke() { this.data.revoke() {
is_available.swap() == true
is_available.swap == false
}
}
// [...]
// device fully unbound
drop_in_place() {
// release device resource
}
}
}
Depending on the specific device resource, this can potentially lead to
user-after-free bugs.
In order to fix this, implement the following logic.
In the devres callback, we're always good when we get to revoke the
device resource ourselves, i.e. Revocable::revoke() returns true.
If Revocable::revoke() returns false, it means that Devres::drop(),
concurrently, already drops the device resource and we have to wait for
Devres::drop() to signal that it finished dropping the device resource.
Note that if we hit the case where we need to wait for the completion of
Devres::drop() in the devres callback, it means that we're actually
racing with a concurrent Devres::drop() call, which already started
revoking the device resource for us. This is rather unlikely and means
that the concurrent Devres::drop() already started doing our work and we
just need to wait for it to complete it for us. Hence, there should not
be any additional overhead from that.
(Actually, for now it's even better if Devres::drop() does the work for
us, since it can bypass the synchronize_rcu() call implied by
Revocable::revoke(), but this goes away anyways once I get to implement
the split devres callback approach, which allows us to first flip the
atomics of all registered Devres objects of a certain device, execute a
single synchronize_rcu() and then drop all revocable objects.)
In Devres::drop() we try to revoke the device resource. If that is *not*
successful, it means that the devres callback already did and we're good.
Otherwise, we try to remove the devres action, which, if successful,
means that we're good, since the device resource has just been revoked
by us *before* we removed the devres action successfully.
If the devres action could not be removed, it means that the devres
callback must be running concurrently, hence we signal that the device
resource has been revoked by us, using the completion.
This makes it safe to drop a Devres object from any task and at any point
of time, which is one of the design goals.
Fixes: 76c01ded724b ("rust: add devres abstraction")
Reported-by: Alice Ryhl <aliceryhl@google.com>
Closes: https://lore.kernel.org/lkml/aD64YNuqbPPZHAa5@google.com/
Reviewed-by: Benno Lossin <lossin@kernel.org>
Link: https://lore.kernel.org/r/20250612121817.1621-4-dakr@kernel.org
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
2025-06-12 12:17:15 +00:00
|
|
|
fn remove_action(this: &Arc<Self>) -> bool {
|
2025-01-07 12:25:11 +00:00
|
|
|
// SAFETY:
|
|
|
|
// - `self.inner.dev` is a valid `Device`,
|
|
|
|
// - the `action` and `data` pointers are the exact same ones as given to devm_add_action()
|
|
|
|
// previously,
|
|
|
|
// - `self` is always valid, even if the action has been released already.
|
rust: devres: fix race in Devres::drop()
In Devres::drop() we first remove the devres action and then drop the
wrapped device resource.
The design goal is to give the owner of a Devres object control over when
the device resource is dropped, but limit the overall scope to the
corresponding device being bound to a driver.
However, there's a race that was introduced with commit 8ff656643d30
("rust: devres: remove action in `Devres::drop`"), but also has been
(partially) present from the initial version on.
In Devres::drop(), the devres action is removed successfully and
subsequently the destructor of the wrapped device resource runs.
However, there is no guarantee that the destructor of the wrapped device
resource completes before the driver core is done unbinding the
corresponding device.
If in Devres::drop(), the devres action can't be removed, it means that
the devres callback has been executed already, or is still running
concurrently. In case of the latter, either Devres::drop() wins revoking
the Revocable or the devres callback wins revoking the Revocable. If
Devres::drop() wins, we (again) have no guarantee that the destructor of
the wrapped device resource completes before the driver core is done
unbinding the corresponding device.
CPU0 CPU1
------------------------------------------------------------------------
Devres::drop() { Devres::devres_callback() {
self.data.revoke() { this.data.revoke() {
is_available.swap() == true
is_available.swap == false
}
}
// [...]
// device fully unbound
drop_in_place() {
// release device resource
}
}
}
Depending on the specific device resource, this can potentially lead to
user-after-free bugs.
In order to fix this, implement the following logic.
In the devres callback, we're always good when we get to revoke the
device resource ourselves, i.e. Revocable::revoke() returns true.
If Revocable::revoke() returns false, it means that Devres::drop(),
concurrently, already drops the device resource and we have to wait for
Devres::drop() to signal that it finished dropping the device resource.
Note that if we hit the case where we need to wait for the completion of
Devres::drop() in the devres callback, it means that we're actually
racing with a concurrent Devres::drop() call, which already started
revoking the device resource for us. This is rather unlikely and means
that the concurrent Devres::drop() already started doing our work and we
just need to wait for it to complete it for us. Hence, there should not
be any additional overhead from that.
(Actually, for now it's even better if Devres::drop() does the work for
us, since it can bypass the synchronize_rcu() call implied by
Revocable::revoke(), but this goes away anyways once I get to implement
the split devres callback approach, which allows us to first flip the
atomics of all registered Devres objects of a certain device, execute a
single synchronize_rcu() and then drop all revocable objects.)
In Devres::drop() we try to revoke the device resource. If that is *not*
successful, it means that the devres callback already did and we're good.
Otherwise, we try to remove the devres action, which, if successful,
means that we're good, since the device resource has just been revoked
by us *before* we removed the devres action successfully.
If the devres action could not be removed, it means that the devres
callback must be running concurrently, hence we signal that the device
resource has been revoked by us, using the completion.
This makes it safe to drop a Devres object from any task and at any point
of time, which is one of the design goals.
Fixes: 76c01ded724b ("rust: add devres abstraction")
Reported-by: Alice Ryhl <aliceryhl@google.com>
Closes: https://lore.kernel.org/lkml/aD64YNuqbPPZHAa5@google.com/
Reviewed-by: Benno Lossin <lossin@kernel.org>
Link: https://lore.kernel.org/r/20250612121817.1621-4-dakr@kernel.org
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
2025-06-12 12:17:15 +00:00
|
|
|
let success = unsafe {
|
2025-01-07 12:25:11 +00:00
|
|
|
bindings::devm_remove_action_nowarn(
|
|
|
|
this.dev.as_raw(),
|
|
|
|
Some(this.callback),
|
|
|
|
this.as_ptr() as _,
|
|
|
|
)
|
rust: devres: fix race in Devres::drop()
In Devres::drop() we first remove the devres action and then drop the
wrapped device resource.
The design goal is to give the owner of a Devres object control over when
the device resource is dropped, but limit the overall scope to the
corresponding device being bound to a driver.
However, there's a race that was introduced with commit 8ff656643d30
("rust: devres: remove action in `Devres::drop`"), but also has been
(partially) present from the initial version on.
In Devres::drop(), the devres action is removed successfully and
subsequently the destructor of the wrapped device resource runs.
However, there is no guarantee that the destructor of the wrapped device
resource completes before the driver core is done unbinding the
corresponding device.
If in Devres::drop(), the devres action can't be removed, it means that
the devres callback has been executed already, or is still running
concurrently. In case of the latter, either Devres::drop() wins revoking
the Revocable or the devres callback wins revoking the Revocable. If
Devres::drop() wins, we (again) have no guarantee that the destructor of
the wrapped device resource completes before the driver core is done
unbinding the corresponding device.
CPU0 CPU1
------------------------------------------------------------------------
Devres::drop() { Devres::devres_callback() {
self.data.revoke() { this.data.revoke() {
is_available.swap() == true
is_available.swap == false
}
}
// [...]
// device fully unbound
drop_in_place() {
// release device resource
}
}
}
Depending on the specific device resource, this can potentially lead to
user-after-free bugs.
In order to fix this, implement the following logic.
In the devres callback, we're always good when we get to revoke the
device resource ourselves, i.e. Revocable::revoke() returns true.
If Revocable::revoke() returns false, it means that Devres::drop(),
concurrently, already drops the device resource and we have to wait for
Devres::drop() to signal that it finished dropping the device resource.
Note that if we hit the case where we need to wait for the completion of
Devres::drop() in the devres callback, it means that we're actually
racing with a concurrent Devres::drop() call, which already started
revoking the device resource for us. This is rather unlikely and means
that the concurrent Devres::drop() already started doing our work and we
just need to wait for it to complete it for us. Hence, there should not
be any additional overhead from that.
(Actually, for now it's even better if Devres::drop() does the work for
us, since it can bypass the synchronize_rcu() call implied by
Revocable::revoke(), but this goes away anyways once I get to implement
the split devres callback approach, which allows us to first flip the
atomics of all registered Devres objects of a certain device, execute a
single synchronize_rcu() and then drop all revocable objects.)
In Devres::drop() we try to revoke the device resource. If that is *not*
successful, it means that the devres callback already did and we're good.
Otherwise, we try to remove the devres action, which, if successful,
means that we're good, since the device resource has just been revoked
by us *before* we removed the devres action successfully.
If the devres action could not be removed, it means that the devres
callback must be running concurrently, hence we signal that the device
resource has been revoked by us, using the completion.
This makes it safe to drop a Devres object from any task and at any point
of time, which is one of the design goals.
Fixes: 76c01ded724b ("rust: add devres abstraction")
Reported-by: Alice Ryhl <aliceryhl@google.com>
Closes: https://lore.kernel.org/lkml/aD64YNuqbPPZHAa5@google.com/
Reviewed-by: Benno Lossin <lossin@kernel.org>
Link: https://lore.kernel.org/r/20250612121817.1621-4-dakr@kernel.org
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
2025-06-12 12:17:15 +00:00
|
|
|
} == 0;
|
2025-01-07 12:25:11 +00:00
|
|
|
|
rust: devres: fix race in Devres::drop()
In Devres::drop() we first remove the devres action and then drop the
wrapped device resource.
The design goal is to give the owner of a Devres object control over when
the device resource is dropped, but limit the overall scope to the
corresponding device being bound to a driver.
However, there's a race that was introduced with commit 8ff656643d30
("rust: devres: remove action in `Devres::drop`"), but also has been
(partially) present from the initial version on.
In Devres::drop(), the devres action is removed successfully and
subsequently the destructor of the wrapped device resource runs.
However, there is no guarantee that the destructor of the wrapped device
resource completes before the driver core is done unbinding the
corresponding device.
If in Devres::drop(), the devres action can't be removed, it means that
the devres callback has been executed already, or is still running
concurrently. In case of the latter, either Devres::drop() wins revoking
the Revocable or the devres callback wins revoking the Revocable. If
Devres::drop() wins, we (again) have no guarantee that the destructor of
the wrapped device resource completes before the driver core is done
unbinding the corresponding device.
CPU0 CPU1
------------------------------------------------------------------------
Devres::drop() { Devres::devres_callback() {
self.data.revoke() { this.data.revoke() {
is_available.swap() == true
is_available.swap == false
}
}
// [...]
// device fully unbound
drop_in_place() {
// release device resource
}
}
}
Depending on the specific device resource, this can potentially lead to
user-after-free bugs.
In order to fix this, implement the following logic.
In the devres callback, we're always good when we get to revoke the
device resource ourselves, i.e. Revocable::revoke() returns true.
If Revocable::revoke() returns false, it means that Devres::drop(),
concurrently, already drops the device resource and we have to wait for
Devres::drop() to signal that it finished dropping the device resource.
Note that if we hit the case where we need to wait for the completion of
Devres::drop() in the devres callback, it means that we're actually
racing with a concurrent Devres::drop() call, which already started
revoking the device resource for us. This is rather unlikely and means
that the concurrent Devres::drop() already started doing our work and we
just need to wait for it to complete it for us. Hence, there should not
be any additional overhead from that.
(Actually, for now it's even better if Devres::drop() does the work for
us, since it can bypass the synchronize_rcu() call implied by
Revocable::revoke(), but this goes away anyways once I get to implement
the split devres callback approach, which allows us to first flip the
atomics of all registered Devres objects of a certain device, execute a
single synchronize_rcu() and then drop all revocable objects.)
In Devres::drop() we try to revoke the device resource. If that is *not*
successful, it means that the devres callback already did and we're good.
Otherwise, we try to remove the devres action, which, if successful,
means that we're good, since the device resource has just been revoked
by us *before* we removed the devres action successfully.
If the devres action could not be removed, it means that the devres
callback must be running concurrently, hence we signal that the device
resource has been revoked by us, using the completion.
This makes it safe to drop a Devres object from any task and at any point
of time, which is one of the design goals.
Fixes: 76c01ded724b ("rust: add devres abstraction")
Reported-by: Alice Ryhl <aliceryhl@google.com>
Closes: https://lore.kernel.org/lkml/aD64YNuqbPPZHAa5@google.com/
Reviewed-by: Benno Lossin <lossin@kernel.org>
Link: https://lore.kernel.org/r/20250612121817.1621-4-dakr@kernel.org
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
2025-06-12 12:17:15 +00:00
|
|
|
if success {
|
2025-01-07 12:25:11 +00:00
|
|
|
// SAFETY: We leaked an `Arc` reference to devm_add_action() in `DevresInner::new`; if
|
|
|
|
// devm_remove_action_nowarn() was successful we can (and have to) claim back ownership
|
|
|
|
// of this reference.
|
|
|
|
let _ = unsafe { Arc::from_raw(this.as_ptr()) };
|
|
|
|
}
|
rust: devres: fix race in Devres::drop()
In Devres::drop() we first remove the devres action and then drop the
wrapped device resource.
The design goal is to give the owner of a Devres object control over when
the device resource is dropped, but limit the overall scope to the
corresponding device being bound to a driver.
However, there's a race that was introduced with commit 8ff656643d30
("rust: devres: remove action in `Devres::drop`"), but also has been
(partially) present from the initial version on.
In Devres::drop(), the devres action is removed successfully and
subsequently the destructor of the wrapped device resource runs.
However, there is no guarantee that the destructor of the wrapped device
resource completes before the driver core is done unbinding the
corresponding device.
If in Devres::drop(), the devres action can't be removed, it means that
the devres callback has been executed already, or is still running
concurrently. In case of the latter, either Devres::drop() wins revoking
the Revocable or the devres callback wins revoking the Revocable. If
Devres::drop() wins, we (again) have no guarantee that the destructor of
the wrapped device resource completes before the driver core is done
unbinding the corresponding device.
CPU0 CPU1
------------------------------------------------------------------------
Devres::drop() { Devres::devres_callback() {
self.data.revoke() { this.data.revoke() {
is_available.swap() == true
is_available.swap == false
}
}
// [...]
// device fully unbound
drop_in_place() {
// release device resource
}
}
}
Depending on the specific device resource, this can potentially lead to
user-after-free bugs.
In order to fix this, implement the following logic.
In the devres callback, we're always good when we get to revoke the
device resource ourselves, i.e. Revocable::revoke() returns true.
If Revocable::revoke() returns false, it means that Devres::drop(),
concurrently, already drops the device resource and we have to wait for
Devres::drop() to signal that it finished dropping the device resource.
Note that if we hit the case where we need to wait for the completion of
Devres::drop() in the devres callback, it means that we're actually
racing with a concurrent Devres::drop() call, which already started
revoking the device resource for us. This is rather unlikely and means
that the concurrent Devres::drop() already started doing our work and we
just need to wait for it to complete it for us. Hence, there should not
be any additional overhead from that.
(Actually, for now it's even better if Devres::drop() does the work for
us, since it can bypass the synchronize_rcu() call implied by
Revocable::revoke(), but this goes away anyways once I get to implement
the split devres callback approach, which allows us to first flip the
atomics of all registered Devres objects of a certain device, execute a
single synchronize_rcu() and then drop all revocable objects.)
In Devres::drop() we try to revoke the device resource. If that is *not*
successful, it means that the devres callback already did and we're good.
Otherwise, we try to remove the devres action, which, if successful,
means that we're good, since the device resource has just been revoked
by us *before* we removed the devres action successfully.
If the devres action could not be removed, it means that the devres
callback must be running concurrently, hence we signal that the device
resource has been revoked by us, using the completion.
This makes it safe to drop a Devres object from any task and at any point
of time, which is one of the design goals.
Fixes: 76c01ded724b ("rust: add devres abstraction")
Reported-by: Alice Ryhl <aliceryhl@google.com>
Closes: https://lore.kernel.org/lkml/aD64YNuqbPPZHAa5@google.com/
Reviewed-by: Benno Lossin <lossin@kernel.org>
Link: https://lore.kernel.org/r/20250612121817.1621-4-dakr@kernel.org
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
2025-06-12 12:17:15 +00:00
|
|
|
|
|
|
|
success
|
2025-01-07 12:25:11 +00:00
|
|
|
}
|
|
|
|
|
2024-12-19 17:04:10 +00:00
|
|
|
#[allow(clippy::missing_safety_doc)]
|
|
|
|
unsafe extern "C" fn devres_callback(ptr: *mut kernel::ffi::c_void) {
|
|
|
|
let ptr = ptr as *mut DevresInner<T>;
|
|
|
|
// Devres owned this memory; now that we received the callback, drop the `Arc` and hence the
|
|
|
|
// reference.
|
|
|
|
// SAFETY: Safe, since we leaked an `Arc` reference to devm_add_action() in
|
|
|
|
// `DevresInner::new`.
|
|
|
|
let inner = unsafe { Arc::from_raw(ptr) };
|
|
|
|
|
rust: devres: fix race in Devres::drop()
In Devres::drop() we first remove the devres action and then drop the
wrapped device resource.
The design goal is to give the owner of a Devres object control over when
the device resource is dropped, but limit the overall scope to the
corresponding device being bound to a driver.
However, there's a race that was introduced with commit 8ff656643d30
("rust: devres: remove action in `Devres::drop`"), but also has been
(partially) present from the initial version on.
In Devres::drop(), the devres action is removed successfully and
subsequently the destructor of the wrapped device resource runs.
However, there is no guarantee that the destructor of the wrapped device
resource completes before the driver core is done unbinding the
corresponding device.
If in Devres::drop(), the devres action can't be removed, it means that
the devres callback has been executed already, or is still running
concurrently. In case of the latter, either Devres::drop() wins revoking
the Revocable or the devres callback wins revoking the Revocable. If
Devres::drop() wins, we (again) have no guarantee that the destructor of
the wrapped device resource completes before the driver core is done
unbinding the corresponding device.
CPU0 CPU1
------------------------------------------------------------------------
Devres::drop() { Devres::devres_callback() {
self.data.revoke() { this.data.revoke() {
is_available.swap() == true
is_available.swap == false
}
}
// [...]
// device fully unbound
drop_in_place() {
// release device resource
}
}
}
Depending on the specific device resource, this can potentially lead to
user-after-free bugs.
In order to fix this, implement the following logic.
In the devres callback, we're always good when we get to revoke the
device resource ourselves, i.e. Revocable::revoke() returns true.
If Revocable::revoke() returns false, it means that Devres::drop(),
concurrently, already drops the device resource and we have to wait for
Devres::drop() to signal that it finished dropping the device resource.
Note that if we hit the case where we need to wait for the completion of
Devres::drop() in the devres callback, it means that we're actually
racing with a concurrent Devres::drop() call, which already started
revoking the device resource for us. This is rather unlikely and means
that the concurrent Devres::drop() already started doing our work and we
just need to wait for it to complete it for us. Hence, there should not
be any additional overhead from that.
(Actually, for now it's even better if Devres::drop() does the work for
us, since it can bypass the synchronize_rcu() call implied by
Revocable::revoke(), but this goes away anyways once I get to implement
the split devres callback approach, which allows us to first flip the
atomics of all registered Devres objects of a certain device, execute a
single synchronize_rcu() and then drop all revocable objects.)
In Devres::drop() we try to revoke the device resource. If that is *not*
successful, it means that the devres callback already did and we're good.
Otherwise, we try to remove the devres action, which, if successful,
means that we're good, since the device resource has just been revoked
by us *before* we removed the devres action successfully.
If the devres action could not be removed, it means that the devres
callback must be running concurrently, hence we signal that the device
resource has been revoked by us, using the completion.
This makes it safe to drop a Devres object from any task and at any point
of time, which is one of the design goals.
Fixes: 76c01ded724b ("rust: add devres abstraction")
Reported-by: Alice Ryhl <aliceryhl@google.com>
Closes: https://lore.kernel.org/lkml/aD64YNuqbPPZHAa5@google.com/
Reviewed-by: Benno Lossin <lossin@kernel.org>
Link: https://lore.kernel.org/r/20250612121817.1621-4-dakr@kernel.org
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
2025-06-12 12:17:15 +00:00
|
|
|
if !inner.data.revoke() {
|
|
|
|
// If `revoke()` returns false, it means that `Devres::drop` already started revoking
|
|
|
|
// `inner.data` for us. Hence we have to wait until `Devres::drop()` signals that it
|
|
|
|
// completed revoking `inner.data`.
|
|
|
|
inner.revoke.wait_for_completion();
|
|
|
|
}
|
2024-12-19 17:04:10 +00:00
|
|
|
}
|
|
|
|
}
|
|
|
|
|
2025-06-26 13:24:46 +00:00
|
|
|
impl<T: Send> Devres<T> {
|
2024-12-19 17:04:10 +00:00
|
|
|
/// Creates a new [`Devres`] instance of the given `data`. The `data` encapsulated within the
|
|
|
|
/// returned `Devres` instance' `data` will be revoked once the device is detached.
|
2025-04-13 17:37:03 +00:00
|
|
|
pub fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Self> {
|
2024-12-19 17:04:10 +00:00
|
|
|
let inner = DevresInner::new(dev, data, flags)?;
|
|
|
|
|
|
|
|
Ok(Devres(inner))
|
|
|
|
}
|
|
|
|
|
|
|
|
/// Same as [`Devres::new`], but does not return a `Devres` instance. Instead the given `data`
|
|
|
|
/// is owned by devres and will be revoked / dropped, once the device is detached.
|
2025-04-13 17:37:03 +00:00
|
|
|
pub fn new_foreign_owned(dev: &Device<Bound>, data: T, flags: Flags) -> Result {
|
2024-12-19 17:04:10 +00:00
|
|
|
let _ = DevresInner::new(dev, data, flags)?;
|
|
|
|
|
|
|
|
Ok(())
|
|
|
|
}
|
rust: devres: implement Devres::access()
Implement a direct accessor for the data stored within the Devres for
cases where we can prove that we own a reference to a Device<Bound>
(i.e. a bound device) of the same device that was used to create the
corresponding Devres container.
Usually, when accessing the data stored within a Devres container, it is
not clear whether the data has been revoked already due to the device
being unbound and, hence, we have to try whether the access is possible
and subsequently keep holding the RCU read lock for the duration of the
access.
However, when we can prove that we hold a reference to Device<Bound>
matching the device the Devres container has been created with, we can
guarantee that the device is not unbound for the duration of the
lifetime of the Device<Bound> reference and, hence, it is not possible
for the data within the Devres container to be revoked.
Therefore, in this case, we can bypass the atomic check and the RCU read
lock, which is a great optimization and simplification for drivers.
Reviewed-by: Christian Schrefl <chrisi.schrefl@gmail.com>
Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
Acked-by: Boqun Feng <boqun.feng@gmail.com>
Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>
Link: https://lore.kernel.org/r/20250428140137.468709-3-dakr@kernel.org
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
2025-04-28 14:00:28 +00:00
|
|
|
|
|
|
|
/// Obtain `&'a T`, bypassing the [`Revocable`].
|
|
|
|
///
|
|
|
|
/// This method allows to directly obtain a `&'a T`, bypassing the [`Revocable`], by presenting
|
|
|
|
/// a `&'a Device<Bound>` of the same [`Device`] this [`Devres`] instance has been created with.
|
|
|
|
///
|
|
|
|
/// # Errors
|
|
|
|
///
|
|
|
|
/// An error is returned if `dev` does not match the same [`Device`] this [`Devres`] instance
|
|
|
|
/// has been created with.
|
|
|
|
///
|
|
|
|
/// # Example
|
|
|
|
///
|
|
|
|
/// ```no_run
|
2025-05-11 18:25:33 +00:00
|
|
|
/// # #![cfg(CONFIG_PCI)]
|
rust: devres: implement Devres::access()
Implement a direct accessor for the data stored within the Devres for
cases where we can prove that we own a reference to a Device<Bound>
(i.e. a bound device) of the same device that was used to create the
corresponding Devres container.
Usually, when accessing the data stored within a Devres container, it is
not clear whether the data has been revoked already due to the device
being unbound and, hence, we have to try whether the access is possible
and subsequently keep holding the RCU read lock for the duration of the
access.
However, when we can prove that we hold a reference to Device<Bound>
matching the device the Devres container has been created with, we can
guarantee that the device is not unbound for the duration of the
lifetime of the Device<Bound> reference and, hence, it is not possible
for the data within the Devres container to be revoked.
Therefore, in this case, we can bypass the atomic check and the RCU read
lock, which is a great optimization and simplification for drivers.
Reviewed-by: Christian Schrefl <chrisi.schrefl@gmail.com>
Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
Acked-by: Boqun Feng <boqun.feng@gmail.com>
Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>
Link: https://lore.kernel.org/r/20250428140137.468709-3-dakr@kernel.org
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
2025-04-28 14:00:28 +00:00
|
|
|
/// # use kernel::{device::Core, devres::Devres, pci};
|
|
|
|
///
|
|
|
|
/// fn from_core(dev: &pci::Device<Core>, devres: Devres<pci::Bar<0x4>>) -> Result {
|
|
|
|
/// let bar = devres.access(dev.as_ref())?;
|
|
|
|
///
|
|
|
|
/// let _ = bar.read32(0x0);
|
|
|
|
///
|
|
|
|
/// // might_sleep()
|
|
|
|
///
|
|
|
|
/// bar.write32(0x42, 0x0);
|
|
|
|
///
|
|
|
|
/// Ok(())
|
|
|
|
/// }
|
|
|
|
/// ```
|
|
|
|
pub fn access<'a>(&'a self, dev: &'a Device<Bound>) -> Result<&'a T> {
|
|
|
|
if self.0.dev.as_raw() != dev.as_raw() {
|
|
|
|
return Err(EINVAL);
|
|
|
|
}
|
|
|
|
|
|
|
|
// SAFETY: `dev` being the same device as the device this `Devres` has been created for
|
|
|
|
// proves that `self.0.data` hasn't been revoked and is guaranteed to not be revoked as
|
|
|
|
// long as `dev` lives; `dev` lives at least as long as `self`.
|
2025-06-11 17:48:25 +00:00
|
|
|
Ok(unsafe { self.0.data.access() })
|
rust: devres: implement Devres::access()
Implement a direct accessor for the data stored within the Devres for
cases where we can prove that we own a reference to a Device<Bound>
(i.e. a bound device) of the same device that was used to create the
corresponding Devres container.
Usually, when accessing the data stored within a Devres container, it is
not clear whether the data has been revoked already due to the device
being unbound and, hence, we have to try whether the access is possible
and subsequently keep holding the RCU read lock for the duration of the
access.
However, when we can prove that we hold a reference to Device<Bound>
matching the device the Devres container has been created with, we can
guarantee that the device is not unbound for the duration of the
lifetime of the Device<Bound> reference and, hence, it is not possible
for the data within the Devres container to be revoked.
Therefore, in this case, we can bypass the atomic check and the RCU read
lock, which is a great optimization and simplification for drivers.
Reviewed-by: Christian Schrefl <chrisi.schrefl@gmail.com>
Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
Acked-by: Boqun Feng <boqun.feng@gmail.com>
Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>
Link: https://lore.kernel.org/r/20250428140137.468709-3-dakr@kernel.org
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
2025-04-28 14:00:28 +00:00
|
|
|
}
|
2024-12-19 17:04:10 +00:00
|
|
|
|
2025-06-11 17:48:25 +00:00
|
|
|
/// [`Devres`] accessor for [`Revocable::try_access`].
|
|
|
|
pub fn try_access(&self) -> Option<RevocableGuard<'_, T>> {
|
|
|
|
self.0.data.try_access()
|
|
|
|
}
|
|
|
|
|
|
|
|
/// [`Devres`] accessor for [`Revocable::try_access_with`].
|
|
|
|
pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
|
|
|
|
self.0.data.try_access_with(f)
|
|
|
|
}
|
2024-12-19 17:04:10 +00:00
|
|
|
|
2025-06-11 17:48:25 +00:00
|
|
|
/// [`Devres`] accessor for [`Revocable::try_access_with_guard`].
|
|
|
|
pub fn try_access_with_guard<'a>(&'a self, guard: &'a rcu::Guard) -> Option<&'a T> {
|
|
|
|
self.0.data.try_access_with_guard(guard)
|
2024-12-19 17:04:10 +00:00
|
|
|
}
|
|
|
|
}
|
|
|
|
|
2025-06-26 13:24:46 +00:00
|
|
|
impl<T: Send> Drop for Devres<T> {
|
2024-12-19 17:04:10 +00:00
|
|
|
fn drop(&mut self) {
|
rust: devres: fix race in Devres::drop()
In Devres::drop() we first remove the devres action and then drop the
wrapped device resource.
The design goal is to give the owner of a Devres object control over when
the device resource is dropped, but limit the overall scope to the
corresponding device being bound to a driver.
However, there's a race that was introduced with commit 8ff656643d30
("rust: devres: remove action in `Devres::drop`"), but also has been
(partially) present from the initial version on.
In Devres::drop(), the devres action is removed successfully and
subsequently the destructor of the wrapped device resource runs.
However, there is no guarantee that the destructor of the wrapped device
resource completes before the driver core is done unbinding the
corresponding device.
If in Devres::drop(), the devres action can't be removed, it means that
the devres callback has been executed already, or is still running
concurrently. In case of the latter, either Devres::drop() wins revoking
the Revocable or the devres callback wins revoking the Revocable. If
Devres::drop() wins, we (again) have no guarantee that the destructor of
the wrapped device resource completes before the driver core is done
unbinding the corresponding device.
CPU0 CPU1
------------------------------------------------------------------------
Devres::drop() { Devres::devres_callback() {
self.data.revoke() { this.data.revoke() {
is_available.swap() == true
is_available.swap == false
}
}
// [...]
// device fully unbound
drop_in_place() {
// release device resource
}
}
}
Depending on the specific device resource, this can potentially lead to
user-after-free bugs.
In order to fix this, implement the following logic.
In the devres callback, we're always good when we get to revoke the
device resource ourselves, i.e. Revocable::revoke() returns true.
If Revocable::revoke() returns false, it means that Devres::drop(),
concurrently, already drops the device resource and we have to wait for
Devres::drop() to signal that it finished dropping the device resource.
Note that if we hit the case where we need to wait for the completion of
Devres::drop() in the devres callback, it means that we're actually
racing with a concurrent Devres::drop() call, which already started
revoking the device resource for us. This is rather unlikely and means
that the concurrent Devres::drop() already started doing our work and we
just need to wait for it to complete it for us. Hence, there should not
be any additional overhead from that.
(Actually, for now it's even better if Devres::drop() does the work for
us, since it can bypass the synchronize_rcu() call implied by
Revocable::revoke(), but this goes away anyways once I get to implement
the split devres callback approach, which allows us to first flip the
atomics of all registered Devres objects of a certain device, execute a
single synchronize_rcu() and then drop all revocable objects.)
In Devres::drop() we try to revoke the device resource. If that is *not*
successful, it means that the devres callback already did and we're good.
Otherwise, we try to remove the devres action, which, if successful,
means that we're good, since the device resource has just been revoked
by us *before* we removed the devres action successfully.
If the devres action could not be removed, it means that the devres
callback must be running concurrently, hence we signal that the device
resource has been revoked by us, using the completion.
This makes it safe to drop a Devres object from any task and at any point
of time, which is one of the design goals.
Fixes: 76c01ded724b ("rust: add devres abstraction")
Reported-by: Alice Ryhl <aliceryhl@google.com>
Closes: https://lore.kernel.org/lkml/aD64YNuqbPPZHAa5@google.com/
Reviewed-by: Benno Lossin <lossin@kernel.org>
Link: https://lore.kernel.org/r/20250612121817.1621-4-dakr@kernel.org
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
2025-06-12 12:17:15 +00:00
|
|
|
// 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.
|
2025-06-11 17:48:25 +00:00
|
|
|
if unsafe { self.0.data.revoke_nosync() } {
|
rust: devres: fix race in Devres::drop()
In Devres::drop() we first remove the devres action and then drop the
wrapped device resource.
The design goal is to give the owner of a Devres object control over when
the device resource is dropped, but limit the overall scope to the
corresponding device being bound to a driver.
However, there's a race that was introduced with commit 8ff656643d30
("rust: devres: remove action in `Devres::drop`"), but also has been
(partially) present from the initial version on.
In Devres::drop(), the devres action is removed successfully and
subsequently the destructor of the wrapped device resource runs.
However, there is no guarantee that the destructor of the wrapped device
resource completes before the driver core is done unbinding the
corresponding device.
If in Devres::drop(), the devres action can't be removed, it means that
the devres callback has been executed already, or is still running
concurrently. In case of the latter, either Devres::drop() wins revoking
the Revocable or the devres callback wins revoking the Revocable. If
Devres::drop() wins, we (again) have no guarantee that the destructor of
the wrapped device resource completes before the driver core is done
unbinding the corresponding device.
CPU0 CPU1
------------------------------------------------------------------------
Devres::drop() { Devres::devres_callback() {
self.data.revoke() { this.data.revoke() {
is_available.swap() == true
is_available.swap == false
}
}
// [...]
// device fully unbound
drop_in_place() {
// release device resource
}
}
}
Depending on the specific device resource, this can potentially lead to
user-after-free bugs.
In order to fix this, implement the following logic.
In the devres callback, we're always good when we get to revoke the
device resource ourselves, i.e. Revocable::revoke() returns true.
If Revocable::revoke() returns false, it means that Devres::drop(),
concurrently, already drops the device resource and we have to wait for
Devres::drop() to signal that it finished dropping the device resource.
Note that if we hit the case where we need to wait for the completion of
Devres::drop() in the devres callback, it means that we're actually
racing with a concurrent Devres::drop() call, which already started
revoking the device resource for us. This is rather unlikely and means
that the concurrent Devres::drop() already started doing our work and we
just need to wait for it to complete it for us. Hence, there should not
be any additional overhead from that.
(Actually, for now it's even better if Devres::drop() does the work for
us, since it can bypass the synchronize_rcu() call implied by
Revocable::revoke(), but this goes away anyways once I get to implement
the split devres callback approach, which allows us to first flip the
atomics of all registered Devres objects of a certain device, execute a
single synchronize_rcu() and then drop all revocable objects.)
In Devres::drop() we try to revoke the device resource. If that is *not*
successful, it means that the devres callback already did and we're good.
Otherwise, we try to remove the devres action, which, if successful,
means that we're good, since the device resource has just been revoked
by us *before* we removed the devres action successfully.
If the devres action could not be removed, it means that the devres
callback must be running concurrently, hence we signal that the device
resource has been revoked by us, using the completion.
This makes it safe to drop a Devres object from any task and at any point
of time, which is one of the design goals.
Fixes: 76c01ded724b ("rust: add devres abstraction")
Reported-by: Alice Ryhl <aliceryhl@google.com>
Closes: https://lore.kernel.org/lkml/aD64YNuqbPPZHAa5@google.com/
Reviewed-by: Benno Lossin <lossin@kernel.org>
Link: https://lore.kernel.org/r/20250612121817.1621-4-dakr@kernel.org
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
2025-06-12 12:17:15 +00:00
|
|
|
// We revoked `self.0.data` before the devres action did, hence try to remove it.
|
|
|
|
if !DevresInner::remove_action(&self.0) {
|
|
|
|
// We could not remove the devres action, which means that it now runs concurrently,
|
|
|
|
// hence signal that `self.0.data` has been revoked successfully.
|
|
|
|
self.0.revoke.complete_all();
|
|
|
|
}
|
|
|
|
}
|
2024-12-19 17:04:10 +00:00
|
|
|
}
|
|
|
|
}
|