Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Unconditionally calling `drm_dev_unplug()` in `Registration::drop` skips hardware shutdown for permanent devices utilizing `devres`-based cleanup. Pre-existing issues: - [High] Managing DRM device registration strictly via `devres` executes `drm_dev_unregister` / `drm_dev_unplug` too late in the device lifecycle, violating DRM teardown ordering. -- commit f0dfb3d109f75fcca313ecc2c206c5b4154e5e05 Author: Mike Lothian <[email protected]> rust: drm: unplug the device on Registration drop This commit replaces drm_dev_unregister() with drm_dev_unplug() in the Drop implementation for Registration. It aims to make device teardown safe for hot-removable buses by returning -ENODEV to open file descriptors instead of causing use-after-free errors. > diff --git a/rust/kernel/drm/driver.rs b/rust/kernel/drm/driver.rs > index 5233bdebc9fcd..78ca589a8b1c4 100644 > --- a/rust/kernel/drm/driver.rs > +++ b/rust/kernel/drm/driver.rs > @@ -119,7 +119,15 @@ pub trait Driver { > > /// The registration type of a `drm::Device`. > /// > -/// Once the `Registration` structure is dropped, the device is unregistered. > +/// Once the `Registration` structure is dropped, the device is unplugged > (which also > +/// unregisters it). [`drm_dev_unplug`] is the hot-unplug-safe teardown: it > sets the device's > +/// `unplugged` flag (so the DRM ioctl/mmap paths return `-ENODEV` to any > process still holding > +/// an open `card`/`render` node, rather than dereferencing freed device > state), drains > +/// in-flight `drm_dev_enter`/`drm_dev_exit` critical sections, unregisters > the minors, and tears > +/// down userspace mappings. For devices on a removable bus (e.g. a USB > dock) this is required > +/// for safe removal; for permanent devices it is a harmless superset of > `drm_dev_unregister`. > +/// > +/// [`drm_dev_unplug`]: https://docs.kernel.org/gpu/drm-internals.html > pub struct Registration<T: Driver>(ARef<drm::Device<T>>); [Severity: High] This is a pre-existing issue, but does managing the DRM device registration strictly via devres execute the unplug/unregister operations too late in the device lifecycle? Looking at Registration::new_foreign_owned(), it transfers ownership directly to devres and does not return a handle: Registration::new_foreign_owned(...) { ... devres::register(dev, reg, GFP_KERNEL) } Because devres cleanup executes after the driver's remove() callback completes, any hardware shutdown manually performed in remove() happens while the DRM device is still fully registered and accessible to userspace. Could this create a race condition where userspace IOCTLs can access the hardware concurrently with or after its shutdown? [ ... ] > @@ -166,7 +174,9 @@ unsafe impl<T: Driver> Send for Registration<T> {} > impl<T: Driver> Drop for Registration<T> { > fn drop(&mut self) { > // SAFETY: Safe by the invariant of `ARef<drm::Device<T>>`. The > existence of this > - // `Registration` also guarantees the this `drm::Device` is actually > registered. > - unsafe { bindings::drm_dev_unregister(self.0.as_raw()) }; > + // `Registration` also guarantees that this `drm::Device` is > actually registered. > + // `drm_dev_unplug` is the hot-unplug-safe teardown (it calls > `drm_dev_unregister` > + // internally after marking the device unplugged) — see the type's > documentation. > + unsafe { bindings::drm_dev_unplug(self.0.as_raw()) }; [Severity: High] Could unconditionally calling drm_dev_unplug() here skip hardware shutdown for permanent devices utilizing devres-based cleanup? The DRM core documentation notes that drm_dev_unplug() marks the device as unplugged before drm_atomic_helper_shutdown() is called. If a driver uses devres to manage hardware teardown, the Registration::drop action executes first on module unload. If the device is unconditionally marked as unplugged, will subsequent devres actions that attempt to shut down the hardware via drm_atomic_helper_shutdown() silently skip execution if their disable paths are protected with drm_dev_enter()? Does leaving the hardware enabled while the rest of the driver and its backing memory are torn down lead to IOMMU faults or memory corruption? > } > } -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
