Re: [PATCH 3/5] rust: serdev: Simplify callbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] TOCTOU race condition causing a NULL pointer dereference in `receive_buf_callback` during device probe.
- [Low] Missing `#[inline]` annotations on abstraction functions `PrivateData::driver_data` and `OpenGuard::drop`.
--

commit 4244bd311fa3c091fa7a05ad05f18ce4e08829f6
Author: Markus Probst <markus.probst@posteo.de>
Subject: rust: serdev: Simplify callbacks

This commit simplifies serdev callbacks by initializing private data
directly on PrivateData. It also introduces OpenGuard for resource cleanup.

> diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
> index c16d6593a8d2d..66543108ec2fa 100644
> --- a/rust/kernel/serdev.rs
> +++ b/rust/kernel/serdev.rs
[ ... ]
> @@ -92,24 +88,35 @@ unsafe fn unregister(sdrv: &Opaque<Self::DriverType>) {
>      }
>  }
>  
> +struct OpenGuard<'bound> {
> +    sdev: &'bound Device<device::Bound>,
> +}
> +
> +impl Drop for OpenGuard<'_> {
> +    fn drop(&mut self) {
> +        // SAFETY:
> +        // - `self.sdev.as_raw()` is guaranteed to be a pointer to a valid
> +        //   `struct serdev_device`.
> +        // - The existence of self proves that the device is open.
> +        unsafe { bindings::serdev_device_close(self.sdev.as_raw()) };
> +    }
> +}

[Severity: Low]
Does this drop implementation need an #[inline] annotation?

The Rust subsystem guidelines state that in abstractions like the kernel crate,
small functions or functions forwarding to a C binding call should be annotated
with #[inline].

> +
>  #[doc(hidden)]
> -#[pin_data(PinnedDrop)]
> +#[pin_data]
>  pub struct PrivateData<'bound, T: Driver> {
> -    sdev: &'bound Device<device::Bound>,
>      #[pin]
> -    driver: UnsafeCell<MaybeUninit<T::Data<'bound>>>,
> -    open: UnsafeCell<bool>,
> +    driver: T::Data<'bound>,
> +    open: OpenGuard<'bound>,
>  }
>  
> -#[pinned_drop]
> -impl<T: Driver> PinnedDrop for PrivateData<'_, T> {
> -    fn drop(self: Pin<&mut Self>) {
> -        // 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
> -            // `struct serdev_device`.
> -            unsafe { bindings::serdev_device_close(self.sdev.as_raw()) };
> -        }
> +impl<'bound, T: Driver> PrivateData<'bound, T> {
> +    fn driver_data(self: Pin<&Self>) -> Pin<&T::Data<'bound>> {
> +        // SAFETY: We treat the result as pinned.
> +        let inner = unsafe { Pin::into_inner_unchecked(self) };
> +
> +        // SAFETY: `self.driver` is pinned.
> +        unsafe { Pin::new_unchecked(&inner.driver) }
>      }
>  }

[Severity: Low]
Does this helper function also need an #[inline] annotation?

Similar to OpenGuard::drop above, this appears to be a small projection
function in an abstraction crate, which the subsystem guidelines suggest
annotating with #[inline].

> @@ -134,46 +141,30 @@ extern "C" fn probe_callback(sdev: *mut bindings::serdev_device) -> kernel::ffi:
>  
>          from_result(|| {
>              sdev.as_ref().set_drvdata(try_pin_init!(PrivateData::<T> {
> -                sdev: &**sdev,
> -                driver: MaybeUninit::<T::Data<'_>>::zeroed().into(),
> -                open: false.into(),
> +                open: {
> +                    // SAFETY:
> +                    // - `sdev.as_raw()` is guaranteed to be a valid pointer to
> +                    //   `serdev_device`.
> +                    // - It is safe to call before open.
> +                    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: `sdev.as_raw()` is guaranteed to be a valid pointer to
> +                    // `serdev_device`.
> +                    to_result(unsafe { bindings::serdev_device_open(sdev.as_raw()) })?;
> +
> +                    OpenGuard { sdev }
> +                },
> +                driver <- T::probe(sdev, info),
>              }))?;

[Severity: High]
Could this sequence introduce a race condition that leads to a NULL pointer
dereference in receive_buf_callback() during device probe?

Because bindings::serdev_device_open() is called inside try_pin_init!(), the
device is exposed to hardware interrupts before the initialized struct is
passed to set_drvdata(). This means dev->driver_data remains NULL during this
window.

While bindings::serdev_device_pause_rx() is called before open, and
bindings::serdev_device_resume_rx() is called after set_drvdata() below,
resume_rx() uses clear_bit(SERPORT_PAUSE_RX) which lacks a memory barrier.

On weakly-ordered CPU architectures, can the CPU reorder the store to
dev->driver_data to happen after the atomic clear?

If a concurrent RX worker thread observes SERPORT_PAUSE_RX == 0 while
dev->driver_data is still NULL, it could trigger receive_buf_callback() and
crash when dereferencing the NULL pointer.

> -            // SAFETY: We just set drvdata to `PrivateData<'_, T>`.
> -            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>>() });
> -            });
> -            // 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()) })?;
> -
> -            // SAFETY: We have exclusive access to `private_data.open`.
> -            unsafe { *private_data.open.get() = true };
> -
> -            let data = T::probe(sdev, info);
> +            unsafe { bindings::serdev_device_resume_rx(sdev.as_raw()) };
>  
> -            // SAFETY: We have exclusive access to `private_data.driver`.
> -            let driver = unsafe { &mut *private_data.driver.get() };
> -            // SAFETY:
> -            // - `driver.as_mut_ptr()` is a valid pointer to uninitialized data.
> -            // - `private_data.driver` is pinned.
> -            let result = unsafe { pin_init::raw_try_init(driver.as_mut_ptr(), data) };
> -
> -            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
> -            })
> +            Ok(0)
>          })
>      }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260906-rust_serdev_probe_refactor-v1-0-69cdae0074ec@posteo.de?part=3