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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Structural deadlock in `probe_callback` prevents synchronous bidirectional I/O during driver initialization.
- [High] Race condition between `drvdata` unlinking and `serdev_device_close` leads to Null Pointer Dereference in `receive_buf_callback`.
- [High] `Device` communication methods are restricted to `Device<Bound>`, breaking `probe` initialization.
- [High] `core::slice::from_raw_parts` is called with a potentially NULL pointer when `length` is 0.
- [Medium] Incomplete `Send` and `Sync` implementations restrict `ARef<Device<Ctx>>` cross-thread usage.
- [Low] Using `select` on a visible `menuconfig` symbol forces its value and violates Kconfig guidelines.
--

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

This commit implements the basic Rust abstractions for the serial device
bus. It introduces the Driver trait and Device representation to allow
writing serdev device drivers in Rust.

> --- a/drivers/tty/serdev/Kconfig
> +++ b/drivers/tty/serdev/Kconfig
> @@ -9,6 +9,13 @@ menuconfig SERIAL_DEV_BUS
>  
>  	  Note that you typically also want to enable TTY port controller support.
>  
> +config RUST_SERIAL_DEV_BUS_ABSTRACTIONS
> +	bool "Rust Serial device bus abstractions"
> +	depends on RUST
> +	select SERIAL_DEV_BUS

[Severity: Low]
Is it intended to use select on a user-visible menuconfig symbol?

SERIAL_DEV_BUS is a user-visible tristate. Using select here forcefully
coerces SERIAL_DEV_BUS to y when the abstraction is enabled, which prevents
building the serdev subsystem as a module. Kconfig guidelines generally
recommend using depends on for visible symbols instead of select.

[ ... ]

> --- /dev/null
> +++ b/rust/kernel/serdev.rs
> @@ -0,0 +1,605 @@
[ ... ]
> +/// #[vtable]
> +/// impl serdev::Driver for MyDriver {
> +///     type IdInfo = ();
> +///     type Data<'bound> = Self;
> +///     const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = Some(&OF_TABLE);
> +///     const ACPI_ID_TABLE: Option<acpi::IdTable<Self::IdInfo>> = Some(&ACPI_TABLE);
> +///
> +///     fn probe<'bound>(
> +///         sdev: &'bound serdev::Device<Core<'_>>,
> +///         _id_info: Option<&'bound Self::IdInfo>,
> +///     ) -> impl PinInit<Self::Data<'bound>, Error> + 'bound {
> +///         sdev.set_baudrate(115200);

[Severity: High]
Will this documentation example compile?

The Driver probe callback provides a Device parameter with the Core context,
but serial communication methods like set_baudrate are exclusively
implemented on Device with the Bound context.

Because the types differ, calling set_baudrate on the Core device during
probe will result in a compilation error.

Furthermore, PrivateData expects the device reference to have the Bound
context, but probe_callback casts it to CoreInternal, which causes an
internal type mismatch during the PrivateData initialization.

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

[Severity: High]
Does this error cleanup path introduce a race condition leading to a null
pointer dereference?

If an error occurs in Adapter::probe_callback, the ScopeGuard runs and
calls drvdata_obtain, which unlinks the driver data from the device by
setting dev->driver_data to NULL.

However, the device remains open until PrivateData::drop subsequently calls
serdev_device_close. If serial data arrives in this window,
receive_buf_callback executes:

rust/kernel/serdev.rs:Adapter::receive_buf_callback() {
    ...
    let private_data = unsafe { sdev.as_ref().drvdata_borrow::<PrivateData<'_, T>>() };
    let active = private_data.active.lock();
    ...
}

The callback will blindly read the now-NULL dev->driver_data and create a
Rust reference to NULL. When private_data.active.lock() is called, the
kernel will crash with a null pointer dereference.

The device should probably be closed synchronously before unlinking the
driver data.

> +            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 this sequence cause a deadlock during driver initialization?

In Adapter::probe_callback, the private_data.active mutex is locked and
held across the entire execution of T::probe. If a driver attempts
synchronous bidirectional I/O (like writing a query command and waiting
for a response) during probe, the system might deadlock.

When the hardware responds, the RX workqueue thread triggers
Adapter::receive_buf_callback, which unconditionally attempts to lock
the exact same private_data.active mutex before forwarding data. Because
the mutex is held by the sleeping probe thread, receive_buf_callback blocks
indefinitely, stalling the RX workqueue, and T::probe will wait forever
for a response.

[ ... ]
> +    extern "C" fn receive_buf_callback(
> +        sdev: *mut bindings::serdev_device,
> +        buf: *const u8,
> +        length: usize,
> +    ) -> usize {
[ ... ]
> +        // SAFETY: No one has exclusive access to `private_data.driver`.
> +        let data = unsafe { &*private_data.driver.get() };
> +        // SAFETY:
> +        // - `private_data.driver` is pinned.
> +        // - `receive_buf_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()) };
> +
> +        // SAFETY: `buf` is guaranteed to be non-null and has the size of `length`.
> +        let buf = unsafe { core::slice::from_raw_parts(buf, length) };

[Severity: High]
Can core::slice::from_raw_parts be called with a null pointer here?

The C serdev core and underlying TTY layer might signal empty events or
flushes by passing a NULL buffer with a length of 0. In Rust, calling
core::slice::from_raw_parts with a NULL pointer is immediate undefined
behavior, regardless of whether the length is zero, because the standard
library requires the pointer to be non-NULL and aligned.

Could a check like if buf.is_null() or if length == 0 be added before
creating the slice?

[ ... ]
> +// SAFETY: A `Device` is always reference-counted and can be released from any thread.
> +unsafe impl Send for Device {}
> +
> +// SAFETY: `Device` can be shared among threads because all methods of `Device`
> +// (i.e. `Device<Normal>) are thread safe.
> +unsafe impl Sync for Device {}
> +
> +// SAFETY: Same as `Device<Normal>` -- the underlying `struct serdev_device` is the same;
> +// `Bound` is a zero-sized type-state marker that does not affect thread safety.
> +unsafe impl Sync for Device<device::Bound> {}

[Severity: Medium]
Does the Device type with the Bound context need a Send implementation?

The trait implementations unsafe impl Send for Device {} and
unsafe impl Sync for Device {} implicitly apply only to Device with the
Normal context due to the default generic parameter.

While unsafe impl Sync for Device with Bound context was explicitly added,
Send is missing. This means a Device with the Bound context is not Send,
so it cannot be moved across thread boundaries, stored in standard
thread-safe driver state, or safely passed to workqueues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260718-rust_serdev-v16-0-5809384d2e1b@posteo.de?part=1