From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id DE1E81EFF9A for ; Thu, 26 Jun 2025 16:25:47 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1750955150; cv=none; b=k9XknvFfxML65Fz6JDV9PH6Y/R3jpyPgZkfsFU2xsBZ2/i7zbU4bdP4osKu4eVyUZOdpSDDI47QgloFHFWKTVu0LjITenRYrpHthr6YCNCh6c8ib67iv4GQ4UKxy1dnIuxN+QrQjDo2nRjv5CkXwPNtVkgW1wckY+3MuHjdnLZA= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1750955150; c=relaxed/simple; bh=QCuehcRv8tJJghnp0+pxFTHFznIyj3PBn/exec45LSI=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=SiMAuzP6X7mBcLzyKcP+PX3AXPTjWBkz+aOlpTNH5SkRTr8NiwvSnBxQRQExEHcfqujisSZPMHuNF53ycOKtJzon3xU9lWQPeJgxB5T5SL44rfZDecQAh+2QMmuZU2TTwkJ9/P83KNaXVxpGqvXbtKmoE6eS6f3CeAuHtFKRgOQ= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 8844826B9; Thu, 26 Jun 2025 09:25:29 -0700 (PDT) Received: from arm.com (usa-sjc-imap-foss1.foss.arm.com [10.121.207.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id C3B0B3F66E; Thu, 26 Jun 2025 09:25:42 -0700 (PDT) Date: Thu, 26 Jun 2025 18:25:34 +0200 From: Beata Michalska To: Boqun Feng Cc: ojeda@kernel.org, alex.gaynor@gmail.com, dakr@kernel.org, aliceryhl@google.com, daniel.almeida@collabora.com, gary@garyguo.net, bjorn3_gh@protonmail.com, lossin@kernel.org, a.hindborg@kernel.org, tmgross@umich.edu, alyssa@rosenzweig.io, lyude@redhat.com, rust-for-linux@vger.kernel.org, dri-devel@lists.freedesktop.org Subject: Re: [PATCH v4] rust: drm: Drop the use of Opaque for ioctl arguments Message-ID: References: <20250625081333.2217887-1-beata.michalska@arm.com> Precedence: bulk X-Mailing-List: rust-for-linux@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: On Wed, Jun 25, 2025 at 08:42:35PM -0700, Boqun Feng wrote: Hi, > Hi Beata, > > On Wed, Jun 25, 2025 at 10:13:33AM +0200, Beata Michalska wrote: > > With the Opaque, the expectations are that Rust should not > > make any assumptions on the layout or invariants of the wrapped > > C types. That runs rather counter to ioctl arguments, which must > > adhere to certain data-layout constraits. By using Opaque, > > ioctl handlers are forced to use unsafe code where none is acually > > needed. This adds needless complexity and maintenance overhead, > > brining no safety benefits. > > Drop the use of Opaque for ioctl arguments as that is not the best > > fit here. > > > > Signed-off-by: Beata Michalska > > Acked-by: Danilo Krummrich > > --- > > Appreciate it if you could put a change log here mentioning changes > between each version. Done > > > drivers/gpu/drm/nova/file.rs | 23 ++++++-------- > > drivers/gpu/drm/nova/nova.rs | 1 - > > drivers/gpu/drm/nova/uapi.rs | 61 ------------------------------------ > > rust/kernel/drm/ioctl.rs | 11 ++++--- > > 4 files changed, 16 insertions(+), 80 deletions(-) > > delete mode 100644 drivers/gpu/drm/nova/uapi.rs > > > > diff --git a/drivers/gpu/drm/nova/file.rs b/drivers/gpu/drm/nova/file.rs > > index 7e59a34b830d..7e7d4e2de2fb 100644 > > --- a/drivers/gpu/drm/nova/file.rs > > +++ b/drivers/gpu/drm/nova/file.rs > > @@ -2,13 +2,11 @@ > > > > use crate::driver::{NovaDevice, NovaDriver}; > > use crate::gem::NovaObject; > > -use crate::uapi::{GemCreate, GemInfo, Getparam}; > > use kernel::{ > > alloc::flags::*, > > drm::{self, gem::BaseObject}, > > pci, > > prelude::*, > > - types::Opaque, > > uapi, > > }; > > > > @@ -26,20 +24,19 @@ impl File { > > /// IOCTL: get_param: Query GPU / driver metadata. > > pub(crate) fn get_param( > > dev: &NovaDevice, > > - getparam: &Opaque, > > + getparam: &mut uapi::drm_nova_getparam, > > _file: &drm::File, > > ) -> Result { > > let adev = &dev.adev; > > let parent = adev.parent().ok_or(ENOENT)?; > > let pdev: &pci::Device = parent.try_into()?; > > - let getparam: &Getparam = getparam.into(); > > > > - let value = match getparam.param() as u32 { > > + let value = match getparam.param as u32 { > > uapi::NOVA_GETPARAM_VRAM_BAR_SIZE => pdev.resource_len(1)?, > > _ => return Err(EINVAL), > > }; > > > > - getparam.set_value(value); > > + getparam.value = value; > > > > Ok(0) > > } > > @@ -47,13 +44,12 @@ pub(crate) fn get_param( > > /// IOCTL: gem_create: Create a new DRM GEM object. > > pub(crate) fn gem_create( > > dev: &NovaDevice, > > - req: &Opaque, > > + req: &mut uapi::drm_nova_gem_create, > > file: &drm::File, > > ) -> Result { > > - let req: &GemCreate = req.into(); > > - let obj = NovaObject::new(dev, req.size().try_into()?)?; > > + let obj = NovaObject::new(dev, req.size.try_into()?)?; > > > > - req.set_handle(obj.create_handle(file)?); > > + req.handle = obj.create_handle(file)?; > > > > Ok(0) > > } > > @@ -61,13 +57,12 @@ pub(crate) fn gem_create( > > /// IOCTL: gem_info: Query GEM metadata. > > pub(crate) fn gem_info( > > _dev: &NovaDevice, > > - req: &Opaque, > > + req: &mut uapi::drm_nova_gem_info, > > file: &drm::File, > > ) -> Result { > > - let req: &GemInfo = req.into(); > > - let bo = NovaObject::lookup_handle(file, req.handle())?; > > + let bo = NovaObject::lookup_handle(file, req.handle)?; > > > > - req.set_size(bo.size().try_into()?); > > + req.size = bo.size().try_into()?; > > > > Ok(0) > > } > > diff --git a/drivers/gpu/drm/nova/nova.rs b/drivers/gpu/drm/nova/nova.rs > > index 902876aa14d1..730598defe04 100644 > > --- a/drivers/gpu/drm/nova/nova.rs > > +++ b/drivers/gpu/drm/nova/nova.rs > > @@ -5,7 +5,6 @@ > > mod driver; > > mod file; > > mod gem; > > -mod uapi; > > > > use crate::driver::NovaDriver; > > > > diff --git a/drivers/gpu/drm/nova/uapi.rs b/drivers/gpu/drm/nova/uapi.rs > > deleted file mode 100644 > > index eb228a58d423..000000000000 > > --- a/drivers/gpu/drm/nova/uapi.rs > > +++ /dev/null > > @@ -1,61 +0,0 @@ > > -// SPDX-License-Identifier: GPL-2.0 > > - > > -use kernel::uapi; > > - > > -// TODO Work out some common infrastructure to avoid boilerplate code for uAPI abstractions. > > - > > -macro_rules! define_uapi_abstraction { > > - ($name:ident <= $inner:ty) => { > > - #[repr(transparent)] > > - pub struct $name(::kernel::types::Opaque<$inner>); > > - > > - impl ::core::convert::From<&::kernel::types::Opaque<$inner>> for &$name { > > - fn from(value: &::kernel::types::Opaque<$inner>) -> Self { > > - // SAFETY: `Self` is a transparent wrapper of `$inner`. > > - unsafe { ::core::mem::transmute(value) } > > - } > > - } > > - }; > > -} > > - > > -define_uapi_abstraction!(Getparam <= uapi::drm_nova_getparam); > > - > > -impl Getparam { > > - pub fn param(&self) -> u64 { > > - // SAFETY: `self.get()` is a valid pointer to a `struct drm_nova_getparam`. > > - unsafe { (*self.0.get()).param } > > - } > > - > > - pub fn set_value(&self, v: u64) { > > - // SAFETY: `self.get()` is a valid pointer to a `struct drm_nova_getparam`. > > - unsafe { (*self.0.get()).value = v }; > > - } > > -} > > - > > -define_uapi_abstraction!(GemCreate <= uapi::drm_nova_gem_create); > > - > > -impl GemCreate { > > - pub fn size(&self) -> u64 { > > - // SAFETY: `self.get()` is a valid pointer to a `struct drm_nova_gem_create`. > > - unsafe { (*self.0.get()).size } > > - } > > - > > - pub fn set_handle(&self, handle: u32) { > > - // SAFETY: `self.get()` is a valid pointer to a `struct drm_nova_gem_create`. > > - unsafe { (*self.0.get()).handle = handle }; > > - } > > -} > > - > > -define_uapi_abstraction!(GemInfo <= uapi::drm_nova_gem_info); > > - > > -impl GemInfo { > > - pub fn handle(&self) -> u32 { > > - // SAFETY: `self.get()` is a valid pointer to a `struct drm_nova_gem_info`. > > - unsafe { (*self.0.get()).handle } > > - } > > - > > - pub fn set_size(&self, size: u64) { > > - // SAFETY: `self.get()` is a valid pointer to a `struct drm_nova_gem_info`. > > - unsafe { (*self.0.get()).size = size }; > > - } > > -} > > diff --git a/rust/kernel/drm/ioctl.rs b/rust/kernel/drm/ioctl.rs > > index 445639404fb7..3425a835f9cd 100644 > > --- a/rust/kernel/drm/ioctl.rs > > +++ b/rust/kernel/drm/ioctl.rs > > @@ -83,7 +83,7 @@ pub mod internal { > > /// > > /// ```ignore > > /// fn foo(device: &kernel::drm::Device, > > -/// data: &Opaque, > > +/// data: &mut uapi::argument_type, > > /// file: &kernel::drm::File, > > /// ) -> Result > > /// ``` > > @@ -138,9 +138,12 @@ pub mod internal { > > // 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. > > - let data = unsafe { > > - &*(raw_data as *const $crate::types::Opaque<$crate::uapi::$struct>) > > - }; > > + // The `ioctl` argument is exclusively owned by the handler > > + // and guaranteed by the C implementation (`drm_ioctl()`) to remain > > + // valid for the entire lifetime of the reference taken here. > > + // There is no concurrent access or aliasing; no other references > > + // to this object exist during this call. > > + let data = unsafe { &mut *(raw_data as *mut $crate::uapi::$struct) }; > > "ptr as ptr" should be avoided, see: > > https://lore.kernel.org/rust-for-linux/20250615-ptr-as-ptr-v12-1-f43b024581e8@gmail.com/ > > so probably > > let data = unsafe { &mut *(raw_data.cast()) }; > > ? > Thank you for pointing that one out. --- BR Beata > With that fixed, feel free to add: > > Reviewed-by: Boqun Feng > > Regards, > Boqun > > > // SAFETY: This is just the DRM file structure > > let file = unsafe { $crate::drm::File::as_ref(raw_file) }; > > > > -- > > 2.25.1 > >