[PATCH] scsi: mpi3mr: Handle SAS port and rphy allocation failures

[PATCH] scsi: mpi3mr: Handle SAS port and rphy allocation failures

[Triet Hoang]

Check the return value of sas_port_alloc_num() and handle failures
before attempting to add the port.

Free the SAS port if sas_port_add() fails. Also handle failures from
sas_end_device_alloc() and sas_expander_alloc() by removing the PHYs
from the port, deleting the SAS port, and returning failure.

This prevents NULL pointer dereferences and ensures resources allocated
during SAS port creation are properly cleaned up on error paths.

Signed-off-by: Triet Hoang <triet.hoang.dev@gmail.com>
---
 drivers/scsi/mpi3mr/mpi3mr_transport.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_transport.c b/drivers/scsi/mpi3mr/mpi3mr_transport.c
index 240f67a8e2e3..2fbfd178c539 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_transport.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_transport.c
@@ -1428,9 +1428,17 @@ static struct mpi3mr_sas_port *mpi3mr_sas_port_add(struct mpi3mr_ioc *mrioc,
 	}
 
 	port = sas_port_alloc_num(mr_sas_node->parent_dev);
+	if (!port) {
+		ioc_err(mrioc, "failure at %s:%d/%s()!\n",
+		    __FILE__, __LINE__, __func__);
+		goto out_fail;
+	}
+
 	if ((sas_port_add(port))) {
 		ioc_err(mrioc, "failure at %s:%d/%s()!\n",
 		    __FILE__, __LINE__, __func__);
+		sas_port_free(port);
+		port = NULL;
 		goto out_fail;
 	}
 
@@ -1455,6 +1463,19 @@ static struct mpi3mr_sas_port *mpi3mr_sas_port_add(struct mpi3mr_ioc *mrioc,
 		rphy = sas_expander_alloc(port,
 		    mr_sas_port->remote_identify.device_type);
 	}
+
+	if (!rphy) {
+		ioc_err(mrioc, "failure at %s:%d/%s()!\n",
+		    __FILE__, __LINE__, __func__);
+		list_for_each_entry(mr_sas_phy, &mr_sas_port->phy_list, port_siblings) {
+			mr_sas_phy->phy_belongs_to_port = 0;
+			mr_sas_phy->hba_port = NULL;
+			sas_port_delete_phy(port, mr_sas_phy->phy);
+		}
+		sas_port_delete(port);
+		goto out_fail;
+	}
+
 	rphy->identify = mr_sas_port->remote_identify;
 
 	if (mrioc->current_event)
-- 
2.53.0

Re: [PATCH] scsi: mpi3mr: Handle SAS port and rphy allocation failures

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Target device reference and state leak on newly added error paths.
- [High] Corruption of SAS PHY state on rphy allocation failure.
--

--- Patch [1]: [PATCH] scsi: mpi3mr: Handle SAS port and rphy allocation failures ---
commit df830714a909590a741f4ae3350492bababc3051
Author: Triet Hoang <triet.hoang.dev@gmail.com>

scsi: mpi3mr: Handle SAS port and rphy allocation failures
    
This commit adds checks for the return values of sas_port_alloc_num(),
sas_port_add(), sas_end_device_alloc(), and sas_expander_alloc(), adding
cleanup and failure paths to prevent NULL pointer dereferences.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_transport.c b/drivers/scsi/mpi3mr/mpi3mr_transport.c
> index 240f67a8e2e3b..2fbfd178c5398 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_transport.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_transport.c
> @@ -1428,9 +1428,17 @@ static struct mpi3mr_sas_port *mpi3mr_sas_port_add(struct mpi3mr_ioc *mrioc,
>  	}
>  
>  	port = sas_port_alloc_num(mr_sas_node->parent_dev);
> +	if (!port) {
> +		ioc_err(mrioc, "failure at %s:%d/%s()!\n",
> +		    __FILE__, __LINE__, __func__);
> +		goto out_fail;

[Severity: High]
Does this error path leak the tgtdev reference and leave its state corrupted?

Earlier in the function, if the device type is SAS_END_DEVICE, tgtdev is
acquired via mpi3mr_get_tgtdev_by_addr() and its pend_sas_rphy_add flag
is set to 1:

	if (mr_sas_port->remote_identify.device_type == SAS_END_DEVICE) {
		tgtdev = mpi3mr_get_tgtdev_by_addr(mrioc, ...);
        ...
		tgtdev->dev_spec.sas_sata_inf.pend_sas_rphy_add = 1;
	}

The success path correctly resets this state and drops the reference via
mpi3mr_tgtdev_put(tgtdev). However, the newly added error paths jumping
to out_fail bypass this cleanup, which leaves the reference leaked and the
pend_sas_rphy_add state inconsistent.

> +	}
> +
>  	if ((sas_port_add(port))) {
>  		ioc_err(mrioc, "failure at %s:%d/%s()!\n",
>  		    __FILE__, __LINE__, __func__);
> +		sas_port_free(port);
> +		port = NULL;
>  		goto out_fail;
>  	}
>  

[ ... ]

> @@ -1455,6 +1463,19 @@ static struct mpi3mr_sas_port *mpi3mr_sas_port_add(struct mpi3mr_ioc *mrioc,
>  		rphy = sas_expander_alloc(port,
>  		    mr_sas_port->remote_identify.device_type);
>  	}
> +
> +	if (!rphy) {
> +		ioc_err(mrioc, "failure at %s:%d/%s()!\n",
> +		    __FILE__, __LINE__, __func__);
> +		list_for_each_entry(mr_sas_phy, &mr_sas_port->phy_list, port_siblings) {
> +			mr_sas_phy->phy_belongs_to_port = 0;
> +			mr_sas_phy->hba_port = NULL;

[Severity: High]
Does clearing hba_port here corrupt the SAS PHY topological state?

The hba_port field appears to be persistently assigned to underlying PHYs
during discovery. It is required and used earlier in this function to match
phys to ports:

	for (i = 0; i < mr_sas_node->num_phys; i++) {
		if ((mr_sas_node->phy[i].remote_identify.sas_address !=
		    mr_sas_port->remote_identify.sas_address) ||
		    (mr_sas_node->phy[i].hba_port != hba_port))
			continue;

If this error cleanup loop unconditionally sets hba_port to NULL, won't it
permanently prevent these PHYs from being matched or added to an HBA port in
future discovery cycles?

> +			sas_port_delete_phy(port, mr_sas_phy->phy);
> +		}
> +		sas_port_delete(port);
> +		goto out_fail;

[Severity: High]
Similar to the sas_port_alloc_num() failure above, jumping to out_fail here
also bypasses the cleanup of the tgtdev reference and pend_sas_rphy_add.

> +	}
> +
>  	rphy->identify = mr_sas_port->remote_identify;
>  
>  	if (mrioc->current_event)

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