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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Drivers cannot synchronously wait for data during `probe` without causing a deadlock, because `receive_buf_callback` blocks on a mutex held by `probe_callback`.
- [High] Concurrent device callbacks during unbind or probe failure lead to Use-After-Free (UAF) or NULL pointer dereference because driver data is cleared before the device is closed.
- [High] `Device::set_baudrate` incorrectly treats approximate baudrates as errors, which will fail driver initialization on many hardware platforms.
--

--- Patch [1]: [PATCH v14 1/3] rust: add basic serial device bus abstractions ---
Note: The format of this review is altered to provide a free-form summary of
the findings, rather than inline diff comments, due to system recitation
restrictions.

commit c091dc3bf08c045ff86b0afe5e41d3fd0a94bb83
Author: Markus Probst <markus.probst@posteo.de>
Subject: rust: add basic serial device bus abstractions

Implement the basic serial device bus abstractions required to write a
serial device bus device driver.

[Severity: High]
In Adapter<T>::probe_callback(), the code acquires the active mutex and
holds it across serdev_device_open() and data.__pinned_init(). If a driver
needs to send a command to the device and synchronously wait for a response
during its probe routine, won't this cause a deadlock?

When the device responds, Adapter<T>::receive_buf_callback() is invoked in
the RX workqueue and attempts to lock the exact same active mutex. Since
probe_callback() is blocked waiting for the response, and
receive_buf_callback() is blocked waiting for the mutex, the system seems
like it will deadlock. Is it possible to unlock before waiting, or use a
different synchronization strategy?

[Severity: High]
Looking at Adapter<T>::remove_callback(), it appears that T::unbind() is
called, but PrivateData is never explicitly dropped or obtained from the
device data, which might leave the device open and leak memory.

When remove_callback() returns, the C driver core sets drvdata to NULL. If
data arrives immediately after, receive_buf_callback() might read a NULL
pointer from drvdata_borrow() and dereference it.

Similarly, on a probe error in Adapter<T>::probe_callback(), ScopeGuard
clears drvdata before PrivateData's PinnedDrop implementation closes the
device. Does this create a race window where the device is still open and
receiving data while drvdata is NULL or in the process of being destroyed?

[Severity: High]
In Device::set_baudrate(), the success condition is checked using strict
equality (ret == speed).

Since serdev_device_set_baudrate() configures the hardware and returns the
actual achieved baudrate (or 0 on failure), hardware clock divider
limitations often result in a baudrate that is close but not exactly equal
to the requested speed (for example, returning 115198 when 115200 is
requested).

Will this strict equality check cause valid driver initializations to fail
on platforms that can only approximate the requested baudrate?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715-rust_serdev-v14-0-b4ed5c93606f@posteo.de?part=1