Re: [PATCH v2 20/33] ibmvfc: allocate targets based on protocol

[sashiko-bot@kernel.org]

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

New issues:
- [High] NVMe targets placed on `nvme_scrqs.targets` are completely ignored by the driver's state machine and teardown paths, resulting in memory leaks and non-functional targets.

Pre-existing issues:
- [High] Unexpected target state causes `ibmvfc_alloc_target` to fall through and allocate a duplicate target with the same WWPN.
--

--- Patch [20]: [PATCH v2 20/33] ibmvfc: allocate targets based on protocol ---
commit c353df4f5747ab4e038043d3b4bccacccf0aa149
Author: Tyrel Datwyler <tyreld@linux.ibm.com>

ibmvfc: allocate targets based on protocol

This commit modifies the target allocation logic to place discovered
targets onto either the SCSI or NVMe channel-group lists based on their
protocol. It keeps protocol-specific discovery results isolated for later
traversal.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index b479537e9e78c..92c286cfc69c6 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c

[ ... ]

> @@ -5014,6 +5021,7 @@ static int ibmvfc_alloc_target(struct ibmvfc_host *vhost,
>  

[Severity: High]
This is a pre-existing issue, but if a target has moved and its state is not
IBMVFC_TGT_ACTION_LOGOUT_DELETED_RPORT, the code prints an "Unexpected target
state" error but fails to abort:

drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_alloc_target() {
        if (wtgt->action == IBMVFC_TGT_ACTION_LOGOUT_DELETED_RPORT) {
                ...
                goto unlock_out;
        } else {
                tgt_err(wtgt, "Unexpected target state: %d, %p\n",
                        wtgt->action, wtgt->rport);
        }
    } else if (stgt) {
        ...
    }
    spin_unlock_irqrestore(vhost->host->host_lock, flags);

Because it lacks a "goto unlock_out;" or "return;" in the else branch, does
this execution fall through to the mempool_alloc() below and erroneously
allocate a duplicate target with the same WWPN?

>  	tgt = mempool_alloc(vhost->tgt_pool, GFP_NOIO);
>  	memset(tgt, 0, sizeof(*tgt));
> +	tgt->protocol = protocol;
>  	tgt->scsi_id = scsi_id;
>  	tgt->wwpn = wwpn;

[ ... ]

> @@ -5042,7 +5050,11 @@ static int ibmvfc_alloc_targets(struct ibmvfc_host *vhost)
>  	int i, rc;
>  
>  	for (i = 0, rc = 0; !rc && i < vhost->scsi_scrqs.num_targets; i++)
> -		rc = ibmvfc_alloc_target(vhost, &vhost->scsi_scrqs.disc_buf[i]);
> +		rc = ibmvfc_alloc_target(vhost, &vhost->scsi_scrqs.disc_buf[i],
> +					 vhost->scsi_scrqs.protocol);
> +	for (i = 0; !rc && i < vhost->nvme_scrqs.num_targets; i++)
> +		rc = ibmvfc_alloc_target(vhost, &vhost->nvme_scrqs.disc_buf[i],
> +					 vhost->nvme_scrqs.protocol);

[Severity: High]
When the hypervisor returns NVMe targets during discovery, they are allocated
and added to vhost->nvme_scrqs.targets here. Are these targets ever cleaned up?

Since the rest of the driver's state machine and teardown paths (like
ibmvfc_remove) currently only iterate over vhost->scsi_scrqs.targets, it
appears these NVMe targets might be completely ignored.

Could this cause a leak of the mempool objects and leave active timers
running that might trigger use-after-free bugs after module removal?

>  
>  	return rc;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723000149.969416-1-tyreld@linux.ibm.com?part=20