Hi Beata,
> On 19 Jun 2025, at 07:21, Beata Michalska <beata.michal...@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.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
Even with this missing, I don’t see a problem with this patch.
In fact, if anything, the result of the discussion so far seems to point
towards automatically implementing FromBytes for all uapi types.
> - 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
