Re: [PATCH v18 06/13] scsi: ibmvfc: add basic FPIN support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] scsi: ibmvfc: uninitialized tgt and fpin in ibmvfc_process_async_work()
--

commit f3f5aed7c70a8803eed36bba7b018020fbce24fa
Author: Dave Marquardt <davemarq@linux.ibm.com>

scsi: ibmvfc: add basic FPIN support

This commit adds basic Fabric Performance Impact Notification (FPIN) support
to the ibmvfc driver, offloading IBMVFC_AE_FPIN async events to a per-adapter
ordered workqueue and converting them into fc_els_fpin descriptors for
fc_host_fpin_rcv(). It also adds a KUnit test suite to validate FPIN event
handling and statistics updates.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index e04c80050b981..a87beaeca13c8 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c

[ ... ]

> @@ -3264,6 +3268,277 @@ static const char *ibmvfc_get_link_state(enum ibmvfc_ae_link_state state)

[ ... ]

> +/**
> + * ibmvfc_process_async_work - Process IBMVFC_AE_FPIN async CRQ from work queue
> + * @work: pointer to work_struct
> + */
> +VISIBLE_IF_KUNIT void ibmvfc_process_async_work(struct work_struct *work)
> +{
> +	struct ibmvfc_async_work *aw;
> +	struct ibmvfc_async_crq *crq;
> +	struct ibmvfc_target *tgt;
> +	struct ibmvfc_host *vhost;
> +	struct fc_els_fpin *fpin;
> +	unsigned long flags;
> +
> +	aw = container_of_const(work, struct ibmvfc_async_work, async_work_s);
> +	vhost = aw->vhost;
> +	crq = &aw->crq;
> +
> +	if (!crq->scsi_id && !crq->wwpn && !crq->node_name) {
> +		if (crq->fpin_status == IBMVFC_AE_FPIN_LINK_CONGESTED ||
> +		    crq->fpin_status == IBMVFC_AE_FPIN_CONGESTION_CLEARED)
> +			fpin = ibmvfc_basic_fpin_to_desc(crq,
> +					cpu_to_be64(fc_host_port_name(vhost->host)), 0);
> +		goto send_fpin;
> +	}
> +
> +	spin_lock_irqsave(&vhost->host->host_lock, flags);
> +	tgt = ibmvfc_find_target(&vhost->scsi_scrqs.targets, crq->scsi_id,
> +				 crq->wwpn, crq->node_name);
> +	if (!tgt) {
> +		/* Target not found in scsi_scrqs, search nvme_scrqs */
> +		tgt = ibmvfc_find_target(&vhost->nvme_scrqs.targets,
> +					 crq->scsi_id, crq->wwpn,
> +					 crq->node_name);
> +	}
> +
> +	if (tgt) {
> +		kref_get(&tgt->kref);
> +		spin_unlock_irqrestore(&vhost->host->host_lock, flags);
> +	} else {
> +		spin_unlock_irqrestore(&vhost->host->host_lock, flags);
> +		dev_err_ratelimited(vhost->dev, "Invalid target for FPIN\n");
> +		goto free;
> +	}
> +
> +	fpin = ibmvfc_basic_fpin_to_desc(crq,
> +				cpu_to_be64(fc_host_port_name(vhost->host)),
> +				tgt->wwpn);
> +send_fpin:
> +	if (fpin) {
> +		fc_host_fpin_rcv(vhost->host,
> +				 sizeof(*fpin) + be32_to_cpu(fpin->desc_len),
> +				 (char *)fpin, 0);
> +		kfree(fpin);
> +	} else
> +		dev_err_ratelimited(vhost->dev, "FPIN event received, unable to process\n");
> +
> +	if (tgt)
> +		kref_put(&tgt->kref, ibmvfc_release_tgt);

[Severity: Critical]
In ibmvfc_process_async_work() in drivers/scsi/ibmvscsi/ibmvfc-core.c, both
tgt and fpin are declared on the stack without being initialized to NULL.

When crq->scsi_id, crq->wwpn, and crq->node_name are all zero, the function
jumps directly to the send_fpin label without ever initializing tgt. At
send_fpin, checking if (tgt) and calling kref_put(&tgt->kref,
ibmvfc_release_tgt) will operate on an uninitialized stack pointer on every
zero-identifier FPIN event, including valid host-scope
IBMVFC_AE_FPIN_LINK_CONGESTED and IBMVFC_AE_FPIN_CONGESTION_CLEARED events.

Furthermore, if crq->fpin_status on that zero-identifier path is neither
IBMVFC_AE_FPIN_LINK_CONGESTED nor IBMVFC_AE_FPIN_CONGESTION_CLEARED (such as
the zero-identifier IBMVFC_AE_FPIN_PORT_CONGESTED case exercised by
ibmvfc_async_fpin_test() in drivers/scsi/ibmvscsi/ibmvfc_kunit.c), fpin is
also left uninitialized before jumping to send_fpin. If the uninitialized
fpin pointer on the stack is non-zero, send_fpin will dereference
fpin->desc_len, pass the garbage pointer to fc_host_fpin_rcv(), and pass it
to kfree(fpin).

Note that while the subsequent commit 6abed589f0fb ("scsi: ibmvfc: extend
async event handlers for async sub-CRQ events") initializes fpin to NULL, tgt
still remains uninitialized on the zero-identifier goto send_fpin path.

Should both tgt and fpin be initialized to NULL at declaration in
ibmvfc_process_async_work()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-ibmvfc-fpin-support-v18-0-d683b74736a3@linux.ibm.com?part=6