Re: [PATCH v3 12/13] rust: drm: Pass registration data to ioctl handlers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing `Sync` bound on `RegistrationData` allows data races via concurrent ioctl calls.
- [High] Unconstrained type inference in `declare_drm_ioctls!` bypasses type invariants, causing Undefined Behavior.
--

commit 4f19dadb5da1b4410f3118837853bdb09c3f4718
Author: Danilo Krummrich <dakr@kernel.org>

rust: drm: Pass registration data to ioctl handlers

Pass registration data to ioctl handlers via
drm::Device<Registered>::registration_data_with().

[ ... ]
> diff --git a/rust/kernel/drm/ioctl.rs b/rust/kernel/drm/ioctl.rs
> index c6a03be018e6d..ecc1a0383635c 100644
> --- a/rust/kernel/drm/ioctl.rs
> +++ b/rust/kernel/drm/ioctl.rs
[ ... ]
> @@ -143,9 +144,16 @@ macro_rules! declare_drm_ioctls {
>                              // type to `$func`'s first parameter, which the compiler cannot infer
>                              // through method resolution and associated-type projections alone.
>                              #[allow(unreachable_code)]
> -                            let _ = || $func(dev, unreachable!(), unreachable!());
> +                            let _ = || {
> +                                $func(
> +                                    dev,
> +                                    unreachable!(),
> +                                    unreachable!(),
> +                                    unreachable!(),
> +                                )
> +                            };

[Severity: High]
Does this type inference bypass the safety invariants of Device::from_raw()?

Because dev is passed directly to the user-provided $func in this dummy 
closure, the compiler infers the DeviceContext of dev from the handler's 
signature. If an ioctl handler expects &Device<T, Registered>, the compiler 
resolves dev to be a Registered device.

This means the preceding call to Device::from_raw(raw_dev) instantiates a 
Registered device reference before registration_guard() establishes the 
drm_dev_enter SRCU critical section. This bypasses the safety contract which 
requires the parent bus device to be bound.

Would it be safer to explicitly constrain dev to Device<_, Normal>, and 
pass &*guard instead of dev to the dummy closure?

>  
> -                            let Some(_guard) = dev.registration_guard() else {
> +                            let Some(guard) = dev.registration_guard() else {
>                                  return $crate::error::code::ENODEV.to_errno();
>                              };
[ ... ]
> @@ -160,7 +168,9 @@ macro_rules! declare_drm_ioctls {
>                              // SAFETY: This is just the DRM file structure
>                              let file = unsafe { $crate::drm::File::from_raw(raw_file) };
>  
> -                            match $func(dev, data, file) {
> +                            match guard.registration_data_with(|reg_data| {
> +                                $func(&*guard, reg_data, data, file)
> +                            }) {

[Severity: High]
Can this allow data races if the RegistrationData type is not Sync?

Looking at the Driver trait definition in rust/kernel/drm/driver.rs:

trait Driver {
    type RegistrationData: ForLt;
    ...
}

RegistrationData is defined without a Sync bound. However, Device<T, C> 
is unconditionally Sync, meaning any thread can access its shared methods 
concurrently. 

Since multiple userspace threads can invoke DRM ioctls concurrently, multiple 
threads can call registration_data_with() at the same time to obtain shared 
references to the same data. If a driver uses a non-Sync type for 
RegistrationData (such as Cell<T>), could this lead to concurrent mutation 
and undefined behavior?

>                                  Err(e) => e.to_errno(),
>                                  Ok(i) => i.try_into()
>                                              .unwrap_or($crate::error::code::ERANGE.to_errno()),

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260620005431.1562115-1-dakr@kernel.org?part=12