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

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

On Thu, Jun 19, 2025 at 09:30:19AM -0300, Daniel Almeida wrote:
> Hi Beata,
> 
> > On 19 Jun 2025, at 07:21, Beata Michalska <beata.michalska@arm.com> 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 adds needless complexity and
> > maintenance overhead, brining no safety benefits.
> 
> I don’t think that “unsound casts” is what is happening here.
> 
> It's mostly the barrage of unsafe blocks to dereference the inner T for a
> problem that does not exist.
> 
> > 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
> 
> Even with this missing, I don’t see a problem with this patch.
The comments here are just informative and those are not a requirements
for this patch. Just a reference for things that might be needed.
> 
> In fact, if anything, the result of the discussion so far seems to point
> towards automatically implementing FromBytes for all uapi types.
That's what the 'pending' was supposed to mean. Will me more specific next
time.

---
BR
Beata
> 
> 
> > - 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)
> >                             };
> >                             // SAFETY: This is just the DRM file structure
> >                             let file = unsafe { $crate::drm::File::as_ref(raw_file) };
> > -- 
> > 2.25.1
> > 
> > 
> 
> — Daniel 
>