There are currently 2 race conditions: - in probe if `Driver::probe` returns Err - in unbind . In those cases the driver data will be set to NULL before the serdev device was closed. If data is received while the driver data is dropped, the `receive_buf_callback` might try to access the `active` mutex on a null pointer.
Removing the need for `receive_buf_callback` to lock the `active` mutex fixes these.
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... Closes: https://lore.kernel.org/linux-serial/20260903222159.70A911F000E9@smtp.kernel... Signed-off-by: Markus Probst markus.probst@posteo.de --- rust/kernel/serdev.rs | 60 ++++++++++++--------------------------------------- 1 file changed, 14 insertions(+), 46 deletions(-)
diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs index 17ca504b7f8d..c16d6593a8d2 100644 --- a/rust/kernel/serdev.rs +++ b/rust/kernel/serdev.rs @@ -13,13 +13,9 @@ to_result, VTABLE_DEFAULT_ERROR, // }, - new_mutex, of, prelude::*, - sync::{ - aref::AlwaysRefCounted, - Mutex, // - }, + sync::aref::AlwaysRefCounted, time::Jiffies, types::{ Opaque, @@ -103,40 +99,11 @@ pub struct PrivateData<'bound, T: Driver> { #[pin] driver: UnsafeCell<MaybeUninit<T::Data<'bound>>>, open: UnsafeCell<bool>, - /// Whether `receive_buf_callback` is allowed to call `Driver::receive`. - /// - /// If locked, the receive_buf_callback will be blocked on data reception. - /// This is the case while the driver is being probed or while [`PrivateData`] is being dropped. - /// This is necessary, because we need to open the serdev device before the driver has been - /// probed in order to allow it to be configured, which allows `receive_buf_callback` to be - /// called. Thus we need to block data until probe completes and the driver data becomes - /// initialized. - /// - /// If unlocked and true, the receive_buf_callback will forward the data to - /// `Driver::receive`. This is the normal state of operation. - /// - /// If unlocked and false, the receive_buf_callback will throw away the data. - /// This is only the case, if the serdev device is open and - /// - the driver returned an error in probe - /// or - /// - the driver data already has been dropped, because it was unbound. - #[pin] - active: Mutex<bool>, }
#[pinned_drop] impl<T: Driver> PinnedDrop for PrivateData<'_, T> { fn drop(self: Pin<&mut Self>) { - let mut active = self.active.lock(); - if *active { - // SAFETY: - // - We have exclusive access to `self.driver`. - // - `self.driver` is guaranteed to be initialized. - unsafe { (*self.driver.get()).assume_init_drop() }; - *active = false; - } - drop(active); - // SAFETY: We have exclusive access to `self.open`. if unsafe { *self.open.get() } { // SAFETY: `self.sdev.as_raw()` is guaranteed to be a pointer to a valid @@ -170,7 +137,6 @@ extern "C" fn probe_callback(sdev: *mut bindings::serdev_device) -> kernel::ffi: sdev: &**sdev, driver: MaybeUninit::<T::Data<'_>>::zeroed().into(), open: false.into(), - active <- new_mutex!(false), }))?; // SAFETY: We just set drvdata to `PrivateData<'_, T>`. let private_data = unsafe { sdev.as_ref().drvdata_borrow::<PrivateData<'_, T>>() }; @@ -178,11 +144,12 @@ extern "C" fn probe_callback(sdev: *mut bindings::serdev_device) -> kernel::ffi: // SAFETY: We just set drvdata to `PrivateData<'_, T>`. drop(unsafe { sdev.as_ref().drvdata_obtain::<PrivateData<'_, T>>() }); }); - let mut active = private_data.active.lock(); - // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to `serdev_device`. unsafe { bindings::serdev_device_set_client_ops(sdev.as_raw(), Self::OPS) };
+ // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to `serdev_device`. + unsafe { bindings::serdev_device_pause_rx(sdev.as_raw()) }; + // SAFETY: The serial device bus only ever calls the probe callback with a valid pointer // to a `serdev_device`. to_result(unsafe { bindings::serdev_device_open(sdev.as_raw()) })?; @@ -199,12 +166,12 @@ extern "C" fn probe_callback(sdev: *mut bindings::serdev_device) -> kernel::ffi: // - `private_data.driver` is pinned. let result = unsafe { pin_init::raw_try_init(driver.as_mut_ptr(), data) };
- *active = result.is_ok(); - - drop(active); - result.map(|()| { private_data.dismiss(); + + // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to `serdev_device`. + unsafe { bindings::serdev_device_resume_rx(sdev.as_raw()) }; + 0 }) }) @@ -231,6 +198,12 @@ extern "C" fn remove_callback(sdev: *mut bindings::serdev_device) { let data_pinned = unsafe { Pin::new_unchecked(data.assume_init_ref()) };
T::unbind(sdev, data_pinned); + + // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to `serdev_device`. + unsafe { bindings::serdev_device_pause_rx(sdev.as_raw()) }; + + // SAFETY: We already established that `data` is guaranteed to be initialized. + unsafe { data.assume_init_drop() }; }
extern "C" fn receive_buf_callback( @@ -248,11 +221,6 @@ extern "C" fn receive_buf_callback( // `probe_callback`, hence it's guaranteed that `Device::set_drvdata()` has been called // and stored a `Pin<KBox<PrivateData<'_, T>>>`. let private_data = unsafe { sdev.as_ref().drvdata_borrow::<PrivateData<'_, T>>() }; - let active = private_data.active.lock(); - - if !*active { - return length; - }
// SAFETY: No one has exclusive access to `private_data.driver`. let data = unsafe { &*private_data.driver.get() };