Re: [PATCH v13 1/3] rust: add basic serial device bus abstractions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] `serdev::Driver::Data` is missing a `Sync` trait bound, allowing safe code to trigger Undefined Behavior via data races.
- [High] The framework defers `serdev_device_close()` to `PrivateData::drop` and fails to close the device synchronously in `remove_callback`, creating a race window that leads to NULL pointer dereferences, Use-After-Free, and deadlocks.
- [High] Holding the `active` mutex across `T::probe` introduces a cyclic AB-BA deadlock if a driver synchronously awaits receive data during initialization.
- [Medium] `write_all` improperly casts the `ssize_t` return value of `serdev_device_write` to `i32`, which causes successful writes larger than 2GB to be incorrectly treated as negative error codes.
--

--- Patch [1]: [PATCH v13 1/3] rust: add basic serial device bus abstractions ---
commit d3271c073101a0f9ce803c8c5d6de6e1b8618c41
Author: Markus Probst <markus.probst@posteo.de>

rust: add basic serial device bus abstractions
    
This commit introduces the Rust abstractions for the serial device bus API.
It adds the serdev::Driver trait, the serdev::Device abstraction representing
a struct serdev_device, and adapter mechanisms to register drivers with the
generic driver core.

> diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
> new file mode 100644
> index 0000000000000..ab5d6446b091e
> --- /dev/null
> +++ b/rust/kernel/serdev.rs
[ ... ]
> +#[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;
> +        }
> +
> +        // 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()) };
> +        }
> +    }
> +}

[Severity: High]
Does deferring serdev_device_close() to drop() introduce an AB-BA deadlock?

If receive_buf_callback() executes on the receive workqueue while PrivateData
is being dropped, the receive thread will block waiting for the 'active'
mutex. Meanwhile, this drop() method holds the 'active' mutex and calls
serdev_device_close(), which synchronously flushes the workqueue. This
seems like it would cause the two threads to deadlock.

[ ... ]
> +    extern "C" fn probe_callback(sdev: *mut bindings::serdev_device) -> kernel::ffi::c_int {
[ ... ]
> +            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: 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);

[Severity: High]
Can holding the 'active' mutex across T::probe() cause a cyclic deadlock
during device initialization?

If a driver sends a hardware identification command in its probe() routine
and synchronously waits for the response, the incoming data will trigger
receive_buf_callback() on a separate thread. Because receive_buf_callback()
unconditionally attempts to acquire the 'active' mutex, it will block
forever while probe_callback() holds the mutex and waits for the receive
data.

[ ... ]
> +    extern "C" fn remove_callback(sdev: *mut bindings::serdev_device) {
> +        // SAFETY: The serial device bus only ever calls the remove callback with a valid pointer
> +        // to a `struct serdev_device`.
> +        //
> +        // INVARIANT: `sdev` is valid for the duration of `remove_callback()`.
> +        let sdev = unsafe { &*sdev.cast::<Device<device::CoreInternal<'_>>>() };
> +
> +        // SAFETY: `remove_callback` is only ever called after a successful call to
> +        // `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>>() };
> +
> +        // SAFETY: No one has exclusive access to `private_data.driver`.
> +        let data = unsafe { &*private_data.driver.get() };
> +        // SAFETY:
> +        // - `private_data.driver` is pinned.
> +        // - `remove_callback` is only ever called after a successful call to `probe_callback`,
> +        //   hence it's guaranteed that `private_data.driver` was initialized.
> +        let data_pinned = unsafe { Pin::new_unchecked(data.assume_init_ref()) };
> +
> +        T::unbind(sdev, data_pinned);
> +    }

[Severity: High]
Should serdev_device_close() be called synchronously in remove_callback()
rather than waiting for PrivateData::drop?

By not closing the device synchronously here, the device remains active
and the receive layer can asynchronously trigger receive_buf_callback() on a
workqueue. In the unbind path, the device core subsequently invokes
post_unbind_rust(), which clears the device's drvdata pointer. 

If receive_buf_callback() executes right after drvdata is cleared, it
blindly dereferences a null pointer via drvdata_borrow().

[ ... ]
> +pub trait Driver {
> +    /// The type holding driver private data about each device id supported by the driver.
> +    // TODO: Use associated_type_defaults once stabilized:
> +    //
> +    // ```
> +    // type IdInfo: 'static = ();
> +    // ```
> +    type IdInfo: 'static;
> +
> +    /// The type of the driver's bus device private data.
> +    type Data<'bound>: Send + 'bound;

[Severity: High]
Is serdev::Driver::Data missing a Sync trait bound?

The serdev framework invokes receive_buf_callback() from the receive
workqueue and remove_callback() from the driver core unbind context. Both
callbacks synthesize a shared reference Pin<&T::Data> from the raw driver
pointer without any mutual exclusion.

Because T::Data does not require Sync, safe Rust code could use
non-thread-safe types (e.g., core::cell::Cell) in the driver state,
leading to data races and undefined behavior.

[ ... ]
> +    pub fn write_all(&self, data: &[u8], timeout: Timeout) -> Result<usize> {
> +        // SAFETY:
> +        // - `self.as_raw()` is guaranteed to be a pointer to a valid `serdev_device`.
> +        // - `data.as_ptr()` is guaranteed to be a valid array pointer with the size of
> +        //   `data.len()`.
> +        let ret = unsafe {
> +            bindings::serdev_device_write(
> +                self.as_raw(),
> +                data.as_ptr(),
> +                data.len(),
> +                timeout.into_jiffies(),
> +            )
> +        };
> +        // CAST: negative return values are guaranteed to be between `-MAX_ERRNO` and `-1`,
> +        // which always fit into a `i32`.
> +        to_result(ret as i32).map(|()| ret.unsigned_abs())
> +    }

[Severity: Medium]
Does casting the ssize_t return value of serdev_device_write() to i32
incorrectly treat large successful writes as errors?

If a caller passes a slice larger than 2GB (e.g., on a 64-bit architecture)
and the write succeeds, the positive isize return value will overflow
during the 'as i32' cast, resulting in a negative number. to_result()
will then erroneously interpret this successful return as a kernel error
code.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260712-rust_serdev-v13-0-2c4060bc8e30@posteo.de?part=1