[PATCH] rust: drm: Drop the use of Opaque for ioctl arguments

[Danilo Krummrich]

On Thu, Jun 19, 2025 at 12:21:02PM +0200, Beata Michalska wrote:
> With the Opaque<T>, 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 constraints. By using Opaque<T>, ioctl handlers
> end up doing unsound castings,

Which unsound casts? Please see [1] and [2] for how nova implements those IOCTL
handlers.

Speaking of which, this patch breaks the build, since it doesn't adjust the
users of the API, i.e. nova.

If you want I can post a diff to fix up nova accordingly for you to add to this
patch.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/nova/uapi.rs
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/nova/file.rs

> which 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 <beata.michalska at arm.com>
> ---
> 
>  Additional comments:
>  - UAPI types already automatically derive MaybeZeroable,
>    so it probably makes little sense to add any verification for that
>  - FromBytes is pending, but due to the orphan rule, adding verification
>    of it being implemented for IOCTL args here is pointless
>  - Verifying pointer alignment could make use of strict_provenance,
>    but that one is unstable and I am not sure what are the exact rules
>    here for using those. Without that one though, verifying alignment in
>    some cases (i.e. pointer tagging) might require more extensive changes.
>    Happy to deal with either.
> 
>  rust/kernel/drm/ioctl.rs | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/rust/kernel/drm/ioctl.rs b/rust/kernel/drm/ioctl.rs
> index 445639404fb7..12b296131672 100644
> --- a/rust/kernel/drm/ioctl.rs
> +++ b/rust/kernel/drm/ioctl.rs
> @@ -139,7 +139,7 @@ pub mod internal {
>                              // 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>)
> +                                &mut *(raw_data as *mut $crate::uapi::$struct)

I think we have to document the guarantees we rely on to create this mutable
reference.

>                              };

This should be formatted as one single line and also adjust the doc-comment of
the macro accordingly, i.e.:

diff --git a/rust/kernel/drm/ioctl.rs b/rust/kernel/drm/ioctl.rs
index 12b296131672..f0c599f15a41 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<Self>,
-///        data: &Opaque<uapi::argument_type>,
+///        data: &mut uapi::argument_type,
 ///        file: &kernel::drm::File<Self::File>,
 /// ) -> Result<u32>
 /// ```
@@ -138,9 +138,7 @@ macro_rules! declare_drm_ioctls {
                             // 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 {
-                                &mut *(raw_data as *mut $crate::uapi::$struct)
-                            };
+                            let data = unsafe { &mut *(raw_data as *mut $crate::uapi::$struct) };
                             // SAFETY: This is just the DRM file structure
                             let file = unsafe { $crate::drm::File::as_ref(raw_file) };