[PATCH] rust: drm: fix unsound initialization in drm::Device::new

[PATCH] rust: drm: fix unsound initialization in drm::Device::new

[Eliot Courtney]

If pinned initialization of drm::Device::Data fails, it calls
drm::Device::release via drm_dev_put. This materializes a reference to
&drm::Device, but it's not fully constructed yet, because initializing
`data` failed. It should not be dropped either. Instead, if pinned
initialization fails, make sure drm::Device::release isn't called.

Fixes: 2e9fdbe5ec7a ("rust: drm: device: drop_in_place() the drm::Device in release()")
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
 rust/kernel/drm/device.rs | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
index adbafe8db54d..78ea0eb12535 100644
--- a/rust/kernel/drm/device.rs
+++ b/rust/kernel/drm/device.rs
@@ -111,6 +111,13 @@ impl<T: drm::Driver> Device<T> {
         fops: &Self::GEM_FOPS,
     };
 
+    // Use a vtable without a `release` callback until `data` is initialized, so init failure
+    // can release the DRM device without dropping uninitialized fields.
+    const ALLOC_VTABLE: bindings::drm_driver = bindings::drm_driver {
+        release: None,
+        ..Self::VTABLE
+    };
+
     const GEM_FOPS: bindings::file_operations = drm::gem::create_fops();
 
     /// Create a new `drm::Device` for a `drm::Driver`.
@@ -120,12 +127,12 @@ pub fn new(dev: &device::Device, data: impl PinInit<T::Data, Error>) -> Result<A
         let layout = Kmalloc::aligned_layout(Layout::new::<Self>());
 
         // SAFETY:
-        // - `VTABLE`, as a `const` is pinned to the read-only section of the compilation,
+        // - `ALLOC_VTABLE`, as a `const` is pinned to the read-only section of the compilation,
         // - `dev` is valid by its type invarants,
         let raw_drm: *mut Self = unsafe {
             bindings::__drm_dev_alloc(
                 dev.as_raw(),
-                &Self::VTABLE,
+                &Self::ALLOC_VTABLE,
                 layout.size(),
                 mem::offset_of!(Self, dev),
             )
@@ -133,6 +140,10 @@ pub fn new(dev: &device::Device, data: impl PinInit<T::Data, Error>) -> Result<A
         .cast();
         let raw_drm = NonNull::new(from_err_ptr(raw_drm)?).ok_or(ENOMEM)?;
 
+        // SAFETY: `raw_drm` is a valid pointer to `Self`, given that `__drm_dev_alloc` was
+        // successful.
+        let drm_dev = unsafe { Self::into_drm_device(raw_drm) };
+
         // SAFETY: `raw_drm` is a valid pointer to `Self`.
         let raw_data = unsafe { ptr::addr_of_mut!((*raw_drm.as_ptr()).data) };
 
@@ -140,15 +151,14 @@ pub fn new(dev: &device::Device, data: impl PinInit<T::Data, Error>) -> Result<A
         // - `raw_data` is a valid pointer to uninitialized memory.
         // - `raw_data` will not move until it is dropped.
         unsafe { data.__pinned_init(raw_data) }.inspect_err(|_| {
-            // SAFETY: `raw_drm` is a valid pointer to `Self`, given that `__drm_dev_alloc` was
-            // successful.
-            let drm_dev = unsafe { Self::into_drm_device(raw_drm) };
-
             // SAFETY: `__drm_dev_alloc()` was successful, hence `drm_dev` must be valid and the
             // refcount must be non-zero.
             unsafe { bindings::drm_dev_put(drm_dev) };
         })?;
 
+        // SAFETY: `drm_dev` is still private to this function.
+        unsafe { (*drm_dev).driver = &Self::VTABLE };
+
         // SAFETY: The reference count is one, and now we take ownership of that reference as a
         // `drm::Device`.
         Ok(unsafe { ARef::from_raw(raw_drm) })

---
base-commit: d9a6809478f9815b6455a327aa001737ac7b2c09
change-id: 20260428-fix-drm-1-459b6357de23

Best regards,
--  
Eliot Courtney <ecourtney@nvidia.com>

Re: [PATCH] rust: drm: fix unsound initialization in drm::Device::new

[Danilo Krummrich]

On Tue Apr 28, 2026 at 2:20 PM CEST, Eliot Courtney wrote:
> If pinned initialization of drm::Device::Data fails, it calls
> drm::Device::release via drm_dev_put. This materializes a reference to
> &drm::Device, but it's not fully constructed yet, because initializing
> `data` failed. It should not be dropped either. Instead, if pinned
> initialization fails, make sure drm::Device::release isn't called.
>
> Fixes: 2e9fdbe5ec7a ("rust: drm: device: drop_in_place() the drm::Device in release()")
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>

There's already a patch from Lyude for this [1].

That said, I like the approach with the ALLOC_VTABLE.

@Lyude: Do you mind if we pick Eliot's patch?

Thanks,
Danilo

[1] https://lore.kernel.org/lkml/20260320233645.950190-2-lyude@redhat.com/

Re: [PATCH] rust: drm: fix unsound initialization in drm::Device::new

[Gary Guo]

On Tue Apr 28, 2026 at 1:43 PM BST, Danilo Krummrich wrote:
> On Tue Apr 28, 2026 at 2:20 PM CEST, Eliot Courtney wrote:
>> If pinned initialization of drm::Device::Data fails, it calls
>> drm::Device::release via drm_dev_put. This materializes a reference to
>> &drm::Device, but it's not fully constructed yet, because initializing
>> `data` failed. It should not be dropped either. Instead, if pinned
>> initialization fails, make sure drm::Device::release isn't called.
>>
>> Fixes: 2e9fdbe5ec7a ("rust: drm: device: drop_in_place() the drm::Device in release()")
>> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
>
> There's already a patch from Lyude for this [1].
>
> That said, I like the approach with the ALLOC_VTABLE.
>
> @Lyude: Do you mind if we pick Eliot's patch?
>
> Thanks,
> Danilo
>
> [1] https://lore.kernel.org/lkml/20260320233645.950190-2-lyude@redhat.com/

I have to second this and I think this solution is very clean. It does mean
that we're always duplicating vtable though, for just one pointer of difference.

Is it possible to have a shared vtable for drm devices that's in the
allocated-but-not-initialized state?

Best,
Gary

Re: [PATCH] rust: drm: fix unsound initialization in drm::Device::new

[Eliot Courtney]

On Tue Apr 28, 2026 at 9:50 PM JST, Gary Guo wrote:
> On Tue Apr 28, 2026 at 1:43 PM BST, Danilo Krummrich wrote:
>> On Tue Apr 28, 2026 at 2:20 PM CEST, Eliot Courtney wrote:
>>> If pinned initialization of drm::Device::Data fails, it calls
>>> drm::Device::release via drm_dev_put. This materializes a reference to
>>> &drm::Device, but it's not fully constructed yet, because initializing
>>> `data` failed. It should not be dropped either. Instead, if pinned
>>> initialization fails, make sure drm::Device::release isn't called.
>>>
>>> Fixes: 2e9fdbe5ec7a ("rust: drm: device: drop_in_place() the drm::Device in release()")
>>> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
>>
>> There's already a patch from Lyude for this [1].
>>
>> That said, I like the approach with the ALLOC_VTABLE.
>>
>> @Lyude: Do you mind if we pick Eliot's patch?

Sorry, I should have checked before sending this!

>>
>> Thanks,
>> Danilo
>>
>> [1] https://lore.kernel.org/lkml/20260320233645.950190-2-lyude@redhat.com/
>
> I have to second this and I think this solution is very clean. It does mean
> that we're always duplicating vtable though, for just one pointer of difference.
>
> Is it possible to have a shared vtable for drm devices that's in the
> allocated-but-not-initialized state?

It looks like __drm_dev_alloc reads `driver_features` from `drm_driver`
so we'd have to have multiple shared ones if we ever have anything other
than FEAT_GEM for that. And I guess it relies on nothing ever changing
to read more from `drm_driver` during alloc. So I feel it's a bit
fragile, personally.

>
> Best,
> Gary

Re: [PATCH] rust: drm: fix unsound initialization in drm::Device::new

[Alice Ryhl]

On Tue, Apr 28, 2026 at 09:20:21PM +0900, Eliot Courtney wrote:
> If pinned initialization of drm::Device::Data fails, it calls
> drm::Device::release via drm_dev_put. This materializes a reference to
> &drm::Device, but it's not fully constructed yet, because initializing
> `data` failed. It should not be dropped either. Instead, if pinned
> initialization fails, make sure drm::Device::release isn't called.
> 
> Fixes: 2e9fdbe5ec7a ("rust: drm: device: drop_in_place() the drm::Device in release()")
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>

For the concerns about duplicating vtables, could the temporary vtable
be a stack variable?

>  rust/kernel/drm/device.rs | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
> index adbafe8db54d..78ea0eb12535 100644
> --- a/rust/kernel/drm/device.rs
> +++ b/rust/kernel/drm/device.rs
> @@ -111,6 +111,13 @@ impl<T: drm::Driver> Device<T> {
>          fops: &Self::GEM_FOPS,
>      };
>  
> +    // Use a vtable without a `release` callback until `data` is initialized, so init failure
> +    // can release the DRM device without dropping uninitialized fields.
> +    const ALLOC_VTABLE: bindings::drm_driver = bindings::drm_driver {
> +        release: None,
> +        ..Self::VTABLE
> +    };
> +
>      const GEM_FOPS: bindings::file_operations = drm::gem::create_fops();
>  
>      /// Create a new `drm::Device` for a `drm::Driver`.
> @@ -120,12 +127,12 @@ pub fn new(dev: &device::Device, data: impl PinInit<T::Data, Error>) -> Result<A
>          let layout = Kmalloc::aligned_layout(Layout::new::<Self>());
>  
>          // SAFETY:
> -        // - `VTABLE`, as a `const` is pinned to the read-only section of the compilation,
> +        // - `ALLOC_VTABLE`, as a `const` is pinned to the read-only section of the compilation,
>          // - `dev` is valid by its type invarants,
>          let raw_drm: *mut Self = unsafe {
>              bindings::__drm_dev_alloc(
>                  dev.as_raw(),
> -                &Self::VTABLE,
> +                &Self::ALLOC_VTABLE,
>                  layout.size(),
>                  mem::offset_of!(Self, dev),
>              )
> @@ -133,6 +140,10 @@ pub fn new(dev: &device::Device, data: impl PinInit<T::Data, Error>) -> Result<A
>          .cast();
>          let raw_drm = NonNull::new(from_err_ptr(raw_drm)?).ok_or(ENOMEM)?;
>  
> +        // SAFETY: `raw_drm` is a valid pointer to `Self`, given that `__drm_dev_alloc` was
> +        // successful.
> +        let drm_dev = unsafe { Self::into_drm_device(raw_drm) };
> +
>          // SAFETY: `raw_drm` is a valid pointer to `Self`.
>          let raw_data = unsafe { ptr::addr_of_mut!((*raw_drm.as_ptr()).data) };
>  
> @@ -140,15 +151,14 @@ pub fn new(dev: &device::Device, data: impl PinInit<T::Data, Error>) -> Result<A
>          // - `raw_data` is a valid pointer to uninitialized memory.
>          // - `raw_data` will not move until it is dropped.
>          unsafe { data.__pinned_init(raw_data) }.inspect_err(|_| {
> -            // SAFETY: `raw_drm` is a valid pointer to `Self`, given that `__drm_dev_alloc` was
> -            // successful.
> -            let drm_dev = unsafe { Self::into_drm_device(raw_drm) };
> -
>              // SAFETY: `__drm_dev_alloc()` was successful, hence `drm_dev` must be valid and the
>              // refcount must be non-zero.
>              unsafe { bindings::drm_dev_put(drm_dev) };
>          })?;
>  
> +        // SAFETY: `drm_dev` is still private to this function.
> +        unsafe { (*drm_dev).driver = &Self::VTABLE };

It would be bad if this ended up being a reference to a local variable.
Please use `&const { Self::VTABLE }` so that it doesn't compile if this
occurs.

Alice

Re: [PATCH] rust: drm: fix unsound initialization in drm::Device::new

[Gary Guo]

On Wed Apr 29, 2026 at 9:03 AM BST, Alice Ryhl wrote:
>
>> @@ -133,6 +140,10 @@ pub fn new(dev: &device::Device, data: impl PinInit<T::Data, Error>) -> Result<A
>>          .cast();
>>          let raw_drm = NonNull::new(from_err_ptr(raw_drm)?).ok_or(ENOMEM)?;
>>  
>> +        // SAFETY: `raw_drm` is a valid pointer to `Self`, given that `__drm_dev_alloc` was
>> +        // successful.
>> +        let drm_dev = unsafe { Self::into_drm_device(raw_drm) };
>> +
>>          // SAFETY: `raw_drm` is a valid pointer to `Self`.
>>          let raw_data = unsafe { ptr::addr_of_mut!((*raw_drm.as_ptr()).data) };
>>  
>> @@ -140,15 +151,14 @@ pub fn new(dev: &device::Device, data: impl PinInit<T::Data, Error>) -> Result<A
>>          // - `raw_data` is a valid pointer to uninitialized memory.
>>          // - `raw_data` will not move until it is dropped.
>>          unsafe { data.__pinned_init(raw_data) }.inspect_err(|_| {
>> -            // SAFETY: `raw_drm` is a valid pointer to `Self`, given that `__drm_dev_alloc` was
>> -            // successful.
>> -            let drm_dev = unsafe { Self::into_drm_device(raw_drm) };
>> -
>>              // SAFETY: `__drm_dev_alloc()` was successful, hence `drm_dev` must be valid and the
>>              // refcount must be non-zero.
>>              unsafe { bindings::drm_dev_put(drm_dev) };
>>          })?;
>>  
>> +        // SAFETY: `drm_dev` is still private to this function.
>> +        unsafe { (*drm_dev).driver = &Self::VTABLE };
>
> It would be bad if this ended up being a reference to a local variable.
> Please use `&const { Self::VTABLE }` so that it doesn't compile if this
> occurs.

Self::VTABLE and `const {}` are both just constants and there's no difference
here.

If you want to guaranteed static promotion it should be

    const { &Self::VTABLE }

Best,
Gary

Re: [PATCH] rust: drm: fix unsound initialization in drm::Device::new

[Eliot Courtney]

On Wed Apr 29, 2026 at 8:34 PM JST, Gary Guo wrote:
> On Wed Apr 29, 2026 at 9:03 AM BST, Alice Ryhl wrote:
>>
>>> @@ -133,6 +140,10 @@ pub fn new(dev: &device::Device, data: impl PinInit<T::Data, Error>) -> Result<A
>>>          .cast();
>>>          let raw_drm = NonNull::new(from_err_ptr(raw_drm)?).ok_or(ENOMEM)?;
>>>  
>>> +        // SAFETY: `raw_drm` is a valid pointer to `Self`, given that `__drm_dev_alloc` was
>>> +        // successful.
>>> +        let drm_dev = unsafe { Self::into_drm_device(raw_drm) };
>>> +
>>>          // SAFETY: `raw_drm` is a valid pointer to `Self`.
>>>          let raw_data = unsafe { ptr::addr_of_mut!((*raw_drm.as_ptr()).data) };
>>>  
>>> @@ -140,15 +151,14 @@ pub fn new(dev: &device::Device, data: impl PinInit<T::Data, Error>) -> Result<A
>>>          // - `raw_data` is a valid pointer to uninitialized memory.
>>>          // - `raw_data` will not move until it is dropped.
>>>          unsafe { data.__pinned_init(raw_data) }.inspect_err(|_| {
>>> -            // SAFETY: `raw_drm` is a valid pointer to `Self`, given that `__drm_dev_alloc` was
>>> -            // successful.
>>> -            let drm_dev = unsafe { Self::into_drm_device(raw_drm) };
>>> -
>>>              // SAFETY: `__drm_dev_alloc()` was successful, hence `drm_dev` must be valid and the
>>>              // refcount must be non-zero.
>>>              unsafe { bindings::drm_dev_put(drm_dev) };
>>>          })?;
>>>  
>>> +        // SAFETY: `drm_dev` is still private to this function.
>>> +        unsafe { (*drm_dev).driver = &Self::VTABLE };
>>
>> It would be bad if this ended up being a reference to a local variable.
>> Please use `&const { Self::VTABLE }` so that it doesn't compile if this
>> occurs.
>
> Self::VTABLE and `const {}` are both just constants and there's no difference
> here.
>
> If you want to guaranteed static promotion it should be
>
>     const { &Self::VTABLE }
>
> Best,
> Gary

Thanks all, I have done both of these things (const{&} + stack alloc).
The `drm_driver` struct is 200 bytes, for reference (w.r.t. stack
alloc).