Re: [PATCH v22 net-next 09/12] net/nebula-matrix: dispatch: add cross-version channel message framework

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
--- Patch [9]: [PATCH v22 net-next 09/12] net/nebula-matrix: dispatch: add cross-version channel message framework ---
commit d32035d9a9bd3114eb253cf0283172847c233d7d
Author: illusion wang <illusion.wang@xxxxxxxxxxxxxxxxx>

net/nebula-matrix: dispatch: add cross-version channel message framework
    
Implement bidirectional channel request/response RPC handlers for 5 dispatch
resource operations: get_vsi_id, get_eth_id, configure_msix_map,
destroy_msix_map, set_mailbox_irq.

> 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 8116643859c79..c82c336f50e5e 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
[ ... ]
> +static int nbl_disp_configure_msix_map(struct nbl_dispatch_mgt *disp_mgt,
> +				       u16 num_net_msix, u16 num_others_msix,
> +				       bool net_msix_mask_en)
> +{
> +	struct nbl_resource_ops *res_ops = disp_mgt->res_ops_tbl->ops;
> +	struct nbl_resource_mgt *p = disp_mgt->res_ops_tbl->priv;
> +	struct nbl_common_info *common = disp_mgt->common;
> +
> +	return NBL_OPS_CALL_RET(res_ops->configure_msix_map, (p,
> +				common->mgt_pf, num_net_msix,
> +				num_others_msix, net_msix_mask_en));
> +}

[Severity: High]
Does nbl_disp_configure_msix_map() need to take a mutex lock before invoking
res_ops->configure_msix_map()?

Calling this resource operation without synchronization might lead to data races
and hardware state corruption if multiple requests arrive simultaneously.

[ ... ]
> +static void nbl_disp_chan_configure_msix_map_resp(void *priv, u16 src_id,
> +						  u16 msg_id, void *data,
> +						  u32 data_len)
> +{
[ ... ]
> +	if (!res_ops->configure_msix_map) {
> +		err = NBL_CHAN_RESP_UNIMPLEMENTED;
> +	} else {
> +		ret = NBL_OPS_CALL_RET(res_ops->configure_msix_map,
> +				       (p, src_id,
> +					le16_to_cpu(param.num_net_msix),
> +					le16_to_cpu(param.num_others_msix),
> +					le16_to_cpu(param.msix_mask_en)));
> +		if (ret)
> +			err = NBL_CHAN_RESP_ERR;
> +		}

[Severity: High]
Should nbl_disp_chan_configure_msix_map_resp() also use locking when invoking
res_ops->configure_msix_map()?

Since this response handler can run from a workqueue context concurrently with
other operations, this might also lead to hardware state corruption.

[ ... ]
> +static void nbl_disp_chan_set_mailbox_irq_resp(void *priv, u16 src_id,
> +					       u16 msg_id, void *data,
> +					       u32 data_len)
> +{
[ ... ]
> +	if (!res_ops->set_mailbox_irq) {
> +		err = NBL_CHAN_RESP_UNIMPLEMENTED;
> +	} else {
> +		ret = NBL_OPS_CALL_RET(res_ops->set_mailbox_irq,
> +				       (p, src_id, vector_id, enable_msix));
> +		if (ret)

[Severity: High]
Similarly, should nbl_disp_chan_set_mailbox_irq_resp() be protected by a lock
when modifying the mailbox IRQ hardware state?

[ ... ]
> @@ -126,6 +562,10 @@ int nbl_disp_init(struct nbl_adapter *adapter)
>  	adapter->core.disp_mgt = disp_mgt;
>  	adapter->intf.dispatch_ops_tbl = disp_ops_tbl;
>  
> +	ret = nbl_disp_setup_msg(disp_mgt);
> +	if (ret)
> +		return ret;
> +
>  	if (common->has_ctrl)
>  		nbl_disp_setup_ctrl_lvl(disp_mgt, NBL_DISP_CTRL_LVL_MGT);
>  

[Severity: High]
Does the dispatch operations table remain uninitialized for non-control PFs
here?

Since nbl_disp_setup_ctrl_lvl() is responsible for populating the disp_ops
table (including setting up the remote message request handlers as fallbacks
when testing control levels), skipping this step when common->has_ctrl is false
might leave the function pointers as NULL.

Could this lead to a NULL pointer dereference when a regular non-control PF
subsequently attempts to invoke a dispatch operation like get_vsi_id or
get_eth_id?