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

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

Pre-existing issues:
- [Medium] Target device reference count leak on `mpi3mr_target_alloc` failure paths.
- [Medium] Silent failure and wedged hardware state machine when device removal handshake TM allocation fails.
--

commit e0b0b39b67b7199d7920836b376fd3080aa21730
Author: Chandrakanth Patil <[email protected]>

mpi3mr: Fix target device reference leak in device removal handshake

The device removal handshake looks up the target device to update its
state but never drops the reference that the lookup takes.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index 2a35f146fe69b..9aeec6146acbb 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -2536,6 +2536,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;
>  	do {

[Severity: Medium]
This isn't a bug introduced by this patch, but further down in
mpi3mr_dev_rmhs_send_tm(), if the pre-allocated command slots are exhausted,
the code falls back to allocating a delayed_dev_rmhs node:

    delayed_dev_rmhs = kzalloc_obj(*delayed_dev_rmhs, GFP_ATOMIC);
    if (!delayed_dev_rmhs)
        return;

If this GFP_ATOMIC allocation fails, the function returns without scheduling a
retry or rolling back the device state (MPI3MR_DEV_REMOVE_HS_STARTED).

Can this silent failure leave the hardware state machine permanently wedged,
waiting for the host to send this TM request?

[Severity: Medium]
This is a pre-existing issue, but looking at mpi3mr_target_alloc() in the same
file, there appears to be a reference count leak on the error paths.

If __mpi3mr_get_tgtdev_by_perst_id() or
__mpi3mr_get_tgtdev_by_addr_and_rphy() successfully finds a target device,
but the device doesn't satisfy the conditional checks for PCIe devices:

    if (tgt_dev && !tgt_dev->is_hidden && tgt_dev->non_stl) {
        ...
    } else
        retval = -ENXIO;

Or for SAS/SATA devices:

    if (tgt_dev && !tgt_dev->is_hidden && !tgt_dev->non_stl &&
        (tgt_dev->dev_type == MPI3_DEVICE_DEVFORM_SAS_SATA)) {
        ...
    } else
        retval = -ENXIO;

In both cases, the code returns -ENXIO without dropping the reference count
via mpi3mr_tgtdev_put(tgt_dev), and without assigning it to
scsi_tgt_priv_data->tgt_dev for later cleanup.

Would this leak the mpi3mr_tgt_dev memory structures during device scanning
if hidden or non-standard devices are present?

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