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

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

[Beata Michalska]

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

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

[Danilo Krummrich]

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.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.

>                              };

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

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

[Benno Lossin]

On Thu Jun 19, 2025 at 12:55 PM CEST, Danilo Krummrich wrote:
> On Thu, Jun 19, 2025 at 12:21:02PM +0200, Beata Michalska wrote:
>> 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.

If the C side is using pointers to read/write the value concurrently,
this is wrong, it needs to be wrapped in Opaque.

---
Cheers,
Benno

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

[Daniel Almeida]

Hi Benno,

> On 19 Jun 2025, at 08:01, Benno Lossin <lossin@kernel.org> wrote:
> 
> On Thu Jun 19, 2025 at 12:55 PM CEST, Danilo Krummrich wrote:
>> On Thu, Jun 19, 2025 at 12:21:02PM +0200, Beata Michalska wrote:
>>> 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.
> 
> If the C side is using pointers to read/write the value concurrently,
> this is wrong, it needs to be wrapped in Opaque.
> 
> ---
> Cheers,
> Benno

How can this happen, exactly? Can you provide an example that corroborates it?

The general pattern for drivers is to fill an uapi type and then wait on an
ioctl. The kernel then copies that using copy_from_user, so we're safe from
that perspective (i.e.: from the perspective of concurrent access from
userspace).

In kernelspace, we usually extract arguments from the uapi types to then
dictate further processing inside drivers. In what way are these shared with
"the C side" ?

If the result of this discussion is that we agree that this Opaque is not
needed, then we definitely need this patch, because using Opaque<T> complicates
all ioctls implementations by making it harder to get to the inner T in the
first place. We would have to needlessly add a lot of unsafe blocks for drivers
that wouldn't otherwise be there.


-- Daniel

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

[Daniel Almeida]

> On 19 Jun 2025, at 09:26, Daniel Almeida <daniel.almeida@collabora.com> wrote:
> 
> Hi Benno,
> 
>> On 19 Jun 2025, at 08:01, Benno Lossin <lossin@kernel.org> wrote:
>> 
>> On Thu Jun 19, 2025 at 12:55 PM CEST, Danilo Krummrich wrote:
>>> On Thu, Jun 19, 2025 at 12:21:02PM +0200, Beata Michalska wrote:
>>>> 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.
>> 
>> If the C side is using pointers to read/write the value concurrently,
>> this is wrong, it needs to be wrapped in Opaque.
>> 
>> ---
>> Cheers,
>> Benno
> 
> How can this happen, exactly? Can you provide an example that corroborates it?
> 
> The general pattern for drivers is to fill an uapi type and then wait on an

The general pattern for userspace, sorry.

— Daniel

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

[Benno Lossin]

On Thu Jun 19, 2025 at 2:26 PM CEST, Daniel Almeida wrote:
> Hi Benno,
>
>> On 19 Jun 2025, at 08:01, Benno Lossin <lossin@kernel.org> wrote:
>> 
>> On Thu Jun 19, 2025 at 12:55 PM CEST, Danilo Krummrich wrote:
>>> On Thu, Jun 19, 2025 at 12:21:02PM +0200, Beata Michalska wrote:
>>>> 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.
>> 
>> If the C side is using pointers to read/write the value concurrently,
>> this is wrong, it needs to be wrapped in Opaque.
>> 
>> ---
>> Cheers,
>> Benno
>
> How can this happen, exactly? Can you provide an example that corroborates it?

I don't have the context on this, I only saw a raw pointer being turned
into a mutable reference and that's only possible if there are no shared
or other exclusive references for the duration of its existence and no
raw pointers are being used to access the value.

---
Cheers,
Benno

> The general pattern for drivers is to fill an uapi type and then wait on an
> ioctl. The kernel then copies that using copy_from_user, so we're safe from
> that perspective (i.e.: from the perspective of concurrent access from
> userspace).
>
> In kernelspace, we usually extract arguments from the uapi types to then
> dictate further processing inside drivers. In what way are these shared with
> "the C side" ?
>
> If the result of this discussion is that we agree that this Opaque is not
> needed, then we definitely need this patch, because using Opaque<T> complicates
> all ioctls implementations by making it harder to get to the inner T in the
> first place. We would have to needlessly add a lot of unsafe blocks for drivers
> that wouldn't otherwise be there.
>
>
> -- Daniel

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

[Boqun Feng]

On Thu, Jun 19, 2025 at 03:17:31PM +0200, Benno Lossin wrote:
> On Thu Jun 19, 2025 at 2:26 PM CEST, Daniel Almeida wrote:
> > Hi Benno,
> >
> >> On 19 Jun 2025, at 08:01, Benno Lossin <lossin@kernel.org> wrote:
> >> 
> >> On Thu Jun 19, 2025 at 12:55 PM CEST, Danilo Krummrich wrote:
> >>> On Thu, Jun 19, 2025 at 12:21:02PM +0200, Beata Michalska wrote:
> >>>> 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.
> >> 
> >> If the C side is using pointers to read/write the value concurrently,
> >> this is wrong, it needs to be wrapped in Opaque.
> >> 
> >> ---
> >> Cheers,
> >> Benno
> >
> > How can this happen, exactly? Can you provide an example that corroborates it?
> 
> I don't have the context on this, I only saw a raw pointer being turned
> into a mutable reference and that's only possible if there are no shared
> or other exclusive references for the duration of its existence and no
> raw pointers are being used to access the value.
> 

I think in this case, as Daniel described below, `data` points to a
buffer either on the stack of drm_ioctl() function or allocated from
kmalloc() in drm_ioctl(), and drm_ioctl() copies userspace data into
that buffer, so at the this point, the data should be owned solely by
this function. But of course the safety comments need to be adjusted.

Regards,
Boqun

> ---
> Cheers,
> Benno
> 
> > The general pattern for drivers is to fill an uapi type and then wait on an
> > ioctl. The kernel then copies that using copy_from_user, so we're safe from
> > that perspective (i.e.: from the perspective of concurrent access from
> > userspace).
> >
> > In kernelspace, we usually extract arguments from the uapi types to then
> > dictate further processing inside drivers. In what way are these shared with
> > "the C side" ?
> >
> > If the result of this discussion is that we agree that this Opaque is not
> > needed, then we definitely need this patch, because using Opaque<T> complicates
> > all ioctls implementations by making it harder to get to the inner T in the
> > first place. We would have to needlessly add a lot of unsafe blocks for drivers
> > that wouldn't otherwise be there.
> >
> >
> > -- Daniel
> 

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

[Beata Michalska]

On Thu, Jun 19, 2025 at 03:17:31PM +0200, Benno Lossin wrote:
> On Thu Jun 19, 2025 at 2:26 PM CEST, Daniel Almeida wrote:
> > Hi Benno,
> >
> >> On 19 Jun 2025, at 08:01, Benno Lossin <lossin@kernel.org> wrote:
> >> 
> >> On Thu Jun 19, 2025 at 12:55 PM CEST, Danilo Krummrich wrote:
> >>> On Thu, Jun 19, 2025 at 12:21:02PM +0200, Beata Michalska wrote:
> >>>> 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.
> >> 
> >> If the C side is using pointers to read/write the value concurrently,
> >> this is wrong, it needs to be wrapped in Opaque.
> >> 
> >> ---
> >> Cheers,
> >> Benno
> >
> > How can this happen, exactly? Can you provide an example that corroborates it?
> 
> I don't have the context on this, I only saw a raw pointer being turned
> into a mutable reference and that's only possible if there are no shared
> or other exclusive references for the duration of its existence and no
> raw pointers are being used to access the value.

In this particular case, that conversion should be sound, as the raw pointer
originates from kernel's drm_ioctl(). The memory is being either allocated
dynamically or provided on the kernel stack for the sole purpose of serving
the handler. It's being exclusively owned by the handler for the duration of
the call. There is no concurrent access nor shared references, unless the
handler decides otherwise, but I do not see the reason why it would.
Within this strict scope, it should be safe to assume exclusive access to the
memory. This should align with Rust's aliasing rules ?

---
BR
Beata
> 
> ---
> Cheers,
> Benno
> 
> > The general pattern for drivers is to fill an uapi type and then wait on an
> > ioctl. The kernel then copies that using copy_from_user, so we're safe from
> > that perspective (i.e.: from the perspective of concurrent access from
> > userspace).
> >
> > In kernelspace, we usually extract arguments from the uapi types to then
> > dictate further processing inside drivers. In what way are these shared with
> > "the C side" ?
> >
> > If the result of this discussion is that we agree that this Opaque is not
> > needed, then we definitely need this patch, because using Opaque<T> complicates
> > all ioctls implementations by making it harder to get to the inner T in the
> > first place. We would have to needlessly add a lot of unsafe blocks for drivers
> > that wouldn't otherwise be there.
> >
> >
> > -- Daniel
> 

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

[Daniel Almeida]

Hi Beata,

>  There is no concurrent access nor shared references, unless the
> handler decides otherwise

It can’t do so in safe code. There is no way to manufacture a shared
reference from a mutable one in safe code and if it passes that to C, then
it’s already using a unsafe block for the ffi call.

Unless I missed something?

— Daniel

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

[Beata Michalska]

On Fri, Jun 20, 2025 at 10:42:09AM -0300, Daniel Almeida wrote:
> Hi Beata,
> 
> >  There is no concurrent access nor shared references, unless the
> > handler decides otherwise
> 
> It can’t do so in safe code. There is no way to manufacture a shared
> reference from a mutable one in safe code and if it passes that to C, then
> it’s already using a unsafe block for the ffi call.
> 
> Unless I missed something?
I do not think you have, though my comment wasn't meant to suggest anything
otherwise, merely emphasising that up to this point, the code upholds
Rust's safety guarantees.What the handler does afterward is a topic for
another time (change).

Hope that this makes the intent more clear (?)

---
BR
Beata

> 
> — Daniel

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

[Alice Ryhl]

On Thu, Jun 19, 2025 at 1:01 PM Benno Lossin <lossin@kernel.org> wrote:
>
> On Thu Jun 19, 2025 at 12:55 PM CEST, Danilo Krummrich wrote:
> > On Thu, Jun 19, 2025 at 12:21:02PM +0200, Beata Michalska wrote:
> >> 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.
>
> If the C side is using pointers to read/write the value concurrently,
> this is wrong, it needs to be wrapped in Opaque.

There's no concurrent reads or writes happening here. I think the
Opaque was added to deal with transmutes to/from byte arrays.

Alice

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

[Danilo Krummrich]

On Thu, Jun 19, 2025 at 12:55:14PM +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.
> 
> 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.

diff --git a/drivers/gpu/drm/nova/file.rs b/drivers/gpu/drm/nova/file.rs
index 7e59a34b830d..7e7d4e2de2fb 100644
--- a/drivers/gpu/drm/nova/file.rs
+++ b/drivers/gpu/drm/nova/file.rs
@@ -2,13 +2,11 @@

 use crate::driver::{NovaDevice, NovaDriver};
 use crate::gem::NovaObject;
-use crate::uapi::{GemCreate, GemInfo, Getparam};
 use kernel::{
     alloc::flags::*,
     drm::{self, gem::BaseObject},
     pci,
     prelude::*,
-    types::Opaque,
     uapi,
 };

@@ -26,20 +24,19 @@ impl File {
     /// IOCTL: get_param: Query GPU / driver metadata.
     pub(crate) fn get_param(
         dev: &NovaDevice,
-        getparam: &Opaque<uapi::drm_nova_getparam>,
+        getparam: &mut uapi::drm_nova_getparam,
         _file: &drm::File<File>,
     ) -> Result<u32> {
         let adev = &dev.adev;
         let parent = adev.parent().ok_or(ENOENT)?;
         let pdev: &pci::Device = parent.try_into()?;
-        let getparam: &Getparam = getparam.into();

-        let value = match getparam.param() as u32 {
+        let value = match getparam.param as u32 {
             uapi::NOVA_GETPARAM_VRAM_BAR_SIZE => pdev.resource_len(1)?,
             _ => return Err(EINVAL),
         };

-        getparam.set_value(value);
+        getparam.value = value;

         Ok(0)
     }
@@ -47,13 +44,12 @@ pub(crate) fn get_param(
     /// IOCTL: gem_create: Create a new DRM GEM object.
     pub(crate) fn gem_create(
         dev: &NovaDevice,
-        req: &Opaque<uapi::drm_nova_gem_create>,
+        req: &mut uapi::drm_nova_gem_create,
         file: &drm::File<File>,
     ) -> Result<u32> {
-        let req: &GemCreate = req.into();
-        let obj = NovaObject::new(dev, req.size().try_into()?)?;
+        let obj = NovaObject::new(dev, req.size.try_into()?)?;

-        req.set_handle(obj.create_handle(file)?);
+        req.handle = obj.create_handle(file)?;

         Ok(0)
     }
@@ -61,13 +57,12 @@ pub(crate) fn gem_create(
     /// IOCTL: gem_info: Query GEM metadata.
     pub(crate) fn gem_info(
         _dev: &NovaDevice,
-        req: &Opaque<uapi::drm_nova_gem_info>,
+        req: &mut uapi::drm_nova_gem_info,
         file: &drm::File<File>,
     ) -> Result<u32> {
-        let req: &GemInfo = req.into();
-        let bo = NovaObject::lookup_handle(file, req.handle())?;
+        let bo = NovaObject::lookup_handle(file, req.handle)?;

-        req.set_size(bo.size().try_into()?);
+        req.size = bo.size().try_into()?;

         Ok(0)
     }
diff --git a/drivers/gpu/drm/nova/nova.rs b/drivers/gpu/drm/nova/nova.rs
index 902876aa14d1..730598defe04 100644
--- a/drivers/gpu/drm/nova/nova.rs
+++ b/drivers/gpu/drm/nova/nova.rs
@@ -5,7 +5,6 @@
 mod driver;
 mod file;
 mod gem;
-mod uapi;

 use crate::driver::NovaDriver;

diff --git a/drivers/gpu/drm/nova/uapi.rs b/drivers/gpu/drm/nova/uapi.rs
deleted file mode 100644
index eb228a58d423..000000000000
--- a/drivers/gpu/drm/nova/uapi.rs
+++ /dev/null
@@ -1,61 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-
-use kernel::uapi;
-
-// TODO Work out some common infrastructure to avoid boilerplate code for uAPI abstractions.
-
-macro_rules! define_uapi_abstraction {
-    ($name:ident <= $inner:ty) => {
-        #[repr(transparent)]
-        pub struct $name(::kernel::types::Opaque<$inner>);
-
-        impl ::core::convert::From<&::kernel::types::Opaque<$inner>> for &$name {
-            fn from(value: &::kernel::types::Opaque<$inner>) -> Self {
-                // SAFETY: `Self` is a transparent wrapper of `$inner`.
-                unsafe { ::core::mem::transmute(value) }
-            }
-        }
-    };
-}
-
-define_uapi_abstraction!(Getparam <= uapi::drm_nova_getparam);
-
-impl Getparam {
-    pub fn param(&self) -> u64 {
-        // SAFETY: `self.get()` is a valid pointer to a `struct drm_nova_getparam`.
-        unsafe { (*self.0.get()).param }
-    }
-
-    pub fn set_value(&self, v: u64) {
-        // SAFETY: `self.get()` is a valid pointer to a `struct drm_nova_getparam`.
-        unsafe { (*self.0.get()).value = v };
-    }
-}
-
-define_uapi_abstraction!(GemCreate <= uapi::drm_nova_gem_create);
-
-impl GemCreate {
-    pub fn size(&self) -> u64 {
-        // SAFETY: `self.get()` is a valid pointer to a `struct drm_nova_gem_create`.
-        unsafe { (*self.0.get()).size }
-    }
-
-    pub fn set_handle(&self, handle: u32) {
-        // SAFETY: `self.get()` is a valid pointer to a `struct drm_nova_gem_create`.
-        unsafe { (*self.0.get()).handle = handle };
-    }
-}
-
-define_uapi_abstraction!(GemInfo <= uapi::drm_nova_gem_info);
-
-impl GemInfo {
-    pub fn handle(&self) -> u32 {
-        // SAFETY: `self.get()` is a valid pointer to a `struct drm_nova_gem_info`.
-        unsafe { (*self.0.get()).handle }
-    }
-
-    pub fn set_size(&self, size: u64) {
-        // SAFETY: `self.get()` is a valid pointer to a `struct drm_nova_gem_info`.
-        unsafe { (*self.0.get()).size = size };
-    }
-}

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

[Beata Michalska]

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

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

[Daniel Almeida]

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.

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 

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

[Beata Michalska]

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 
>