Re: [PATCH v28 net-next 08/10] net/nebula-matrix: dispatch: implement channel RPC framework and serialize hardware ops

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 1 · Low: 2

- [High] The five new mailbox responders in nbl_dispatch.c perform no
  authorization of the requesting function, contradicting the commit…
- [Medium] The five new request helpers switch on chan_ops->send_msg()'s
  return value as if it were a positive NBL_CHAN_RESP_* wire status, but…
- [Low] Claim: nbl_disp_setup_msg() registers five mailbox responders
  with no matching unregistration, and nbl_disp_remove() is empty, so in…
- [Low] Claim: the NBL_CHAN_MSG_MAILBOX_SET_IRQ RPC reprograms the
  requesting function's own mailbox MSI-X routing on the responder side…

--

net/nebula-matrix: dispatch: implement channel RPC framework and serialize hardware ops

This adds bidirectional mailbox RPC to the dispatch layer for five
operations (configure_msix_map, destroy_msix_map, set_mailbox_irq,
get_vsi_id, get_eth_id), resolving the dispatch ops at probe time
depending on whether the PF is the control PF or a net-only PF.  It also
introduces disp_mgt->ops_mutex_lock to serialize the mutable hardware
ops and extends the wire protocol with new message types and
NBL_CHAN_RESP_PERM_DENY.

> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dispatch.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dispatch.c
> index b7f404b319fac..7254ed10d478f 100644
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dispatch.c
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dispatch.c
> @@ -3,9 +3,192 @@
>   * Copyright (c) 2026 Nebula Matrix Limited.
>   */
>  #include <linux/device.h>
> +#include <linux/mutex.h>
>  #include <linux/pci.h>
>  #include "nbl_dispatch.h"
>  
> +static int nbl_disp_chan_get_vsi_id_req(struct nbl_dispatch_mgt *disp_mgt,
> +					u16 type, u16 *vsi_id)
> +{
> +	struct nbl_channel_ops *chan_ops = disp_mgt->chan_ops_tbl->ops;
> +	struct nbl_common_info *common = disp_mgt->common;
> +	struct nbl_chan_param_get_vsi_id result = { 0 };
> +	struct nbl_chan_param_get_vsi_id param = { 0 };
> +	struct nbl_chan_send_info chan_send = {0};
> +	int ret;
> +
> +	param.type = cpu_to_le16(type);
> +
> +	nbl_chan_fill_send_info(&chan_send, common->mgt_pf,
> +				NBL_CHAN_MSG_GET_VSI_ID,
> +				&param, sizeof(param), &result,
> +				sizeof(result), 1);
> +	ret = chan_ops->send_msg(disp_mgt->chan_ops_tbl->priv, &chan_send);
> +	if (ret < 0)
> +		return ret;
> +	switch (ret) {
> +	case NBL_CHAN_RESP_OK:
> +		break;
> +	case NBL_CHAN_RESP_UNIMPLEMENTED:
> +		return -EOPNOTSUPP;
> +	case NBL_CHAN_RESP_ERR:
> +		return -EREMOTEIO;
> +	case NBL_CHAN_RESP_PERM_DENY:
> +		return -EPERM;
> +	default:
> +		return -EREMOTEIO;
> +	}

[Severity: Medium]
Can this switch ever observe a value other than 0?

nbl_chan_send_msg() returns READ_ONCE(wait_head->ack_err), and
nbl_chan_recv_ack_msg() clamps every positive wire status before storing
it:

nbl_channel.c:nbl_chan_recv_ack_msg() {
	raw_err = (s32)le32_to_cpu(*(__le32 *)&payload[NBL_CHAN_ACK_RET_POS]);
	if (raw_err > 0 || raw_err < -MAX_ERRNO)
		raw_err = -EREMOTEIO;

	WRITE_ONCE(wait_head->ack_err, raw_err);
}

The responders put the raw positive status on the wire unchanged
(nbl_chan_send_ack() does tmp[NBL_CHAN_ACK_RET_POS] =
cpu_to_le32(chan_ack->err)), so NBL_CHAN_RESP_ERR(1),
NBL_CHAN_RESP_UNIMPLEMENTED(2) and NBL_CHAN_RESP_PERM_DENY(3) all arrive
as -EREMOTEIO and are consumed by the preceding "if (ret < 0) return
ret;".

That would make the UNIMPLEMENTED / ERR / PERM_DENY arms here (and the
identical arms in nbl_disp_chan_get_eth_id_req(),
nbl_disp_chan_cfg_msix_map_req(), nbl_disp_chan_destroy_msix_map_req()
and nbl_disp_chan_set_mailbox_irq_req()) unreachable.

The commit message states:

  "Request-side helpers translate channel wire status codes to standard
   Linux errnos: OK->0, UNIMPLEMENTED->-EOPNOTSUPP, ERR->-EREMOTEIO,
   PERM_DENY->-EPERM."

Does that translation actually happen, given the clamp above?  A remote
UNIMPLEMENTED (returned whenever a res_ops pointer is NULL) would surface
as -EREMOTEIO rather than -EOPNOTSUPP.

> +	if (chan_send.ack_len != sizeof(result)) {
> +		dev_err(disp_mgt->common->dev,
> +			"get_vsi_id: short ACK, ack_len=%u expected %zu\n",
> +			chan_send.ack_len, sizeof(result));
> +		return -EREMOTEIO;
> +	}
> +	*vsi_id = le16_to_cpu(result.vsi_id);
> +	return 0;
> +}
> +
> +static void nbl_disp_chan_get_vsi_id_resp(void *priv, u16 src_id, u16 msg_id,
> +					  void *data, u32 data_len)
> +{
> +	struct nbl_dispatch_mgt *disp_mgt = (struct nbl_dispatch_mgt *)priv;
> +	struct nbl_channel_ops *chan_ops = disp_mgt->chan_ops_tbl->ops;
> +	struct nbl_resource_ops *res_ops = disp_mgt->res_ops_tbl->ops;
> +	struct nbl_resource_mgt *p = disp_mgt->res_ops_tbl->priv;
> +	struct device *dev = disp_mgt->common->dev;
> +	struct nbl_chan_param_get_vsi_id result = { 0 };
> +	struct nbl_chan_param_get_vsi_id param = { 0 };
> +	struct nbl_chan_ack_info chan_ack;
> +	int err = NBL_CHAN_RESP_OK;
> +	u16 vsi_id = 0;
> +	u32 rel_pf_id;
> +	int ret;
> +
> +	ret = nbl_common_func_id_to_rel_pf_id(disp_mgt->common, src_id,
> +					      &rel_pf_id);
> +	if (ret) {
> +		err = NBL_CHAN_RESP_PERM_DENY;
> +		goto ack_out;
> +	}

[Severity: High]
Can this gate ever reject anything?  The commit message says:

  "Every responder validates:
   - src_id against common->max_pf at runtime; rejects VF and
     out-of-range function IDs with PERM_DENY;"

but the helper only does a lower-bound test and then throws the result
away:

nbl_common.c:nbl_common_func_id_to_rel_pf_id() {
	if (!rel_pf_id)
		return -EINVAL;

	if (pf_id < common->mgt_pf)
		return -EINVAL;
	*rel_pf_id = pf_id - common->mgt_pf;
	return 0;
}

With common->mgt_pf = 0 (nbl_core_init() in nbl_main.c hard-codes it),
"pf_id < common->mgt_pf" is never true for the unsigned src_id, so the
helper always returns 0.  rel_pf_id is then unused, and there is no
comparison against common->max_pf in any of the five responders.

Should these responders carry the same rejection that the other caller of
this helper, nbl_res_func_id_to_bdf() in nbl_resource.c, performs?

	if (rel_pf_id >= common->max_pf) {
		dev_err(common->dev,
			"func_id=%u rel_pf_id=%u exceeds max_pf=%u, VF BDF unsupported\n",
			pfid, rel_pf_id, common->max_pf);
		return -EOPNOTSUPP;
	}

As written, can NBL_CHAN_RESP_PERM_DENY be produced by any responder in
this patch?

> +	if (data_len < sizeof(param)) {
> +		err = NBL_CHAN_RESP_ERR;
> +		goto ack_out;
> +	}
> +	memcpy(&param, data, sizeof(param));

[ ... ]

> +static void nbl_disp_chan_destroy_msix_map_resp(void *priv, u16 src_id,
> +						u16 msg_id, void *data,
> +						u32 data_len)
> +{

[ ... ]

> +	ret = nbl_common_func_id_to_rel_pf_id(disp_mgt->common, src_id,
> +					      &rel_pf_id);
> +	if (ret) {
> +		err = NBL_CHAN_RESP_PERM_DENY;
> +		goto ack_out;
> +	}
> +	if (res_ops->destroy_msix_map) {
> +		mutex_lock(&disp_mgt->ops_mutex_lock);
> +		ret = res_ops->destroy_msix_map(p, src_id);
> +		mutex_unlock(&disp_mgt->ops_mutex_lock);

[Severity: High]
Following on from the missing max_pf rejection above, src_id is forwarded
here as func_id with no PF/VF distinction.  The downstream ops do not add
one either:

nbl_res_intr_destroy_msix_map() only rejects func_id >= NBL_MAX_FUNC
(520), and __nbl_res_intr_set_mailbox_irq() in the en_msix == false case
writes mailbox routing for any func_id below that:

nbl_interrupt.c:__nbl_res_intr_set_mailbox_irq() {
	if (!en_msix) {
		hw_ops->set_mailbox_irq(res_mgt->hw_ops_tbl->priv,
					func_id, false, 0);
		hw_ops->flush_write(res_mgt->hw_ops_tbl->priv);
		return 0;
	}

Should a VF function id (8..519) be able to reach destroy_msix_map and
set_mailbox_irq on the control PF?

There is also a cost per message here: __nbl_res_intr_destroy_msix_map()
sleeps unconditionally, even when the function had nothing configured:

nbl_interrupt.c:__nbl_res_intr_destroy_msix_map() {
	ret = __nbl_res_intr_prepare_destroy_msix_map(res_mgt, func_id);
	if (ret)
		return ret;
	mutex_unlock(&intr_mgt->lock);
	usleep_range(NBL_MSIX_DMA_SYNC_MIN_US, NBL_MSIX_DMA_SYNC_MAX_US);
	mutex_lock(&intr_mgt->lock);

Since this runs from the mailbox RX work item with
disp_mgt->ops_mutex_lock held, does each unauthorized
NBL_CHAN_MSG_DESTROY_MSIX_MAP message hold up the cross-PF control path
for a millisecond?

[ ... ]

> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_include/nbl_def_channel.h b/drivers/net/ethernet/nebula-matrix/nbl/nbl_include/nbl_def_channel.h
> index 58a9fa97fbf39..6f8fc14a51f0e 100644
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_include/nbl_def_channel.h
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_include/nbl_def_channel.h
> @@ -18,6 +18,7 @@ enum {
>  	NBL_CHAN_RESP_OK = 0,
>  	NBL_CHAN_RESP_ERR = 1,
>  	NBL_CHAN_RESP_UNIMPLEMENTED = 2,
> +	NBL_CHAN_RESP_PERM_DENY = 3,
>  };

[Severity: Medium]
Given the clamp in nbl_chan_recv_ack_msg() and the always-succeeding
authorization helper, is this new enumerator reachable in either
direction - can a responder set it, and can a requester ever see it?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914123429.56596-1-illusion.wang%40nebula-matrix.com