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