drm::Device is allocated through __drm_dev_alloc() (which uses
kmalloc()) and the driver private data, <T as drm::Driver>::Data, is
initialized in-place.

Due to the order of fields in drm::Device

  pub struct Device<T: drm::Driver> {
     dev: Opaque<bindings::drm_device>,
     data: T::Data,
  }

even with an arbitrary large alignment requirement of T::Data it can't
happen that the size of Device is smaller than its alignment requirement.

However, let's not rely on this subtle circumstance and create a proper
kmalloc() compatible Layout.

Fixes: 1e4b8896c0f3 ("rust: drm: add device abstraction")
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/drm/device.rs | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
index 3bb7c83966cf..d19410deaf6c 100644
--- a/rust/kernel/drm/device.rs
+++ b/rust/kernel/drm/device.rs
@@ -5,6 +5,7 @@
 //! C header: [`include/linux/drm/drm_device.h`](srctree/include/linux/drm/drm_device.h)
 
 use crate::{
+    alloc::allocator::Kmalloc,
     bindings, device, drm,
     drm::driver::AllocImpl,
     error::from_err_ptr,
@@ -12,7 +13,7 @@
     prelude::*,
     types::{ARef, AlwaysRefCounted, Opaque},
 };
-use core::{mem, ops::Deref, ptr, ptr::NonNull};
+use core::{alloc::Layout, mem, ops::Deref, ptr, ptr::NonNull};
 
 #[cfg(CONFIG_DRM_LEGACY)]
 macro_rules! drm_legacy_fields {
@@ -96,6 +97,10 @@ impl<T: drm::Driver> Device<T> {
 
     /// Create a new `drm::Device` for a `drm::Driver`.
     pub fn new(dev: &device::Device, data: impl PinInit<T::Data, Error>) -> Result<ARef<Self>> {
+        // `__drm_dev_alloc` uses `kmalloc()` to allocate memory, hence ensure a `kmalloc()`
+        // compatible `Layout`.
+        let layout = Kmalloc::aligned_layout(Layout::new::<Self>());
+
         // SAFETY:
         // - `VTABLE`, as a `const` is pinned to the read-only section of the compilation,
         // - `dev` is valid by its type invarants,
@@ -103,7 +108,7 @@ pub fn new(dev: &device::Device, data: impl PinInit<T::Data, Error>) -> Result<A
             bindings::__drm_dev_alloc(
                 dev.as_raw(),
                 &Self::VTABLE,
-                mem::size_of::<Self>(),
+                layout.size(),
                 mem::offset_of!(Self, dev),
             )
         }
-- 
2.50.0

On Thu, Jul 31, 2025 at 05:48:07PM +0200, Danilo Krummrich wrote:
> drm::Device is allocated through __drm_dev_alloc() (which uses
> kmalloc()) and the driver private data, <T as drm::Driver>::Data, is
> initialized in-place.
> 
> Due to the order of fields in drm::Device
> 
>   pub struct Device<T: drm::Driver> {
>      dev: Opaque<bindings::drm_device>,
>      data: T::Data,
>   }

I'm not convinced this patch is right.

Imagine this scenario: T::Data has size and alignment both equal to 16,
and lets say that drm_device has a size that is a multiple of 8 but not
16 such as 72. In that case, you will allocate 72+16=88 bytes for
Device, but actually the size of Device is 96 because there is 8 bytes
of padding between dev and data.

Alice

> even with an arbitrary large alignment requirement of T::Data it can't
> happen that the size of Device is smaller than its alignment requirement.
> 
> However, let's not rely on this subtle circumstance and create a proper
> kmalloc() compatible Layout.
> 
> Fixes: 1e4b8896c0f3 ("rust: drm: add device abstraction")
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  rust/kernel/drm/device.rs | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
> index 3bb7c83966cf..d19410deaf6c 100644
> --- a/rust/kernel/drm/device.rs
> +++ b/rust/kernel/drm/device.rs
> @@ -5,6 +5,7 @@
>  //! C header: [`include/linux/drm/drm_device.h`](srctree/include/linux/drm/drm_device.h)
>  
>  use crate::{
> +    alloc::allocator::Kmalloc,
>      bindings, device, drm,
>      drm::driver::AllocImpl,
>      error::from_err_ptr,
> @@ -12,7 +13,7 @@
>      prelude::*,
>      types::{ARef, AlwaysRefCounted, Opaque},
>  };
> -use core::{mem, ops::Deref, ptr, ptr::NonNull};
> +use core::{alloc::Layout, mem, ops::Deref, ptr, ptr::NonNull};
>  
>  #[cfg(CONFIG_DRM_LEGACY)]
>  macro_rules! drm_legacy_fields {
> @@ -96,6 +97,10 @@ impl<T: drm::Driver> Device<T> {
>  
>      /// Create a new `drm::Device` for a `drm::Driver`.
>      pub fn new(dev: &device::Device, data: impl PinInit<T::Data, Error>) -> Result<ARef<Self>> {
> +        // `__drm_dev_alloc` uses `kmalloc()` to allocate memory, hence ensure a `kmalloc()`
> +        // compatible `Layout`.
> +        let layout = Kmalloc::aligned_layout(Layout::new::<Self>());
> +
>          // SAFETY:
>          // - `VTABLE`, as a `const` is pinned to the read-only section of the compilation,
>          // - `dev` is valid by its type invarants,
> @@ -103,7 +108,7 @@ pub fn new(dev: &device::Device, data: impl PinInit<T::Data, Error>) -> Result<A
>              bindings::__drm_dev_alloc(
>                  dev.as_raw(),
>                  &Self::VTABLE,
> -                mem::size_of::<Self>(),
> +                layout.size(),
>                  mem::offset_of!(Self, dev),
>              )
>          }
> -- 
> 2.50.0
>

On Fri Aug 1, 2025 at 11:18 AM CEST, Alice Ryhl wrote:
> On Thu, Jul 31, 2025 at 05:48:07PM +0200, Danilo Krummrich wrote:
>> drm::Device is allocated through __drm_dev_alloc() (which uses
>> kmalloc()) and the driver private data, <T as drm::Driver>::Data, is
>> initialized in-place.
>> 
>> Due to the order of fields in drm::Device
>> 
>>   pub struct Device<T: drm::Driver> {
>>      dev: Opaque<bindings::drm_device>,
>>      data: T::Data,
>>   }
>
> I'm not convinced this patch is right.
>
> Imagine this scenario: T::Data has size and alignment both equal to 16,
> and lets say that drm_device has a size that is a multiple of 8 but not
> 16 such as 72. In that case, you will allocate 72+16=88 bytes for
> Device, but actually the size of Device is 96 because there is 8 bytes
> of padding between dev and data.

Are you saying that there is an issue with

  (1) the existing implementation with uses mem::size_of::<Self>() or

  (2) the proper one that uses Kmalloc::aligned_layout(Layout::new::<Self>())?

I think neither has, because we're not allocating
size_of::<Opaque<bindings::drm_device>>() + size_of::<T::Data>() as you seem to
assume above, but size_of::<Device<T>>().

>> even with an arbitrary large alignment requirement of T::Data it can't
>> happen that the size of Device is smaller than its alignment requirement.
>> 
>> However, let's not rely on this subtle circumstance and create a proper
>> kmalloc() compatible Layout.
>> 
>> Fixes: 1e4b8896c0f3 ("rust: drm: add device abstraction")
>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>> ---
>>  rust/kernel/drm/device.rs | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>> 
>> diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs
>> index 3bb7c83966cf..d19410deaf6c 100644
>> --- a/rust/kernel/drm/device.rs
>> +++ b/rust/kernel/drm/device.rs
>> @@ -5,6 +5,7 @@
>>  //! C header: [`include/linux/drm/drm_device.h`](srctree/include/linux/drm/drm_device.h)
>>  
>>  use crate::{
>> +    alloc::allocator::Kmalloc,
>>      bindings, device, drm,
>>      drm::driver::AllocImpl,
>>      error::from_err_ptr,
>> @@ -12,7 +13,7 @@
>>      prelude::*,
>>      types::{ARef, AlwaysRefCounted, Opaque},
>>  };
>> -use core::{mem, ops::Deref, ptr, ptr::NonNull};
>> +use core::{alloc::Layout, mem, ops::Deref, ptr, ptr::NonNull};
>>  
>>  #[cfg(CONFIG_DRM_LEGACY)]
>>  macro_rules! drm_legacy_fields {
>> @@ -96,6 +97,10 @@ impl<T: drm::Driver> Device<T> {
>>  
>>      /// Create a new `drm::Device` for a `drm::Driver`.
>>      pub fn new(dev: &device::Device, data: impl PinInit<T::Data, Error>) -> Result<ARef<Self>> {
>> +        // `__drm_dev_alloc` uses `kmalloc()` to allocate memory, hence ensure a `kmalloc()`
>> +        // compatible `Layout`.
>> +        let layout = Kmalloc::aligned_layout(Layout::new::<Self>());
>> +
>>          // SAFETY:
>>          // - `VTABLE`, as a `const` is pinned to the read-only section of the compilation,
>>          // - `dev` is valid by its type invarants,
>> @@ -103,7 +108,7 @@ pub fn new(dev: &device::Device, data: impl PinInit<T::Data, Error>) -> Result<A
>>              bindings::__drm_dev_alloc(
>>                  dev.as_raw(),
>>                  &Self::VTABLE,
>> -                mem::size_of::<Self>(),
>> +                layout.size(),
>>                  mem::offset_of!(Self, dev),
>>              )
>>          }
>> -- 
>> 2.50.0
>> 

On Fri, Aug 1, 2025 at 11:29 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Fri Aug 1, 2025 at 11:18 AM CEST, Alice Ryhl wrote:
> > On Thu, Jul 31, 2025 at 05:48:07PM +0200, Danilo Krummrich wrote:
> >> drm::Device is allocated through __drm_dev_alloc() (which uses
> >> kmalloc()) and the driver private data, <T as drm::Driver>::Data, is
> >> initialized in-place.
> >>
> >> Due to the order of fields in drm::Device
> >>
> >>   pub struct Device<T: drm::Driver> {
> >>      dev: Opaque<bindings::drm_device>,
> >>      data: T::Data,
> >>   }
> >
> > I'm not convinced this patch is right.
> >
> > Imagine this scenario: T::Data has size and alignment both equal to 16,
> > and lets say that drm_device has a size that is a multiple of 8 but not
> > 16 such as 72. In that case, you will allocate 72+16=88 bytes for
> > Device, but actually the size of Device is 96 because there is 8 bytes
> > of padding between dev and data.
>
> Are you saying that there is an issue with
>
>   (1) the existing implementation with uses mem::size_of::<Self>() or
>
>   (2) the proper one that uses Kmalloc::aligned_layout(Layout::new::<Self>())?
>
> I think neither has, because we're not allocating
> size_of::<Opaque<bindings::drm_device>>() + size_of::<T::Data>() as you seem to
> assume above, but size_of::<Device<T>>().

Yes, you're right. I misunderstood how __drm_dev_alloc works.

Alice

On Thu, Jul 31, 2025 at 5:49 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> drm::Device is allocated through __drm_dev_alloc() (which uses
> kmalloc()) and the driver private data, <T as drm::Driver>::Data, is
> initialized in-place.
>
> Due to the order of fields in drm::Device
>
>   pub struct Device<T: drm::Driver> {
>      dev: Opaque<bindings::drm_device>,
>      data: T::Data,
>   }
>
> even with an arbitrary large alignment requirement of T::Data it can't
> happen that the size of Device is smaller than its alignment requirement.
>
> However, let's not rely on this subtle circumstance and create a proper
> kmalloc() compatible Layout.
>
> Fixes: 1e4b8896c0f3 ("rust: drm: add device abstraction")
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>