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.michal...@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) };
