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 <[email protected]> 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/[email protected]?part=12
