Re: [PATCH v2 5/7] drivers: greybus: Add software SVC implementation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] `Msg<P>` unsoundly implements `AsBytes` for an unconstrained `P`, allowing types with pointers, padding, or interior mutability to be read as bytes.
- [High] Kconfig `GREYBUS_SOFTSVC` relies on the `kernel::greybus` Rust module but lacks a dependency on `RUST_GREYBUS_ABSTRACTIONS`.
- [High] The `message_send` function calls `intf_by_id`, which acquires a `Mutex` (`self.intf_map.lock()`), but `message_send` can be called from atomic contexts.
- [Medium] Integer underflow panic in `Svc::intf_by_id` if `id` is 0.
--

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

drivers: greybus: Add software SVC implementation

This commit adds gb-softsvc, a Rust implementation of the Greybus SVC
(Supervisory Controller) that lives in the kernel. It provides an SVC
emulation for host drivers on transports without a native SVC.

Link: https://lore.kernel.org/all/20260622-nova-exports-v5-0-6191773fc977@nvidia.com/

> diff --git a/drivers/greybus/Kconfig b/drivers/greybus/Kconfig
> index 3bf3b86914423..381d1a6ee135c 100644
> --- a/drivers/greybus/Kconfig
> +++ b/drivers/greybus/Kconfig

[ ... ]

> @@ -48,5 +48,17 @@ config GREYBUS_ES2
>  	  To compile this code as a module, choose M here: the module
>  	  will be called gb-es2.ko
>  
> +config GREYBUS_SOFTSVC
> +	tristate "Greybus software SVC"
> +	depends on RUST

[Severity: High]
Will this fail to build if RUST_GREYBUS_ABSTRACTIONS is disabled?
Since this driver relies on the kernel::greybus Rust module, it seems it
needs a dependency on RUST_GREYBUS_ABSTRACTIONS to ensure the abstractions
are compiled in.

> +	help
> +	  Select this option to enable a software implementation of the
> +	  Greybus SVC (Supervisory Controller) running in the kernel. It can

[ ... ]

> diff --git a/drivers/greybus/gb_softsvc.rs b/drivers/greybus/gb_softsvc.rs
> new file mode 100644
> index 0000000000000..79e2752e42ba0
> --- /dev/null
> +++ b/drivers/greybus/gb_softsvc.rs

[ ... ]

> +#[repr(C, packed)]
> +struct Msg<P> {
> +    hdr: protocols::GbOperationMsgHdr,
> +    payload: P,
> +}
> +
> +// SAFETY: `Msg` is a POD type with no padding and no interior mutability.
> +unsafe impl<P> kernel::transmute::AsBytes for Msg<P> {}

[Severity: High]
Does this implementation allow unsound transmutations for unconstrained types?
Because P is generic and lacks bounds, any type could be instantiated here,
including types with pointers, padding, or interior mutability. Should this
implementation require P: kernel::transmute::AsBytes to ensure the type
is actually safe to read as bytes?

[ ... ]

> +#[pin_data]
> +struct Svc {
> +    #[pin]
> +    svc_operation_id: SpinLock<IdPool>,
> +    // Pos 0 = INTF_ID_START
> +    #[pin]
> +    intf_map: Mutex<[Option<Arc<dyn InterfaceOps>>; INTF_MAP_LEN]>,
> +}

[ ... ]

> +    fn intf_by_id(&self, id: u8) -> Result<Arc<dyn InterfaceOps>> {
> +        let id = id - INTF_ID_START;

[Severity: Medium]
Can this subtraction underflow and cause a kernel panic?
If a malformed message or core bug routes a message to the AP interface
(where id is 0), id - INTF_ID_START will underflow since INTF_ID_START
is 1.

> +        let guard = self.intf_map.lock();
> +
> +        guard
> +            .get(usize::from(id))
> +            .ok_or(code::ENODEV)?
> +            .as_ref()
> +            .ok_or(code::ENODEV)
> +            .cloned()
> +    }

[ ... ]

> +#[vtable]
> +impl hd::HdDriver for GbHdDriver {
> +    fn message_send(
> +        data: &Self,
> +        dest_cport_id: u16,
> +        msg: &greybus::Message,
> +        gfp_mask: Flags,
> +    ) -> Result {
> +        if dest_cport_id == protocols::GB_SVC_CPORT_ID {
> +            data.0.handler(msg)
> +        } else {
> +            let conn = msg.operation().connection();
> +            let intf = data.0.intf_by_id(conn.interface().unwrap().id())?;

[Severity: High]
Can this lead to a scheduling-while-atomic oops?

The Greybus core can call message_send() from atomic contexts (which is why
it passes a gfp_mask). However, intf_by_id() acquires intf_map.lock(), which
is a sleepable Mutex. Should intf_map use a SpinLock instead to prevent
sleeping in atomic context?

> +
> +            let mut buf = KVec::with_capacity(msg.header().size().into(), gfp_mask)?;

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