Re: [PATCH] rust: serdev: Fix race condition on driver probe fail

[Markus Probst <markus.probst@posteo.de>]

[-- Attachment #1: Type: text/plain, Size: 3256 bytes --]

On Fri, 2026-09-04 at 23:54 +0000, Markus Probst wrote:
> If `Driver::probe` fails, the pointer to the driver data (`PrivateData`)
> will first be set to NULL by `drvdata_obtain` and only after that the
> serdev device will be closed by Drop. Thus there is a small window in
> which the serdev device is still open, but the pointer to the driver data
> is NULL. Therefore it is possible that `receive_buf_callback` might try to
> access the `active` mutex on a null pointer.
It seems, Sashiko found the same issue in a different unrelated place
too (while reviewing this patch):

https://sashiko.dev/#/patchset/20260905-rust_serdev_fix-v1-1-2ea92b154a6b%40posteo.de

Unfortunately, I cannot fix this one so easily, because the serdev
device needs to be open for the entire lifetime of the "real" driver
data (Driver::Data).

Thanks
- Markus Probst

> 
> Add a separate `ScopeGuard` that will close the device before the driver
> data will be set to NULL on probe failure.
> 
> Fixes: 99f59aa82341 ("rust: add basic serial device bus abstractions")
> Reported-by: Sashiko Bot <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/linux-serial/20260903222159.70A911F000E9@smtp.kernel.org/
> Signed-off-by: Markus Probst <markus.probst@posteo.de>
> ---
> Unfortunately, this also makes the code in probe even more convoluted
> than it already is. I will submit a patch (for the next merge cycle)
> soon, which will address this concern.
> ---
>  rust/kernel/serdev.rs | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/rust/kernel/serdev.rs b/rust/kernel/serdev.rs
> index 17ca504b7f8d..4f57f4c45329 100644
> --- a/rust/kernel/serdev.rs
> +++ b/rust/kernel/serdev.rs
> @@ -190,6 +190,16 @@ extern "C" fn probe_callback(sdev: *mut bindings::serdev_device) -> kernel::ffi:
>              // SAFETY: We have exclusive access to `private_data.open`.
>              unsafe { *private_data.open.get() = true };
>  
> +            let open_guard = ScopeGuard::new(|| {
> +                // SAFETY:
> +                // - `private_data.sdev.as_raw()` is guaranteed to be a pointer to a valid
> +                //   `struct serdev_device`.
> +                // - We just opened the device, thus it is guaranteed to be open.
> +                unsafe { bindings::serdev_device_close(private_data.sdev.as_raw()) };
> +                // SAFETY: We have exclusive access to `private_data.open`.
> +                unsafe { *private_data.open.get() = false };
> +            });
> +
>              let data = T::probe(sdev, info);
>  
>              // SAFETY: We have exclusive access to `private_data.driver`.
> @@ -203,10 +213,12 @@ extern "C" fn probe_callback(sdev: *mut bindings::serdev_device) -> kernel::ffi:
>  
>              drop(active);
>  
> -            result.map(|()| {
> -                private_data.dismiss();
> -                0
> -            })
> +            result?;
> +
> +            open_guard.dismiss();
> +            private_data.dismiss();
> +
> +            Ok(0)
>          })
>      }
>  
> 
> ---
> base-commit: e5e04726cdd043e309677071ab1b65a4b18f422b
> change-id: 20260904-rust_serdev_fix-be3ff9c8a5e8

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]