Re: [PATCH v2 1/2] rust: serdev: Fix race condition on driver unbind

[Markus Probst <markus.probst@posteo.de>]

[-- Attachment #1: Type: text/plain, Size: 5039 bytes --]

On Sat, 2026-09-05 at 15:16 +0100, Gary Guo wrote:
> On Sat Sep 5, 2026 at 2:30 PM BST, Markus Probst wrote:
> > On device unbind, the pointer to the driver data (`PrivateData`) will first
> > be set to NULL by `drvdata_obtain` and only after that the serdev device
> > will be closed by Drop. Thus there is a small window in which the serdev
> > device is still open, but the pointer to the driver data is NULL. Therefore
> > it is possible that `receive_buf_callback` might try to access the `active`
> > mutex on a null pointer.
> > 
> > Add function `drvdata_drop` that leaves the pointer to the driver data
> > valid until the Drop has completed. Use it in the post unbind callback.
> > 
> > Fixes: 99f59aa82341 ("rust: add basic serial device bus abstractions")
> > Reported-by: Sashiko Bot <sashiko-bot@kernel.org>
> > Closes: https://lore.kernel.org/linux-serial/20260905000836.C8FC91F00A3D@smtp.kernel.org/
> > Signed-off-by: Markus Probst <markus.probst@posteo.de>
> > ---
> >  rust/kernel/device.rs | 27 +++++++++++++++++++++++++++
> >  rust/kernel/driver.rs |  2 +-
> >  2 files changed, 28 insertions(+), 1 deletion(-)
> > 
> > diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> > index 2291d85b6849..3886cc713c28 100644
> > --- a/rust/kernel/device.rs
> > +++ b/rust/kernel/device.rs
> > @@ -219,6 +219,7 @@ pub fn set_drvdata<T>(&self, data: impl PinInit<T, Error>) -> Result {
> >      ///
> >      /// - The type `T` must match the type of the `ForeignOwnable` previously stored by
> >      ///   [`Device::set_drvdata`].
> > +    /// - Must only be called before the device is fully unbound.
> >      pub(crate) unsafe fn drvdata_obtain<T>(&self) -> Option<Pin<KBox<T>>> {
> >          // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
> >          let ptr = unsafe { bindings::dev_get_drvdata(self.as_raw()) };
> > @@ -236,6 +237,32 @@ pub(crate) unsafe fn drvdata_obtain<T>(&self) -> Option<Pin<KBox<T>>> {
> >          //   in `into_foreign()`.
> >          Some(unsafe { Pin::<KBox<T>>::from_foreign(ptr.cast()) })
> >      }
> > +
> > +    /// Drop the private data stored in this [`Device`].
> > +    ///
> > +    /// The pointer to the private data remains valid until the drop is complete.
> > +    ///
> > +    /// # Safety
> > +    ///
> > +    /// - The type `T` must match the type of the `ForeignOwnable` previously stored by
> > +    ///   [`Device::set_drvdata`].
> > +    pub(crate) unsafe fn drvdata_drop<T>(&self) {
> > +        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
> > +        let ptr = unsafe { bindings::dev_get_drvdata(self.as_raw()) };
> > +
> > +        if ptr.is_null() {
> > +            return;
> > +        }
> 
> How does this help the problem? While drop is running, other code should not
> attempt to obtain a reference to the data anymore. Otherwise this still have UB
> potential by accessing fields that are just destroyed (not to mention that Rust
> alias model also forbid it).
> 
> I think the existing actually catches it better, because *if* NULL pointer can
> be observed by callbacks, a synchronization is missing in the subsystem. The bus
> should first perform a synchronization to ensure callbacks are no longer fired,
> and then proceed to clean up resources.
The abstraction has been written, so the serdev device stays open until
the drivers private data has been dropped. Until then, the driver can
still have a reference to the device, which can access calls that are
only valid if open.

I don't think I am allowed to rewrite that logic in a rc period.

Thanks
- Markus Probst

> 
> Best,
> Gary
> 
> > +
> > +        // SAFETY:
> > +        // - If `ptr` is not NULL, it comes from a previous call to `into_foreign()`.
> > +        // - `dev_get_drvdata()` guarantees to return the same pointer given to `dev_set_drvdata()`
> > +        //   in `into_foreign()`.
> > +        drop(unsafe { Pin::<KBox<T>>::from_foreign(ptr.cast()) });
> > +
> > +        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
> > +        unsafe { bindings::dev_set_drvdata(self.as_raw(), core::ptr::null_mut()) };
> > +    }
> >  }
> >  
> >  impl<Ctx: InternalBoundContext> Device<Ctx> {
> > diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs
> > index c9c74c4dde8f..83410141ef1c 100644
> > --- a/rust/kernel/driver.rs
> > +++ b/rust/kernel/driver.rs
> > @@ -204,7 +204,7 @@ extern "C" fn post_unbind_callback(dev: *mut bindings::device) {
> >          //
> >          // SAFETY: By the safety requirements of the `Driver` trait, `T::DriverData` is the
> >          // driver's bus device private data type.
> > -        drop(unsafe { dev.drvdata_obtain::<T::DriverData<'_>>() });
> > +        unsafe { dev.drvdata_drop::<T::DriverData<'_>>() };
> >      }
> >  
> >      /// Attach generic `struct device_driver` callbacks.
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]