Hi Benno,
> On 19 Jun 2025, at 08:01, Benno Lossin <los...@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
