[PATCH v2 0/2] rust: serdev: Mitigate race conditions

[PATCH v2 0/2] rust: serdev: Mitigate race conditions

[Markus Probst]

For details, see the commit messages.

I will submit a patch (for the next merge cycle) soon, which will
make the probe and unbind code less convoluted. This will make bugs like
these more unlikely.

Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
Changes in v2:
- also fix race condition on unbind
- Link to v1: https://patch.msgid.link/20260905-rust_serdev_fix-v1-1-2ea92b154a6b@posteo.de

---
Markus Probst (2):
      rust: serdev: Fix race condition on driver unbind
      rust: serdev: Fix race condition on driver probe

 rust/kernel/device.rs | 16 +++++++++-------
 rust/kernel/driver.rs |  2 +-
 rust/kernel/serdev.rs |  2 +-
 3 files changed, 11 insertions(+), 9 deletions(-)
---
base-commit: e5e04726cdd043e309677071ab1b65a4b18f422b
change-id: 20260904-rust_serdev_fix-be3ff9c8a5e8

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

[Markus Probst]

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;
+        }
+
+        // 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.

-- 
2.55.0

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

[Gary Guo]

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.

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.

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

[Markus Probst]

[-- 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 --]

[PATCH v2 2/2] rust: serdev: Fix race condition on driver probe

[Markus Probst]

If `Driver::probe` fails, 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.

Use previously added `drvdata_drop` instead of `drvdata_obtain`, so the
serdev device will first be closed with Drop and after that the pointer to
the driver data will be set to NULL.

Remove `drvdata_obtain`, as it is now dead code.

Fixes: 99f59aa82341 ("rust: add basic serial device bus abstractions")
Reported-by: Sashiko Bot <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/linux-serial/20260903222159.70A911F000E9@smtp.kernel.org/
Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
 rust/kernel/device.rs | 25 -------------------------
 rust/kernel/serdev.rs |  2 +-
 2 files changed, 1 insertion(+), 26 deletions(-)

diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index 3886cc713c28..834fea0eb0a7 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -213,31 +213,6 @@ pub fn set_drvdata<T>(&self, data: impl PinInit<T, Error>) -> Result {
         Ok(())
     }
 
-    /// Take ownership of the private data stored in this [`Device`].
-    ///
-    /// # Safety
-    ///
-    /// - 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()) };
-
-        // 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()) };
-
-        if ptr.is_null() {
-            return None;
-        }
-
-        // 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()`.
-        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.
diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
index 17ca504b7f8d..a12b1dea12aa 100644
--- a/rust/kernel/serdev.rs
+++ b/rust/kernel/serdev.rs
@@ -176,7 +176,7 @@ extern "C" fn probe_callback(sdev: *mut bindings::serdev_device) -> kernel::ffi:
             let private_data = unsafe { sdev.as_ref().drvdata_borrow::<PrivateData<'_, T>>() };
             let private_data = ScopeGuard::new_with_data(private_data, |_| {
                 // SAFETY: We just set drvdata to `PrivateData<'_, T>`.
-                drop(unsafe { sdev.as_ref().drvdata_obtain::<PrivateData<'_, T>>() });
+                unsafe { sdev.as_ref().drvdata_drop::<PrivateData<'_, T>>() };
             });
             let mut active = private_data.active.lock();
 

-- 
2.55.0