[PATCH] cxgb4: fix FW SGE queue cleanup on remove

[PATCH] cxgb4: fix FW SGE queue cleanup on remove

[Guangshuo Li]

init_one() allocates the FW SGE queues with setup_fw_sge_queues()
before registering the network devices.

The probe failure path releases these queues with
t4_free_sge_resources(). However, the normal removal path only calls
t4_free_sge_resources() through cxgb_down() when
CXGB4_FULL_INIT_DONE is set.

If the device is successfully probed and removed before it is fully
initialized, such as when none of its network interfaces has been
opened, CXGB4_FULL_INIT_DONE remains clear and the FW SGE queues are
left allocated.

Release any remaining FW SGE queues when removing an adapter which has
not completed full initialization.

This issue was found by manual code inspection.

Fixes: 843bd7db79c8 ("cxgb4: Setup FW queues before registering netdev")
Cc: stable@vger.kernel.org
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
index 1ced6df6eac8..146419c66348 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
@@ -7116,6 +7116,9 @@ static void remove_one(struct pci_dev *pdev)
 
 		if (adapter->flags & CXGB4_FULL_INIT_DONE)
 			cxgb_down(adapter);
+		else if (adapter->sge.fw_evtq.desc ||
+			 adapter->sge.intrq.desc)
+			t4_free_sge_resources(adapter);
 
 		if (adapter->flags & CXGB4_USING_MSIX)
 			free_msix_info(adapter);
-- 
2.43.0

Re: [PATCH] cxgb4: fix FW SGE queue cleanup on remove

[netdev-bot+sashiko]

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

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

- [Medium] Incomplete fix: the CXGB4_FULL_INIT_DONE-gated release of the
  probe-time FW SGE queues is corrected only in remove_one(); the…

--

cxgb4: fix FW SGE queue cleanup on remove

The patch adds a branch to remove_one() so that the FW SGE queues
allocated by setup_fw_sge_queues() during init_one() are released when
CXGB4_FULL_INIT_DONE was never set, for example when no interface of the
adapter was ever opened.

> diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> index 1ced6df6eac8c..146419c663488 100644
> --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> @@ -7116,6 +7116,9 @@ static void remove_one(struct pci_dev *pdev)
>  
>  		if (adapter->flags & CXGB4_FULL_INIT_DONE)
>  			cxgb_down(adapter);
> +		else if (adapter->sge.fw_evtq.desc ||
> +			 adapter->sge.intrq.desc)
> +			t4_free_sge_resources(adapter);

[Severity: Medium]
The same CXGB4_FULL_INIT_DONE assumption that this patch corrects in
remove_one() also exists in the PCI reset pair eeh_reset_prepare() /
eeh_reset_done().  Should that path be fixed too?

eeh_reset_prepare() only releases SGE resources through the same gate:

	for_each_port(adapter, i)
		if (adapter->port[i]->reg_state == NETREG_REGISTERED)
			cxgb_close(adapter->port[i]);
	...
	if (adapter->flags & CXGB4_FULL_INIT_DONE)
		cxgb_down(adapter);

cxgb_close() does not free SGE resources and does not clear
CXGB4_FULL_INIT_DONE, so on an adapter that was probed but never opened
the FW queues stay allocated across the reset.

eeh_reset_done() then re-runs the allocation unconditionally:

	err = setup_fw_sge_queues(adapter);
	if (err) {
		dev_err(adapter->pdev_dev,
			"FW sge queue allocation failed, err %d", err);
		return;
	}

Does this leak the previous fw_evtq (and intrq in the MSI/INTx case)
ring?  setup_fw_sge_queues() overwrites s->fwevtq_msix_idx after taking a
fresh index from cxgb4_get_msix_idx_from_bmap(), and t4_sge_alloc_rxq()
overwrites the ring pointer without looking at or releasing an existing
one:

drivers/net/ethernet/chelsio/cxgb4/sge.c:t4_sge_alloc_rxq() {
	...
	iq->desc = alloc_ring(adap->pdev_dev, iq->size, iq->iqe_len, 0,
			      &iq->phys_addr, NULL, 0,
			      dev_to_node(adap->pdev_dev));
	if (!iq->desc)
		return -ENOMEM;
	...
}

So the old dma_alloc_coherent() ingress ring becomes unreachable, the
previously reserved bit in adapter->msix_bmap is never returned via
cxgb4_free_msix_idx_in_bmap(), and the old firmware ingress queue context
is never released with t4_iq_free().  This repeats on every reset of the
pf == 4 function (sysfs reset, FLR, VFIO reset), and once the msix bitmap
is exhausted setup_fw_sge_queues() returns -ENOMEM and eeh_reset_done()
bails out, leaving the adapter unrecovered.

The new branch in remove_one() cannot recover those allocations, since
their only pointers were already overwritten during the reset.

Would an unconditional t4_free_sge_resources() at both teardown sites be
simpler?  t4_free_sge_resources() already performs the same checks
internally:

drivers/net/ethernet/chelsio/cxgb4/sge.c:t4_free_sge_resources() {
	...
	if (adap->sge.fw_evtq.desc) {
		free_rspq_fl(adap, &adap->sge.fw_evtq, NULL);
		...
	}
	...
	if (adap->sge.intrq.desc)
		free_rspq_fl(adap, &adap->sge.intrq, NULL);
	...
}

and init_one()'s out_free_dev label already calls it unconditionally:

 out_free_dev:
	t4_free_sge_resources(adapter);

Could the changelog also mention the reset path, so that a backporter
knows whether the identical lifecycle mismatch there was considered?

>  
>  		if (adapter->flags & CXGB4_USING_MSIX)
>  			free_msix_info(adapter);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260916025215.2780658-1-lgs201920130244%40gmail.com