Re: [PATCH v2 4/7] rust: kernel: Add greybus abstractions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Memory leak of `gb_host_device` and driver data on error paths in `Registration::new`
- [High] Use-After-Free in safe `Connection::host_device()` and `Connection::interface()` methods
- [High] Race condition causing Use-After-Free of driver data during `message_send`
- [Medium] Kconfig selects GREYBUS without inheriting SYSFS dependency
- [Medium] Incomplete abstraction: `HdDriver::message_cancel` lacks access to driver state
- [Low] API Documentation erroneously instructs users to call a non-existent method
- [Low] Erroneous safety justification masks protocol struct misunderstanding
--

commit aca3dd0934984891ba1944b198bd9679ec786c2e
Author: Ayush Singh <ayush@beagleboard.org>

rust: kernel: Add greybus abstractions

This commit adds Rust abstractions for the Greybus core, providing the
basic structures, host device abstractions, and protocol types needed to
implement a Greybus host driver in Rust.

> diff --git a/drivers/greybus/Kconfig b/drivers/greybus/Kconfig
> --- a/drivers/greybus/Kconfig
> +++ b/drivers/greybus/Kconfig
[ ... ]
> +config RUST_GREYBUS_ABSTRACTIONS
> +	bool "Rust greybus abstractions"
> +	depends on RUST
> +	select GREYBUS
> +	help
> +	  This enables the Rust abstraction for the Greybus API.

[Severity: Medium]
Does this Kconfig entry miss a dependency on SYSFS?

The menuconfig GREYBUS depends on SYSFS. By using select GREYBUS here
without inheriting the depends on SYSFS clause, could this violate Kconfig
dependency rules and cause unmet direct dependency warnings or build
failures on minimal configurations where SYSFS is disabled?

> diff --git a/rust/kernel/greybus/hd.rs b/rust/kernel/greybus/hd.rs
> --- /dev/null
> +++ b/rust/kernel/greybus/hd.rs
[ ... ]
> +    /// Transmits `msg` to `dest_cport_id`.
> +    ///
> +    /// This may be called in atomic context and therefore must not sleep; queue the message and
> +    /// return. Once the core is allowed to release the message, call
> +    /// [`HostDevice::message_sent`].
> +    fn message_send(data: &Self, dest_cport_id: u16, msg: &Message, gfp_mask: Flags) -> Result;

[Severity: Low]
Is the documentation instructing users to call a non-existent method?

The comment mentions calling HostDevice::message_sent, but this method
does not seem to exist. The framework handles calling sent(0) automatically
when Ok(()) is returned. Should the documentation be updated to reflect the
actual API contract?

[ ... ]
> +    /// Aborts the transmission of a message previously handed to [`HdDriver::message_send`].
> +    ///
> +    /// Always called in process context.
> +    fn message_cancel(msg: &Message);

[Severity: Medium]
Is there a way for a driver to safely access its private state when
cancelling a message?

To cancel a message, a driver typically must remove it from internal
hardware queues, which requires access to its private state. Since
message_cancel omits the &Self parameter (unlike message_send), drivers
might be forced to write unsafe code to chase pointers through the message's
connection to extract their data. Could we add &Self to the trait method?

[ ... ]
> +    pub fn new(
> +        parent: &device::Device,
> +        buffer_size_max: usize,
> +        num_cports: usize,
> +        data: impl PinInit<T, Error>,
> +    ) -> Result<Self> {
> +        // SAFETY: `parent` is a valid device, and the driver table is `'static`. The core only
> +        // ever reads through the driver pointer, so casting away `const` is fine.
> +        let hd = from_err_ptr(unsafe {
> +            bindings::gb_hd_create(
[ ... ]
> +            )
> +        })?;
> +
> +        // SAFETY: `hd` is a valid pointer to `gb_host_device`.
> +        let hd_dev: &Device<device::CoreInternal<'_>> = unsafe { Device::from_raw(hd) };
> +        hd_dev.as_ref().set_drvdata(data)?;
> +
> +        // SAFETY: By the type invariant the host device is valid, and it has not been added yet.
> +        to_result(unsafe { bindings::gb_hd_add(hd) })?;

[Severity: High]
Can this leak the gb_host_device and pinned driver data on error paths?

bindings::gb_hd_create() allocates the gb_host_device and sets its reference
count to 1. If either set_drvdata or bindings::gb_hd_add fails, the ? operator
will return early.

Since the Registration structure hasn't been instantiated yet, its Drop
implementation won't run, meaning there is no cleanup to call gb_hd_put(hd)
or free the pinned driver data, causing a permanent leak.

[ ... ]
> +    fn drop<'a>(&'a mut self) {
> +        // SAFETY: By the type invariant of `Self`, `self.as_raw()` is a pointer to a valid
> +        // `struct gb_host_device`.
> +        unsafe { bindings::gb_hd_del(self.as_raw()) }
> +
> +        {
> +            // SAFETY: `hd` is a valid pointer to `gb_host_device`.
> +            let hd_dev = unsafe { Device::<device::CoreInternal<'_>>::from_raw(self.as_raw()) };
> +            // SAFETY: The driver data was set to a `T` in `Registration::new()` and has not been
> +            // taken since, and this is the only place that takes it.
> +            drop(unsafe { hd_dev.as_ref().drvdata_obtain::<T>() });
> +        }

[Severity: High]
Does this create a use-after-free race condition with preempted message
send threads?

gb_hd_del() is invoked when Registration drops, and it returns synchronously
without waiting for all in-flight gb_message_send operations to finish.
The driver data (T) is immediately extracted and dropped following
gb_hd_del().

If a thread executing gb_operation_request_send is preempted just before
calling gb_message_send, it could resume and invoke the Rust message_send
wrapper, which calls hd.as_ref().drvdata_borrow() and passes a reference
of the freed T to the driver's implementation.

> diff --git a/rust/kernel/greybus/mod.rs b/rust/kernel/greybus/mod.rs
> --- /dev/null
> +++ b/rust/kernel/greybus/mod.rs
[ ... ]
> +    pub const fn interface(&self) -> Option<&Interface> {
> +        // SAFETY: By the type invariant the connection is valid.
> +        let intf_ptr = unsafe { (*self.0.get()).intf };
> +
> +        if intf_ptr.is_null() {
> +            None
> +        } else {
> +            // SAFETY: By the previous check,  intf_ptr is valid.
> +            Some(unsafe { Interface::from_raw(intf_ptr) })
> +        }
> +    }
[ ... ]
> +    pub const fn host_device(&self) -> &hd::Device {
> +        // SAFETY: By the type invariant the connection is valid, and its `hd` is alive for as
> +        // long as the connection is.
> +        unsafe { hd::Device::from_raw((*self.0.get()).hd) }
> +    }

[Severity: High]
Can these methods lead to a use-after-free by returning a dangling pointer?

Safe Rust code can hold an ARef<Connection>, which increments the refcount
of the gb_connection. However, the Greybus core doesn't appear to take a
reference count on the gb_host_device or gb_interface stored in the
gb_connection structure.

When the device is disconnected, if the core frees the host device and
interface, won't this leave dangling pointers inside the still-alive
gb_connection? Calling Connection::interface() or Connection::host_device()
would then safely wrap and return these dangling raw pointers.

> diff --git a/rust/kernel/greybus/protocols.rs b/rust/kernel/greybus/protocols.rs
> --- /dev/null
> +++ b/rust/kernel/greybus/protocols.rs
[ ... ]
> +/// Request for [`GB_SVC_TYPE_INTF_SET_PWRM`].
> +#[repr(transparent)]
> +pub struct GbSvcIntfSetPwrmRequest(bindings::gb_svc_intf_set_pwrm_request);
> +
> +// SAFETY: `gb_svc_intf_set_pwrm_request` is a struct of `u8` fields, so every bit pattern of its
> +// size is a valid instance.
> +unsafe impl kernel::transmute::FromBytes for GbSvcIntfSetPwrmRequest {}

[Severity: Low]
Is the safety comment here factually accurate?

The comment claims the struct contains only u8 fields. However, the
underlying C structure actually contains __le32 quirks and two nested
struct gb_svc_l2_timer_cfg containing __le16 fields. While the FromBytes
implementation might still be safe due to the struct being packed and
composed of integers, could this justification mask a misunderstanding
of the hardware protocol layout?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827-gb-uart-transport-v2-0-a03bb1f5fbd1@beagleboard.org?part=4