rust: drm: Higher-Ranked Lifetime private data

References to dev, data, and file in the declare_drm_ioctls! macro are created via unsafe pointer dereferences, producing unbounded lifetimes. If an ioctl handler explicitly annotates its parameters with 'static, the compiler accepts this, allowing the handler to stash references that outlive the ioctl call.

Fix this by routing all references through a helper function whose lifetime parameter 'a is tied to a local anchor variable. Since 'a is bounded by the anchor's stack lifetime, handlers can no longer demand 'static on any parameter.

Cc [email protected]

Fixes 9a69570682b1 ("rust: drm: ioctl: Add DRM ioctl abstraction")

Reported-by [email protected]

Closes https://lore.kernel.org/all/[email protected]/ open here

Signed-off-by Danilo Krummrich <[email protected]>

---
 rust/kernel/drm/ioctl.rs | 59 +++++++++++++++++++++++++++++++---------
 1 file changed, 46 insertions(+), 13 deletions(-)

rust/kernel/drm/ioctl.rs +33 -2

index cf328101dde4..023e6da5c1e4 100644
@@ -70,6 +70,39 @@ pub mod internal {
     pub use bindings::drm_device;
     pub use bindings::drm_file;
     pub use bindings::drm_ioctl_desc;
+
+    /// Call an ioctl handler with lifetime-bounded references.
+    ///
+    /// The lifetime `'a` is tied to the `_anchor` parameter. This prevents handlers from
+    /// declaring `'static` on `dev`, `data`, or `file`.
+    ///
+    /// # Safety
+    ///
+    /// - `raw_data` must point to a valid, exclusively-owned instance of `Data` for the duration
+    ///   of the call.
+    /// - `raw_file` must be a valid pointer to a `struct drm_file`.
+    #[doc(hidden)]
+    #[inline(always)]
+    pub unsafe fn __call_ioctl<
+        'a,
+        Dev: 'a,
+        Data: 'a,
+        F: super::super::file::DriverFile + 'a,
+        Ret,
+    >(
+        _anchor: &'a (),
+        dev: &'a Dev,
+        raw_data: *mut ::core::ffi::c_void,
+        raw_file: *mut drm_file,
+        f: impl FnOnce(&'a Dev, &'a mut Data, &'a super::super::File<F>) -> Ret,
+    ) -> Ret {
+        // SAFETY: Caller guarantees raw_data points to a valid instance of Data with the correct
+        // size and alignment, exclusively owned for the duration of the ioctl call.
+        let data = unsafe { &mut *(raw_data.cast::<Data>()) };
+        // SAFETY: Caller guarantees raw_file is a valid pointer to a `struct drm_file`.
+        let file = unsafe { super::super::File::<F>::from_raw(raw_file) };
+        f(dev, data, file)
+    }
 }
 
 /// Declare the DRM ioctls for a driver.

@@ -135,19 +168,19 @@ 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);
-                            // SAFETY: The ioctl argument has size `_IOC_SIZE(cmd)`, which we
-                            // asserted above matches the size of this type, and all bit patterns of

rust/kernel/drm/ioctl.rs +13 -1

-                            match $func(dev, data, file) {
+                            let __anchor = ();
+
+                            // 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. The argument is exclusively owned by this
+                            //   handler, guaranteed by `drm_ioctl()` to remain valid for the
+                            //   duration of the call.
+                            // - `raw_file` is a valid `struct drm_file` pointer provided by the
+                            //   DRM core.
+                            match unsafe { $crate::drm::ioctl::internal::__call_ioctl(
+                                &__anchor, dev, raw_data, raw_file, $func,
+                            ) } {
                                 Err(e) => e.to_errno(),
                                 Ok(i) => i.try_into()
                                             .unwrap_or($crate::error::code::ERANGE.to_errno()),

Restrict 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.

Signed-off-by Danilo Krummrich <[email protected]>

---
 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

index 8ddb81fd0c87..e3c54303d70e 100644
@@ -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

index 9d8ff7de2c0f..2b6fe9dc0bfa 100644
@@ -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

index 180631daff02..7f082de6d6dc 100644
@@ -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

index c6d4d6f9bae3..1640a161754b 100644
@@ -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

index 9825d52832af..6f3af46ff647 100644
@@ -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

index 802e7fc13e30..5152a18a8312 100644
@@ -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

index 1023ddccd785..d56cbe2663e2 100644
@@ -10,8 +10,7 @@
         self,
         device::{
             DeviceContext,
-            Normal,
-            Registered, //
+            Normal, //
         },
         driver::{
             AllocImpl,

@@ -82,8 +81,7 @@ unsafe fn dec_ref(obj: core::ptr::NonNull<Self>) {
 /// A type alias for retrieving the current [`AllocImpl`] for a given [`DriverObject`].
 ///
 /// [`Driver`]: drm::Driver
-pub type DriverAllocImpl<T, Ctx = Registered> =
-    <<T as DriverObject>::Driver as drm::Driver>::Object<Ctx>;
+pub type DriverAllocImpl<T> = <<T as DriverObject>::Driver as drm::Driver>::Object;
 
 /// GEM object functions, which must be implemented by drivers.
 pub trait DriverObject: Sync + Send + Sized {

@@ -94,8 +92,8 @@ pub trait DriverObject: Sync + Send + Sized {
     type Args;
 
     /// Create a new driver data object for a GEM object of a given size.
-    fn new<Ctx: DeviceContext>(
-        dev: &drm::Device<Self::Driver, Ctx>,
+    fn new(
+        dev: &drm::Device<Self::Driver>,
         size: usize,
         args: Self::Args,
     ) -> impl PinInit<Self, Error>;

@@ -110,7 +108,7 @@ fn close(_obj: &DriverAllocImpl<Self>, _file: &DriverFile<Self>) {}
 }
 
 /// Trait that represents a GEM object subtype
-pub trait IntoGEMObject: Sized + super::private::Sealed + AlwaysRefCounted {
+pub trait IntoGEMObject: Sized + super::private::Sealed {
     /// Returns a reference to the raw `drm_gem_object` structure, which must be valid as long as
     /// this owning object is valid.
     fn as_raw(&self) -> *mut bindings::drm_gem_object;

@@ -184,7 +182,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<Normal> = Self, File = F>,
+        D: drm::Driver<Object = Self, File = F>,
         F: drm::file::DriverFile<Driver = D>,
     {
         let mut handle: u32 = 0;

@@ -198,8 +196,8 @@ fn create_handle<D, F>(&self, file: &drm::File<F>) -> Result<u32>
     /// Looks up an object by its handle for a given `File`.
     fn lookup_handle<D, F>(file: &drm::File<F>, handle: u32) -> Result<ARef<Self>>
     where
-        Self: AllocImpl<Driver = D>,
-        D: drm::Driver<Object<Normal> = Self, File = F>,
+        Self: AllocImpl<Driver = D> + AlwaysRefCounted,
+        D: drm::Driver<Object = Self, File = F>,
         F: drm::file::DriverFile<Driver = D>,
     {
         // SAFETY: The arguments are all valid per the type invariants.

@@ -281,12 +279,43 @@ impl<T: DriverObject, Ctx: DeviceContext> Object<T, Ctx> {
         rss: None,
     };
 
+    /// Returns the `Device` that owns this GEM object.
+    pub fn dev(&self) -> &drm::Device<T::Driver, Ctx> {
+        // SAFETY:
+        // - `struct drm_gem_object.dev` is initialized and valid for as long as the GEM
+        //   object lives.
+        // - The device we used for creating the gem object is passed as &drm::Device<T::Driver> to
+        //   Object::<T>::new(), so we know that `T::Driver` is the right generic parameter to use
+        //   here.
+        // - Any type invariants of `Ctx` are upheld by using the same `Ctx` for the `Device` we
+        //   return.
+        unsafe { drm::Device::from_raw((*self.as_raw()).dev) }
+    }
+
+    fn as_raw(&self) -> *mut bindings::drm_gem_object {
+        self.obj.get()
+    }
+
+    extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) {
+        let ptr: *mut Opaque<bindings::drm_gem_object> = obj.cast();
+
+        // SAFETY: All of our objects are of type `Object<T>`.
+        let this = unsafe { crate::container_of!(ptr, Self, obj) };
+
+        // SAFETY: The C code only ever calls this callback with a valid pointer to a `struct
+        // drm_gem_object`.
+        unsafe { bindings::drm_gem_object_release(obj) };
+
+        // SAFETY: All of our objects are allocated via `KBox`, and we're in the
+        // free callback which guarantees this object has zero remaining references,
+        // so we can drop it.
+        let _ = unsafe { KBox::from_raw(this) };
+    }
+}
+
+impl<T: DriverObject> Object<T> {
     /// Create a new GEM object.
-    pub fn new(
-        dev: &drm::Device<T::Driver, Ctx>,
-        size: usize,
-        args: T::Args,
-    ) -> Result<ARef<Self>> {
+    pub fn new(dev: &drm::Device<T::Driver>, size: usize, args: T::Args) -> Result<ARef<Self>> {
         let obj: Pin<KBox<Self>> = KBox::pin_init(
             try_pin_init!(Self {
                 obj: Opaque::new(bindings::drm_gem_object::default()),

@@ -322,46 +351,12 @@ pub fn new(
         // SAFETY: We take over the initial reference count from `drm_gem_object_init()`.
         Ok(unsafe { ARef::from_raw(ptr) })
     }
-
-    /// Returns the `Device` that owns this GEM object.
-    pub fn dev(&self) -> &drm::Device<T::Driver, Ctx> {
-        // SAFETY:
-        // - `struct drm_gem_object.dev` is initialized and valid for as long as the GEM
-        //   object lives.
-        // - The device we used for creating the gem object is passed as &drm::Device<T::Driver> to
-        //   Object::<T>::new(), so we know that `T::Driver` is the right generic parameter to use
-        //   here.
-        // - Any type invariants of `Ctx` are upheld by using the same `Ctx` for the `Device` we
-        //   return.
-        unsafe { drm::Device::from_raw((*self.as_raw()).dev) }
-    }
-
-    fn as_raw(&self) -> *mut bindings::drm_gem_object {
-        self.obj.get()
-    }
-
-    extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) {
-        let ptr: *mut Opaque<bindings::drm_gem_object> = obj.cast();
-
-        // SAFETY: All of our objects are of type `Object<T>`.
-        let this = unsafe { crate::container_of!(ptr, Self, obj) };
-
-        // SAFETY: The C code only ever calls this callback with a valid pointer to a `struct
-        // drm_gem_object`.
-        unsafe { bindings::drm_gem_object_release(obj) };
-
-        // SAFETY: All of our objects are allocated via `KBox`, and we're in the
-        // free callback which guarantees this object has zero remaining references,
-        // so we can drop it.
-        let _ = unsafe { KBox::from_raw(this) };
-    }
 }
 
 impl_aref_for_gem_obj! {
-    impl<T, C> for Object<T, C>
+    impl<T> for Object<T>
     where
-        T: DriverObject,
-        C: DeviceContext
+        T: DriverObject
 }
 
 impl<T: DriverObject, Ctx: DeviceContext> super::private::Sealed for Object<T, Ctx> {}

rust/kernel/drm/gem/shmem.rs +38 -37

index f47a90cdb95b..146e8cfc2cdf 100644
@@ -43,14 +43,14 @@
 /// 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 = Normal> {
+pub struct ObjectConfig<'a, T: DriverObject> {
     /// Whether to set the write-combine map flag.
     pub map_wc: bool,
 
     /// Reuse the DMA reservation from another GEM object.
     ///
     /// The newly created [`Object`] will hold an owned refcount to `parent_resv_obj` if specified.
-    pub parent_resv_obj: Option<&'a Object<T, C>>,
+    pub parent_resv_obj: Option<&'a Object<T>>,
 }
 
 /// A shmem-backed GEM object.

@@ -66,17 +66,16 @@ 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.
-    parent_resv_obj: Option<ARef<Object<T, C>>>,
+    parent_resv_obj: Option<ARef<Object<T>>>,
     #[pin]
     inner: T,
     _ctx: PhantomData<C>,
 }
 
 super::impl_aref_for_gem_obj! {
-    impl<T, C> for Object<T, C>
+    impl<T> for Object<T>
     where
-        T: DriverObject,
-        C: DeviceContext
+        T: DriverObject
 }
 
 // SAFETY: All GEM objects are thread-safe.

@@ -112,13 +111,43 @@ fn as_raw_shmem(&self) -> *mut bindings::drm_gem_shmem_object {
         self.obj.get()
     }
 
+    /// Returns the `Device` that owns this GEM object.
+    pub fn dev(&self) -> &Device<T::Driver, C> {
+        // SAFETY: `dev` will have been initialized in `Self::new()` by `drm_gem_shmem_init()`.
+        unsafe { Device::from_raw((*self.as_raw()).dev) }
+    }
+
+    extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) {
+        // SAFETY:
+        // - DRM always passes a valid gem object here
+        // - We used drm_gem_shmem_create() in our create_gem_object callback, so we know that
+        //   `obj` is contained within a drm_gem_shmem_object
+        let this = unsafe { container_of!(obj, bindings::drm_gem_shmem_object, base) };
+
+        // SAFETY:
+        // - We're in free_callback - so this function is safe to call.
+        // - We won't be using the gem resources on `this` after this call.
+        unsafe { bindings::drm_gem_shmem_release(this) };
+
+        // SAFETY:
+        // - We verified above that `obj` is valid, which makes `this` valid
+        // - This function is set in AllocOps, so we know that `this` is contained within a
+        //   `Object<T, C>`
+        let this = unsafe { container_of!(Opaque::cast_from(this), Self, obj) }.cast_mut();
+
+        // SAFETY: We're recovering the Kbox<> we created in gem_create_object()
+        let _ = unsafe { KBox::from_raw(this) };
+    }
+}
+
+impl<T: DriverObject> Object<T> {
     /// Create a new shmem-backed DRM object of the given size.
     ///
     /// Additional config options can be specified using `config`.
     pub fn new(
-        dev: &Device<T::Driver, C>,
+        dev: &Device<T::Driver>,
         size: usize,
-        config: ObjectConfig<'_, T, C>,
+        config: ObjectConfig<'_, T>,
         args: T::Args,
     ) -> Result<ARef<Self>> {
         let new: Pin<KBox<Self>> = KBox::try_pin_init(

@@ -126,7 +155,7 @@ pub fn new(
                 obj <- Opaque::init_zeroed(),
                 parent_resv_obj: config.parent_resv_obj.map(|p| p.into()),
                 inner <- T::new(dev, size, args),
-                _ctx: PhantomData::<C>,
+                _ctx: PhantomData,
             }),
             GFP_KERNEL,
         )?;

@@ -157,34 +186,6 @@ pub fn new(
 
         Ok(obj)
     }
-
-    /// Returns the `Device` that owns this GEM object.
-    pub fn dev(&self) -> &Device<T::Driver, C> {
-        // SAFETY: `dev` will have been initialized in `Self::new()` by `drm_gem_shmem_init()`.
-        unsafe { Device::from_raw((*self.as_raw()).dev) }
-    }
-
-    extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) {
-        // SAFETY:
-        // - DRM always passes a valid gem object here
-        // - We used drm_gem_shmem_create() in our create_gem_object callback, so we know that
-        //   `obj` is contained within a drm_gem_shmem_object
-        let this = unsafe { container_of!(obj, bindings::drm_gem_shmem_object, base) };
-
-        // SAFETY:
-        // - We're in free_callback - so this function is safe to call.
-        // - We won't be using the gem resources on `this` after this call.
-        unsafe { bindings::drm_gem_shmem_release(this) };
-
-        // SAFETY:
-        // - We verified above that `obj` is valid, which makes `this` valid
-        // - This function is set in AllocOps, so we know that `this` is contained within a
-        //   `Object<T, C>`
-        let this = unsafe { container_of!(Opaque::cast_from(this), Self, obj) }.cast_mut();
-
-        // SAFETY: We're recovering the Kbox<> we created in gem_create_object()
-        let _ = unsafe { KBox::from_raw(this) };
-    }
 }
 
 impl<T: DriverObject, C: DeviceContext> Deref for Object<T, C> {

Maybe there's a reason for you doing this I'm missing but, wouldn't it be a better idea to simply remove the DeviceContext generic from gem objects entirely and instead always (with the exception of Ioctl callbacks, of course) just use the Normal context?

One thing I had in mind is that we could have TTM callbacks that guarantee us a Registered DeviceContext that could be represented by the corresonding GEM object. Of course, it could also be represented by a separate &Device<Registered> being passed to the callback.

Quoted 4 lines · Danilo Krummrich

One thing I had in mind is that we could have TTM callbacks that guarantee us a Registered DeviceContext that could be represented by the corresonding GEM object. Of course, it could also be represented by a separate &Device<Registered> being passed to the callback.

IMO I think this is better - especially since when I originally introduced DeviceContext the intent was that it should be something that we try to omit wherever possible, so users mainly only need to interact with it in various callbacks that have assumptions about the state of the Device. This makes sense logically as well, since the only reason you would need to manually specify the DeviceContext a Device is in is when you're running Rust code that has been executed from a non-Rust context.

I think for Registration this is definitely ideal, since now that we're trying to give it the ability to represent "This device is -currently- registered" vs. "This device was registered at some point" trying to avoid generalizing types over DeviceContext helps to ensure we don't mistakenly pass through a Registered DeviceContext somewhere we didn't mean to. E.g., allowing a user to store a gem Object with the Registered device context would technically be a leak as it would then be impossible for us to guarantee that all Registered devices are presently registered.

In general I agree; and I think I also brought it up a few times that it would be nice to avoid making GEM objects generic over the DeviceContext.

But now that we already have it (and it is out of the way with this patch already), I think it could make sense to wait a bit and see how the TTM stuff turns out before we go back and forth (I think it may be useful there)?

At least I'd prefer to do this in a follow-up patch and, if required, discuss it there. Do you mind sending a follow-up patch, given that you already looked into removing it entirely?

Ioctl handlers now receive a &Device<T, Registered> reference, proving at the type level that the device is registered and its parent bus device is bound.

This is achieved by calling registration_guard() on the Device<T, Ioctl> obtained in ioctl dispatch context. If the device has been unplugged, the ioctl returns -ENODEV without calling the handler.

To resolve the driver type parameter T for type inference, which the compiler cannot propagate through method resolution and associated-type projections alone, a dead-code closure and a helper function are used as a type-inference anchor.

Signed-off-by Danilo Krummrich <[email protected]>

---
 drivers/gpu/drm/nova/file.rs | 12 ++++++----
 drivers/gpu/drm/tyr/file.rs  |  7 ++++--
 rust/kernel/drm/ioctl.rs     | 45 +++++++++++++++++++++++++++++++++---
 3 files changed, 55 insertions(+), 9 deletions(-)

drivers/gpu/drm/nova/file.rs +8 -4

index a3b7bd36792c..19fb89b28984 100644
@@ -4,7 +4,11 @@
 use crate::gem::NovaObject;
 use kernel::{
     alloc::flags::*,
-    drm::{self, gem::BaseObject},
+    drm::{
+        self,
+        gem::BaseObject,
+        Registered, //
+    },
     pci,
     prelude::*,
     uapi,

@@ -23,7 +27,7 @@ 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>,
         getparam: &mut uapi::drm_nova_getparam,
         _file: &drm::File<File>,
     ) -> Result<u32> {

@@ -43,7 +47,7 @@ pub(crate) fn get_param(
 
     /// IOCTL: gem_create: Create a new DRM GEM object.
     pub(crate) fn gem_create(
-        dev: &NovaDevice,
+        dev: &NovaDevice<Registered>,
         req: &mut uapi::drm_nova_gem_create,
         file: &drm::File<File>,
     ) -> Result<u32> {

@@ -56,7 +60,7 @@ pub(crate) fn gem_create(
 
     /// IOCTL: gem_info: Query GEM metadata.
     pub(crate) fn gem_info(
-        _dev: &NovaDevice,
+        _dev: &NovaDevice<Registered>,
         req: &mut uapi::drm_nova_gem_info,
         file: &drm::File<File>,
     ) -> Result<u32> {

drivers/gpu/drm/tyr/file.rs +5 -2

index 31411da203c5..fb9233eae01c 100644
@@ -1,7 +1,10 @@
 // SPDX-License-Identifier: GPL-2.0 or MIT
 
 use kernel::{
-    drm,
+    drm::{
+        self,
+        Registered, //
+    },
     prelude::*,
     uaccess::UserSlice,
     uapi, //

@@ -28,7 +31,7 @@ fn open(_dev: &drm::Device<Self::Driver>) -> Result<Pin<KBox<Self>>> {
 
 impl TyrDrmFileData {
     pub(crate) fn dev_query(
-        ddev: &TyrDrmDevice,
+        ddev: &TyrDrmDevice<Registered>,
         devquery: &mut uapi::drm_panthor_dev_query,
         _file: &TyrDrmFile,
     ) -> Result<u32> {

rust/kernel/drm/ioctl.rs +2 -0

index 70cf1aa4d788..6cefd26b31f9 100644
@@ -71,6 +71,18 @@ pub mod internal {
     pub use bindings::drm_file;
     pub use bindings::drm_ioctl_desc;
 
+    /// Reinterpret a pointer to a DRM device with a different [`DeviceContext`], preserving the
+    /// driver type parameter `T`.

rust/kernel/drm/ioctl.rs +30 -3

     /// Call an ioctl handler with lifetime-bounded references.
     ///
     /// The lifetime `'a` is tied to the `_anchor` parameter. This prevents handlers from

@@ -115,7 +127,7 @@ pub unsafe fn __call_ioctl<
 /// `user_callback` should have the following prototype:
 ///
 /// ```ignore
-/// fn foo(device: &kernel::drm::Device<Self>,
+/// fn foo(device: &kernel::drm::Device<Self, kernel::drm::Registered>,
 ///        data: &mut uapi::argument_type,
 ///        file: &kernel::drm::File<Self::File>,
 /// ) -> Result<u32>

@@ -164,11 +176,38 @@ macro_rules! declare_drm_ioctls {
                             // - The DRM device must have been registered when we're called through
                             //   an IOCTL.
                             //
+                            // INVARIANT: The `Ioctl` context requires that the device has been
+                            // registered via `drm_dev_register()` at some point; the DRM core
+                            // guarantees this for ioctl dispatch callbacks.
+                            //
                             // FIXME: Currently there is nothing enforcing that the types of the
                             // 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<_, $crate::drm::Normal> =
+                            let dev: &$crate::drm::device::Device<_, $crate::drm::Ioctl> =
                                 $crate::drm::device::Device::from_raw(raw_dev);
+                            // Cast to Registered preserving the driver type parameter.
+                            let __ptr = $crate::drm::ioctl::internal::__dev_ctx_cast(
+                                ::core::ptr::from_ref(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(
+                                    // SAFETY: This closure is never executed; the dereference
+                                    // exists purely to unify the type parameter with `$func`.
+                                    // The pointer is valid regardless.
+                                    unsafe { &*__ptr },
+                                    unreachable!(),
+                                    unreachable!(),
+                                )
+                            };
+
+                            let Some(guard) = dev.registration_guard() else {
+                                return $crate::error::code::ENODEV.to_errno();
+                            };
                             let __anchor = ();
 
                             // SAFETY:

@@ -180,7 +219,7 @@ macro_rules! declare_drm_ioctls {
                             // - `raw_file` is a valid `struct drm_file` pointer provided by the
                             //   DRM core.
                             match unsafe { $crate::drm::ioctl::internal::__call_ioctl(
-                                &__anchor, dev, raw_data, raw_file, $func,
+                                &__anchor, &*guard, raw_data, raw_file, $func,
                             ) } {
                                 Err(e) => e.to_errno(),
                                 Ok(i) => i.try_into()

Add 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.

Signed-off-by Danilo Krummrich <[email protected]>

---
 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      | 106 +++++++++++++++++++++------------
 4 files changed, 143 insertions(+), 50 deletions(-)

drivers/gpu/drm/nova/driver.rs +14 -6

index e3c54303d70e..131212df94d3 100644
@@ -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

index 7f082de6d6dc..e017448aabab 100644
@@ -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

index 8f63276c9b62..fc79f90a197d 100644
@@ -20,6 +20,7 @@
         AlwaysRefCounted, //
     },
     types::{
+        ForLt,
         NotThreadSafe,
         Opaque, //
     },

@@ -32,6 +33,7 @@
 };
 use core::{
     alloc::Layout,
+    cell::UnsafeCell,
     marker::PhantomData,
     mem,
     ops::Deref,

@@ -247,6 +249,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()) };
+
         // 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`.

@@ -270,6 +275,7 @@ pub fn new(
 pub struct Device<T: drm::Driver, C: DeviceContext = Normal> {
     dev: Opaque<bindings::drm_device>,
     data: T::Data,
+    pub(super) registration_data: UnsafeCell<NonNull<<T::RegistrationData as ForLt>::Of<'static>>>,
     _ctx: PhantomData<C>,
 }
 

@@ -278,6 +284,28 @@ pub(crate) fn as_raw(&self) -> *mut bindings::drm_device {
         self.dev.get()
     }
 
+    /// Returns a reference to the registration data with lifetime shortened from `'static`.
+    ///
+    /// # Safety
+    ///
+    /// The caller must ensure that:
+    ///
+    /// - The parent bus device is bound (e.g. by holding an active `drm_dev_enter()` critical
+    ///   section via [`RegistrationGuard`]).
+    /// - The returned reference is not exposed to code that can choose a concrete lifetime for
+    ///   it, as that would be unsound for types that are invariant over their lifetime parameter
+    ///   (e.g. it must be passed through an HRTB-bounded closure).
+    #[inline]
+    unsafe fn registration_data_unchecked(&self) -> &<T::RegistrationData as ForLt>::Of<'_> {
+        // SAFETY:
+        // - Caller guarantees the parent bus device is bound, hence the pointer is valid.
+        // - The pointer cast from `Of<'static>` to `Of<'_>` is layout-compatible since lifetimes
+        //   are erased at runtime.
+        // - Caller guarantees the reference is only used behind an HRTB, making the lifetime
+        //   shortening sound regardless of variance.
+        unsafe { (*self.registration_data.get()).cast::<_>().as_ref() }
+    }
+
     /// # Safety
     ///
     /// `ptr` must be a valid pointer to a `struct device` embedded in `Self`.

@@ -385,6 +413,27 @@ pub struct RegistrationGuard<'a, T: drm::Driver> {
     _not_send: NotThreadSafe,
 }
 
+impl<T: drm::Driver> Device<T, Registered> {
+    /// Access the registration data through a closure, with the lifetime tied to the closure
+    /// scope.
+    ///
+    /// The data is owned by [`Registration`](drm::Registration) and is guaranteed to remain valid
+    /// as long as the device is registered, since [`Registration`](drm::Registration)'s `drop`
+    /// calls `drm_dev_unplug()` which waits for all `drm_dev_enter()` critical sections to
+    /// complete.
+    #[inline]
+    pub fn registration_data_with<R, F>(&self, f: F) -> R
+    where
+        F: for<'a> FnOnce(&'a <T::RegistrationData as ForLt>::Of<'a>) -> R,
+    {
+        // SAFETY: `Registered` guarantees the device is registered and the parent bus device is
+        // bound. The closure's HRTB `for<'a>` prevents the caller from smuggling in references
+        // with a concrete short lifetime, satisfying the lifetime requirement of
+        // `registration_data_unchecked`.
+        f(unsafe { self.registration_data_unchecked() })
+    }
+}
+
 impl<T: drm::Driver> Deref for RegistrationGuard<'_, T> {
     type Target = Device<T, Registered>;
 

rust/kernel/drm/driver.rs +68 -38

index ceb2829985c7..e42b48e5cad2 100644
@@ -7,11 +7,11 @@
 use crate::{
     bindings,
     device,
-    devres,
     drm,
     error::to_result,
     prelude::*,
-    sync::aref::ARef, //
+    sync::aref::ARef,
+    types::ForLt, //
 };
 use core::{
     mem,

@@ -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;
+
     /// The type used to manage memory for this driver.
     type Object: AllocImpl;
 

@@ -139,65 +149,82 @@ pub trait Driver {
 /// The registration type of a `drm::Device`.
 ///
 /// Once the `Registration` structure is dropped, the device is unregistered.
-pub struct Registration<T: Driver>(ARef<drm::Device<T>>);
-
-impl<T: Driver> Registration<T> {
-    fn new(drm: drm::UnregisteredDevice<T>, flags: usize) -> Result<Self> {
-        // SAFETY: `drm.as_raw()` is valid by the invariants of `drm::Device`.
-        to_result(unsafe { bindings::drm_dev_register(drm.as_raw(), flags) })?;
-
-        // SAFETY: We just called `drm_dev_register` above
-        let new = NonNull::from(unsafe { drm.assume_ctx() });
-
-        // Leak the ARef from UnregisteredDevice in preparation for transferring its ownership.
-        mem::forget(drm);
-
-        // SAFETY: `drm`'s `Drop` constructor was never called, ensuring that there remains at least
-        // one reference to the device - which we take ownership over here.
-        let new = unsafe { ARef::from_raw(new) };
-
-        Ok(Self(new))
-    }
+pub struct Registration<'a, T: Driver> {
+    drm: ARef<drm::Device<T>>,
+    _reg_data: Pin<KBox<<T::RegistrationData as ForLt>::Of<'a>>>,
+}
 
-    /// Registers a new [`UnregisteredDevice`](drm::UnregisteredDevice) with userspace.
+impl<'a, T: Driver> Registration<'a, T>
+where
+    for<'b> <T::RegistrationData as ForLt>::Of<'b>: Send + Sync,
+{
+    /// Register a new [`UnregisteredDevice`](drm::UnregisteredDevice) with userspace.
     ///
-    /// Ownership of the [`Registration`] object is passed to [`devres::register`].
-    pub fn new_foreign_owned<'a>(
-        drm: drm::UnregisteredDevice<T>,
+    /// # Safety
+    ///
+    /// The caller must not `mem::forget()` the returned [`Registration`] or otherwise prevent its
+    /// [`Drop`] implementation from running, since the registration data may contain borrowed
+    /// references that become invalid after `'a` ends.
+    pub unsafe fn new<E>(
         dev: &'a device::Device<device::Bound>,
+        drm: drm::UnregisteredDevice<T>,
+        reg_data: impl PinInit<<T::RegistrationData as ForLt>::Of<'a>, E>,
         flags: usize,
-    ) -> Result<&'a drm::Device<T>>
+    ) -> Result<Self>
     where
-        T: 'static,
+        Error: From<E>,
     {
         if drm.as_ref().as_ref().as_raw() != dev.as_raw() {
             return Err(EINVAL);
         }
 
-        let reg = Registration::<T>::new(drm, flags)?;
-        let drm = NonNull::from(reg.device());
+        let reg_data: Pin<KBox<<T::RegistrationData as ForLt>::Of<'a>>> =
+            KBox::pin_init(reg_data, GFP_KERNEL)?;
 
-        devres::register(dev, reg, GFP_KERNEL)?;
+        // Store the registration data pointer in the device before registration, so that it is
+        // visible once ioctls can be called.
+        //
+        // SAFETY: Lifetimes do not affect layout, so the pointer cast is sound.
+        let ptr: NonNull<<T::RegistrationData as ForLt>::Of<'static>> =
+            unsafe { mem::transmute(NonNull::from(Pin::get_ref(reg_data.as_ref()))) };
+
+        // SAFETY: No concurrent access; the device is not yet registered.
+        unsafe { *drm.registration_data.get() = ptr };
+
+        // SAFETY: `drm` is a valid, initialized but not yet registered DRM device.
+        let ret = unsafe { bindings::drm_dev_register(drm.as_raw(), flags) };
+        if let Err(e) = to_result(ret) {
+            // SAFETY: No concurrent access; registration failed.
+            unsafe { *drm.registration_data.get() = NonNull::dangling() };
+            return Err(e);
+        }
 
-        // SAFETY: Since `reg` was passed to devres::register(), the device now owns the lifetime
-        // of the DRM registration - ensuring that this references lives for at least as long as 'a.
-        Ok(unsafe { drm.as_ref() })
+        Ok(Self {
+            drm: (&*drm).into(),
+            _reg_data: reg_data,
+        })
     }
 
     /// Returns a reference to the `Device` instance for this registration.
     pub fn device(&self) -> &drm::Device<T> {
-        &self.0
+        &self.drm
     }
 }
 
 // SAFETY: `Registration` doesn't offer any methods or access to fields when shared between
 // threads, hence it's safe to share it.
-unsafe impl<T: Driver> Sync for Registration<T> {}
+unsafe impl<T: Driver> Sync for Registration<'_, T> where
+    for<'a> <T::RegistrationData as ForLt>::Of<'a>: Send + Sync
+{
+}
 
 // SAFETY: Registration with and unregistration from the DRM subsystem can happen from any thread.
-unsafe impl<T: Driver> Send for Registration<T> {}
+unsafe impl<T: Driver> Send for Registration<'_, T> where
+    for<'a> <T::RegistrationData as ForLt>::Of<'a>: Send + Sync
+{
+}
 
-impl<T: Driver> Drop 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

@@ -207,6 +234,9 @@ fn drop(&mut self) {
         //
         // SAFETY: Safe by the invariant of `ARef<drm::Device<T>>`. The existence of this
         // `Registration` also guarantees that this `drm::Device` is actually registered.
-        unsafe { bindings::drm_dev_unplug(self.0.as_raw()) };
+        unsafe { bindings::drm_dev_unplug(self.drm.as_raw()) };
+        // After drm_dev_unplug(), the SRCU barrier guarantees that all RegistrationGuard critical
+        // sections have completed, so no one holds a reference to reg_data anymore.
+        // reg_data is dropped here automatically.
     }
 }