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

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

[Runyu Xiao]

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.

Fixes: 831488669a33 ("scsi: be2iscsi: switch to pci_alloc_irq_vectors")
Cc: stable@vger.kernel.org

Assisted-by: OpenAI Codex <noreply@openai.com>
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
---
 drivers/scsi/be2iscsi/be_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
index dc88bc46dcc0..3801da1c0229 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;
-- 
2.34.1

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Flawed MSI-X fallback logic completely breaks the driver on MSI-X allocation failure and disables multiqueue on success.
--

--- Patch [1]: [PATCH] scsi: be2iscsi: use rollback index when freeing MSI-X vectors ---
commit 8e4e7a262620af574aa50d93aa71fefbc719c7d1
Author: Runyu Xiao <runyu.xiao@seu.edu.cn>

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.

> 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: High]
This isn't a bug introduced by this patch, but the MSI-X fallback logic
completely breaks the driver on MSI-X allocation failure and disables
multiqueue on success.

In be2iscsi_enable_msix(), if pci_alloc_irq_vectors_affinity() fails (returns
< 0), the code incorrectly sets phba->num_cpus = nvec - 1:

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;
}

The driver then falls back to INTx and provisions multiple event queues, but
the INTx handler be_isr() is hardcoded to only process be_eq[0]:

drivers/scsi/be2iscsi/be_main.c:be_isr() {
    ...
	pbe_eq = &phwi_context->be_eq[0];

	eq = &phwi_context->be_eq[0].q;
    ...
}

Any I/O submitted to other queues will time out and hang. Conversely, if
allocation succeeds (returns > 0), the code skips the error branch and
unconditionally executes phba->num_cpus = 1, artificially restricting the
driver to 1 I/O queue and effectively disabling multiqueue.

Is it possible to correct the logic so the driver does not hang on fallback
and properly utilizes multiqueue on success?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824092712.2130707-1-runyu.xiao@seu.edu.cn?part=1

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

[Runyu Xiao]

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.

Fixes: 831488669a33 ("scsi: be2iscsi: switch to pci_alloc_irq_vectors")
Cc: stable@vger.kernel.org
Assisted-by: Codex:GPT-5
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
---
Changes in v2:
- Use the mainline Assisted-by format.
- Keep the commit trailers contiguous.

 drivers/scsi/be2iscsi/be_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
index dc88bc46dcc0..3801da1c0229 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;
-- 
2.34.1

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

[sashiko-bot]

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 <runyu.xiao@seu.edu.cn>

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/20260824113246.2235666-1-runyu.xiao@seu.edu.cn?part=1