On Sun, 2026-07-12 at 17:15 +0000, [email protected] wrote: > 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. That is an issue.
>
> [ ... ]
> > + 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.
False positive, kinda.
It is not possbile yet, to retrieve initial device 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().
False positive.
impl Drop for PrivateData locks the active mutex and sets its value to
false.
receive_buf_callback trys to acquire this mutex and if the value is
false, it never accesses the driver data. serdev_device_close gets
called before private data gets deallocated.
>
> [ ... ]
> > +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.
I suppose thats an issue too.
>
> [ ... ]
> > + 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?
Yes, but thats more an issue with the input not being checked than the
return type.
>
> 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.
signature.asc
Description: This is a digitally signed message part
