Re: [PATCH v2] scsi: be2iscsi: use rollback index when freeing MSI-X vectors

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

Pre-existing issues:
- [Critical] Critical out-of-bounds array access and multi-queue degradation due to inverted MSI-X allocation logic.
- [High] Use-After-Free in INTx error recovery due to incorrect MCC work cancellation index.
--

--- Patch [1]: [PATCH v2] scsi: be2iscsi: use rollback index when freeing MSI-X vectors ---
commit fbea15e24723499491cb379659e8bf49028d4fb3
Author: Runyu Xiao <[email protected]>

scsi: be2iscsi: use rollback index when freeing MSI-X vectors

beiscsi_init_irqs() unwinds previously registered MSI-X handlers with
free_irq(pci_irq_vector(pcidev, i), &phwi_context->be_eq[j]). The IRQ
number should match the rollback slot j, not the current loop index i.
Using i passes the wrong IRQ/dev_id pair and leaves earlier handlers
registered on failure.

Use j for pci_irq_vector() in the rollback loop.

> diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
> index fd18d4d3d219b..9833ab06b299a 100644
> --- a/drivers/scsi/be2iscsi/be_main.c
> +++ b/drivers/scsi/be2iscsi/be_main.c
> @@ -878,7 +878,7 @@ static int beiscsi_init_irqs(struct beiscsi_hba *phba)
>  	return 0;
>  free_msix_irqs:
>  	for (j = i - 1; j >= 0; j--) {
> -		free_irq(pci_irq_vector(pcidev, i), &phwi_context->be_eq[j]);
> +		free_irq(pci_irq_vector(pcidev, j), &phwi_context->be_eq[j]);
>  		kfree(phba->msi_name[j]);
>  	}
>  	return ret;

[Severity: Critical]
This is a pre-existing issue, but I noticed a potential problem in
be2iscsi_enable_msix() while reviewing this code.

Is the logic for handling the return value of
pci_alloc_irq_vectors_affinity() inverted?

drivers/scsi/be2iscsi/be_main.c:be2iscsi_enable_msix() {
	if (pci_alloc_irq_vectors_affinity(phba->pcidev, 2, nvec,
			PCI_IRQ_MSIX | PCI_IRQ_AFFINITY, &desc) < 0) {
		phba->num_cpus = nvec - 1;
		return;
	}
	...
	phba->num_cpus = 1;
}

If allocation fails, phba->num_cpus is set up to nvec - 1. Could this
cause an out-of-bounds write on the 64-element be_eq array later in
beiscsi_enable_port()?

drivers/scsi/be2iscsi/be_main.c:beiscsi_enable_port() {
	for (i = 0; i < phba->num_cpus; i++) {
		pbe_eq = &phwi_context->be_eq[i];
		irq_poll_init(&pbe_eq->iopoll, be_iopoll_budget, be_iopoll);
	}
}

[Severity: High]
This is also a pre-existing issue, but I have a question about the error
recovery path in beiscsi_disable_port().

When msix_enabled is false, mcc_work is queued on be_eq[0]. Does this
code fail to cancel the active work because it uses the leftover loop
counter 'i'?

drivers/scsi/be2iscsi/be_main.c:beiscsi_disable_port() {
	if (!unload && beiscsi_hba_in_error(phba)) {
		pbe_eq = &phwi_context->be_eq[i];
		cancel_work_sync(&pbe_eq->mcc_work);
	}
}

Could this result in a use-after-free when the hardware queues are
subsequently destroyed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1