Re: [PATCH 4/5] rust: Add `Device::drvdata_borrow_mut`

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Incomplete safety contract for `drvdata_borrow` fails to forbid overlapping with `drvdata_borrow_mut`, leading to an API soundness hole.
- [Low] Missing `#[inline]` attribute on the abstraction function `drvdata_borrow_mut` which forwards to a C binding.
- [Low] Grammatical error in the documentation comment for `drvdata_borrow_mut`.

Pre-existing issues:
- [High] Potential NULL pointer dereference in `receive_buf_callback` due to reliance on an optional C callback (`pause_rx`) to suppress data reception during probe.
--

commit 0556399b0fd78424186d5ad317732507484df8d2
Author: Markus Probst <markus.probst@posteo.de>

rust: Add Device::drvdata_borrow_mut

This patch adds Device::drvdata_borrow_mut to allow callers with exclusive
access to obtain a mutable reference to the driver's private data. This
functionality is intended for use in serdev to provide mutable references
in callbacks.

> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> index 2291d85b6849e..aa2d87c6f4d3e 100644
> --- a/rust/kernel/device.rs
> +++ b/rust/kernel/device.rs
> @@ -258,6 +258,30 @@ pub unsafe fn drvdata_borrow<T>(&self) -> Pin<&T> {
>          //   in `into_foreign()`.
>          unsafe { Pin::<KBox<T>>::borrow(ptr.cast()) }
>      }

[Severity: High]
With the introduction of the mutable variant below, should drvdata_borrow
have an explicit safety contract requiring callers to ensure no overlapping
mutable references exist? Since drvdata_borrow_mut is being added, the
missing contract on drvdata_borrow could lead to aliasing mutable and
immutable references.

> +
> +    /// Borrow the driver's private data bound to this [Device] mutable.

[Severity: Low]
This isn't a bug, but there is a minor grammatical error. Should "mutable"
be "mutably" to properly modify the verb "Borrow"?

> +    ///
> +    /// # Safety
> +    ///
> +    /// - Must only be called after a preceding call to [Device::set_drvdata] and before the
> +    ///   device is fully unbound.
> +    /// - The type T must match the type of the ForeignOwnable previously stored by
> +    ///   [Device::set_drvdata].
> +    /// - The caller must have exclusive access to T.
> +    #[expect(clippy::mut_from_ref)]
> +    pub unsafe fn drvdata_borrow_mut<T>(&self) -> Pin<&mut T> {

[Severity: Low]
Should this abstraction function include the #[inline] attribute? According
to the Rust subsystem guidelines, small functions in abstractions that forward
to C bindings should be annotated with #[inline].

> +        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
> +        let ptr = unsafe { bindings::dev_get_drvdata(self.as_raw()) };
> +
> +        // SAFETY:
> +        // - By the safety requirements of this function, `ptr` comes from a previous call to
> +        //   `into_foreign()`.
> +        // - `dev_get_drvdata()` guarantees to return the same pointer given to `dev_set_drvdata()`
> +        //   in `into_foreign()`.
> +        // - By the safety requirements of this function, `borrow` and `borrow_mut` do not overlap
> +        //   on the same object.
> +        unsafe { Pin::<KBox<T>>::borrow_mut(ptr.cast()) }
> +    }

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem in how
serdev handles data reception during probe, which might be relevant
since this patch is intended for serdev callbacks.

Can receive_buf_callback dereference a NULL pointer if a serdev controller 
does not implement pause_rx?

In rust/kernel/serdev.rs:probe_callback(), the code opens the device and 
relies on pause_rx to suppress data:

    unsafe { bindings::serdev_device_pause_rx(sdev.as_raw()) };
    to_result(unsafe { bindings::serdev_device_open(sdev.as_raw()) })?;

However, the C API treats pause_rx as optional and fails silently:

drivers/tty/serdev/core.c:serdev_device_pause_rx() {
    if (!ctrl || !ctrl->ops->pause_rx)
        return;
    ...
}

If a controller missing pause_rx is probed, serdev_device_open allows 
data to be received. If data arrives asynchronously before probe_callback 
finishes and calls set_drvdata, wouldn't receive_buf_callback unconditionally 
borrow a NULL pointer here:

    let private_data = unsafe { sdev.as_ref().drvdata_borrow::<PrivateData<'_, T>>() };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260906-rust_serdev_probe_refactor-v1-0-69cdae0074ec@posteo.de?part=4