Re: [PATCH v15 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] Delayed device closure and improperly synchronized teardown lead to race conditions, causing NULL pointer dereferences or Use-After-Free in `receive_buf_callback` during device unbind or probe failure.
- [High] Holding the `active` mutex across driver initialization and teardown causes deadlocks if a driver communicates synchronously with the hardware.
- [Low] Code fails to compile due to the use of an undefined type `device::BoundInternal` and an unavailable `drvdata_borrow` method for it.
- [Low] Small forwarding functions in the `Device` abstraction lack the required `#[inline]` annotation.
--

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

rust: add basic serial device bus abstractions

This commit introduces the basic abstractions for writing serial device bus
(serdev) drivers in Rust. It provides traits and structures representing
serdev::Driver, serdev::Device, and adapter logic to bridge the C API.

> diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
> new file mode 100644
> index 0000000000000..0fdb37cff15dd
> --- /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() };

[Severity: High]
Does this code introduce a deadlock? It looks like the active mutex is held
across the driver drop execution. If the driver communicates synchronously with
the hardware and waits for a response, it might block forever because
Adapter::receive_buf_callback() also waits on this active mutex.

> +            *active = false;
> +        }
> +        drop(active);
> +
> +        // 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]
Could this result in a NULL pointer dereference or use-after-free? The device
is closed here, but the driver data has already been cleared by
drvdata_obtain() in the probe error path or post_unbind_rust. This leaves a
window where the device is still active, and if data arrives,
receive_buf_callback() might blindly dereference the NULL drvdata pointer.

> +        }
> +    }
> +}

[ ... ]

> +    extern "C" fn probe_callback(sdev: *mut bindings::serdev_device) -> kernel::ffi::c_int {

[ ... ]

> +            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>>() });
> +            });
> +            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]
Is it safe to hold the active lock across the entire driver initialization
in T::probe? If a driver needs to send a command to the serial device during
probe and wait for an acknowledgment, it will deadlock since
receive_buf_callback() unconditionally waits on this same active mutex.

> +
> +            // SAFETY: We have exclusive access to `private_data.driver`.
> +            let driver = unsafe { &mut *private_data.driver.get() };

[ ... ]

> +    extern "C" fn receive_buf_callback(
> +        sdev: *mut bindings::serdev_device,
> +        buf: *const u8,
> +        length: usize,
> +    ) -> usize {
> +        // SAFETY: The serial device bus only ever calls the receive buf callback with a valid
> +        // pointer to a `struct serdev_device`.
> +        //
> +        // INVARIANT: `sdev` is valid for the duration of `receive_buf_callback()`.
> +        let sdev = unsafe { &*sdev.cast::<Device<device::BoundInternal>>() };

[Severity: Low]
Will this compile when CONFIG_RUST_SERIAL_DEV_BUS_ABSTRACTIONS is enabled?
It appears device::BoundInternal is an undefined type, as the valid contexts
in the kernel are Normal, Bound, Core, and CoreInternal.

> +
> +        // SAFETY: `receive_buf_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>>() };

[Severity: Low]
Does this method exist? It seems drvdata_borrow() is only implemented for
Device<CoreInternal>, so even if BoundInternal existed, this method might not
be available.

> +        let active = private_data.active.lock();

[Severity: High]
Could this unconditionally block and cause a deadlock? As mentioned earlier,
if probe_callback() or PrivateData::drop() holds the active lock while
waiting for incoming data, this callback will be blocked and the workqueue
will deadlock.

> +
> +        if !*active {

[ ... ]

> +// SAFETY: Instances of `Device` are always reference-counted.
> +unsafe impl AlwaysRefCounted for Device {
> +    fn inc_ref(&self) {
> +        self.as_ref().inc_ref();
> +    }

[Severity: Low]
this isn't a bug, but shouldn't small forwarding functions like this be
annotated with #[inline] according to the Rust coding guidelines?

> +
> +    unsafe fn dec_ref(obj: NonNull<Self>) {
> +        // SAFETY: The safety requirements guarantee that the refcount is non-zero.
> +        unsafe { bindings::serdev_device_put(obj.cast().as_ptr()) }
> +    }

[Severity: Low]
this isn't a bug, but does this function also need an #[inline] annotation
for being a small forwarding function?

> +}
> +
> +impl<Ctx: device::DeviceContext> AsRef<device::Device<Ctx>> for Device<Ctx> {
> +    fn as_ref(&self) -> &device::Device<Ctx> {
> +        // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
> +        // `struct serdev_device`.
> +        let dev = unsafe { &raw mut (*self.as_raw()).dev };
> +
> +        // SAFETY: `dev` points to a valid `struct device`.
> +        unsafe { device::Device::from_raw(dev) }
> +    }

[Severity: Low]
this isn't a bug, but should this be annotated with #[inline] as well?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260718-rust_serdev-v15-0-208ed2e6ec15@posteo.de?part=1