Re: [PATCH 2/2] scsi: mpi3mr: Fix target device refcount leak in mpi3mr_sas_port_add()

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

Pre-existing issues:
- [High] Missing NULL check for the return value of sas_end_device_alloc() and sas_expander_alloc() leads to an immediate NULL pointer dereference.
- [High] Failure of sas_rphy_add() is ignored and does not abort the operation, leaving the driver state inconsistent and leaking rphy and port structures.
--

--- Patch [2]: [PATCH 2/2] scsi: mpi3mr: Fix target device refcount leak in mpi3mr_sas_port_add() ---
commit 6d1c7833feb76d25951cbe5a46e2637e0f883658
Author: Milan P. Gandhi <[email protected]>

scsi: mpi3mr: Fix target device refcount leak in mpi3mr_sas_port_add()

This commit fixes a reference count leak for target devices in
mpi3mr_sas_port_add() when an error triggers the out_fail path. It adds
a NULL-checked mpi3mr_tgtdev_put() call to ensure the device reference
is properly released during cleanup.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_transport.c b/drivers/scsi/mpi3mr/mpi3mr_transport.c
> index ea2c04384a0e1..232af978d7372 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_transport.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_transport.c
> @@ -1507,6 +1507,8 @@ static struct mpi3mr_sas_port *mpi3mr_sas_port_add(struct mpi3mr_ioc *mrioc,
>  	list_for_each_entry_safe(mr_sas_phy, next, &mr_sas_port->phy_list,
>  	    port_siblings)
>  		list_del(&mr_sas_phy->port_siblings);
> +	if (tgtdev)
> +		mpi3mr_tgtdev_put(tgtdev);
>  	kfree(mr_sas_port);
>  	return NULL;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the error
handling in this function, I noticed there is no check for the return value
of sas_end_device_alloc() or sas_expander_alloc() before dereferencing it:

drivers/scsi/mpi3mr/mpi3mr_transport.c:mpi3mr_sas_port_add() {
    ...
    if (mr_sas_port->remote_identify.device_type == SAS_END_DEVICE) {
        rphy = sas_end_device_alloc(port);
        tgtdev->dev_spec.sas_sata_inf.rphy = rphy;
    } else {
        rphy = sas_expander_alloc(port,
            mr_sas_port->remote_identify.device_type);
    }
    rphy->identify = mr_sas_port->remote_identify;
    ...
}

If sas_end_device_alloc() or sas_expander_alloc() returns NULL due to
memory constraints, will the assignment to rphy->identify cause a NULL
pointer dereference? Could we add a check and jump to the out_fail path
here?

[Severity: High]
This is also a pre-existing issue, but I noticed the failure of
sas_rphy_add() does not abort the operation:

drivers/scsi/mpi3mr/mpi3mr_transport.c:mpi3mr_sas_port_add() {
    ...
    if ((sas_rphy_add(rphy))) {
        ioc_err(mrioc, "failure at %s:%d/%s()!\n",
            __FILE__, __LINE__, __func__);
    }
    if (mr_sas_port->remote_identify.device_type == SAS_END_DEVICE) {
        tgtdev->dev_spec.sas_sata_inf.pend_sas_rphy_add = 0;
        tgtdev->dev_spec.sas_sata_inf.sas_transport_attached = 1;
        mpi3mr_tgtdev_put(tgtdev);
    }
    ...
}

If sas_rphy_add() fails, the code merely logs an error but proceeds to mark
the transport as attached (sas_transport_attached = 1). Does this leave the
driver state inconsistent and leak the rphy and port structures, since
teardown might attempt to remove a device that was never fully added?
Should this failure jump to an appropriate error handling label to unwind
the state?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2