Re: [PATCH v5 3/4] rust: serdev: Replace `active` mutex with receive pause

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] rust: serdev: mutable borrow of shared ref in remove_callback()
--

commit 7873e32e25475629867563ba84c28fd279d7f25d
Author: Markus Probst <markus.probst@posteo.de>

rust: serdev: Replace active mutex with receive pause

This commit replaces the active mutex in the Rust serdev abstraction with
serdev_device_pause_rx() and serdev_device_resume_rx() to fix race
conditions during probe failure and driver unbind.

> diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
> index 17ca504b7f8d6..c16d6593a8d2d 100644
> --- a/rust/kernel/serdev.rs
> +++ b/rust/kernel/serdev.rs
[ ... ]
> @@ -231,6 +198,12 @@ extern "C" fn remove_callback(sdev: *mut bindings::serdev_device) {
>          let data_pinned = unsafe { Pin::new_unchecked(data.assume_init_ref()) };
>  
>          T::unbind(sdev, data_pinned);
> +
> +        // SAFETY: `sdev.as_raw()` is guaranteed to be a valid pointer to `serdev_device`.
> +        unsafe { bindings::serdev_device_pause_rx(sdev.as_raw()) };
> +
> +        // SAFETY: We already established that `data` is guaranteed to be initialized.
> +        unsafe { data.assume_init_drop() };
>      }

[Severity: Medium]
Does Adapter::remove_callback() in rust/kernel/serdev.rs fail to compile
when building this commit with CONFIG_RUST_SERIAL_DEV_BUS_ABSTRACTIONS=y?

Earlier in Adapter::remove_callback(), data is bound as an immutable
shared reference of type &MaybeUninit<T::Data<'_>>:

    let data = unsafe { &*private_data.driver.get() };
    let data_pinned = unsafe { Pin::new_unchecked(data.assume_init_ref()) };

    T::unbind(sdev, data_pinned);

Because MaybeUninit::assume_init_drop(&mut self) requires a mutable
reference, calling data.assume_init_drop() fails with rustc error E0596
(cannot borrow *data as mutable, as it is behind a & reference).

Since receive_buf_callback() can still run concurrently during T::unbind()
and create a shared reference to *private_data.driver.get() before
serdev_device_pause_rx() is called, data cannot be changed to a mutable
reference at the start of remove_callback().

Although a later commit in this series ("rust: serdev: Simplify callbacks")
refactors PrivateData to store T::Data directly and removes the
assume_init_drop() call, should this commit dereference the UnsafeCell
pointer directly after serdev_device_pause_rx() establishes exclusive
access:

    unsafe { (*private_data.driver.get()).assume_init_drop() };

so that git bisection and standalone backporting remain unbroken?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-rust_serdev_probe_refactor-v5-0-a27d862a415a@posteo.de?part=3