Re: [PATCH 4/7] rust: kernel: Add greybus abstractions
flat view
From: sashiko-bot@kernel.org
Date: 2026-08-20 09:42:56
Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider: - [Critical] `Registration::drop` frees driver private data before calling `gb_hd_del()`, allowing Use-After-Free in concurrent callbacks. - [High] Missing and incorrect `Send`/`Sync` bounds on Greybus types break async usage and Rust thread safety. - [High] `Message` does not implement `Drop`, allowing drivers to silently leak active operations. - [High] `message_cancel` bypasses ownership guarantees, allowing an aliased `Message` to be constructed by value. - [High] The `Registration` API leaks the `gb_host_device` memory and crashes by calling `device_del` on an un-added device on error paths. - [High] Safe Rust API allows double-free of gb_operation if a driver completes a message and then returns an error. - [Medium] `HdDriver::message_send` ignores the provided `gfp_mask`, breaking context-aware memory allocation. - [Low] Conditional compilation of `greybus` abstraction causes linker errors when `CONFIG_GREYBUS=m`. - [Low] Missing `// INVARIANT:` comments on struct constructions. - [Low] Missing `#[inline]` annotations on abstraction wrappers. --
--- Patch [4]: [PATCH 4/7] rust: kernel: Add greybus abstractions ---commit a8c7543823c875208896bddcd8e1e440536e1645
Author: Ayush Singh [off-list ref]
rust: kernel: Add greybus abstractions
Add Rust abstractions for the Greybus core, enough to implement a
Greybus host driver in Rust.
quoted hunk ↗ jump to hunk
diff --git a/rust/kernel/greybus/hd.rs b/rust/kernel/greybus/hd.rs index 0000000000000..56435074f8dc8 --- /dev/null +++ b/rust/kernel/greybus/hd.rs
[ ... ]
quoted hunk ↗ jump to hunk
+ unsafe extern "C" fn message_send( + hd: *mut bindings::gb_host_device, + dest_cport_id: u16, + msg: *mut bindings::gb_message, + _gfp_mask: bindings::gfp_t, + ) -> c_int {
[Severity: Medium] Does this ignore the provided _gfp_mask? If it is not passed to the driver implementation, safe Rust drivers lack the context to determine whether they can sleep (e.g., if they need to allocate memory to queue the message), which could lead to might_sleep() panics.
quoted hunk ↗ jump to hunk
+ // SAFETY: `gb_host_device` and `HostDevice` have the same layout. + let hd = unsafe { Device::<device::CoreInternal<'_>>::from_raw(hd) }; + // SAFETY: `message_send` is only ever called after a successful call to + // `gb_hd_add`, hence it's guaranteed that `Device::set_drvdata()` has been called + // and stored a `Pin<KBox<T>>`. + let data = unsafe { hd.as_ref().drvdata_borrow() }; + // SAFETY: The caller guarantees `msg` is valid for the duration of this call. + let msg = unsafe { Message::from_raw(msg) }; + + match T::message_send(&data, dest_cport_id, msg) { + Ok(()) => 0, + Err(e) => e.to_errno(), + } + }
[Severity: High] Does this safe Rust API allow a double-free of gb_operation? If a driver completes a message by calling msg.sent() (which drops the operation refcount) and then returns Err(e), this wrapper propagates the error back to the C core. The C core will then unconditionally drop the operation refcount again in gb_operation_request_send(), resulting in a use-after-free.
quoted hunk ↗ jump to hunk
+ + /// # Safety + /// + /// `msg` must point at a valid message of a registered host device of this driver. + unsafe extern "C" fn message_cancel(msg: *mut bindings::gb_message) { + // SAFETY: The caller guarantees `msg` is valid for the duration of this call. + let msg = unsafe { Message::from_raw(msg) }; + + T::message_cancel(msg); + }
[Severity: High] Does message_cancel bypass ownership guarantees by constructing an aliased Message by value? If the driver had previously received this same gb_message and stashed it for asynchronous processing, there are now two owned Message instances representing the same underlying C struct. A safe driver could call msg.sent() on both, leading to a double-free. [ ... ]
quoted hunk ↗ jump to hunk
+ pub fn new( + parent: &device::Device, + buffer_size_max: usize, + num_cports: usize, + data: impl PinInit<T, Error>, + ) -> Result<Self> {
[ ... ]
quoted hunk ↗ jump to hunk
+ let res = Self { + ptr: NonNull::new(hd).ok_or(code::ENOMEM)?, + _data: PhantomData, + };
[Severity: Low] Are we missing an // INVARIANT: comment on this struct construction, as required by subsystem/rust.md?
quoted hunk ↗ jump to hunk
+ + res.add()?; + + Ok(res) + }
[Severity: High] If res.add() fails, res is dropped and Registration::drop() is invoked, which calls gb_hd_del(). Since gb_hd_add() never succeeded, does calling device_del() via gb_hd_del() on an un-added device trigger a kobject crash? [ ... ]
+impl<T> Registration<T> {
+ fn add(&self) -> Result<()> {[Severity: Low] Should this small forwarding function have an #[inline] annotation, as per the subsystem guidelines? [ ... ]
quoted hunk ↗ jump to hunk
+impl<T> Drop for Registration<T> { + fn drop<'a>(&'a mut self) { + { + let hd = self.as_raw(); + // SAFETY: By the type invariant `hd` points at a valid host device, and + // `gb_host_device` and `Device` have the same layout. + let hd_dev = unsafe { &*hd.cast::<Device<device::CoreInternal<'a>>>() }; + // 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. + let data = unsafe { hd_dev.as_ref().drvdata_obtain::<T>() }; + drop(data); + } + + // 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()) } + } +}
[Severity: Critical] By destroying the private data T before calling gb_hd_del(), could this lead to a use-after-free in concurrent callbacks? gb_hd_del() is synchronous and flushes active connections. This flushing can trigger the C core to send responses via message_send. The message_send callback borrows the driver data, which was already freed here. [Severity: High] After removing the device, does Registration::drop() need to call gb_hd_put() to release the refcount? gb_hd_create() returns a device with a refcount of 1. By never calling gb_hd_put(), it appears the host device memory is permanently leaked upon unregistering.
quoted hunk ↗ jump to hunk
+ +// SAFETY: The greybus host device API is thread-safe as guaranteed by the device core, as long as +// gb_hd_del() is guaranteed to only be called once - which is guaranteed by our type not +// having Copy/Clone. +unsafe impl<T> Send for Registration<T> {}
[Severity: High] Does this Send implementation require a T: Send bound? Without it, a safe consumer could create a Registration<T> where T is !Send, move it to another thread, and drop it, causing a !Send type to be dropped concurrently.
quoted hunk ↗ jump to hunk
diff --git a/rust/kernel/greybus/mod.rs b/rust/kernel/greybus/mod.rs index 0000000000000..5dd1941574cf8 --- /dev/null +++ b/rust/kernel/greybus/mod.rs
[ ... ]
quoted hunk ↗ jump to hunk
+/// A Greybus message handed to a host driver for transmission. +/// +/// # Invariants +/// +/// The shared reference is only ever handed out for the duration of a [`HdDriver`] callback, during +/// which the Greybus core guarantees the message and its buffer stay alive. +#[repr(transparent)] +pub struct Message(NonNull<bindings::gb_message>);
[Severity: High] Is there a reason this struct doesn't implement Send? Without it, safe asynchronous drivers cannot queue operations to worker threads. [Severity: High] Does this struct need a Drop implementation to prevent silent resource leaks? If a driver simply drops the Message struct without calling sent(), the underlying C gb_operation is permanently leaked.
quoted hunk ↗ jump to hunk
+ +impl Message { + /// # Safety + /// + /// `ptr` must be non-null and point at a valid `struct gb_message` which outlives the + /// returned `Self`. + #[inline] + pub(crate) const unsafe fn from_raw(ptr: *mut bindings::gb_message) -> Self { + // SAFETY: The caller guarantees `ptr` is non-null. + Self(unsafe { NonNull::new_unchecked(ptr) }) + }
[Severity: Low] Is this struct construction missing an // INVARIANT: comment, as required by the subsystem guidelines?
quoted hunk ↗ jump to hunk
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index 17af6e7969751..6d6378ade24ea 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs
[ ... ]
quoted hunk ↗ jump to hunk
@@ -77,6 +77,8 @@ pub mod fs; #[cfg(CONFIG_GPU_BUDDY = "y")] pub mod gpu; +#[cfg(CONFIG_GREYBUS)] +pub mod greybus;
[Severity: Low] Does this cause linker errors when CONFIG_GREYBUS is built as a loadable module? When it is configured as =m, the build system still passes --cfg=CONFIG_GREYBUS to rustc. This statically compiles the module into vmlinux, leading to undefined references to C symbols like gb_hd_create which only exist in the dynamically loadable greybus.ko module. -- Sashiko AI review · https://sashiko.dev/#/patchset/20260820-gb-uart-transport-v1-0-282da14ab7b7@beagleboard.org?part=4