Re: [PATCH v14 net-next 10/11] net/nebula-matrix: add common/ctrl dev init/reinit operation

[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.
---
net/nebula-matrix: add common/ctrl dev init/reinit operation

This commit introduces common and control device setup and teardown routines
for the nebula-matrix network driver. It configures mailbox channels, queue
tasks, and MSI-X initialization.

> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dev.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dev.c
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dev.c
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_core/nbl_dev.c

[ ... ]

> @@ -6,6 +6,159 @@
>  #include <linux/pci.h>
>  #include "nbl_dev.h"

[ ... ]

> +static int nbl_dev_setup_common_dev(struct nbl_adapter *adapter)
> +{
> +	struct nbl_dev_mgt *dev_mgt = adapter->core.dev_mgt;
> +	struct nbl_dispatch_ops *disp_ops = dev_mgt->disp_ops_tbl->ops;
> +	struct nbl_dispatch_mgt *priv = dev_mgt->disp_ops_tbl->priv;
> +	struct nbl_common_info *common = dev_mgt->common;
> +	struct nbl_dev_common *common_dev;
> +	int ret;
> +
> +	common_dev = devm_kzalloc(&adapter->pdev->dev, sizeof(*common_dev),
> +				  GFP_KERNEL);
> +	if (!common_dev)
> +		return -ENOMEM;
> +	common_dev->dev_mgt = dev_mgt;
> +
> +	ret = nbl_dev_setup_chan_queue(dev_mgt, NBL_CHAN_TYPE_MAILBOX);
> +	if (ret)
> +		return ret;
> +
> +	INIT_WORK(&common_dev->clean_mbx_task, nbl_dev_clean_mailbox_task);
> +	common->vsi_id = disp_ops->get_vsi_id(priv, NBL_VSI_DATA);
> +	if (common->vsi_id == U32_MAX)
> +		return -ENOENT;

If disp_ops->get_vsi_id() or disp_ops->get_eth_id() fail below, does this
leak the mailbox channel queue?

Because dev_mgt->common_dev is not assigned until the very end of this
function, the standard cleanup routine nbl_dev_remove_common_dev() will
return early due to its NULL check. It seems the queue will not be destroyed
in these error paths.

> +	ret = disp_ops->get_eth_id(priv, common->vsi_id, &common->eth_num,
> +			     &common->eth_id, &common->logic_eth_id);
> +	if (ret)
> +		return ret;
> +	nbl_dev_register_chan_task(dev_mgt, NBL_CHAN_TYPE_MAILBOX,
> +				   &common_dev->clean_mbx_task);
> +
> +	dev_mgt->common_dev = common_dev;
> +	nbl_dev_init_msix_cnt(dev_mgt);
> +
> +	return 0;
> +}
> +
> +static void nbl_dev_remove_common_dev(struct nbl_adapter *adapter)
> +{
> +	struct nbl_dev_mgt *dev_mgt = adapter->core.dev_mgt;
> +	struct nbl_dev_common *common_dev = dev_mgt->common_dev;
> +
> +	if (!common_dev)
> +		return;
> +
> +	nbl_dev_remove_chan_queue(dev_mgt, NBL_CHAN_TYPE_MAILBOX);

Can this lead to a race condition and a potential use-after-free?

By tearing down the channel queue before unregistering the task, a pending
or running clean_mbx_task could execute on the destroyed queue.

Additionally, is a call to cancel_work_sync() needed here for the
clean_mbx_task? Since common_dev is allocated via devm_kzalloc, it will be
freed when the device detaches. If the work is still pending or running, it
might try to access the freed memory.

> +	nbl_dev_register_chan_task(dev_mgt, NBL_CHAN_TYPE_MAILBOX, NULL);
> +}

[ ... ]