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 <[email protected]> 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/[email protected]?part=1
