rust: drm: Higher-Ranked Lifetime private data
Subsystems (7)
Danilo Krummrich
maintainerv3 cover rust: drm: Higher-Ranked Lifetime private data
DRM ioctls run in process context without any guarantee that the parent bus device is still bound. This series solves the problem by introducing RegistrationGuard -- a guard representing a drm_dev_enter/exit SRCU critical section that proves the parent bus device is bound for the lifetime of the guard.
As initial plumbing for this, the DRM DeviceContext typestates are
reworked: Uninit is renamed to Normal, defaults are adjusted, AlwaysRefCounted is restricted to Normal, and a Deref chain from Device<T, Registered> to Device<T, Normal> is established. This gives Device<T, Registered> the semantic that the device is currently registered and the parent bus device is bound, which makes the RegistrationGuard and ioctl dispatch much cleaner.
On top of that, add RegistrationData as a ForLt associated type on drm::Driver, allowing drivers to store data whose lifetime is tied to the parent bus device binding scope. The data is allocated in Registration::new(), lifetime-erased to 'static for storage, and made accessible through Device<T, Registered>::registration_data_with(). The closure's HRTB ties the lifetime to the closure scope; internally the 'static pointer is cast back to the closure-scoped lifetime. The reference is valid for the duration of the drm_dev_enter/exit critical section held by RegistrationGuard.
Also update the ioctl dispatch macro to wrap every handler in a RegistrationGuard, returning ENODEV if the device has been unplugged, and pass the registration data to handlers.
This series is based on [1]; a branch with all patches can be found in [2].
[1] https://lore.kernel.org/driver-core/[email protected]/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=drm-lifetime
Changes in v3:
- Rename UnbindGuard to RegistrationGuard
- RegistrationGuard no longer dereferences to &Device<Bound>; it dereferences to &drm::Device<T, Registered> instead
- Drop Registration::new() and rename Registration::new_with_lt() to Registration::new()
- Rework DeviceContext typestates: rename Uninit to Normal, restrict AlwaysRefCounted to Normal, establish Deref chain from Registered to Normal
- Add AsRef<ParentDevice<Bound>> on Device<T, Registered> for parent device access
- Move registration_data_with() from RegistrationGuard to drm::Device<T, Registered>
- Ioctl handlers no longer receive &Device<Bound>, only registration data and drm::Device<T, Registered>
- Use Device<Registered>::as_ref() to access parent device in nova-drm
Changes in v2:
- Replace unsafe direct registration data access in ioctl dispatch with safe UnbindGuard::registration_data_with() closure
- Eliminate unbind_guard() free function; use type-inference anchor to enable direct dev.unbind_guard() method call in the ioctl macro
- UnbindGuard::registration_data_with() provides both parent device and registration data to the closure
- Add nova-drm conversion patch demonstrating lifetime-aware registration data with &'bound auxiliary::Device<Bound>
- Various safety comment and documentation improvements
Danilo Krummrich (13):
rust: drm: rename Uninit DeviceContext to Normal
rust: drm: Add Driver::ParentDevice associated type
rust: drm: change default DeviceContext to Normal
rust: drm: restrict AlwaysRefCounted to Normal Device context
rust: drm: restrict AlwaysRefCounted to Normal GEM Object context
rust: drm: split Deref for Device context typestates
rust: drm: return ParentDevice from Device AsRef
rust: drm: add AsRef<ParentDevice<Bound>> for Device<Registered>
rust: drm: Add RegistrationGuard for drm_dev_enter/exit critical
sections
rust: drm: Add RegistrationData to drm::Driver
rust: drm: Wrap ioctl dispatch in RegistrationGuard
rust: drm: Pass registration data to ioctl handlers
drm: nova: Use drm::Device<Registered> to access the parent bus device
drivers/gpu/drm/nova/driver.rs | 41 ++--- drivers/gpu/drm/nova/file.rs | 18 ++- drivers/gpu/drm/nova/gem.rs | 18 +-- drivers/gpu/drm/tyr/driver.rs | 28 ++-- drivers/gpu/drm/tyr/file.rs | 1 + drivers/gpu/drm/tyr/gem.rs | 11 +- rust/kernel/drm/device.rs | 264 +++++++++++++++++++++++---------- rust/kernel/drm/driver.rs | 129 +++++++++++----- rust/kernel/drm/gem/mod.rs | 98 ++++++------ rust/kernel/drm/gem/shmem.rs | 79 +++++----- rust/kernel/drm/ioctl.rs | 27 +++- rust/kernel/drm/mod.rs | 3 +- 12 files changed, 449 insertions(+), 268 deletions(-)
base-commit: 9ece8b7075e983bc01223a4aa1eb1c99285f83ad
Signature · 1 line
2.54.0
Danilo Krummrich
maintainerPATCH v3 1/13 rust: drm: rename Uninit DeviceContext to Normal
Rename the Uninit DeviceContext to Normal to better reflect its purpose as the general-purpose, reference-counted device context. The Uninit name was a leftover from when DRM device private data initialization was planned to split across UnregisteredDevice::new() and Registration::new(); with the introduction of RegistrationData, this distinction is no longer needed.
This also simplifies the DeviceContext documentation, trimming the multi-stage initialization description that no longer applies. Subsequent patches will refine the semantics of the Registered context accordingly.
No functional change.
--- rust/kernel/drm/device.rs | 92 ++++++++++++--------------------------- rust/kernel/drm/mod.rs | 2 +- 2 files changed, 28 insertions(+), 66 deletions(-)
rust/kernel/drm/device.rs +27 -65
@@ -74,36 +74,22 @@ macro_rules! drm_legacy_fields {/// A trait implemented by all possible contexts a [`Device`] can be used in. /// -/// Setting up a new [`Device`] is a multi-stage process. Each step of the process that a user -/// interacts with in Rust has a respective [`DeviceContext`] typestate. For example, -/// `Device<T, Registered>` would be a [`Device`] that reached the [`Registered`] [`DeviceContext`]. +/// A [`Device`] can be in one of two contexts: /// -/// Each stage of this process is described below: -/// -/// ```text -/// 1 2 3 -/// +--------------+ +------------------+ +-----------------------+ -/// |Device created| → |Device initialized| → |Registered w/ userspace| -/// +--------------+ +------------------+ +-----------------------+ -/// (Uninit) (Registered) -/// ``` -/// -/// 1. The [`Device`] is in the [`Uninit`] context and is not guaranteed to be initialized or -/// registered with userspace. Only a limited subset of DRM core functionality is available. -/// 2. The [`Device`] is guaranteed to be fully initialized, but is not guaranteed to be registered -/// with userspace. All DRM core functionality which doesn't interact with userspace is -/// available. We currently don't have a context for representing this. -/// 3. The [`Device`] is guaranteed to be fully initialized, and is guaranteed to have been -/// registered with userspace at some point - thus putting it in the [`Registered`] context. -/// -/// An important caveat of [`DeviceContext`] which must be kept in mind: when used as a typestate -/// for a reference type, it can only guarantee that a [`Device`] reached a particular stage in the -/// initialization process _at the time the reference was taken_. No guarantee is made in regards to -/// what stage of the process the [`Device`] is currently in. This means for instance that a -/// `&Device<T, Uninit>` may actually be registered with userspace, it just wasn't known to be -/// registered at the time the reference was taken. +/// - [`Normal`]: The general-purpose, reference-counted context. A [`Device`] in this context may +/// or may not be registered with userspace. +/// - [`Registered`]: The device has been registered with userspace at some point. pub trait DeviceContext :Sealed +Send +Sync {} +/// The general-purpose, reference-counted [`DeviceContext`]. +/// +/// A [`Device`] in this context may or may not be registered with userspace. This context is used +/// for reference-counted device handles and during device setup via [`UnregisteredDevice`]. +pub struct Normal ; + +impl Sealed for Normal {} +impl DeviceContext for Normal {} +/// The [`DeviceContext`] of a [`Device`] that was registered with userspace at some point. /// /// This represents a [`Device`] which is guaranteed to have been registered with userspace at
@@ -121,20 +107,6 @@ pub trait DeviceContext: Sealed + Send + Sync {}impl Sealed for Registered {}impl DeviceContext for Registered {} -/// The [`DeviceContext`] of a [`Device`] that may be unregistered and partly uninitialized. -/// -/// A [`Device`] in this context is only guaranteed to be partly initialized, and may or may not -/// be registered with userspace. Thus operations which depend on the [`Device`] being fully -/// initialized, or which depend on the [`Device`] being registered with userspace are not -/// available through this [`DeviceContext`]. -/// -/// A [`Device`] in this context can be used to create a -/// [`Registration`](drm::driver::Registration). -pub struct Uninit ; - -impl Sealed for Uninit {} -impl DeviceContext for Uninit {} -/// A [`Device`] which is known at compile-time to be unregistered with userspace. /// /// This type allows performing operations which are only safe to do before userspace registration,
@@ -147,10 +119,10 @@ impl DeviceContext for Uninit {}/// /// The device in `self.0` is guaranteed to be a newly created [`Device`] that has not yet been /// registered with userspace until this type is dropped. -pub struct UnregisteredDevice <T : drm::Driver >(ARef <Device <T ,Uninit >>,NotThreadSafe ); +pub struct UnregisteredDevice <T : drm::Driver >(ARef <Device <T ,Normal >>,NotThreadSafe );impl <T : drm::Driver >Deref for UnregisteredDevice <T > { -type Target =Device <T ,Uninit >; +type Target =Device <T ,Normal >;fn deref (&self) -> &Self ::Target { &self.0
@@ -178,15 +150,13 @@ const fn compute_features() -> u32 { master_drop: None, debugfs_init: None, -// Ignore the Uninit DeviceContext below. It is only provided because it is required by the -// compiler, and it is not actually used by these functions. - gem_create_object: T::Object::<Uninit>::ALLOC_OPS.gem_create_object, - prime_handle_to_fd: T::Object::<Uninit>::ALLOC_OPS.prime_handle_to_fd, - prime_fd_to_handle: T::Object::<Uninit>::ALLOC_OPS.prime_fd_to_handle, - gem_prime_import: T::Object::<Uninit>::ALLOC_OPS.gem_prime_import, - gem_prime_import_sg_table: T::Object::<Uninit>::ALLOC_OPS.gem_prime_import_sg_table, - dumb_create: T::Object::<Uninit>::ALLOC_OPS.dumb_create, - dumb_map_offset: T::Object::<Uninit>::ALLOC_OPS.dumb_map_offset, + gem_create_object: T::Object::<Normal>::ALLOC_OPS.gem_create_object, + prime_handle_to_fd: T::Object::<Normal>::ALLOC_OPS.prime_handle_to_fd, + prime_fd_to_handle: T::Object::<Normal>::ALLOC_OPS.prime_fd_to_handle, + gem_prime_import: T::Object::<Normal>::ALLOC_OPS.gem_prime_import, + gem_prime_import_sg_table: T::Object::<Normal>::ALLOC_OPS.gem_prime_import_sg_table, + dumb_create: T::Object::<Normal>::ALLOC_OPS.dumb_create, + dumb_map_offset: T::Object::<Normal>::ALLOC_OPS.dumb_map_offset, show_fdinfo: None, fbdev_probe: None,
@@ -211,7 +181,7 @@ const fn compute_features() -> u32 {pub fn new (dev: &device::Device , data:impl PinInit <T ::Data ,Error >) ->Result <Self > {// `__drm_dev_alloc` uses `kmalloc()` to allocate memory, hence ensure a `kmalloc()` // compatible `Layout`. -let layout =Kmalloc ::aligned_layout (Layout ::new ::<Device <T ,Uninit >>()); +let layout =Kmalloc ::aligned_layout (Layout ::new ::<Device <T ,Normal >>());// Use a temporary vtable without a `release` callback until `data` is initialized, so // init failure can release the DRM device without dropping uninitialized fields.
@@ -223,12 +193,12 @@ pub fn new(dev: &device::Device, data: impl PinInit<T::Data, Error>) -> Result<S// SAFETY: // - `alloc_vtable` reference remains valid until no longer used, // - `dev` is valid by its type invarants, -let raw_drm: *mut Device <T ,Uninit > =unsafe { +let raw_drm: *mut Device <T ,Normal > =unsafe { bindings::__drm_dev_alloc ( dev.as_raw (), &alloc_vtable, layout.size (), - mem::offset_of!(Device <T ,Uninit >, dev), + mem::offset_of!(Device <T ,Normal >, dev), ) } .cast ();
@@ -264,16 +234,8 @@ pub fn new(dev: &device::Device, data: impl PinInit<T::Data, Error>) -> Result<S/// A typed DRM device with a specific [`drm::Driver`] implementation and [`DeviceContext`]. /// -/// Since DRM devices can be used before being fully initialized and registered with userspace, `C` -/// represents the furthest [`DeviceContext`] we can guarantee that this [`Device`] has reached. -/// -/// Keep in mind: this means that an unregistered device can still have the registration state -/// [`Registered`] as long as it was registered with userspace once in the past, and that the -/// behavior of such a device is still well-defined. Additionally, a device with the registration -/// state [`Uninit`] simply does not have a guaranteed registration state at compile time, and could -/// be either registered or unregistered. Since there is no way to guarantee a long-lived reference -/// to an unregistered device would remain unregistered, we do not provide a [`DeviceContext`] for -/// this. +/// A device in the [`Registered`] context is guaranteed to have been registered with userspace +/// at some point. The [`Normal`] context is the general-purpose, reference-counted context. /// /// # Invariants ///
rust/kernel/drm/mod.rs +1 -1
@@ -11,8 +11,8 @@pub use self::device::Device ;pub use self::device::DeviceContext ; +pub use self::device::Normal ;pub use self::device::Registered ; -pub use self::device::Uninit ;pub use self::device::UnregisteredDevice ;pub use self::driver::Driver ;pub use self::driver::DriverInfo ;
Signature · 1 line
2.54.0
Danilo Krummrich
maintainerPATCH v3 2/13 rust: drm: Add Driver::ParentDevice associated type
In reply to Danilo KrummrichAdd a ParentDevice associated type to the Driver trait, allowing each DRM driver to declare its parent bus device type (e.g. auxiliary::Device, platform::Device).
Change UnregisteredDevice::new() to take &T::ParentDevice<Bound>, ensuring at the type level that the DRM device's parent matches the declared bus device type.
--- drivers/gpu/drm/nova/driver.rs | 8 ++++++-- drivers/gpu/drm/tyr/driver.rs | 6 ++++-- rust/kernel/drm/device.rs | 7 +++++-- rust/kernel/drm/driver.rs | 3 +++ 4 files changed, 18 insertions(+), 6 deletions(-)
drivers/gpu/drm/nova/driver.rs +6 -2
@@ -2,7 +2,10 @@use kernel::{ auxiliary, - device::Core , + device::{ +Core , +DeviceContext ,// + }, drm::{ self, gem,
@@ -62,7 +65,7 @@ fn probe<'bound>( ) -> impl PinInit<Self::Data<'bound>, Error> + 'bound {let data =try_pin_init! (NovaData { adev: adev.into () }); -let drm = drm::UnregisteredDevice ::<Self >::new (adev.as_ref (), data)?; +let drm = drm::UnregisteredDevice ::<Self >::new (adev, data)?;let drm = drm::Registration ::new_foreign_owned (drm, adev.as_ref (),0 )?; Ok(Nova { drm: drm.into() })
@@ -74,6 +77,7 @@ impl drm::Driver for NovaDriver {type Data =NovaData ;type File =File ;type Object <Ctx : drm::DeviceContext > = gem::Object <NovaObject ,Ctx >; +type ParentDevice <Ctx :DeviceContext > = auxiliary::Device <Ctx >;const INFO : drm::DriverInfo =INFO ;
drivers/gpu/drm/tyr/driver.rs +4 -2
@@ -7,7 +7,8 @@ }, device::{ Core, - Device, // + Device, + DeviceContext, // }, dma::{ Device as DmaDevice,
@@ -148,7 +149,7 @@ fn probe<'bound>( gpu_info, }); -let tdev = drm::UnregisteredDevice ::<TyrDrmDriver >::new (pdev.as_ref (), data)?; +let tdev = drm::UnregisteredDevice ::<TyrDrmDriver >::new (pdev, data)?;let tdev = drm::driver::Registration ::new_foreign_owned (tdev, pdev.as_ref (),0 )?;let driver =TyrPlatformDriverData {
@@ -182,6 +183,7 @@ impl drm::Driver for TyrDrmDriver {type Data =TyrDrmDeviceData ;type File =TyrDrmFileData ;type Object <R : drm::DeviceContext > = drm::gem::shmem::Object <BoData ,R >; +type ParentDevice <Ctx :DeviceContext > = platform::Device <Ctx >;const INFO : drm::DriverInfo =INFO ;const FEAT_RENDER :bool =true ;
rust/kernel/drm/device.rs +5 -2
@@ -178,7 +178,10 @@ const fn compute_features() -> u32 {/// Create a new `UnregisteredDevice` for a `drm::Driver`. /// /// This can be used to create a [`Registration`](kernel::drm::Registration). -pub fn new (dev: &device::Device , data:impl PinInit <T ::Data ,Error >) ->Result <Self > { +pub fn new ( + dev: &T ::ParentDevice <device::Bound >, + data:impl PinInit <T ::Data ,Error >, + ) ->Result <Self > {// `__drm_dev_alloc` uses `kmalloc()` to allocate memory, hence ensure a `kmalloc()` // compatible `Layout`. let layout =Kmalloc ::aligned_layout (Layout ::new ::<Device <T ,Normal >>());
@@ -195,7 +198,7 @@ pub fn new(dev: &device::Device, data: impl PinInit<T::Data, Error>) -> Result<S// - `dev` is valid by its type invarants, let raw_drm: *mut Device <T ,Normal > =unsafe { bindings::__drm_dev_alloc ( - dev.as_raw (), + dev.as_ref ().as_raw (), &alloc_vtable, layout.size (), mem::offset_of!(Device<T, Normal>, dev),
rust/kernel/drm/driver.rs +3 -0
@@ -116,6 +116,9 @@ pub trait Driver {/// The type used to represent a DRM File (client) type File : drm::file::DriverFile ; +/// The bus device type of the parent device that the DRM device is associated with. +type ParentDevice <Ctx : device::DeviceContext >: device::AsBusDevice <Ctx >; +/// Driver metadata const INFO :DriverInfo ;
Signature · 1 line
2.54.0
Danilo Krummrich
maintainerPATCH v3 3/13 rust: drm: change default DeviceContext to Normal
In reply to Danilo KrummrichChange the default DeviceContext from Registered to Normal for drm::Device, gem::Object, gem::shmem::Object and gem::shmem::ObjectConfig.
Normal is the general-purpose, reference-counted context suitable for most uses; Registered represents a device that was registered with userspace and will become a non-owning context obtained through a RegistrationGuard.
Update the create_handle/lookup_handle bounds from Object<Registered> to Object<Normal> to match the new default context of GEM objects, and update the driver device type aliases (NovaDevice, TyrDrmDevice) to default to Normal.
--- drivers/gpu/drm/nova/driver.rs | 2 +- drivers/gpu/drm/tyr/driver.rs | 2 +- rust/kernel/drm/device.rs | 2 +- rust/kernel/drm/gem/mod.rs | 7 ++++--- rust/kernel/drm/gem/shmem.rs | 6 +++--- 5 files changed, 10 insertions(+), 9 deletions(-)
drivers/gpu/drm/nova/driver.rs +1 -1
@@ -26,7 +26,7 @@ pub(crate) struct Nova { }/// Convienence type alias for the DRM device type for this driver -pub (crate )type NovaDevice <Ctx = drm::Registered > = drm::Device <NovaDriver ,Ctx >; +pub (crate )type NovaDevice <Ctx = drm::Normal > = drm::Device <NovaDriver ,Ctx >;#[pin_data] pub (crate )struct NovaData {
drivers/gpu/drm/tyr/driver.rs +1 -1
@@ -47,7 +47,7 @@pub (crate )struct TyrDrmDriver ;/// Convenience type alias for the DRM device type for this driver. -pub (crate )type TyrDrmDevice <Ctx = drm::Registered > = drm::Device <TyrDrmDriver ,Ctx >; +pub (crate )type TyrDrmDevice <Ctx = drm::Normal > = drm::Device <TyrDrmDriver ,Ctx >;pub (crate )struct TyrPlatformDriver ;
rust/kernel/drm/device.rs +1 -1
@@ -246,7 +246,7 @@ pub fn new(/// * The data layout of `Self` remains the same across all implementations of `C`. /// * Any invariants for `C` also apply. #[repr( C )] -pub struct Device <T : drm::Driver ,C :DeviceContext =Registered > { +pub struct Device <T : drm::Driver ,C :DeviceContext =Normal > { dev:Opaque <bindings::drm_device >, data:T ::Data , _ctx:PhantomData <C >,
rust/kernel/drm/gem/mod.rs +4 -3
@@ -10,6 +10,7 @@ self, device::{ DeviceContext, + Normal, Registered, // }, driver::{
@@ -183,7 +184,7 @@ fn size(&self) -> usize {fn create_handle <D ,F >(&self, file: &drm::File <F >) ->Result <u32 >where Self :AllocImpl <Driver =D >, -D : drm::Driver <Object <Registered > =Self ,File =F >, +D : drm::Driver <Object <Normal > =Self ,File =F >,F : drm::file::DriverFile <Driver =D >, {let mut handle:u32 =0 ;
@@ -198,7 +199,7 @@ fn create_handle<D, F>(&self, file: &drm::File<F>) -> Result<u32>fn lookup_handle <D ,F >(file: &drm::File <F >, handle:u32 ) ->Result <ARef <Self >>where Self :AllocImpl <Driver =D >, -D : drm::Driver <Object <Registered > =Self ,File =F >, +D : drm::Driver <Object <Normal > =Self ,File =F >,F : drm::file::DriverFile <Driver =D >, {// SAFETY: The arguments are all valid per the type invariants.
@@ -254,7 +255,7 @@ impl<T: IntoGEMObject> BaseObjectPrivate for T {}/// * Any type invariants of `Ctx` apply to the parent DRM device for this GEM object. #[repr( C )] #[pin_data] -pub struct Object <T :DriverObject +Send +Sync ,Ctx :DeviceContext =Registered > { +pub struct Object <T :DriverObject +Send +Sync ,Ctx :DeviceContext =Normal > { obj:Opaque <bindings::drm_gem_object >,#[pin] data:T ,
rust/kernel/drm/gem/shmem.rs +3 -3
@@ -17,7 +17,7 @@ private::Sealed, Device, DeviceContext, - Registered, // + Normal, // }, error::to_result, prelude::*,
@@ -43,7 +43,7 @@/// This is used with [`Object::new()`] to control various properties that can only be set when /// initially creating a shmem-backed GEM object. #[derive( Default )] -pub struct ObjectConfig <'a,T :DriverObject ,C :DeviceContext =Registered > { +pub struct ObjectConfig <'a,T :DriverObject ,C :DeviceContext =Normal > {/// Whether to set the write-combine map flag. pub map_wc:bool ,
@@ -62,7 +62,7 @@ pub struct ObjectConfig<'a, T: DriverObject, C: DeviceContext = Registered> {/// - Any type invariants of `C` apply to the parent DRM device for this GEM object. #[repr( C )] #[pin_data] -pub struct Object <T :DriverObject ,C :DeviceContext =Registered > { +pub struct Object <T :DriverObject ,C :DeviceContext =Normal > {#[pin] obj:Opaque <bindings::drm_gem_shmem_object >,/// Parent object that owns this object's DMA reservation object.
Signature · 1 line
2.54.0
Danilo Krummrich
maintainerPATCH v3 4/13 rust: drm: restrict AlwaysRefCounted to Normal Device context
In reply to Danilo KrummrichRestrict the AlwaysRefCounted implementation for drm::Device to the Normal context. Registered devices represent a non-owning view of a device within a RegistrationGuard scope and must not be independently reference-counted.
--- rust/kernel/drm/device.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
rust/kernel/drm/device.rs +5 -4
@@ -85,6 +85,9 @@ pub trait DeviceContext: Sealed + Send + Sync {}/// /// A [`Device`] in this context may or may not be registered with userspace. This context is used /// for reference-counted device handles and during device setup via [`UnregisteredDevice`]. +/// +/// [`AlwaysRefCounted`] is only implemented for `Device<T, Normal>`, making this the required +/// context for [`ARef`]-based device handles. pub struct Normal ;impl Sealed for Normal {}
@@ -327,7 +330,7 @@ fn deref(&self) -> &Self::Target {// SAFETY: DRM device objects are always reference counted and the get/put functions // satisfy the requirements. -unsafe impl <T : drm::Driver ,C :DeviceContext >AlwaysRefCounted for Device <T ,C > { +unsafe impl <T : drm::Driver >AlwaysRefCounted for Device <T > {fn inc_ref (&self) {// SAFETY: The existence of a shared reference guarantees that the refcount is non-zero. unsafe { bindings::drm_dev_get (self.as_raw ()) };
@@ -357,12 +360,10 @@ unsafe impl<T: drm::Driver, C: DeviceContext> Send for Device<T, C> {}// by the synchronization in `struct drm_device`. unsafe impl <T : drm::Driver ,C :DeviceContext >Sync for Device <T ,C > {} -impl <T ,C ,const ID :u64 >WorkItem <ID >for Device <T ,C > +impl <T : drm::Driver ,const ID :u64 >WorkItem <ID >for Device <T >where -T : drm::Driver ,T ::Data :WorkItem <ID ,Pointer =ARef <Self >>,T ::Data :HasWork <Self ,ID >, -C :DeviceContext , {type Pointer =ARef <Self >;
Signature · 1 line
2.54.0
Danilo Krummrich
maintainerPATCH v3 5/13 rust: drm: restrict AlwaysRefCounted to Normal GEM Object context
In reply to Danilo KrummrichRestrict AlwaysRefCounted for gem::Object and gem::shmem::Object to the Normal context, since only Normal objects should be independently reference-counted.
To avoid cascading through IntoGEMObject (which had AlwaysRefCounted as a supertrait), remove AlwaysRefCounted from IntoGEMObject's supertraits and instead add it as an explicit bound on lookup_handle(), which is the only BaseObject method that returns an ARef.
Since Object::new() and shmem::Object::new() return ARef<Self>, move them to Normal-only impl blocks. Similarly, simplify ObjectConfig and shmem's parent_resv_obj field to the Normal context.
Remove the DeviceContext generic from DriverObject::new() and Driver::Object, since GEM objects can only be constructed in the Normal context. Simplify DriverAllocImpl accordingly.
--- drivers/gpu/drm/nova/driver.rs | 2 +- drivers/gpu/drm/nova/gem.rs | 18 +++---- drivers/gpu/drm/tyr/driver.rs | 2 +- drivers/gpu/drm/tyr/gem.rs | 11 +--- rust/kernel/drm/device.rs | 14 ++--- rust/kernel/drm/driver.rs | 2 +- rust/kernel/drm/gem/mod.rs | 97 ++++++++++++++++------------------ rust/kernel/drm/gem/shmem.rs | 75 +++++++++++++------------- 8 files changed, 103 insertions(+), 118 deletions(-)
drivers/gpu/drm/nova/driver.rs +1 -1
@@ -76,7 +76,7 @@ fn probe<'bound>(impl drm::Driver for NovaDriver {type Data =NovaData ;type File =File ; -type Object <Ctx : drm::DeviceContext > = gem::Object <NovaObject ,Ctx >; +type Object = gem::Object <NovaObject >;type ParentDevice <Ctx :DeviceContext > = auxiliary::Device <Ctx >;const INFO : drm::DriverInfo =INFO ;
drivers/gpu/drm/nova/gem.rs +7 -11
@@ -2,7 +2,10 @@use kernel::{ drm, - drm::{gem, gem::BaseObject ,DeviceContext }, + drm::{ + gem, + gem::BaseObject ,// + }, page, prelude::*, sync::aref::ARef ,
@@ -21,27 +24,20 @@ impl gem::DriverObject for NovaObject {type Driver =NovaDriver ;type Args = (); -fn new <Ctx :DeviceContext >( - _dev: &NovaDevice <Ctx >, - _size:usize , - _args:Self ::Args , - ) ->impl PinInit <Self ,Error > { +fn new (_dev: &NovaDevice , _size:usize , _args:Self ::Args ) ->impl PinInit <Self ,Error > {try_pin_init! (NovaObject {}) } }impl NovaObject {/// Create a new DRM GEM object. -pub (crate )fn new <Ctx :DeviceContext >( - dev: &NovaDevice <Ctx >, - size:usize , - ) ->Result <ARef <gem::Object <Self ,Ctx >>> { +pub (crate )fn new (dev: &NovaDevice , size:usize ) ->Result <ARef <gem::Object <Self >>> {if size ==0 {return Err (EINVAL ); }let aligned_size = page::page_align (size).ok_or (EINVAL )?; - gem::Object ::<Self ,Ctx >::new (dev, aligned_size, ()) + gem::Object ::<Self >::new (dev, aligned_size, ()) }/// Look up a GEM object handle for a `File` and return an `ObjectRef` for it.
drivers/gpu/drm/tyr/driver.rs +1 -1
@@ -182,7 +182,7 @@ fn drop(self: Pin<&mut Self>) {}impl drm::Driver for TyrDrmDriver {type Data =TyrDrmDeviceData ;type File =TyrDrmFileData ; -type Object <R : drm::DeviceContext > = drm::gem::shmem::Object <BoData ,R >; +type Object = drm::gem::shmem::Object <BoData >;type ParentDevice <Ctx :DeviceContext > = platform::Device <Ctx >;const INFO : drm::DriverInfo =INFO ;
drivers/gpu/drm/tyr/gem.rs +2 -9
@@ -5,10 +5,7 @@//! DRM's GEM subsystem with shmem backing. use kernel::{ - drm::{ - gem, -DeviceContext ,// - }, + drm::gem, prelude::*,// };
@@ -33,11 +30,7 @@ impl gem::DriverObject for BoData {type Driver =TyrDrmDriver ;type Args =BoCreateArgs ; -fn new <Ctx :DeviceContext >( - _dev: &TyrDrmDevice <Ctx >, - _size:usize , - args:BoCreateArgs , - ) ->impl PinInit <Self ,Error > { +fn new (_dev: &TyrDrmDevice , _size:usize , args:BoCreateArgs ) ->impl PinInit <Self ,Error > {try_pin_init! (Self { flags: args.flags }) } }
rust/kernel/drm/device.rs +7 -7
@@ -153,13 +153,13 @@ const fn compute_features() -> u32 { master_drop: None, debugfs_init: None, - gem_create_object: T::Object::<Normal>::ALLOC_OPS.gem_create_object, - prime_handle_to_fd: T::Object::<Normal>::ALLOC_OPS.prime_handle_to_fd, - prime_fd_to_handle: T::Object::<Normal>::ALLOC_OPS.prime_fd_to_handle, - gem_prime_import: T::Object::<Normal>::ALLOC_OPS.gem_prime_import, - gem_prime_import_sg_table: T::Object::<Normal>::ALLOC_OPS.gem_prime_import_sg_table, - dumb_create: T::Object::<Normal>::ALLOC_OPS.dumb_create, - dumb_map_offset: T::Object::<Normal>::ALLOC_OPS.dumb_map_offset, + gem_create_object: T::Object::ALLOC_OPS.gem_create_object, + prime_handle_to_fd: T::Object::ALLOC_OPS.prime_handle_to_fd, + prime_fd_to_handle: T::Object::ALLOC_OPS.prime_fd_to_handle, + gem_prime_import: T::Object::ALLOC_OPS.gem_prime_import, + gem_prime_import_sg_table: T::Object::ALLOC_OPS.gem_prime_import_sg_table, + dumb_create: T::Object::ALLOC_OPS.dumb_create, + dumb_map_offset: T::Object::ALLOC_OPS.dumb_map_offset, show_fdinfo: None, fbdev_probe: None,
rust/kernel/drm/driver.rs +1 -1
@@ -111,7 +111,7 @@ pub trait Driver {type Data :Sync +Send ;/// The type used to manage memory for this driver. -type Object <Ctx : drm::DeviceContext >:AllocImpl ; +type Object :AllocImpl ;/// The type used to represent a DRM File (client) type File : drm::file::DriverFile ;
rust/kernel/drm/gem/mod.rs +46 -51
150 lines not on this page · raw message
rust/kernel/drm/gem/shmem.rs +38 -37
123 lines not on this page · raw message
Signature · 1 line
2.54.0
Danilo Krummrich
maintainerPATCH v3 6/13 rust: drm: split Deref for Device context typestates
In reply to Danilo KrummrichSplit the Deref implementation for drm::Device by context:
- Device<T> (Normal) dereferences to T::Data.
- Device<T, Registered> dereferences to Device<T> (Normal).
--- rust/kernel/drm/device.rs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)
rust/kernel/drm/device.rs +15 -1
@@ -79,6 +79,9 @@ macro_rules! drm_legacy_fields {/// - [`Normal`]: The general-purpose, reference-counted context. A [`Device`] in this context may /// or may not be registered with userspace. /// - [`Registered`]: The device has been registered with userspace at some point. +/// +/// `Device<T, Registered>` dereferences to `Device<T>` ([`Normal`]), so any method available on a +/// [`Normal`] device is also available on a [`Registered`] one. pub trait DeviceContext :Sealed +Send +Sync {}/// The general-purpose, reference-counted [`DeviceContext`].
@@ -320,7 +323,7 @@ pub(crate) unsafe fn assume_ctx<NewCtx: DeviceContext>(&self) -> &Device<T, NewC } } -impl <T : drm::Driver ,C :DeviceContext >Deref for Device <T ,C > { +impl <T : drm::Driver >Deref for Device <T > {type Target =T ::Data ;fn deref (&self) -> &Self ::Target {
@@ -328,6 +331,17 @@ fn deref(&self) -> &Self::Target { } } +impl <T : drm::Driver >Deref for Device <T ,Registered > { +type Target =Device <T >; + +#[inline] +fn deref (&self) -> &Self ::Target { +// SAFETY: The caller holds a `Device<T, Registered>`, which guarantees all invariants +// of the weaker `Normal` context. +unsafe { self.assume_ctx () } + } +} +// SAFETY: DRM device objects are always reference counted and the get/put functions // satisfy the requirements. unsafe impl <T : drm::Driver >AlwaysRefCounted for Device <T > {
Signature · 1 line
2.54.0
Danilo Krummrich
maintainerPATCH v3 7/13 rust: drm: return ParentDevice from Device AsRef
In reply to Danilo KrummrichChange AsRef for drm::Device to return &T::ParentDevice<device::Normal> instead of &device::Device, and restrict it to the Normal context. Device<T, Registered> still gets this through Deref coercion.
This provides access to the typed parent bus device rather than the raw base device.
--- rust/kernel/drm/device.rs | 10 +++++++--- rust/kernel/drm/driver.rs | 2 +- 2 files changed, 8 insertions(+), 4 deletions(-)
rust/kernel/drm/device.rs +7 -3
@@ -359,11 +359,15 @@ unsafe fn dec_ref(obj: NonNull<Self>) { } } -impl <T : drm::Driver ,C :DeviceContext >AsRef <device::Device >for Device <T ,C > { -fn as_ref (&self) -> &device::Device { +impl <T : drm::Driver >AsRef <T ::ParentDevice <device::Normal >>for Device <T > { +fn as_ref (&self) -> &T ::ParentDevice <device::Normal > {// SAFETY: `bindings::drm_device::dev` is valid as long as the DRM device itself is valid, // which is guaranteed by the type invariant. -unsafe { device::Device ::from_raw ((*self.as_raw ()).dev) } +let dev =unsafe { device::Device ::from_raw ((*self.as_raw ()).dev) }; + +// SAFETY: By the type invariant of `Device`, the parent device is embedded in +// `T::ParentDevice`. +unsafe { device::AsBusDevice ::from_device (dev) } } }
rust/kernel/drm/driver.rs +1 -1
@@ -170,7 +170,7 @@ pub fn new_foreign_owned<'a>( where T: 'static, { - if drm.as_ref().as_raw() != dev.as_raw() { + if drm.as_ref().as_ref().as_raw() != dev.as_raw() {return Err (EINVAL ); }
Signature · 1 line
2.54.0
Danilo Krummrich
maintainerPATCH v3 8/13 rust: drm: add AsRef<ParentDevice<Bound>> for Device<Registered>
In reply to Danilo KrummrichImplement AsRef<T::ParentDevice<Bound>> for Device<T, Registered>, providing access to the bound parent bus device for registered DRM devices.
Since a Device<T, Registered> guarantees that the parent bus device is bound, the conversion to T::ParentDevice<Bound> is safe.
--- rust/kernel/drm/device.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+)
rust/kernel/drm/device.rs +13 -0
@@ -371,6 +371,19 @@ fn as_ref(&self) -> &T::ParentDevice<device::Normal> { } } +impl <T : drm::Driver >AsRef <T ::ParentDevice <device::Bound >>for Device <T ,Registered > { +fn as_ref (&self) -> &T ::ParentDevice <device::Bound > { +let dev = (**self).as_ref ().as_ref (); + +// SAFETY: A `Device<T, Registered>` guarantees that the parent device is bound. +let dev =unsafe { dev.as_bound () }; + +// SAFETY: By the type invariant of `Device`, the parent device is embedded in +// `T::ParentDevice`. +unsafe { device::AsBusDevice ::from_device (dev) } + } +} +// SAFETY: A `drm::Device` can be released from any thread. unsafe impl <T : drm::Driver ,C :DeviceContext >Send for Device <T ,C > {}
Signature · 1 line
2.54.0
Danilo Krummrich
maintainerPATCH v3 9/13 rust: drm: Add RegistrationGuard for drm_dev_enter/exit critical sections
In reply to Danilo KrummrichDRM ioctls do not guarantee that the parent bus device is still bound. However, since DRM device registration is managed through Devres, using drm_dev_unplug() on unregistration ensures that between drm_dev_enter() and drm_dev_exit() the parent device must be bound.
Add RegistrationGuard, a guard object representing a drm_dev_enter/exit SRCU critical section that dereferences to &Device<T, Registered>. The guard is obtained from Device<T> (Normal) and proves at runtime that the device is still registered.
Switch Registration::drop from drm_dev_unregister() to drm_dev_unplug() to provide the SRCU barrier that RegistrationGuard's safety argument relies on.
--- rust/kernel/drm/device.rs | 76 +++++++++++++++++++++++++++++++++------ rust/kernel/drm/driver.rs | 10 ++++-- rust/kernel/drm/mod.rs | 1 + 3 files changed, 74 insertions(+), 13 deletions(-)
rust/kernel/drm/device.rs +65 -11
@@ -78,7 +78,8 @@ macro_rules! drm_legacy_fields {/// /// - [`Normal`]: The general-purpose, reference-counted context. A [`Device`] in this context may /// or may not be registered with userspace. -/// - [`Registered`]: The device has been registered with userspace at some point. +/// - [`Registered`]: The device is currently registered with userspace and the parent bus device +/// is bound. /// /// `Device<T, Registered>` dereferences to `Device<T>` ([`Normal`]), so any method available on a /// [`Normal`] device is also available on a [`Registered`] one.
@@ -96,18 +97,15 @@ pub trait DeviceContext: Sealed + Send + Sync {}impl Sealed for Normal {}impl DeviceContext for Normal {} -/// The [`DeviceContext`] of a [`Device`] that was registered with userspace at some point. +/// The [`DeviceContext`] of a [`Device`] that is currently registered with userspace. /// -/// This represents a [`Device`] which is guaranteed to have been registered with userspace at -/// some point in time. Such a DRM device is guaranteed to have been fully-initialized. -/// -/// Note: A device in this context is not guaranteed to remain registered with userspace for its -/// entire lifetime, as this is impossible to guarantee at compile-time. +/// A [`Device`] in this context is guaranteed to be registered and its parent bus device is +/// guaranteed to be bound. This is enforced at runtime by [`RegistrationGuard`], which holds a +/// `drm_dev_enter()` / `drm_dev_exit()` SRCU critical section. /// /// # Invariants /// -/// A [`Device`] in this [`DeviceContext`] is guaranteed to have been registered with userspace -/// at some point in time. +/// The parent bus device is bound for the duration of any reference to a `Device<T, Registered>`. pub struct Registered ;impl Sealed for Registered {}
@@ -243,8 +241,8 @@ pub fn new(/// A typed DRM device with a specific [`drm::Driver`] implementation and [`DeviceContext`]. /// -/// A device in the [`Registered`] context is guaranteed to have been registered with userspace -/// at some point. The [`Normal`] context is the general-purpose, reference-counted context. +/// A device in the [`Registered`] context is currently registered with userspace and its parent +/// bus device is bound. The [`Normal`] context is the general-purpose, reference-counted context. /// /// # Invariants ///
@@ -323,6 +321,62 @@ pub(crate) unsafe fn assume_ctx<NewCtx: DeviceContext>(&self) -> &Device<T, NewC } } +impl <T : drm::Driver >Device <T > { +/// Guard against the parent bus device being unbound. +/// +/// Returns a [`RegistrationGuard`] if the device has not been unplugged, [`None`] otherwise. +/// +/// While [`RegistrationGuard`] is held the parent device is guaranteed to be bound. +#[must_use] +pub fn registration_guard (&self) ->Option <RegistrationGuard <'_,T >> { +let mut idx:i32 =0 ; +// SAFETY: `self.as_raw()` is a valid pointer to a `struct drm_device`. +if unsafe { bindings::drm_dev_enter (self.as_raw (), &mut idx) } { +Some (RegistrationGuard { dev: self, idx }) + }else { +None + } + } +} + +/// A guard proving the DRM device is registered and the parent bus device is bound. +/// +/// The guard dereferences to [`Device<T, Registered>`], providing access to the DRM device with +/// the guarantee that the parent bus device is bound for the entire duration of the critical +/// section. +/// +/// Internally this is backed by a `drm_dev_enter()` / `drm_dev_exit()` SRCU critical section. +/// +/// # Invariants +/// +/// - `idx` is the SRCU read lock index returned by a successful `drm_dev_enter()` call. +/// - The parent bus device of `dev` is bound for the lifetime of this guard. +#[must_use] +pub struct RegistrationGuard <'a,T : drm::Driver > { + dev: &'aDevice <T >, + idx:i32 , +} + +impl <T : drm::Driver >Deref for RegistrationGuard <'_,T > { +type Target =Device <T ,Registered >; + +#[inline] +fn deref (&self) -> &Self ::Target { +// SAFETY: A successful `drm_dev_enter()` guarantees that the device is registered and +// the parent bus device is bound. +unsafe { self.dev.assume_ctx () } + } +} + +impl <T : drm::Driver >Drop for RegistrationGuard <'_,T > { +#[inline] +fn drop (&mut self) { +// SAFETY: `self.idx` was returned by a successful `drm_dev_enter()` call, as guaranteed +// by the type invariants of `RegistrationGuard`. +unsafe { bindings::drm_dev_exit (self.idx) }; + } +} +impl <T : drm::Driver >Deref for Device <T > {type Target =T ::Data ;
rust/kernel/drm/driver.rs +8 -2
@@ -199,8 +199,14 @@ unsafe impl<T: Driver> Send for Registration<T> {}impl <T :Driver >Drop for Registration <T > {fn drop (&mut self) { +// Use `drm_dev_unplug` rather than `drm_dev_unregister` to ensure that existing +// `drm_dev_enter()` critical sections complete before unregistration proceeds. This +// is required for the safety of `RegistrationGuard`, which relies on the SRCU barrier in +// `drm_dev_unplug()` to guarantee that the parent device is still bound within the +// critical section. +// // SAFETY: Safe by the invariant of `ARef<drm::Device<T>>`. The existence of this -// `Registration` also guarantees the this `drm::Device` is actually registered. -unsafe { bindings::drm_dev_unregister (self.0 .as_raw ()) }; +// `Registration` also guarantees that this `drm::Device` is actually registered. +unsafe { bindings::drm_dev_unplug (self.0 .as_raw ()) }; } }
rust/kernel/drm/mod.rs +1 -0
@@ -13,6 +13,7 @@pub use self::device::DeviceContext ;pub use self::device::Normal ;pub use self::device::Registered ; +pub use self::device::RegistrationGuard ;pub use self::device::UnregisteredDevice ;pub use self::driver::Driver ;pub use self::driver::DriverInfo ;
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v3 09/13] rust: drm: Add RegistrationGuard for drm_dev_enter/exit critical sections
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Device::registration_guard() grants a `Registered` context to unregistered devices, allowing…
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Device::registration_guard() grants a Registered context to unregistered devices, allowing access to unbound hardware.
- [High] RegistrationGuard is implicitly Send, causing lockdep splats if dropped on a different thread.
- [Medium] Safe Rust can cause an SRCU deadlock by dropping Registration while holding RegistrationGuard. --
commit 3b6c73976ebe0a7066664f9ddc910e85fe1582da
Author: Danilo Krummrich <[email protected]>
rust: drm: Add RegistrationGuard for drm_dev_enter/exit critical sections
This patch adds RegistrationGuard, a guard object representing a drm_dev_enter/exit SRCU critical section. It also changes Registration::drop to use drm_dev_unplug() instead of drm_dev_unregister() to provide the SRCU barrier required for safety.
Quoted 4 lines
rust/kernel/drm/device.rs +0
-0
[ ... ]
Quoted 5 lines
rust/kernel/drm/device.rs +1
-0@@ -323,6 +321,62 @@ pub(crate) unsafe fn assume_ctx<NewCtx: DeviceContext>(&self) -> &Device<T, NewC } } +impl <T : drm::Driver >Device <T > {
rust/kernel/drm/device.rs pub(crate) unsafe fn assume_ctx<NewCtx: DeviceContext>(&self) -> &Device<T, NewC · line 334
Quoted 10 lines
rust/kernel/drm/device.rs +10
-0+/// Guard against the parent bus device being unbound. +/// +/// Returns a [`RegistrationGuard`] if the device has not been unplugged, [`None`] otherwise. +/// +/// While [`RegistrationGuard`] is held the parent device is guaranteed to be bound. +#[must_use] +pub fn registration_guard (&self) ->Option <RegistrationGuard <'_,T >> { +let mut idx:i32 =0 ; +// SAFETY: `self.as_raw()` is a valid pointer to a `struct drm_device`. +if unsafe { bindings::drm_dev_enter (self.as_raw (), &mut idx) } {
[Severity: High]
Does this logic correctly prove the device is registered?
Because dev->unplugged defaults to false upon allocation, drm_dev_enter() will return true for an UnregisteredDevice that has not yet called drm_dev_register().
If device probe fails, the kernel unwinds devres-managed resources but never creates a Registration or calls drm_dev_unplug(). If an ARef<Device> is kept alive by a background thread, registration_guard() will still succeed and return a RegistrationGuard.
This guard dereferences to Device<T, Registered>, which falsely exposes the parent bus device as bound. Could this lead to a Use-After-Free or invalid memory access if the parent device's devres-managed hardware resources are accessed after probe failure?
Should the guard explicitly verify the registration state (e.g., dev->registered) after acquiring the SRCU lock?
Quoted 14 lines
rust/kernel/drm/device.rs +14
-0+Some (RegistrationGuard { dev: self, idx }) + }else { +None + } + } +} + +/// A guard proving the DRM device is registered and the parent bus device is bound. +/// +/// The guard dereferences to [`Device<T, Registered>`], providing access to the DRM device with +/// the guarantee that the parent bus device is bound for the entire duration of the critical +/// section. +/// +/// Internally this is backed by a `drm_dev_enter()` / `drm_dev_exit()` SRCU critical section.
rust/kernel/drm/device.rs pub(crate) unsafe fn assume_ctx<NewCtx: DeviceContext>(&self) -> &Device<T, NewC · line 358
Quoted 10 lines
rust/kernel/drm/device.rs +10
-0+/// +/// # Invariants +/// +/// - `idx` is the SRCU read lock index returned by a successful `drm_dev_enter()` call. +/// - The parent bus device of `dev` is bound for the lifetime of this guard. +#[must_use] +pub struct RegistrationGuard <'a,T : drm::Driver > { + dev: &'aDevice <T >, + idx:i32 , +}
[Severity: High]
Is it intentional that RegistrationGuard is implicitly Send?
Because its inner fields are Send, this allows safe Rust to acquire the guard on one thread and drop it on another.
While SRCU technically allows cross-thread unlocks, the kernel's lockdep implementation tracks SRCU read locks per-task. If a guard is dropped on a different thread, will lockdep emit a bad unlock balance detected splat?
Should RegistrationGuard be explicitly marked !Send (e.g., by adding a PhantomData<*mut ()> field)?
rust/kernel/drm/device.rs pub(crate) unsafe fn assume_ctx<NewCtx: DeviceContext>(&self) -> &Device<T, NewC · line 367
Quoted 9 lines
rust/kernel/drm/device.rs +9
-0+ +impl <T : drm::Driver >Deref for RegistrationGuard <'_,T > { +type Target =Device <T ,Registered >; + +#[inline] +fn deref (&self) -> &Self ::Target { +// SAFETY: A successful `drm_dev_enter()` guarantees that the device is registered and +// the parent bus device is bound. +unsafe { self.dev.assume_ctx () }
[Severity: High]
Does a successful drm_dev_enter() alone guarantee the Registered state?
As mentioned above, if drm_dev_enter() succeeds on an UnregisteredDevice after probe failure, this unsafe block will grant the Registered state incorrectly.
Quoted 2 lines
rust/kernel/drm/device.rs +2
-0+ } +}
[ ... ]
Quoted 4 lines
rust/kernel/drm/driver.rs +0
-0
[ ... ]
Quoted 5 lines
rust/kernel/drm/driver.rs +1
-0@@ -199,8 +199,14 @@ unsafe impl<T: Driver> Send for Registration<T> {}impl <T :Driver >Drop for Registration <T > {fn drop (&mut self) { +// Use `drm_dev_unplug` rather than `drm_dev_unregister` to ensure that existing
rust/kernel/drm/driver.rs unsafe impl<T: Driver> Send for Registration<T> {} · line 210
Quoted 10 lines
rust/kernel/drm/driver.rs +7
-2+// `drm_dev_enter()` critical sections complete before unregistration proceeds. This +// is required for the safety of `RegistrationGuard`, which relies on the SRCU barrier in +// `drm_dev_unplug()` to guarantee that the parent device is still bound within the +// critical section. +// // SAFETY: Safe by the invariant of `ARef<drm::Device<T>>`. The existence of this -// `Registration` also guarantees the this `drm::Device` is actually registered. -unsafe { bindings::drm_dev_unregister (self.0 .as_raw ()) }; +// `Registration` also guarantees that this `drm::Device` is actually registered. +unsafe { bindings::drm_dev_unplug (self.0 .as_raw ()) };
[Severity: Medium]
Can dropping Registration while holding RegistrationGuard on the same thread cause an SRCU deadlock?
drm_dev_unplug() synchronously waits for all SRCU readers by calling synchronize_srcu(&drm_unplug_srcu). Since safe Rust can acquire an independent reference to the device, obtain a RegistrationGuard (which holds the SRCU read lock), and then drop Registration on the same thread, synchronize_srcu() could be called from within its own read-side critical section.
Would this deadlock the thread and trigger an SRCU lockdep warning?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9
Danilo Krummrich
maintainerPATCH v3 10/13 rust: drm: Add RegistrationData to drm::Driver
In reply to Danilo KrummrichAdd a RegistrationData associated type to drm::Driver. This is a ForLt type whose lifetime is tied to the parent bus device binding scope. Registration<'a, T> takes ownership of the data via Pin<KBox<_>>, storing it with its real lifetime. The pointer is written to drm::Device before drm_dev_register() to ensure it is already in place when ioctls arrive.
Device<T, Registered>::registration_data_with() provides access with the lifetime shortened from 'static via a pointer cast. Since Registration::drop() calls drm_dev_unplug(), which performs an SRCU barrier waiting for all drm_dev_enter() critical sections to complete, the data is guaranteed to remain valid for the duration of any RegistrationGuard.
--- drivers/gpu/drm/nova/driver.rs | 20 ++++-- drivers/gpu/drm/tyr/driver.rs | 18 +++-- rust/kernel/drm/device.rs | 49 ++++++++++++++ rust/kernel/drm/driver.rs | 116 ++++++++++++++++++++++----------- 4 files changed, 153 insertions(+), 50 deletions(-)
drivers/gpu/drm/nova/driver.rs +14 -6
@@ -12,7 +12,8 @@ ioctl, // }, prelude::*, - sync::aref::ARef, // + sync::aref::ARef, + types::ForLt, // };use crate ::file::File ;
@@ -20,9 +21,10 @@pub (crate )struct NovaDriver ; -pub (crate )struct Nova { +pub (crate )struct Nova <'bound> {#[expect(unused)] drm:ARef <drm::Device <NovaDriver >>, + _reg: drm::Registration <'bound,NovaDriver >, }/// Convienence type alias for the DRM device type for this driver
@@ -56,7 +58,7 @@ pub(crate) struct NovaData {impl auxiliary::Driver for NovaDriver {type IdInfo = (); -type Data <'bound> =Nova ; +type Data <'bound> =Nova <'bound>;const ID_TABLE : auxiliary::IdTable <Self ::IdInfo > = &AUX_TABLE ; fn probe<'bound>(
@@ -66,15 +68,21 @@ fn probe<'bound>(let data =try_pin_init! (NovaData { adev: adev.into () });let drm = drm::UnregisteredDevice ::<Self >::new (adev, data)?; -let drm = drm::Registration ::new_foreign_owned (drm, adev.as_ref (),0 )?; - -Ok (Nova { drm: drm.into () }) +// SAFETY: `reg` is stored in `Nova` and dropped when the driver is unbound; it is +// never forgotten. +let reg =unsafe { drm::Registration ::new (adev.as_ref (), drm, (),0 )? }; + +Ok (Nova { + drm: reg.device ().into (), + _reg: reg, + }) } }#[vtable] impl drm::Driver for NovaDriver {type Data =NovaData ; +type RegistrationData =ForLt! (());type File =File ;type Object = gem::Object <NovaObject >;type ParentDevice <Ctx :DeviceContext > = auxiliary::Device <Ctx >;
drivers/gpu/drm/tyr/driver.rs +12 -6
@@ -31,7 +31,8 @@ aref::ARef, Mutex, // }, - time, // + time, + types::ForLt, // };use crate ::{
@@ -52,8 +53,9 @@pub (crate )struct TyrPlatformDriver ;#[pin_data( PinnedDrop )] -pub (crate )struct TyrPlatformDriverData { +pub (crate )struct TyrPlatformDriverData <'bound> { _device:ARef <TyrDrmDevice >, + _reg: drm::Registration <'bound,TyrDrmDriver >, }#[pin_data]
@@ -98,7 +100,7 @@ fn issue_soft_reset(dev: &Device, iomem: &IoMem<'_>) -> Result {impl platform::Driver for TyrPlatformDriver {type IdInfo = (); -type Data <'bound> =TyrPlatformDriverData ; +type Data <'bound> =TyrPlatformDriverData <'bound>;const OF_ID_TABLE :Option <of::IdTable <Self ::IdInfo >> =Some (&OF_TABLE ); fn probe<'bound>(
@@ -150,10 +152,13 @@ fn probe<'bound>( });let tdev = drm::UnregisteredDevice ::<TyrDrmDriver >::new (pdev, data)?; -let tdev = drm::driver::Registration ::new_foreign_owned (tdev, pdev.as_ref (),0 )?; +// SAFETY: `reg` is stored in `TyrPlatformDriverData` and dropped when the driver is +// unbound; it is never forgotten. +let reg =unsafe { drm::Registration ::new (pdev.as_ref (), tdev, (),0 )? };let driver =TyrPlatformDriverData { - _device: tdev.into (), + _device: reg.device ().into (), + _reg: reg, };// We need this to be dev_info!() because dev_dbg!() does not work at
@@ -164,7 +169,7 @@ fn probe<'bound>( }#[pinned_drop] -impl PinnedDrop for TyrPlatformDriverData { +impl PinnedDrop for TyrPlatformDriverData <'_> {fn drop (self:Pin <&mut Self >) {} }
@@ -181,6 +186,7 @@ fn drop(self: Pin<&mut Self>) {}#[vtable] impl drm::Driver for TyrDrmDriver {type Data =TyrDrmDeviceData ; +type RegistrationData =ForLt! (());type File =TyrDrmFileData ;type Object = drm::gem::shmem::Object <BoData >;type ParentDevice <Ctx :DeviceContext > = platform::Device <Ctx >;
rust/kernel/drm/device.rs +49 -0
85 lines not on this page · raw message
rust/kernel/drm/driver.rs +78 -38
166 lines not on this page · raw message
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v3 10/13] rust: drm: Add RegistrationData to drm::Driver
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Missing `Sync` and `Send` bounds on `RegistrationData` allows safe code to introduce data races…
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing Sync and Send bounds on RegistrationData allows safe code to introduce data races when concurrently accessed via Device or Registration.
- [High] registration_guard() can be called on an UnregisteredDevice, allowing access to an uninitialized (dangling) registration_data pointer. --
commit 44df82d554a5a6b1d866d87bea0b20d5dd954c66
Author: Danilo Krummrich <[email protected]>
rust: drm: Add RegistrationData to drm::Driver
This commit adds a RegistrationData associated type to drm::Driver. It is tied to the parent bus device binding scope and stored in drm::Device before registration. Access is provided via Device::registration_data_with().
Quoted 4 lines
rust/kernel/drm/device.rs +0
-0
[ ... ]
rust/kernel/drm/device.rs pub fn new( · line 236
Quoted 6 lines
rust/kernel/drm/device.rs +2
-0@@ -230,6 +232,9 @@ pub fn new(// SAFETY: `drm_dev` is still private to this function. unsafe { (*drm_dev).driver =const { &Self ::VTABLE } }; +// SAFETY: `raw_drm` is valid; no concurrent access before registration. +unsafe { (*raw_drm.as_ptr ()).registration_data =UnsafeCell ::new (NonNull ::dangling ()) };
[Severity: High]
Could a caller safely dereference this dangling pointer by calling registration_guard() before the device is actually registered?
Since UnregisteredDevice dereferences to Device, a caller can invoke registration_guard() before registration. The C function drm_dev_enter() only checks if the device is unplugged, not if it has been registered, so it will return true and grant a RegistrationGuard prematurely.
If safe code then calls registration_data_with() on that guard, it seems it would result in a dangling pointer dereference here:
unsafe { (*self.registration_data.get()).cast::<_>().as_ref() }Quoted 4 lines
rust/kernel/drm/device.rs +1
-0+// SAFETY: The reference count is one, and now we take ownership of that reference as a // `drm::Device`. // INVARIANT: We just created the device above, but have yet to call `drm_dev_register`.
[ ... ]
Quoted 4 lines
rust/kernel/drm/driver.rs +0
-0
[ ... ]
rust/kernel/drm/driver.rs pub trait Driver { · line 121
Quoted 13 lines
rust/kernel/drm/driver.rs +9
-0@@ -110,6 +110,16 @@ pub trait Driver {/// Context data associated with the DRM driver type Data :Sync +Send ; +/// Data owned by the [`Registration`] and accessible within a +/// [`RegistrationGuard`](drm::RegistrationGuard) critical section via +/// [`Device::registration_data_with()`](drm::Device::registration_data_with). +/// +/// This is a [`ForLt`](trait@ForLt) type whose lifetime is tied to the parent bus +/// device binding scope. References handed out by +/// [`Device::registration_data_with()`](drm::Device::registration_data_with) are tied to +/// the closure scope. +type RegistrationData :ForLt ;
[Severity: High]
Does this type definition need Sync and Send bounds to prevent data races?
Since drm::Device unconditionally implements Sync, references to the device can be shared across multiple threads. The new registration_data_with() method allows these threads to concurrently obtain shared references to RegistrationData.
If a driver author provides a type that is not thread-safe (such as Cell) for RegistrationData in safe Rust, multiple threads could concurrently access and mutate the inner data, potentially leading to data races.
Quoted 4 lines
rust/kernel/drm/driver.rs +1
-0+/// The type used to manage memory for this driver. type Object :AllocImpl ;
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10
Danilo Krummrich
maintainerPATCH v3 11/13 rust: drm: Wrap ioctl dispatch in RegistrationGuard
In reply to Danilo KrummrichRun every ioctl handler inside a drm_dev_enter/exit critical section via RegistrationGuard. If the device has been unplugged, the ioctl returns ENODEV without calling the handler.
A never-called closure anchors the driver type for the compiler by tying dev's type to the handler's first parameter, which the compiler cannot infer through method resolution and associated-type projections alone.
--- rust/kernel/drm/ioctl.rs | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)
rust/kernel/drm/ioctl.rs +14 -1
@@ -87,7 +87,10 @@ pub mod internal {/// file: &kernel::drm::File<Self::File>, /// ) -> Result<u32> /// ``` -/// where `Self` is the drm::drv::Driver implementation these ioctls are being declared within. +/// where `Self` is the `drm::Driver` implementation these ioctls are being declared within. +/// +/// The ioctl runs inside a `drm_dev_enter/exit` critical section. If the device has been +/// unplugged, the ioctl returns `ENODEV` without calling the handler. /// /// # Examples ///
@@ -135,6 +138,16 @@ macro_rules! declare_drm_ioctls {// dev/file match the current driver these ioctls are being declared // for, and it's not clear how to enforce this within the type system. let dev = $crate::drm::device::Device ::from_raw (raw_dev); + +// Type-inference anchor: the closure is never called but ties `dev`'s +// type to `$func`'s first parameter, which the compiler cannot infer +// through method resolution and associated-type projections alone. +#[allow(unreachable_code)] +let _ = || $func(dev,unreachable! (),unreachable! ()); + +let Some (_guard) = dev.registration_guard ()else { +return $crate::error::code::ENODEV .to_errno (); + };// SAFETY: The ioctl argument has size `_IOC_SIZE(cmd)`, which we // asserted above matches the size of this type, and all bit patterns of // UAPI structs must be valid.
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v3 11/13] rust: drm: Wrap ioctl dispatch in RegistrationGuard
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] Type inference hole in declare_drm_ioctls! allows safe handlers to trigger UB by inferring a Registered context before registration is verified.
- [High] Unbounded lifetimes for data and file in declare_drm_ioctls! allow safe ioctl handlers to stash references, causing use-after-free. --
commit 28720068d91e18a10855651aab56d3a9e81f5d62
Author: Danilo Krummrich <[email protected]>
rust: drm: Wrap ioctl dispatch in RegistrationGuard
This commit wraps every ioctl handler inside a drm_dev_enter/exit critical section using RegistrationGuard. It returns ENODEV if the device is unplugged and anchors the driver type with a never-called closure to assist compiler type inference.
Quoted 4 lines
rust/kernel/drm/ioctl.rs +0
-0
[ ... ]
Quoted 4 lines
rust/kernel/drm/ioctl.rs +0
-0@@ -135,6 +138,16 @@ macro_rules! declare_drm_ioctls {// dev/file match the current driver these ioctls are being declared // for, and it's not clear how to enforce this within the type system. let dev = $crate::drm::device::Device ::from_raw (raw_dev);
rust/kernel/drm/ioctl.rs macro_rules! declare_drm_ioctls { · line 150
Quoted 10 lines
rust/kernel/drm/ioctl.rs +10
-0+ +// Type-inference anchor: the closure is never called but ties `dev`'s +// type to `$func`'s first parameter, which the compiler cannot infer +// through method resolution and associated-type projections alone. +#[allow(unreachable_code)] +let _ = || $func(dev,unreachable! (),unreachable! ()); + +let Some (_guard) = dev.registration_guard ()else { +return $crate::error::code::ENODEV .to_errno (); + };
[Severity: High]
This is a pre-existing issue, but does this type inference anchor create a scenario where safe driver code can trigger undefined behavior?
If a driver author writes a safe ioctl handler that expects a registered context, such as &Device<T, Registered>, the type-inference anchor will tie the type of dev to that first parameter. This causes Device::from_raw() to be evaluated with C = Registered.
Since Device::from_raw() is called before dev.registration_guard() can verify the device is actually bound, doesn't this create a Registered reference for an unplugged device? Would this violate the type invariant and safety contract of from_raw() before the guard is even checked?
[Severity: High]
This is also a pre-existing issue, but are the lifetimes of the data and file arguments passed to the user's $func properly constrained?
While the guard bounds the lifetime of the device access, the data and file parameters instantiated from raw pointers later in this macro don't appear to have explicit lifetime bounds. If $func demands a 'static lifetime for these arguments, will the compiler incorrectly infer them as 'static?
If so, could a safe ioctl handler stash these references in a global variable or async context, causing a use-after-free once the ioctl callback returns and the C API invalidates the pointers?
Quoted 3 lines
rust/kernel/drm/ioctl.rs +0
-0// SAFETY: The ioctl argument has size `_IOC_SIZE(cmd)`, which we // asserted above matches the size of this type, and all bit patterns of // UAPI structs must be valid.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=11
Danilo Krummrich
maintainerPATCH v3 12/13 rust: drm: Pass registration data to ioctl handlers
In reply to Danilo KrummrichPass registration data to ioctl handlers via drm::Device<Registered>::registration_data_with(). The closure's HRTB ties the lifetime to the closure scope, and the pointer cast shortens it from 'static internally. The reference is valid for the duration of the drm_dev_enter/exit critical section held by RegistrationGuard.
--- drivers/gpu/drm/nova/file.rs | 3 +++ drivers/gpu/drm/tyr/file.rs | 1 + rust/kernel/drm/ioctl.rs | 16 +++++++++++++--- 3 files changed, 17 insertions(+), 3 deletions(-)
drivers/gpu/drm/nova/file.rs +3 -0
@@ -24,6 +24,7 @@ impl File {/// IOCTL: get_param: Query GPU / driver metadata. pub (crate )fn get_param ( dev: &NovaDevice , + _reg_data: &(), getparam: &mut uapi::drm_nova_getparam , _file: &drm::File <File >, ) ->Result <u32 > {
@@ -44,6 +45,7 @@ pub(crate) fn get_param(/// IOCTL: gem_create: Create a new DRM GEM object. pub (crate )fn gem_create ( dev: &NovaDevice , + _reg_data: &(), req: &mut uapi::drm_nova_gem_create , file: &drm::File <File >, ) ->Result <u32 > {
@@ -57,6 +59,7 @@ pub(crate) fn gem_create(/// IOCTL: gem_info: Query GEM metadata. pub (crate )fn gem_info ( _dev: &NovaDevice , + _reg_data: &(), req: &mut uapi::drm_nova_gem_info , file: &drm::File <File >, ) ->Result <u32 > {
drivers/gpu/drm/tyr/file.rs +1 -0
@@ -29,6 +29,7 @@ fn open(_dev: &drm::Device<Self::Driver>) -> Result<Pin<KBox<Self>>> {impl TyrDrmFileData {pub (crate )fn dev_query ( ddev: &TyrDrmDevice , + _reg_data: &(), devquery: &mut uapi::drm_panthor_dev_query , _file: &TyrDrmFile , ) ->Result <u32 > {
rust/kernel/drm/ioctl.rs +13 -3
@@ -83,6 +83,7 @@ pub mod internal {/// /// ```ignore /// fn foo(device: &kernel::drm::Device<Self>, +/// reg_data: &<Self::RegistrationData as kernel::types::ForLt>::Of<'_>, /// data: &mut uapi::argument_type, /// file: &kernel::drm::File<Self::File>, /// ) -> Result<u32>
@@ -143,9 +144,16 @@ macro_rules! declare_drm_ioctls {// type to `$func`'s first parameter, which the compiler cannot infer // through method resolution and associated-type projections alone. #[allow(unreachable_code)] -let _ = || $func(dev,unreachable! (),unreachable! ()); +let _ = || { + $func( + dev, +unreachable! (), +unreachable! (), +unreachable! (), + ) + }; -let Some (_guard) = dev.registration_guard ()else { +let Some (guard) = dev.registration_guard ()else {return $crate::error::code::ENODEV .to_errno (); };// SAFETY: The ioctl argument has size `_IOC_SIZE(cmd)`, which we
@@ -160,7 +168,9 @@ macro_rules! declare_drm_ioctls {// SAFETY: This is just the DRM file structure let file =unsafe { $crate::drm::File ::from_raw (raw_file) }; -match $func(dev, data, file) { +match guard.registration_data_with (|reg_data| { + $func(&*guard, reg_data, data, file) + }) {Err (e) => e.to_errno (),Ok (i) => i.try_into () .unwrap_or ($crate::error::code::ERANGE .to_errno ()),
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v3 12/13] rust: drm: Pass registration data to ioctl handlers
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Missing `Sync` bound on `RegistrationData` allows data races via concurrent ioctl calls. - [High]…
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing Sync bound on RegistrationData allows data races via concurrent ioctl calls.
- [High] Unconstrained type inference in declare_drm_ioctls! bypasses type invariants, causing Undefined Behavior. --
commit 4f19dadb5da1b4410f3118837853bdb09c3f4718
Author: Danilo Krummrich <[email protected]>
rust: drm: Pass registration data to ioctl handlers
Pass registration data to ioctl handlers via drm::Device<Registered>::registration_data_with().
[ ... ]
Quoted 4 lines
rust/kernel/drm/ioctl.rs +0
-0
[ ... ]
rust/kernel/drm/ioctl.rs macro_rules! declare_drm_ioctls { · line 154
Quoted 13 lines
rust/kernel/drm/ioctl.rs +8
-1@@ -143,9 +144,16 @@ macro_rules! declare_drm_ioctls {// type to `$func`'s first parameter, which the compiler cannot infer // through method resolution and associated-type projections alone. #[allow(unreachable_code)] -let _ = || $func(dev,unreachable! (),unreachable! ()); +let _ = || { + $func( + dev, +unreachable! (), +unreachable! (), +unreachable! (), + ) + };
[Severity: High]
Does this type inference bypass the safety invariants of Device::from_raw()?
Because dev is passed directly to the user-provided $func in this dummy closure, the compiler infers the DeviceContext of dev from the handler's signature. If an ioctl handler expects &Device<T, Registered>, the compiler resolves dev to be a Registered device.
This means the preceding call to Device::from_raw(raw_dev) instantiates a Registered device reference before registration_guard() establishes the drm_dev_enter SRCU critical section. This bypasses the safety contract which requires the parent bus device to be bound.
Would it be safer to explicitly constrain dev to Device<_, Normal>, and pass &*guard instead of dev to the dummy closure?
Quoted 5 lines
rust/kernel/drm/ioctl.rs +1
-1-let Some (_guard) = dev.registration_guard ()else { +let Some (guard) = dev.registration_guard ()else {return $crate::error::code::ENODEV .to_errno (); };
[ ... ]
rust/kernel/drm/ioctl.rs macro_rules! declare_drm_ioctls { · line 173
Quoted 8 lines
rust/kernel/drm/ioctl.rs +3
-1@@ -160,7 +168,9 @@ macro_rules! declare_drm_ioctls {// SAFETY: This is just the DRM file structure let file =unsafe { $crate::drm::File ::from_raw (raw_file) }; -match $func(dev, data, file) { +match guard.registration_data_with (|reg_data| { + $func(&*guard, reg_data, data, file) + }) {
[Severity: High]
Can this allow data races if the RegistrationData type is not Sync?
Looking at the Driver trait definition in rust/kernel/drm/driver.rs:
trait Driver {
type RegistrationData: ForLt;
...
}RegistrationData is defined without a Sync bound. However, Device<T, C> is unconditionally Sync, meaning any thread can access its shared methods concurrently.
Since multiple userspace threads can invoke DRM ioctls concurrently, multiple threads can call registration_data_with() at the same time to obtain shared references to the same data. If a driver uses a non-Sync type for RegistrationData (such as Cell<T>), could this lead to concurrent mutation and undefined behavior?
Quoted 3 lines
rust/kernel/drm/ioctl.rs +0
-0Err(e) => e.to_errno(), Ok(i) => i.try_into() .unwrap_or($crate::error::code::ERANGE.to_errno()),
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=12
Danilo Krummrich
maintainerPATCH v3 13/13 drm: nova: Use drm::Device<Registered> to access the parent bus device
In reply to Danilo KrummrichThe get_param ioctl needs access to the parent auxiliary device. Since ioctl handlers run inside a RegistrationGuard, accept &NovaDevice<Registered> to obtain &auxiliary::Device<Bound> via as_ref() directly. This removes the need for drm::Device data, hence set it to ().
--- drivers/gpu/drm/nova/driver.rs | 11 ++--------- drivers/gpu/drm/nova/file.rs | 15 ++++++++++----- 2 files changed, 12 insertions(+), 14 deletions(-)
drivers/gpu/drm/nova/driver.rs +2 -9
@@ -30,11 +30,6 @@ pub(crate) struct Nova<'bound> {/// Convienence type alias for the DRM device type for this driver pub (crate )type NovaDevice <Ctx = drm::Normal > = drm::Device <NovaDriver ,Ctx >; -#[pin_data] -pub (crate )struct NovaData { -pub (crate ) adev:ARef <auxiliary::Device >, -} -const INFO : drm::DriverInfo = drm::DriverInfo { major:0 , minor:0 ,
@@ -65,9 +60,7 @@ fn probe<'bound>( adev: &'bound auxiliary::Device<Core<'_>>, _info: &'bound Self::IdInfo, ) -> impl PinInit<Self::Data<'bound>, Error> + 'bound { -let data =try_pin_init! (NovaData { adev: adev.into () }); - -let drm = drm::UnregisteredDevice ::<Self >::new (adev, data)?; +let drm = drm::UnregisteredDevice ::<Self >::new (adev,Ok (()))?;// SAFETY: `reg` is stored in `Nova` and dropped when the driver is unbound; it is // never forgotten. let reg =unsafe { drm::Registration ::new (adev.as_ref (), drm, (),0 )? };
@@ -81,7 +74,7 @@ fn probe<'bound>(#[vtable] impl drm::Driver for NovaDriver { -type Data =NovaData ; +type Data = ();type RegistrationData =ForLt! (());type File =File ;type Object = gem::Object <NovaObject >;
drivers/gpu/drm/nova/file.rs +10 -5
@@ -4,7 +4,13 @@use crate ::gem::NovaObject ;use kernel::{ alloc::flags::*, - drm::{self, gem::BaseObject }, + auxiliary, + device::Bound , + drm::{ + self, + gem::BaseObject , +Registered ,// + }, pci, prelude::*, uapi,
@@ -23,14 +29,13 @@ fn open(_dev: &NovaDevice) -> Result<Pin<KBox<Self>>> {impl File {/// IOCTL: get_param: Query GPU / driver metadata. pub (crate )fn get_param ( - dev: &NovaDevice , + dev: &NovaDevice <Registered >, _reg_data: &(), getparam: &mut uapi::drm_nova_getparam , _file: &drm::File <File >, ) ->Result <u32 > { -let adev = &dev.adev; -let parent = adev.parent (); -let pdev: &pci::Device = parent.try_into ()?; +let adev: &auxiliary::Device <Bound > = dev.as_ref (); +let pdev: &pci::Device <Bound > = adev.parent ().try_into ()?;let value =match getparam.paramas u32 { uapi::NOVA_GETPARAM_VRAM_BAR_SIZE => pdev.resource_len(1)?,
Signature · 1 line
2.54.0
[email protected]
AI reviewRe: [PATCH v3 13/13] drm: nova: Use drm::Device<Registered> to access the parent bus device
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Changing the IOCTL handler signature to accept `&Device<Registered>` forces the…
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Changing the IOCTL handler signature to accept &Device<Registered> forces the declare_drm_ioctls! macro to unsafely assert registration invariants before verifying them. --
commit ae89d443b44234dbccc8b42ac767d5eb9bdfb248
Author: Danilo Krummrich <[email protected]>
drm: nova: Use drm::Device<Registered> to access the parent bus device
This commit modifies the get_param ioctl handler to accept a &NovaDevice<Registered> instead of &NovaDevice, avoiding the need for storing device data.
Quoted 4 lines
drivers/gpu/drm/nova/file.rs +0
-0
[ ... ]
drivers/gpu/drm/nova/file.rs fn open(_dev: &NovaDevice) -> Result<Pin<KBox<Self>>> { · line 32
Quoted 6 lines
drivers/gpu/drm/nova/file.rs +1
-1@@ -23,14 +29,13 @@ fn open(_dev: &NovaDevice) -> Result<Pin<KBox<Self>>> {impl File {/// IOCTL: get_param: Query GPU / driver metadata. pub (crate )fn get_param ( - dev: &NovaDevice, + dev: &NovaDevice<Registered>,
[Severity: High]
Does changing the ioctl handler signature to accept &Device<Registered> cause the declare_drm_ioctls! macro to unsafely assert registration invariants before verifying them?
The declare_drm_ioctls! macro instantiates a reference to the DRM device using Device::from_raw(raw_dev). Because of the type-inference anchor let _ = || { $func(dev, ...) }, changing the first parameter to &NovaDevice<Registered> causes from_raw to be invoked with the Registered context type.
The safety contract of from_raw requires callers to promise that any type invariants of the generic context C are upheld. The Registered context guarantees that the DRM device is currently registered and its parent bus device is bound.
However, the macro calls from_raw before acquiring the RegistrationGuard, which verifies via drm_dev_enter that the device hasn't been unplugged:
rust/kernel/drm/ioctl.rs:declare_drm_ioctls!() {
...
let dev = $crate::drm::device::Device::from_raw(raw_dev);
...
}If the device is concurrently unplugged between drm_ioctl's initial check and from_raw, the invariant is violated, which breaks the from_raw safety contract and could cause undefined behavior.
Would it be safer for the handler to continue accepting &NovaDevice (the Normal context) and call dev.registration_guard() internally to obtain a Registered reference?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=13
Danilo Krummrich
maintainerQuoted 2 lines · Danilo Krummrich
rust: drm: Add RegistrationGuard for drm_dev_enter/exit critical
sections
As also pointed out by Sashiko, I forgot to consider that constructing a RegistrationGuard still requires that the drm::Device has been registered at some point.
Theoretically, this could be addressed in a way that it is still valid to construct a RegistrationGuard from a drm::Device<Normal> (e.g. by leveraging dev->unplugged with corresponding synchronization), but other than from IOCTLs there is no use-case for this guard. Since IOCTLs already carry the required invariant implicitly, I went with a separate Ioctl typestate.
This and the fact that we can't let drivers infer any other typestate than Normal before the RegistrationGuard is in place, results in a few changes that are worth considering for review, so I decided to resend early.