Re: [PATCH v3 04/10] mpi3mr: Fix NVMe page size caching for non-operational devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Double-write of `reset_to` and `abort_to` timeouts introduces a race condition where concurrent readers may observe a reduced default timeout.
- [Medium] The bounds check for `page_size` accepts invalid NVMe page size shifts (1 to 11), which causes management IOCTL validation to fail before reaching the firmware.
--

--- Patch [4]: [PATCH v3 04/10] mpi3mr: Fix NVMe page size caching for non-operational devices ---
commit b27f8d86d47076feb2c7ac1adf8a15a3291cf854
Author: Ranjan Kumar <ranjan.kumar@broadcom.com>

mpi3mr: Fix NVMe page size caching for non-operational devices

This commit populates the NVMe page size attribute for devices in an error
state so management IOCTLs can reach the firmware. It adds bounds checking
for page_size and initializes reset_to and abort_to with default values.

Closes: https://sashiko.dev/#/patchset/20260626114109.43685-1-ranjan.kumar@broadcom.com?part=4
Closes: https://sashiko.dev/#/patchset/20260708183305.244485-1-ranjan.kumar@broadcom.com?part=4

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index d2a20f2721dbf..7b86152922bad 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -1354,12 +1354,16 @@ static void mpi3mr_update_tgtdev(struct mpi3mr_ioc *mrioc,
>  		tgtdev->dev_spec.pcie_inf.capb =
>  		    le32_to_cpu(pcieinf->capabilities);
>  		tgtdev->dev_spec.pcie_inf.mdts = MPI3MR_DEFAULT_MDTS;
> -		/* 2^12 = 4096 */
> -		tgtdev->dev_spec.pcie_inf.pgsz = 12;
> +		tgtdev->dev_spec.pcie_inf.reset_to = MPI3MR_INTADMCMD_TIMEOUT;
> +		tgtdev->dev_spec.pcie_inf.abort_to = MPI3MR_INTADMCMD_TIMEOUT;

[Severity: Medium]
Can a concurrent reader observe the intermediate default timeout values here?

If a Task Management Request (via mpi3mr_issue_tm()) executes concurrently
with a DevStatusChange event processed by mpi3mr_devinfochg_evt_bh(), it might
read these prematurely shortened 60-second timeouts.

Since mpi3mr_update_tgtdev() modifies tgtdev without holding tgtdev_lock, the
reader can observe this intermediate value before it is overwritten with the
final firmware-provided timeout a few lines further down. This could cause
the command to time out early and trigger an unintended full controller reset.

> +		/* Validate firmware page size to prevent undefined shift behavior */
> +		if (pcieinf->page_size > 0 && pcieinf->page_size < 31)
> +			tgtdev->dev_spec.pcie_inf.pgsz = pcieinf->page_size;

[Severity: Medium]
Does this bounds check accept values that are logically invalid for NVMe
memory page sizes?

If the firmware provides corrupted device page0 data with a page_size between
1 and 11, this condition incorrectly accepts it.

Later, when an application issues an IOCTL, the PRP builder in mpi3mr_app.c
evaluates dev_pgsz = 1 << pgsz to a small value (e.g., 2 to 2048 bytes). This
fails size constraints and rejects the IOCTL before it reaches the firmware,
returning incorrect errors to userspace.

> +		else
> +			tgtdev->dev_spec.pcie_inf.pgsz = 12; /* Default to 4096 (2^12) */
>  		if (dev_pg0->access_status == MPI3_DEVICE0_ASTATUS_NO_ERRORS) {
>  			tgtdev->dev_spec.pcie_inf.mdts =
>  			    le32_to_cpu(pcieinf->maximum_data_transfer_size);
> -			tgtdev->dev_spec.pcie_inf.pgsz = pcieinf->page_size;
>  			tgtdev->dev_spec.pcie_inf.reset_to =
>  			    max_t(u8, pcieinf->controller_reset_to,
>  			     MPI3MR_INTADMCMD_TIMEOUT);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724102505.115136-1-ranjan.kumar@broadcom.com?part=4