Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Re-acquiring the DRM minor by ID in `fops_open` is vulnerable to a TOCTOU race, breaking type safety invariants. --
--- Patch [5]: [PATCH 5/7] rust: drm: wrap fops open with RegistrationGuard --- commit f19fe1023e3626e0af6b5b4f7cdaf8f59acc5f98 Author: Danilo Krummrich <[email protected]> rust: drm: wrap fops open with RegistrationGuard Add a Minor abstraction with RAII release and a fops_open() wrapper that holds a RegistrationGuard (drm_dev_enter / drm_dev_exit) across the entire drm_open() call. This guarantees that drm_dev_unplug() in Registration::drop() waits for the full open sequence to complete. > diff --git a/rust/kernel/drm/device.rs b/rust/kernel/drm/device.rs > index a2940e172073a..09903ed783e15 100644 > --- a/rust/kernel/drm/device.rs > +++ b/rust/kernel/drm/device.rs [ ... ] > + /// Wrapper for `fops.open` that holds a [`RegistrationGuard`] across > the entire `drm_open()` > + /// call. This guarantees that `drm_dev_unplug()` in > `Registration::drop()` waits for the full > + /// open sequence. > + extern "C" fn fops_open(inode: *mut bindings::inode, filp: *mut > bindings::file) -> c_int { > + let f = || -> Result<c_int> { > + // SAFETY: `inode` is valid. > + let minor_id = unsafe { bindings::iminor(inode) }; > + let minor = Minor::acquire(minor_id)?; > + > + // SAFETY: `fops_open` is only installed for devices of type `T` > (via `FOPS`). > + let _guard = (unsafe { minor.device::<T>() }) [Severity: High] Can this sequence cause type confusion if the DRM minor ID is reassigned? If a device T is unplugged concurrently with the open call, the minor_id can be freed and reassigned to a different device U. Since the VFS already resolved filp->f_op to T::fops_open during drm_stub_open, the following sequence could occur: 1. VFS calls chrdev_open which resolves to drm_stub_open 2. drm_stub_open looks up minor_id, gets device T, and sets filp->f_op to T::FOPS 3. Device T is unplugged, and its minor_id is freed 4. Device U is hot-plugged, and drm_minor_alloc reuses the same minor_id 5. T::fops_open is called by VFS and invokes Minor::acquire(minor_id) 6. The returned minor now belongs to device U, not T When minor.device::<T>() is called here, it unsafely downcasts Device<U> to Device<T>. This violates Rust's type safety and aliasing invariants. Is there a way to avoid looking up the minor by ID again to prevent this race condition? > + .registration_guard() > + .ok_or(ENODEV)?; > + > + // SAFETY: `inode` and `filp` are valid. The RegistrationGuard > ensures the entire > + // `drm_open()` runs within the SRCU critical section. > + Ok(unsafe { bindings::drm_open(inode, filp) }) > + }; > + > + match f() { > + Ok(ret) => ret, > + Err(e) => e.to_errno(), > + } > + } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
