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

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 <[email protected]>

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/[email protected]?part=1