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

[Beata Michalska <beata.michalska@arm.com>]

On Thu, Jun 19, 2025 at 12:55:08PM +0200, Danilo Krummrich wrote:
> 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.
>
That's a bad warding on my side.
> Speaking of which, this patch breaks the build, since it doesn't adjust the
> users of the API, i.e. nova.
> 
True, totally slipped my mind that there are already users of those.
Apologies for that.
> If you want I can post a diff to fix up nova accordingly for you to add to this
> patch.
I think you already did - so thank you.
> 
> [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@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.
> 
Will do.

---
BR
Beata
> >                              };
> 
> 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) };