Alloc and drm::Device fixes

aligned_size() dates back to when Rust did support kmalloc() only, but is now used in ReallocFunc::call() and hence for all allocators.

However, the additional padding applied by aligned_size() is only required by the kmalloc() allocator backend.

Hence, replace aligned_size() with Kmalloc::aligned_layout() and use it for the affected allocators, i.e. kmalloc() and kvmalloc(), only.

While at it, make Kmalloc::aligned_layout() public, such that Rust abstractions, which have to call subsystem specific kmalloc() based allocation primitives directly, can make use of it.

Fixes 8a799831fc63 ("rust: alloc: implement `ReallocFunc`")

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

aligned_size() dates back to when Rust did support kmalloc() only, but is now used in ReallocFunc::call() and hence for all allocators.

However, the additional padding applied by aligned_size() is only required by the kmalloc() allocator backend.

Hence, replace aligned_size() with Kmalloc::aligned_layout() and use it for the affected allocators, i.e. kmalloc() and kvmalloc(), only.

While at it, make Kmalloc::aligned_layout() public, such that Rust abstractions, which have to call subsystem specific kmalloc() based allocation primitives directly, can make use of it.

Fixes 8a799831fc63 ("rust: alloc: implement `ReallocFunc`")

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

Did this need Cc: stable or was it skipped since it is just extra padding?

I don't think so, it just lead to pad to the alignment for Vmalloc too.

Technically, this makes no difference, since Vmalloc is always PAGE_SIZE aligned and the size always a multiple of PAGE_SIZE.

Quoted 5 lines · Danilo Krummrich

I don't think so, it just lead to pad to the alignment for Vmalloc too.

Technically, this makes no difference, since Vmalloc is always PAGE_SIZE aligned and the size always a multiple of PAGE_SIZE.

Got it, thanks for the quick reply! Then I guess we could have skipped the Fixes in this one, but it is not a big deal and as usual it depends on how one defines "fix".

Review

Quoted 5 lines · Danilo Krummrich

(quoted diff) +4 -0

+impl Kmalloc {
+    /// Returns a [`Layout`] that makes [`Kmalloc`] fulfill the requested size and alignment of
+    /// `layout`.
+    pub const fn aligned_layout(layout: Layout) -> Layout {

I think this const fn here was removed when applying to make it work with older compilers, right?

I was fixing another rusttest thing and noticed while applying these. I had a patch to fix it, since we can actually just use the feature, and then I noticed it wasn't in the tree. Since I have it, I am attaching it for reference in case the now-stable feature is needed, e.g. if you want to make that const fn again.

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 <[email protected]>

---
 rust/kernel/drm/device.rs | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

rust/kernel/drm/device.rs +7 -2

index 3bb7c83966cf..d19410deaf6c 100644
@@ -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),
             )
         }

Signature · 1 line

2.50.0

Quoted 10 lines · Danilo Krummrich

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.

Quoted 56 lines

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 <[email protected]>

---
 rust/kernel/drm/device.rs | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

rust/kernel/drm/device.rs +7 -2

index 3bb7c83966cf..d19410deaf6c 100644
@@ -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),
             )
         }

Signature · 1 line

2.50.0

Quoted 12 lines · Alice Ryhl

Quoted 10 lines · Danilo Krummrich

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

Quoted 7 lines

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>>().

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 <[email protected]>

The #[pin_data] and #[pin] annotations are not necessary for drm::Device, since we don't use any pin-init macros, but only __pinned_init() on the impl PinInit<T::Data, Error> argument of drm::Device::new().

Fixes 1e4b8896c0f3 ("rust: drm: add device abstraction")

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

---
 rust/kernel/drm/device.rs | 2 --
 1 file changed, 2 deletions(-)

rust/kernel/drm/device.rs +0 -0

index d19410deaf6c..d0a9528121f1 100644

rust/kernel/drm/device.rs +0 -0

 }
 

Quoted 4 lines · Danilo Krummrich

The #[pin_data] and #[pin] annotations are not necessary for drm::Device, since we don't use any pin-init macros, but only __pinned_init() on the impl PinInit<T::Data, Error> argument of drm::Device::new().

But you're still pinning Device, right?

Quoted 15 lines

Fixes 1e4b8896c0f3 ("rust: drm: add device abstraction")

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

---
 rust/kernel/drm/device.rs | 2 --
 1 file changed, 2 deletions(-)

rust/kernel/drm/device.rs +0 -1

index d19410deaf6c..d0a9528121f1 100644
@@ -54,10 +54,8 @@ macro_rules! drm_legacy_fields {
 ///
 /// `self.dev` is a valid instance of a `struct device`.
 #[repr(C)]
-#[pin_data]

rust/kernel/drm/device.rs macro_rules! drm_legacy_fields { · line 59

Quoted 4 lines

rust/kernel/drm/device.rs +0 -1

 pub struct Device<T: drm::Driver> {
     dev: Opaque<bindings::drm_device>,
-    #[pin]
     data: T::Data,

Looking at this code again, I also noticed that it was wrong before this
patch: Device<T> implemented Unpin if T::Data did which is most likely wrong (or is drm_device not address sensitive?).

So good to see that fixed, thanks!

---
Cheers,
Benno

Quoted 2 lines

rust/kernel/drm/device.rs +0 -0

 }
 

Quoted 8 lines · Benno Lossin

Quoted 6 lines · Danilo Krummrich

#[repr(C)]

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

-#[pin_data]
 pub struct Device<T: drm::Driver> {
     dev: Opaque<bindings::drm_device>,
-    #[pin]
     data: T::Data,

Quoted 3 lines

Looking at this code again, I also noticed that it was wrong before this
patch: Device<T> implemented Unpin if T::Data did which is most likely wrong (or is drm_device not address sensitive?).

It is, but as mentioned above a drm::Device only ever exists as ARef<drm::Device>.

The #[pin_data] and #[pin] annotations are not necessary for drm::Device, since we don't use any pin-init macros, but only __pinned_init() on the impl PinInit<T::Data, Error> argument of drm::Device::new().

Fixes 1e4b8896c0f3 ("rust: drm: add device abstraction")

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

---
 rust/kernel/drm/device.rs | 2 --
 1 file changed, 2 deletions(-)

In drm_dev_put() call in AlwaysRefCounted::dec_ref() we rely on struct drm_device to be the first field in drm::Device, whereas everywhere else we correctly obtain the address of the actual struct drm_device.

Analogous to the from_drm_device() helper, provide the into_drm_device() helper in order to address this.

Fixes 1e4b8896c0f3 ("rust: drm: add device abstraction")

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

This patch series replaces aligned_size() with Kmalloc::aligned_layout(), which is subsequently used to obtain a kmalloc() compatible Layout for __drm_dev_alloc() in drm::Device::new().

It also contains two additional drm::Device fixes; the commits are based on next-20250731.