[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <DBQZHIS4VQBN.WSKBML2WYQE@kernel.org>
Date: Fri, 01 Aug 2025 11:29:23 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Alice Ryhl" <aliceryhl@...gle.com>
Cc: <lorenzo.stoakes@...cle.com>, <vbabka@...e.cz>,
<Liam.Howlett@...cle.com>, <urezki@...il.com>, <ojeda@...nel.org>,
<alex.gaynor@...il.com>, <boqun.feng@...il.com>, <gary@...yguo.net>,
<bjorn3_gh@...tonmail.com>, <lossin@...nel.org>, <a.hindborg@...nel.org>,
<tmgross@...ch.edu>, <maarten.lankhorst@...ux.intel.com>,
<mripard@...nel.org>, <tzimmermann@...e.de>, <airlied@...il.com>,
<simona@...ll.ch>, <rust-for-linux@...r.kernel.org>,
<dri-devel@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/4] rust: drm: ensure kmalloc() compatible Layout
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@...nel.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
>>
Powered by blists - more mailing lists