Re: [PATCH v3 08/10] mpi3mr: Fix SAS port allocation and registration error handling

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Calling `sas_port_delete()` when `sas_rphy_add()` fails causes a fatal `device_del()` on an un-added device.
- [High] The target device structure retains a dangling pointer to a freed `rphy` and a stale `pend_sas_rphy_add` flag if `sas_rphy_add()` fails.
- [High] Failure to clear `mr_sas_phy->phy_belongs_to_port` on error paths causes phys to be permanently orphaned.
--

--- Patch [8]: [PATCH v3 08/10] mpi3mr: Fix SAS port allocation and registration error handling ---
commit 196b5015246a90c5d75f6d4437141992643236da
Author: Ranjan Kumar <ranjan.kumar@broadcom.com>

mpi3mr: Fix SAS port allocation and registration error handling
    
During SAS port creation, the driver does not verify successful port
allocation before attempting registration, which can lead to a NULL
pointer dereference. Additionally, if registration fails, the allocated
port is not freed, resulting in a memory leak.

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

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_transport.c b/drivers/scsi/mpi3mr/mpi3mr_transport.c
> --- a/drivers/scsi/mpi3mr/mpi3mr_transport.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_transport.c
[ ... ]
> @@ -1450,10 +1456,22 @@ static struct mpi3mr_sas_port *mpi3mr_sas_port_add(struct mpi3mr_ioc *mrioc,
>  	mr_sas_port->port = port;
>  	if (mr_sas_port->remote_identify.device_type == SAS_END_DEVICE) {
>  		rphy = sas_end_device_alloc(port);
> +		if (!rphy) {
> +			ioc_err(mrioc, "failure at %s:%d/%s() (sas_end_device_alloc)!\n",
> +			    __FILE__, __LINE__, __func__);
> +			sas_port_delete(port);
> +			goto out_fail;
> +		}
>  		tgtdev->dev_spec.sas_sata_inf.rphy = rphy;
>  	} else {
>  		rphy = sas_expander_alloc(port,
>  		    mr_sas_port->remote_identify.device_type);
> +		if (!rphy) {
> +			ioc_err(mrioc, "failure at %s:%d/%s() (sas_expander_alloc)!\n",
> +			    __FILE__, __LINE__, __func__);
> +			sas_port_delete(port);
> +			goto out_fail;
> +		}
>  	}
>  	rphy->identify = mr_sas_port->remote_identify;
>  
> @@ -1463,6 +1481,8 @@ static struct mpi3mr_sas_port *mpi3mr_sas_port_add(struct mpi3mr_ioc *mrioc,
>  	if ((sas_rphy_add(rphy))) {
>  		ioc_err(mrioc, "failure at %s:%d/%s()!\n",
>  		    __FILE__, __LINE__, __func__);
> +		sas_port_delete(port);
> +		goto out_fail;

[Severity: Critical]
This is a pre-existing issue, but does calling sas_port_delete() here cause a
fatal device_del() on an un-added device?

When sas_rphy_add() fails, it will have already internally set port->rphy = rphy
before failing during device_add(). Because port->rphy is set,
sas_port_delete() invokes sas_rphy_delete(), which explicitly calls
sas_rphy_remove() and device_del(dev) on the rphy.

Calling device_del() on a device that was never successfully added via
device_add() violates the kobject API and can trigger a panic. Should we
call sas_rphy_unlink(rphy) and sas_rphy_free(rphy) before
sas_port_delete(port) instead?

[Severity: High]
This isn't a bug introduced by this patch, but could this error path leave
dangling pointers in the tgtdev structure?

Before calling sas_rphy_add(), the rphy pointer is cached in the target
device structure:

mpi3mr_transport.c:mpi3mr_sas_port_add() {
    ...
    if (mr_sas_port->remote_identify.device_type == SAS_END_DEVICE) {
        ...
        tgtdev->dev_spec.sas_sata_inf.rphy = rphy;
    }
    ...
}

If we jump to out_fail when sas_rphy_add() fails, we clean up the rphy
indirectly via sas_port_delete(), but tgtdev->dev_spec.sas_sata_inf.rphy
still points to the freed memory, and pend_sas_rphy_add remains set to 1.

Could a future allocation reuse this memory and cause functions like
__mpi3mr_get_tgtdev_by_addr_and_rphy() to incorrectly match the corrupted
state?

>  	}
>  	if (mr_sas_port->remote_identify.device_type == SAS_END_DEVICE) {
>  		tgtdev->dev_spec.sas_sata_inf.pend_sas_rphy_add = 0;
> @@ -1498,6 +1518,9 @@ static struct mpi3mr_sas_port *mpi3mr_sas_port_add(struct mpi3mr_ioc *mrioc,
>  	return mr_sas_port;
>  
>   out_fail:
> +	if (tgtdev)
> +		mpi3mr_tgtdev_put(tgtdev);
> +
>  	list_for_each_entry_safe(mr_sas_phy, next, &mr_sas_port->phy_list,
>  	    port_siblings)
>  		list_del(&mr_sas_phy->port_siblings);

[Severity: High]
This is a pre-existing issue, but might this error path fail to clear the
phy_belongs_to_port flag, causing phys to be permanently orphaned?

During port addition, mr_sas_phy->phy_belongs_to_port is set to 1 for all
elements in mr_sas_port->phy_list. When we reach out_fail, we remove the
PHYs from the sibling list:

mpi3mr_transport.c:mpi3mr_sas_port_add() {
    ...
 out_fail:
    ...
    list_for_each_entry_safe(mr_sas_phy, next, &mr_sas_port->phy_list,
        port_siblings)
        list_del(&mr_sas_phy->port_siblings);
    ...
}

Because the underlying mr_sas_phy structures persist in mr_sas_node->phy[],
failing to reset the flag means they will permanently claim to belong to a
port. Will future attempts to add these PHYs fail as a result?

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