[PATCH v2] scsi: mpt3sas: fix nr_msix underflow in _base_assign_reply_queues()

[PATCH v2] scsi: mpt3sas: fix nr_msix underflow in _base_assign_reply_queues()

[Ivy Lopez]

The fallback path incorrectly subtracts iopoll_q_count when computing
nr_msix, when the intent is to reserve both high_iops_queues and
iopoll_q_count reply queues from the round-robin pool. Since
iopoll_q_count can be positive, the current subtraction inflates
nr_msix instead of reducing it, leaving far more queues in the
round-robin pool than actually available once high-iops and iopoll
queues are accounted for.

Beyond producing an incorrect grouping of cpus onto msix vectors, the
corrected formula can still drive nr_msix to zero or below under
plausible queue configurations, which would wrap to a large unsigned
value and silently break affinity grouping, or hit a divide-by-zero in
the following nr_cpus / nr_msix computation. Check whether the
reserved queue count meets or exceeds nr_msix before performing the
subtraction, and warn if the reply queue budget is exhausted.

Fixes: 432bc7caef4e ("scsi: mpt3sas: Add io_uring iopoll support")
Signed-off-by: Ivy Lopez <skunkolee@gmail.com>
---
v2: check reserved queue count against nr_msix before the subtraction
    instead of testing nr_msix for zero afterward, since the subtraction
    itself could wrap an unsigned int rather than land on exactly zero.
    Thanks to Sashiko AI review for catching this.
---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
index ce5a5882acc8..3810038dc2ac 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -3275,7 +3275,11 @@ _base_assign_reply_queues(struct MPT3SAS_ADAPTER *ioc)
 
 fall_back:
 	cpu = cpumask_first(cpu_online_mask);
-	nr_msix -= (ioc->high_iops_queues - iopoll_q_count);
+	if (ioc->high_iops_queues + iopoll_q_count >= nr_msix) {
+		ioc_warn(ioc, "high_iops_queues and iopoll_q_count exceed available MSI-X vectors\n");
+		return;
+	}
+	nr_msix -= (ioc->high_iops_queues + iopoll_q_count);
 	index = 0;
 
 	list_for_each_entry(reply_q, &ioc->reply_queue_list, list) {
-- 
2.55.0

Re: [PATCH v2] scsi: mpt3sas: fix nr_msix underflow in _base_assign_reply_queues()

[sashiko-bot]

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

Pre-existing issues:
- [High] Clamping `ioc->reply_queue_count` to `MaxMSIxVectors` without adjusting `ioc->iopoll_q_start_index` or `ioc->high_iops_queues` causes severe integer underflows when setting up block multi-queue mappings.
- [High] Missing bounds check on `cpu` in `_base_assign_reply_queues` fallback path allows out-of-bounds heap write.
- [Medium] Incorrect loop termination condition `cpu >= nr_cpus` breaks MSI-X vector assignment on sparse CPU masks.
--

commit ae1909467e242e2fb7afde51ded872b5492ad061
Author: Ivy Lopez <skunkolee@gmail.com>

scsi: mpt3sas: fix nr_msix underflow in _base_assign_reply_queues()

This commit fixes a bug in the fallback path of _base_assign_reply_queues()
where iopoll_q_count was incorrectly subtracted from nr_msix. It ensures
that the reserved queue count is checked against nr_msix before performing
the subtraction to prevent underflows that could break affinity grouping or
cause a divide-by-zero.

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
> index 1af25a22611a1..40888b07b5a26 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
> @@ -3275,7 +3275,11 @@ _base_assign_reply_queues(struct MPT3SAS_ADAPTER *ioc)
>  
>  fall_back:
>  	cpu = cpumask_first(cpu_online_mask);
> -	nr_msix -= (ioc->high_iops_queues - iopoll_q_count);
> +	if (ioc->high_iops_queues + iopoll_q_count >= nr_msix) {
> +		ioc_warn(ioc, "high_iops_queues and iopoll_q_count exceed available MSI-X vectors\n");
> +		return;
> +	}
> +	nr_msix -= (ioc->high_iops_queues + iopoll_q_count);
>  	index = 0;
>  
>  	list_for_each_entry(reply_q, &ioc->reply_queue_list, list) {

[Severity: High]
This is a pre-existing issue, but does clamping ioc->reply_queue_count
to MaxMSIxVectors earlier in this function cause integer underflows in
the block layer mappings?

In _base_assign_reply_queues(), the driver initially clamps the count:

drivers/scsi/mpt3sas/mpt3sas_base.c:_base_assign_reply_queues() {
    ...
	nr_msix = ioc->reply_queue_count = min(ioc->reply_queue_count,
					       ioc->facts.MaxMSIxVectors);
    ...
}

But it doesn't adjust ioc->iopoll_q_start_index or ioc->high_iops_queues.
Later, this clamped value is subtracted in scsih_map_queues():

drivers/scsi/mpt3sas/mpt3sas_scsih.c:scsih_map_queues() {
    ...
	int iopoll_q_count = ioc->reply_queue_count - nr_msix_vectors;
    ...
}

And in scsih_init():

drivers/scsi/mpt3sas/mpt3sas_scsih.c:scsih_init() {
    ...
		shost->nr_hw_queues =
		    ioc->reply_queue_count - ioc->high_iops_queues;
    ...
		iopoll_q_count =
		    ioc->reply_queue_count - ioc->iopoll_q_start_index;
    ...
}

If the firmware advertises fewer MSI-X vectors than initially reserved,
could these subtractions evaluate to negative integers that wrap around
to massive unsigned values, causing out-of-bounds loop iterations and
memory allocation failures?

[Severity: Medium]
This is a pre-existing issue, but does the fallback loop handle sparse CPU
masks correctly?

drivers/scsi/mpt3sas/mpt3sas_base.c:_base_assign_reply_queues() {
    ...
	list_for_each_entry(reply_q, &ioc->reply_queue_list, list) {
    ...
		if (cpu >= nr_cpus)
			break;
    ...
		for (i = 0 ; i < group ; i++) {
			ioc->cpu_msix_table[cpu] = reply_q->msix_index;
			cpu = cpumask_next(cpu, cpu_online_mask);
		}
    ...
}

If a system has a sparse CPU topology (e.g., CPU IDs 0 and 4, meaning
nr_cpus is 2), wouldn't the check cpu >= nr_cpus improperly compare a
discrete CPU ID against the aggregate online CPU count and terminate the
loop prematurely?

[Severity: High]
This isn't a bug introduced by this patch, but is there a missing bounds
check that could lead to an out-of-bounds heap write?

Earlier in _base_assign_reply_queues(), nr_cpus is sampled locklessly:

drivers/scsi/mpt3sas/mpt3sas_base.c:_base_assign_reply_queues() {
    ...
	nr_cpus = num_online_cpus();
    ...
}

Later, the inner fallback loop assigns array elements using the CPU ID:

drivers/scsi/mpt3sas/mpt3sas_base.c:_base_assign_reply_queues() {
    ...
		for (i = 0 ; i < group ; i++) {
			ioc->cpu_msix_table[cpu] = reply_q->msix_index;
			cpu = cpumask_next(cpu, cpu_online_mask);
		}
    ...
}

If a new CPU is hotplugged with a higher ID, or a concurrent hot-unplug
reduces the online count while this loop runs, could cpumask_next()
return a CPU ID that exceeds ioc->cpu_msix_table_sz?

Since this fallback loop lacks the bounds check present in the main
SMP affinity path (if (cpu >= ioc->cpu_msix_table_sz)), could this
write past the end of the dynamically allocated cpu_msix_table array?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260906211454.71432-1-skunkolee@gmail.com?part=1