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

[PATCH] 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 (verified arithmetically), which would
either 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. Add an explicit check for nr_msix == 0 and also 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>
---
 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..31b80ed6ddec 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);
+	nr_msix -= (ioc->high_iops_queues + iopoll_q_count);
+	if (!nr_msix) {
+		ioc_warn(ioc, "high_iops_queues and iopoll_q_count exceed available MSI-X vectors\n");
+		return;
+	}
 	index = 0;
 
 	list_for_each_entry(reply_q, &ioc->reply_queue_list, list) {
-- 
2.55.0

Re: [PATCH] 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:

New issues:
- [High] The newly added safety check `if (!nr_msix)` mathematically fails to catch integer underflow, defeating the patch's intent to prevent `nr_msix` wrap-around.

Pre-existing issues:
- [Critical] Missing bounds check and lack of CPU hotplug lock (`cpus_read_lock`) allows a TOCTOU race leading to an out-of-bounds array write into `ioc->cpu_msix_table`.
- [High] The fallback CPU assignment loop improperly compares a CPU hardware ID against the aggregate count of online CPUs, terminating assignments prematurely on sparse CPU topologies.
--

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

scsi: mpt3sas: fix nr_msix underflow in _base_assign_reply_queues()

The commit updates the fallback path to correctly reserve both high-iops and
iopoll queues from the round-robin pool. It also adds a check for zero to
prevent divide-by-zero or unsigned wrap-around when the available MSI-X
vectors are exhausted.

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
> index 1af25a22611a1..df9d7affcd0d1 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);
> +	nr_msix -= (ioc->high_iops_queues + iopoll_q_count);
> +	if (!nr_msix) {

[Severity: High]
Since nr_msix is an unsigned int, will this subtraction underflow to a massive
unsigned value if ioc->high_iops_queues + iopoll_q_count is strictly greater
than nr_msix?

If the subtraction wraps around, this check for exactly zero is bypassed,
defeating the patch's intent. This allows the fallback loop to execute with a
massive nr_msix value, which silently breaks CPU affinity grouping.

> +		ioc_warn(ioc, "high_iops_queues and iopoll_q_count exceed available MSI-X vectors\n");
> +		return;
> +	}
>  	index = 0;
>  
>  	list_for_each_entry(reply_q, &ioc->reply_queue_list, list) {

[ ... ]

Later in drivers/scsi/mpt3sas/mpt3sas_base.c:_base_assign_reply_queues():

> 		if (cpu >= nr_cpus)
> 			break;

[Severity: High]
This is a pre-existing issue, but does this check improperly compare a CPU
hardware ID against the aggregate count of online CPUs?

On sparse topologies where CPU hotplugging or VM setups are used, nr_cpus
reflects the total count (e.g. 4), while cpu holds the physical hardware ID
obtained via cpumask_first() and cpumask_next().

If the physical ID reaches 8, this check would cause assignments to terminate
prematurely, leaving remaining CPUs defaulting to vector 0.

Further down in 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);
> 		}

[Severity: High]
This is a pre-existing issue, but is there a missing bounds check and lack of
CPU hotplug lock (cpus_read_lock) here?

During CPU hotplug, if a CPU is brought online concurrently, cpumask_next()
could return a CPU ID that equals or exceeds the snapshotted
ioc->cpu_msix_table_sz.

Because this inner loop lacks a bounds check, this allows a TOCTOU race that
results in an out-of-bounds array write into ioc->cpu_msix_table and memory
corruption.

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