| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <dcab1e61e451aeba27575c8245aef687caf94b23.camel@posteo.de>
Date: Tue, 13 Jan 2026 17:59:16 +0000
From: Markus Probst <markus.probst@...teo.de>
To: Danilo Krummrich <dakr@...nel.org>
Cc: Kari Argillander <kari.argillander@...il.com>, Rob Herring
<robh@...nel.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Jiri
Slaby <jirislaby@...nel.org>, Miguel Ojeda <ojeda@...nel.org>, Boqun Feng
<boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
Björn Roy Baron <bjorn3_gh@...tonmail.com>, Benno
Lossin <lossin@...nel.org>, Andreas Hindborg <a.hindborg@...nel.org>,
Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org,
rust-for-linux@...r.kernel.org
Subject: Re: [PATCH RFC 2/4] rust: add basic serial device bus abstractions
On Tue, 2026-01-13 at 18:37 +0100, Danilo Krummrich wrote:
> On Tue Jan 13, 2026 at 5:15 PM CET, Markus Probst wrote:
> > > > +impl<T: Driver + 'static> Adapter<T> {
> > > > + const OPS: &'static bindings::serdev_device_ops = &bindings::serdev_device_ops {
> > > > + receive_buf: if T::HAS_RECEIVE {
> > > > + Some(Self::receive_buf_callback)
> > > > + } else {
> > > > + None
> > > > + },
> > > > + write_wakeup: if T::HAS_WRITE_WAKEUP {
> > > > + Some(Self::write_wakeup_callback)
> > > > + } else {
> > > > + Some(bindings::serdev_device_write_wakeup)
> > > > + },
> > > > + };
> > > > + const INITIAL_OPS: &'static bindings::serdev_device_ops = &bindings::serdev_device_ops {
> > > > + receive_buf: Some(Self::initial_receive_buf_callback),
> > > > + write_wakeup: if T::HAS_WRITE_WAKEUP_INITIAL {
> > > > + Some(Self::initial_write_wakeup_callback)
> > > > + } else {
> > > > + Some(bindings::serdev_device_write_wakeup)
> > > > + },
> > > > + };
> > > > + const NO_OPS: &'static bindings::serdev_device_ops = &bindings::serdev_device_ops {
> > > > + receive_buf: None,
> > > > + write_wakeup: Some(bindings::serdev_device_write_wakeup),
> > > > + };
> > > > +
> > > > + extern "C" fn probe_callback(sdev: *mut bindings::serdev_device) -> kernel::ffi::c_int {
> > > > + // SAFETY: The serial device bus only ever calls the probe callback with a valid pointer to
> > > > + // a `struct serdev_device`.
> > > > + //
> > > > + // INVARIANT: `sdev` is valid for the duration of `probe_callback()`.
> > > > + let sdev = unsafe { &*sdev.cast::<Device<device::CoreInternal>>() };
> > > > + let info = <Self as driver::Adapter>::id_info(sdev.as_ref());
> > > > +
> > > > + from_result(|| {
> > > > + let data = try_pin_init!(Drvdata {
> > > > + driver <- T::probe(sdev, info),
> > > > + initial_data: Some(Default::default()).into(),
> > > > + late_probe_data: None.into(),
> > > > + });
> > > > +
> > > > + sdev.as_ref().set_drvdata(data)?;
>
> This does not work, a driver can obtain its device private data with
> Device::<Bound>::drvdata() [1].
>
> For this the driver must assert the correct type, but since you use a private
> type instead of the type given by the driver, i.e. T, Device::<Bound>::drvdata()
> will always fail for the driver.
>
> [1] https://rust.docs.kernel.org/kernel/device/struct.Device.html#method.drvdata
I need to fix that.
>
> > > > +
> > > > + // SAFETY: We just set drvdata to `Drvdata<T>`.
> > > > + let data = unsafe { sdev.as_ref().drvdata_borrow::<Drvdata<T>>() };
> > > > +
> > > > + // 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::INITIAL_OPS) };
> > > > +
> > > > + // SAFETY: The serial device bus only ever calls the probe callback with a valid pointer
> > > > + // to a `struct serdev_device`.
> > > > + to_result(unsafe {
> > > > + bindings::devm_serdev_device_open(sdev.as_ref().as_raw(), sdev.as_raw())
> > > > + })?;
>
> I don't think it is a good idea to hardcode this into the probe() callback and
> split it up in multiple stages, we can always solve things like ordering with
> new types, type states and guards.
>
> > > > +
> > > > + // SAFETY: `&data.driver` is guaranteed to be pinned.
> > > > + T::configure(sdev, unsafe { Pin::new_unchecked(&data.driver) }, info)?;
> > > > +
> > > > + if !T::HAS_RECEIVE_INITIAL {
> > > > + // SAFETY:
> > > > + // - It is guaranteed that we have exclusive access to `data.initial_data` and
> > > > + // `data.late_probe_data`.
>
> How is it ensured that this does not run concurrently with
> initial_receive_buf_callback(), etc.?
```
if !T::HAS_RECEIVE_INITIAL {
return 0;
}
```
at the beginning of the function does.
>
> > > > + // - We just initialized `data.initial_data` with Some.
> > > > + unsafe { Self::do_late_probe(sdev, data)? };
> > > > + }
>
> It is a bit unclear to me what you try to achieve here.
>
> Do you want to synchronize an initial data transfer? Then something along the
> lines of what Kari proposes seems reasonable.
>
> Or is the intention that this will run entirely async? But then distinct
> initialization stages as they appear above won't work.
Async.
The driver gets probed.
late_probe will be called, after all the necessary data for the
initialization is there.
Can you explain what doesn't work? The code has been tested with a
prototype driver before submission.
Kari his approach is synchronize inside the probe function, until all
the necessary data is there.
Which one do you think should be used for the abstraction?
>
> > > > +
> > > > + Ok(0)
> > > > + })
> > > > + }
> > > > +
> > > > + /// # Safety
> > > > + ///
> > > > + /// The caller must guarantee, that we have exclusive access to `data.initial_data` and
> > > > + /// `data.late_probe_data`. `data.initial_data` must be Some.
> > > > + /// (i. e. `late_probe` has not been called yet).
> > > > + unsafe fn do_late_probe(sdev: &Device<device::CoreInternal>, data: Pin<&Drvdata<T>>) -> Result {
> > > > + // SAFETY: `&data.driver` is guaranteed to be pinned.
> > > > + let data_driver = unsafe { Pin::new_unchecked(&data.driver) };
> > > > +
> > > > + // SAFETY: The function contract guarantees that we have exclusive access to
> > > > + // `data.initial_data`.
> > > > + let initial_data = unsafe { &mut *data.initial_data.get() };
> > > > +
> > > > + // SAFETY: The function contract guarantees that we have exclusive access to
> > > > + // `data.late_probe_data`.
> > > > + let late_probe_data = unsafe { &mut *data.late_probe_data.get() };
> > > > +
> > > > + *late_probe_data = Some(KBox::pin_init(
> > > > + T::late_probe(
> > > > + sdev,
> > > > + data_driver,
> > > > + // SAFETY: The function contract guarantees that `data.initial_data` is Some.
> > > > + unsafe { initial_data.take().unwrap_unchecked() },
> > > > + ),
> > > > + GFP_KERNEL,
> > > > + )?);
> > > > + // 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) };
> > > > + Ok(())
> > > > + }
>
> <snip>
>
> > > > + extern "C" fn initial_receive_buf_callback(
> > > > + sdev: *mut bindings::serdev_device,
> > > > + buf: *const u8,
> > > > + length: usize,
> > > > + ) -> usize {
> > > > + if !T::HAS_RECEIVE_INITIAL {
> > > > + return 0;
> > > > + }
> > > > +
> > > > + // 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::CoreInternal>>() };
> > > > +
> > > > + // SAFETY: `buf` is guaranteed to be non-null and has the size of `length`.
> > > > + let buf = unsafe { core::slice::from_raw_parts(buf, length) };
> > > > +
> > > > + // SAFETY: `initial_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<Drvdata<T>>>`.
> > > > + let data = unsafe { sdev.as_ref().drvdata_borrow::<Drvdata<T>>() };
> > > > +
> > > > + // SAFETY: `&data.driver` is guaranteed to be pinned.
> > > > + let driver_data = unsafe { Pin::new_unchecked(&data.driver) };
> > > > +
> > > > + // SAFETY:
> > > > + // - `data.initial_data` is always Some until `InitialReceiveResult::Ready` is
> > > > + // returned below.
> > > > + // - It is guaranteed that we have exclusive access to `data.initial_data`.
> > > > + let initial_data = unsafe { (*data.initial_data.get()).as_mut().unwrap_unchecked() };
> > > > +
> > > > + match T::receive_initial(sdev, driver_data, initial_data, buf) {
> > > > + Ok(InitialReceiveResult::Pending(size)) => size,
> > > > + Ok(InitialReceiveResult::Ready(size)) => {
> > > > + // SAFETY:
> > > > + // - It is guaranteed that we have exclusive access to `data.initial_data` and
> > > > + // `data.late_probe_data`.
> > > > + // - We just initialized `data.initial_data` with Some.
> > > > + if let Err(err) = unsafe { Self::do_late_probe(sdev, data) } {
>
> We are calling late_probe() again from initial_receive_buf_callback()? Why?
It only gets called once.
This is gated behind "if T::HAS_RECEIVE_INITIAL". And in probe its
gated behind "if T::HAS_RECEIVE_INITIAL".
If "receive_initial" does not get implemented by the driver, late probe
will be run immediately after the regular probe.
>
> > > > + dev_err!(sdev.as_ref(), "Unable to late probe device: {err:?}\n");
> > > > + // 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::NO_OPS) };
> > > > + return length;
> > > > + }
> > > > + size
> > > > + }
> > > > + Err(err) => {
> > > > + dev_err!(
> > > > + sdev.as_ref(),
> > > > + "Unable to receive initial data for probe: {err:?}.\n"
> > > > + );
> > > > + // 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::NO_OPS) };
> > > > + length
> > > > + }
> > > > + }
> > > > + }
>
> <snip>
>
> > There currently is only one serial device bus driver in the kernel,
> > which needs initial data:
> > drivers/bluetooth/hci_uart.h
> > drivers/bluetooth/hci_ldisc.c
> > drivers/bluetooth/hci_serdev.c
> >
> > This driver retrieves this initial data after probe (not in the probe)
> > and then initializes it with a workqueue. Given it is part of the
> > kernel, I assume this is the intended behaviour.
>
> In this case I assume the driver has a lock protected buffer in its private
> data? Which would be entirely different than what you implement above, no?
Powered by blists - more mailing lists