Re: [PATCH v22 net-next 12/12] net/nebula-matrix: add common dev start/stop 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.
---
--- Patch [12]: [PATCH v22 net-next 12/12] net/nebula-matrix: add common dev start/stop operation ---
commit 87deb0868f9edbdb2f584ecc55b9f7839da1fb23
Author: illusion wang <illusion.wang@xxxxxxxxxxxxxxxxx>

net/nebula-matrix: add common dev start/stop operation

This patch adds device start/stop helper functions for MSI-X mapping and 
mailbox interrupts. It creates an unbound global workqueue for mailbox tasks. 
It also extends channel TX logic with polling fallback.

> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_common/nbl_common.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_common/nbl_common.c
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_common/nbl_common.c
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_common/nbl_common.c
[ ... ]
> +int nbl_common_create_wq(struct nbl_common_info *common)
> +{
> +	char wq_name[32];
> +
> +	snprintf(wq_name, sizeof(wq_name), "nbl_wq_%s", pci_name(common->pdev));
> +	common->wq = alloc_workqueue(wq_name, WQ_UNBOUND, 0);

[Severity: Medium]
Is wq_name safely used as the format string here?

Since alloc_workqueue() takes a printf-style format string, passing the
variable directly can cause a format string vulnerability if the PCI name
contains format specifiers. Should this be updated to:
alloc_workqueue("%s", WQ_UNBOUND, 0, wq_name)?

[Severity: Medium]
This patch creates a dedicated workqueue for mailbox tasks, but does the
cleanup task occasionally reschedule itself to the default system workqueue?

In nbl_chan_clean_queue():

    if (more_work && READ_ONCE(chan_info->clean_task))
        schedule_work(chan_info->clean_task);

Should this use queue_work(common->wq, ...) instead to ensure the task
stays on the dedicated workqueue?

> +	if (!common->wq) {
> +		pr_err("Failed to create workqueue nbl_wq\n");
> +		goto alloc_wq_failed;
> +	}
> +
> +	return 0;
> +alloc_wq_failed:
> +	return -ENOMEM;
> +}
[ ... ]
> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_channel/nbl_channel.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_channel/nbl_channel.c
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_channel/nbl_channel.c
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_channel/nbl_channel.c
> @@ -725,6 +725,7 @@ static int nbl_chan_send_msg(struct nbl_channel_mgt *chan_mgt,
>  	struct nbl_common_info *common = chan_mgt->common;
>  	struct nbl_chan_waitqueue_head *wait_head;
>  	struct nbl_chan_tx_param tx_param = { 0 };
> +	int i = NBL_CHAN_TX_WAIT_ACK_TIMES;
>  	struct nbl_chan_info *chan_info =
>  		chan_mgt->chan_info[NBL_CHAN_TYPE_MAILBOX];
>  	struct device *dev = common->dev;

[Severity: High]
Does checking the shutdown flag before incrementing the inflight counter
create a race condition?

Earlier in nbl_chan_send_msg():

    if (READ_ONCE(chan_info->shutdown))
        return -ESHUTDOWN;
    /* Ensure shutdown load is ordered before inflight counter increment */
    smp_rmb();
    atomic_inc(&chan_info->inflight_tx_cnt);

If a thread is preempted after checking the shutdown flag but before
incrementing the counter, could another thread set the shutdown flag, observe
a zero counter, and free the queues? When the first thread resumes, it would
operate on the freed queues. Should the counter be incremented before checking
the flag?

[Severity: High]
When waiting for an interrupt ack in nbl_chan_send_msg(), does a timeout
correctly return an error?

    if (ret == 0) {
        if (READ_ONCE(wait_head->status) ==
            NBL_MBX_STATUS_WAITING) {
            wait_head->status =
                NBL_MBX_STATUS_TIMEOUT;
            wait_head->acked = 0;
            wait_head->ack_data = NULL;
            wait_head->ack_data_len = 0;
        }
        spin_unlock_irq(&wait_head->status_lock);
        dev_err(dev,
            "Channel waiting ack failed, message type: %d, msg id: %u\n",
            chan_send->msg_type, msgid);
        goto inflight_dec_out;
    }

Because ret is 0 from wait_event_timeout() and isn't reassigned to an error
code like -ETIMEDOUT, will the function return 0 and cause the caller to
consume uninitialized or stale response data?

> @@ -852,6 +853,49 @@ static int nbl_chan_send_msg(struct nbl_channel_mgt *chan_mgt,
>  		goto inflight_dec_out;
>  	}
>  
> +	/* Polling: only init/deinit, add shutdown detect to avoid deadlock */
> +	while (i--) {
> +		if (READ_ONCE(chan_info->shutdown)) {
> +			ret = -ESHUTDOWN;
> +			goto inflight_dec_out;
> +		}
> +		/* Order shutdown read before accessing clean_task/workqueue */
> +		smp_rmb();
> +		if (READ_ONCE(chan_info->clean_task) && common->wq)
> +			queue_work(common->wq, chan_info->clean_task);

[Severity: High]
Can this sequence race with device teardown?

In nbl_dev_remove_common_dev(), the teardown sequence appears to set the
task to NULL and cancels the work before the shutdown flag is set:

nbl_dev_remove_common_dev()
    nbl_dev_register_chan_task(dev_mgt, NBL_CHAN_TYPE_MAILBOX, NULL);
    cancel_work_sync(&common_dev->clean_mbx_task);
    nbl_dev_remove_chan_queue(dev_mgt, NBL_CHAN_TYPE_MAILBOX);

If a sender bypasses the shutdown check and enters the polling loop,
can the double read of chan_info->clean_task allow queue_work()
to be called with a NULL pointer, or to queue the work after it was already
cancelled, leading to a use-after-free when the work accesses the freed queues?

> +
> +		spin_lock_irq(&wait_head->status_lock);
> +		if (READ_ONCE(wait_head->acked)) {
[ ... ]