Re: [PATCH 09/17] mpi3mr: Fix target device reference leak in device removal handshake

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

Pre-existing issues:
- [High] Memory leak of tgt_dev reference in mpi3mr_target_alloc() error paths.
- [High] Memory leak of tgtdev reference in mpi3mr_get_port_id_by_rphy() when hba_port is NULL.
- [High] Memory leak of tgtdev reference in mpi3mr_sas_port_add() error paths.
--

--- Patch [9]: [PATCH 09/17] mpi3mr: Fix target device reference leak in device removal handshake ---
commit d4f3aa04629aa5ef9b7db8bd5ef68d81cfceef82
Author: Chandrakanth Patil <[email protected]>

mpi3mr: Fix target device reference leak in device removal handshake

This commit fixes a target device reference leak in the device removal
handshake. It drops the reference acquired by the device lookup once the
state has been updated.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index 7ae4b4fad3f4a..b18e7ea873030 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -2522,6 +2522,9 @@ static void mpi3mr_dev_rmhs_send_tm(struct mpi3mr_ioc *mrioc, u16 handle,
>  		tgtdev->state = MPI3MR_DEV_REMOVE_HS_STARTED;
>  	spin_unlock_irqrestore(&mrioc->tgtdev_lock, flags);
>  
> +	if (tgtdev)
> +		mpi3mr_tgtdev_put(tgtdev);
> +
>  	if (drv_cmd)
>  		goto issue_cmd;

[Severity: High]
This isn't a bug introduced by this patch, but since this commit fixes a
target device reference leak, I noticed a similar pre-existing leak in
mpi3mr_target_alloc() in drivers/scsi/mpi3mr/mpi3mr_os.c.

If the validation checks fail, the function sets retval = -ENXIO and exits
without releasing the acquired tgt_dev reference:

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_target_alloc() {
    ...
    tgt_dev = __mpi3mr_get_tgtdev_by_perst_id(mrioc, starget->id);
    if (tgt_dev && !tgt_dev->is_hidden && tgt_dev->non_stl) {
        ...
    } else {
        retval = -ENXIO;
    }
    ...
}

Does this cause a permanent memory leak of the target device structure?


[Severity: High]
This is a pre-existing issue, but there appears to be another leak in
mpi3mr_get_port_id_by_rphy() in drivers/scsi/mpi3mr/mpi3mr_transport.c.

The code retrieves the target device and takes a reference, but it only
puts the reference inside the condition block that checks if the device's
hba_port is non-NULL:

drivers/scsi/mpi3mr/mpi3mr_transport.c:mpi3mr_get_port_id_by_rphy() {
    ...
    tgtdev = __mpi3mr_get_tgtdev_by_addr_and_rphy(mrioc, ...);
    if (tgtdev && tgtdev->dev_spec.sas_sata_inf.hba_port) {
        ...
        mpi3mr_tgtdev_put(tgtdev);
    }
    ...
}

If a device is found but its hba_port is NULL, the function exits without
releasing the reference. Does this result in a persistent memory leak?


[Severity: High]
This is also a pre-existing issue, but does mpi3mr_sas_port_add() in
drivers/scsi/mpi3mr/mpi3mr_transport.c leak a reference during error paths?

When adding an end device, a reference to the target device is acquired. If
any subsequent setup or allocation fails, the code jumps to the out_fail
label. The cleanup path handles freeing the SAS port structure but seems to
bypass calling mpi3mr_tgtdev_put():

drivers/scsi/mpi3mr/mpi3mr_transport.c:mpi3mr_sas_port_add() {
    ...
    if (rphy->identify.device_type == SAS_END_DEVICE) {
        tgtdev = mpi3mr_get_tgtdev_by_addr(...);
    }
    ...
out_fail:
    ...
}

Could this leak the target device reference during port addition failures?

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