Re: [PATCH v4 4/6] drivers: greybus: Add software SVC implementation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] greybus: gb_softsvc: synchronous data_rcvd() in message_send() races timer
- [High] greybus: gb_softsvc: GLOBAL_STATE published before SVC handshake in init()
- [High] greybus: gb_softsvc: premature intf_map removal and race in module_remove()
- [Medium] greybus: gb_softsvc: drop under spinlock and non-atomic loop in map_remove()
- [Medium] greybus: gb_softsvc: Svc::handler() leaks internal ENOTSUPP to userspace
- [Low] greybus: gb_softsvc: stale API docs and wrong constant in refclk helper
--

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

drivers: greybus: Add software SVC implementation

This commit adds gb-softsvc, an in-kernel Rust implementation of the Greybus
supervisory controller for transports that do not have a hardware controller
on the bus. It registers a Greybus host device and exposes an interface for
transport drivers to attach modules and exchange Greybus messages.

> diff --git a/drivers/greybus/gb_softsvc.rs b/drivers/greybus/gb_softsvc.rs
> new file mode 100644
> index 0000000000000..f88d9b3fd5655
> --- /dev/null
> +++ b/drivers/greybus/gb_softsvc.rs
> @@ -0,0 +1,520 @@

[ ... ]

> +impl Svc {

[ ... ]

> +    fn map_remove(&self, primary_id: u8, intf_count: u8, dev: &kernel::device::Device) {
> +        let id = match primary_id.checked_sub(INTF_ID_START) {
> +            Some(x) => x,
> +            None => {
> +                dev_warn!(dev, "Invalid interace id");
> +                return;
> +            }
> +        };
> +
> +        for i in 0..intf_count {
> +            let _ = self.intf_map.lock()[usize::from(id + i)].take();
> +        }
> +    }

[Severity: Medium]
In Svc::map_remove(), does the loop drop the removed Arc<dyn InterfaceOps>
while holding the intf_map spinlock, and clear multi-interface ranges
non-atomically?

Because let _ = self.intf_map.lock()[usize::from(id + i)].take(); uses a
wildcard pattern that does not bind the returned Option<Arc<dyn InterfaceOps>>,
Rust drops the temporary Option<Arc<dyn InterfaceOps>> at the end of the
initializer expression before dropping the statement-scoped temporary spinlock
Guard returned by self.intf_map.lock() at the semicolon. When intf_map holds
the last Arc reference, the transport driver's InterfaceOps destructor and
kfree run inside the SpinLock section.

In addition, acquiring and releasing self.intf_map on each iteration of the
0..intf_count loop rather than holding the lock across the entire range allows
a concurrent Svc::map_insert() to observe a partially removed multi-interface
window.

[ ... ]

> +    fn module_remove(&self, hd: &hd::Device, intf_id: u8, intf_count: u8) -> Result<()> {
> +        let payload = protocols::GbSvcModuleRemovedRequest::new(intf_id);
> +        let res = self.send_request(hd, protocols::GB_SVC_TYPE_MODULE_REMOVED, payload);
> +
> +        if res.is_err() {
> +            dev_warn!(
> +                hd.as_ref(),
> +                "Failed to send module_removed svc request for {}",
> +                intf_id
> +            );
> +        }
> +
> +        self.map_remove(intf_id, intf_count, hd.as_ref());
> +
> +        Ok(())
> +    }

[Severity: High]
When Module::drop() calls Svc::module_remove(), can clearing self.intf_map
immediately after queuing GB_SVC_TYPE_MODULE_REMOVED race with asynchronous
Greybus core teardown and concurrent GbHdDriver::message_send() calls?

Svc::send_request() calls hd.data_rcvd(), which only queues operation->work
onto svc->connection->wq, and gb_svc_request_handler() then queues
gb_svc_process_module_removed() onto svc->wq. Until
gb_svc_process_module_removed() runs gb_module_del() on svc->wq, the old
module's struct gb_module, struct gb_interface, and struct gb_connection
objects remain active in hd->modules and hd->connections:

1. If Module::new() is called before gb_svc_process_module_removed() finishes
(or after send_request() in Svc::module_remove() fails), Svc::map_insert() can
reallocate the cleared intf_id to the new module while the old module is still
registered. Outbound messages on the old module's connections then look up
intf_by_id(intf_id) in GbHdDriver::message_send() and are misrouted to the new
module's InterfaceOps::write(), inbound messages from the new module via
Module::submit_message() match the old module's connections in
hd->connections, and gb_svc_process_module_inserted() rejects the new module
if the old gb_module is still present in hd->modules.

2. While gb_module_del() tears down the old module's connections on svc->wq,
clearing intf_map beforehand causes outgoing teardown messages to fail with
ENODEV.

3. In GbHdDriver::message_send(), data.0.intf_by_id() clones the
Arc<dyn InterfaceOps> and drops the intf_map spinlock before calling
intf.write(&buf, conn.intf_cport_id(), gfp_mask). Because Module::drop() does
not synchronize with in-flight GbHdDriver::message_send() calls or wait for
connection teardown, InterfaceOps::write() can still execute after
Module::drop() returns, contrary to the doc comment on impl Drop for Module.

[ ... ]

> +    fn send_response<P: AsBytes>(
> +        &self,
> +        hd: &hd::Device,
> +        msg: &protocols::GbOperationMsgHdr,
> +        payload: P,
> +    ) {
> +        let msg = Msg::response(msg, 0, payload);
> +        hd.data_rcvd(protocols::GB_SVC_CPORT_ID, msg.as_bytes());
> +    }

[Severity: Critical]
When GbHdDriver::message_send() handles a request for GB_SVC_CPORT_ID by
synchronously calling Svc::handler() -> Svc::send_response() ->
hd.data_rcvd(), can the response completion race with timer setup in
gb_operation_request_send() and leave an active timer on a freed
struct gb_operation?

Every synchronous request sent by the Greybus core with a non-zero timeout
(such as gb_svc_pwrmon_rail_count_get(), gb_svc_intf_device_id(),
gb_svc_intf_set_power_mode(), gb_svc_intf_vsys_set(),
gb_svc_intf_refclk_set(), gb_svc_intf_unipro_set(), gb_svc_intf_activate(),
gb_svc_dme_peer_get(), gb_svc_dme_peer_set(), gb_svc_route_create(), and
gb_svc_connection_create()) calls gb_operation_sync_timeout() ->
gb_operation_request_send().

In drivers/greybus/operation.c:gb_operation_request_send(), gb_message_send()
is invoked before operation->timer is armed:

drivers/greybus/operation.c:gb_operation_request_send() {
    ...
	ret = gb_message_send(operation->request, gfp);
	if (ret)
		goto err_put_active;

	if (timeout) {
		operation->timer.expires = jiffies + msecs_to_jiffies(timeout);
		add_timer(&operation->timer);
	}
    ...
}

Because Svc::send_response() calls hd.data_rcvd() synchronously inside
GbHdDriver::message_send() before gb_message_send() returns,
gb_connection_recv_response() immediately queues operation->work
(gb_operation_work()) onto gb_operation_completion_wq (WQ_PERCPU):

drivers/greybus/operation.c:gb_operation_work() {
    ...
	} else {
		ret = timer_delete_sync(&operation->timer);
		if (!ret) {
			/* Cancel request message if scheduled by timeout. */
			if (gb_operation_result(operation) == -ETIMEDOUT)
				gb_message_cancel(operation->request);
		}

		operation->callback(operation);
	}

	gb_operation_put_active(operation);
	gb_operation_put(operation);
}

If gb_operation_work() runs on another processor or preempts the sending
thread before gb_operation_request_send() reaches add_timer(&operation->timer),
timer_delete_sync(&operation->timer) runs while the timer is still unarmed,
operation->callback() completes operation->completion, and the active
reference is dropped.

When the sending thread resumes in gb_operation_request_send(), it calls
add_timer(&operation->timer) on the already-completed operation;
wait_for_completion_interruptible() then returns immediately and
gb_operation_sync_timeout() calls gb_operation_put(operation), freeing
struct gb_operation to gb_operation_cache while operation->timer remains
queued in the timer wheel and causing a use-after-free in
gb_operation_timeout() when the timer expires.

[ ... ]

> +    fn intf_refclk_enable_disable(&self, hd: &hd::Device, hdr: &protocols::GbOperationMsgHdr) {
> +        let payload = protocols::GbSvcIntfRefclkResponse::new(protocols::GB_SVC_INTF_VSYS_OK);
> +        self.send_response(hd, hdr, payload);
> +    }

[Severity: Low]
This isn't a runtime bug since GB_SVC_INTF_VSYS_OK and GB_SVC_INTF_REFCLK_OK
both evaluate to 0x00, but should Svc::intf_refclk_enable_disable() pass
protocols::GB_SVC_INTF_REFCLK_OK to protocols::GbSvcIntfRefclkResponse::new()?

There are also a few minor comment and documentation mismatches in
drivers/greybus/gb_softsvc.rs and the commit description:

1. The commit description and the comments in Svc::conn_create() and
Svc::conn_destroy() refer to NodeOps, module_insert(), and module_remove(),
whereas the public trait and lifecycle interface are InterfaceOps,
Module::new(), and Drop for Module.

2. The doc comment on Module::new() refers to parameter node (now intfs) and
states that it returns the interface id rather than Result<Self>, while the
doc comment on Module::submit_message() states that it fails with EAGAIN if
the host device has not been brought up yet (which is checked in Module::new()
via GLOBAL_STATE, whereas Module::submit_message() calls
hd::Device::data_rcvd_by_intf() and only returns Ok(()) or EINVAL).

3. Svc::map_remove() logs "Invalid interace id" (missing 'f' in "interface").

[ ... ]

> +            (true, protocols::GB_SVC_TYPE_PROTOCOL_VERSION) => self.send_svc_hello(hd)?,
> +            (
> +                true,
> +                protocols::GB_SVC_TYPE_MODULE_INSERTED
> +                | protocols::GB_SVC_TYPE_SVC_HELLO
> +                | protocols::GB_SVC_TYPE_MODULE_REMOVED,
> +            ) => {}
> +            _ => return Err(code::ENOTSUPP),
> +        };
> +
> +        Ok(())
> +    }
> +}

[Severity: Medium]
Should the fallback arm in Svc::handler() return a standard user space error
code such as EOPNOTSUPP or EPROTONOSUPPORT instead of internal kernel errno
ENOTSUPP (524)?

When gb_softsvc sends GB_SVC_TYPE_SVC_HELLO and attaches a module via
Module::new() -> Svc::module_insert(), the Greybus core exposes the
intf_eject sysfs attribute on the greybus_svc device, the eject sysfs
attribute on the greybus_module device, and (because Svc::intf_activate()
reports GB_SVC_INTF_TYPE_GREYBUS and interface_power_is_visible() in
drivers/greybus/interface.c checks only intf->type) the voltage_now,
current_now, and power_now sysfs attributes on the interface device.

When user space reads voltage_now, current_now, or power_now or writes to
eject or intf_eject, the Greybus core sends
GB_SVC_TYPE_PWRMON_INTF_SAMPLE_GET or GB_SVC_TYPE_INTF_EJECT synchronously via
gb_operation_sync_timeout() -> gb_operation_request_send_sync_timeout() ->
gb_operation_request_send() -> gb_message_send() ->
HdDriverVTable::<GbHdDriver>::message_send() -> GbHdDriver::message_send() ->
Svc::handler().

Unlike wire responses that pass through gb_operation_status_map(), the
Err(code::ENOTSUPP) returned from GbHdDriver::message_send() is propagated
directly by gb_operation_request_send() to the sysfs callbacks, leaking -524
(-ENOTSUPP) to user space.

> +
> +struct GbHdDriver(Arc<Svc>);
> +
> +#[vtable]
> +impl hd::HdDriver for GbHdDriver {
> +    fn message_send(
> +        data: &Self,
> +        hd: &hd::Device,
> +        dest_cport_id: u16,
> +        msg: &greybus::Message,
> +        gfp_mask: Flags,
> +    ) -> Result {
> +        if dest_cport_id == protocols::GB_SVC_CPORT_ID {
> +            data.0.handler(hd, msg)
> +        } else {
> +            let conn = msg.operation().connection();
> +            let intf = data.0.intf_by_id(conn.interface().unwrap().id())?;
> +
> +            let mut buf = KVec::with_capacity(msg.header().size().into(), gfp_mask)?;
> +
> +            buf.extend_from_slice(msg.header().as_bytes(), gfp_mask)?;
> +            buf.extend_from_slice(msg.payload_bytes(), gfp_mask)?;
> +
> +            intf.write(&buf, conn.intf_cport_id(), gfp_mask)
> +        }
> +    }
> +
> +    fn message_cancel(_msg: &greybus::Message) {}
> +}
> +
> +struct GreybusSoftSvc {
> +    _hd: hd::Registration<GbHdDriver>,
> +    _faux: faux::Registration,
> +}
> +
> +impl kernel::Module for GreybusSoftSvc {
> +    fn init(_module: &'static ThisModule) -> Result<Self> {
> +        pr_info!("gb_softsvc (init)\n");
> +
> +        // SAFETY: This runs once at module init, before anything else can reach `GLOBAL_STATE`.
> +        unsafe { GLOBAL_STATE.init() };
> +
> +        let faux = faux::Registration::new(c_str!("gb-softsvc"), None)?;
> +        let svc = Arc::pin_init(Svc::new(), GFP_KERNEL)?;
> +        let data = GbHdDriver(svc.clone());
> +
> +        let dev = faux.as_ref().as_ref();
> +        let hd = hd::Registration::new(dev, GB_OPERATION_SIZE_MAX, CPORT_ID_MAX + 1, Ok(data))?;
> +
> +        svc.send_version(hd.as_ref())?;
> +
> +        let global_state = GlobalState::new(svc.clone(), hd.as_ref().into());
> +        let _ = GLOBAL_STATE.lock().replace(global_state);

[Severity: High]
In GreybusSoftSvc::init(), is GLOBAL_STATE published too early, before the
asynchronous GB_SVC_TYPE_PROTOCOL_VERSION and GB_SVC_TYPE_SVC_HELLO handshake
completes?

svc.send_version(hd.as_ref()) calls hd.data_rcvd(GB_SVC_CPORT_ID, ...), which
invokes gb_connection_recv_request() and only queues operation->work onto the
ordered workqueue svc->connection->wq without processing it synchronously.

In drivers/greybus/svc.c:gb_svc_request_handler(), the state machine remains
in GB_SVC_STATE_RESET until the work item for GB_SVC_TYPE_PROTOCOL_VERSION
runs, sends its response back to Svc::handler() to queue
Svc::send_svc_hello(), and the subsequent work item for GB_SVC_TYPE_SVC_HELLO
transitions svc->state to GB_SVC_STATE_SVC_HELLO:

drivers/greybus/svc.c:gb_svc_request_handler() {
    ...
	switch (type) {
	case GB_SVC_TYPE_PROTOCOL_VERSION:
		if (svc->state != GB_SVC_STATE_RESET)
			ret = -EINVAL;
		break;
	case GB_SVC_TYPE_SVC_HELLO:
		if (svc->state != GB_SVC_STATE_PROTOCOL_VERSION)
			ret = -EINVAL;
		break;
	default:
		if (svc->state != GB_SVC_STATE_SVC_HELLO)
			ret = -EINVAL;
		break;
	}
    ...
}

If a consumer driver calls Module::new() after GLOBAL_STATE is populated in
GreybusSoftSvc::init() but before the GB_SVC_TYPE_PROTOCOL_VERSION response
triggers Svc::send_svc_hello(), Module::new() sees GLOBAL_STATE is Some
(instead of returning EAGAIN) and calls state.svc.module_insert(), queuing
GB_SVC_TYPE_MODULE_INSERTED onto svc->connection->wq ahead of
GB_SVC_TYPE_SVC_HELLO.

When gb_svc_request_handler() processes GB_SVC_TYPE_MODULE_INSERTED while
svc->state != GB_SVC_STATE_SVC_HELLO, it rejects the request with -EINVAL.
Because Svc::handler() ignores the response status for
GB_SVC_TYPE_MODULE_INSERTED, Module::new() returns Ok(Module) while no
gb_module or gb_interface is created in the Greybus core, and the allocated
interface ids and Arc<dyn InterfaceOps> entries remain occupied in
Svc::intf_map until Module is dropped.

[ ... ]

> +impl Module {
> +    /// Attaches `node` as a new module and returns the interface id assigned to it.
> +    pub fn new(intfs: &[Arc<dyn InterfaceOps>]) -> Result<Self> {
> +        let Ok(intf_count) = u8::try_from(intfs.len()) else {
> +            return Err(code::E2BIG);
> +        };
> +
> +        if intf_count == 0 {
> +            return Err(code::EINVAL);
> +        }
> +
> +        let guard = GLOBAL_STATE.lock();
> +        let state = guard.as_ref().ok_or(code::EAGAIN)?;
> +        let id = state.svc.module_insert(&state.gb_hd, intfs)?;
> +
> +        Ok(Self {
> +            id,
> +            intf_count,
> +            gb_hd: state.gb_hd.clone(),
> +            svc: state.svc.clone(),
> +        })
> +    }

[ ... ]

> +/// Dropping the module drops all the interfaces of the module. So none of the interface ops should
> +/// be called after this point.
> +impl Drop for Module {
> +    fn drop(&mut self) {
> +        let _ = self
> +            .svc
> +            .module_remove(&self.gb_hd, self.id, self.intf_count);
> +    }
> +}

[ ... ]

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