Re: [PATCH v9 4/9] scsi: ibmvfc: extend async event handlers for async sub-CRQ events

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] ibmvfc_full_fpin_to_desc() incorrectly relies on the CRQ event's WWPN rather than the resolved target's WWPN, risking zero-initialized FPIN descriptors.

Pre-existing issues:
- [High] ibmvfc_find_target() will always return NULL due to a faulty logical OR condition checking for both rport and nvme_remote_port.
--

commit 944a7c8d64284dbbdeb6a30246074960364f88e5
Author: Dave Marquardt <davemarq@linux.ibm.com>
scsi: ibmvfc: extend async event handlers for async sub-CRQ events

This commit refactors async event handling to support both traditional
async CRQs and new asynchronous sub-queue CRQs using a tagged union.
It also adds functionality to convert full FPIN messages from async
sub-queue formats into fc_els_fpin structures.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index 6b2eb82ceb98d..daec81f48857d 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
[ ... ]
> @@ -3426,6 +3426,45 @@ ibmvfc_basic_fpin_to_desc(struct ibmvfc_async_crq *crq, u64 wwpn)
>  					  cpu_to_be32(1));
>  }
>  
> +/**
> + * ibmvfc_full_fpin_to_desc(): allocate and populate a struct fc_els_fpin struct
> + * containing a descriptor.
> + * @ibmvfc_fpin: Pointer to async subq FPIN data
> + *
> + * Allocate a struct fc_els_fpin containing a descriptor and populate
> + * based on data from *ibmvfc_fpin.
> + *
> + * Return:
> + * NULL     - unable to allocate structure
> + * non-NULL - pointer to populated struct fc_els_fpin
> + */
> +static struct fc_els_fpin *
> +ibmvfc_full_fpin_to_desc(struct ibmvfc_async_sub_crq *ibmvfc_fpin)
> +{
> +	__be16 type;
> +
> +	switch (ibmvfc_fpin->fpin_status) {
> +	case IBMVFC_AE_FPIN_LINK_CONGESTED:
> +	case IBMVFC_AE_FPIN_PORT_CONGESTED:
> +		type = cpu_to_be16(FPIN_CONGN_DEVICE_SPEC);
> +		break;
> +	case IBMVFC_AE_FPIN_PORT_CLEARED:
> +	case IBMVFC_AE_FPIN_CONGESTION_CLEARED:
> +		type = cpu_to_be16(FPIN_CONGN_CLEAR);
> +		break;
> +	case IBMVFC_AE_FPIN_PORT_DEGRADED:
> +		type = cpu_to_be16(FPIN_LI_UNKNOWN);
> +		break;
> +	default:
> +		return NULL;
> +	}
> +
> +	return ibmvfc_common_fpin_to_desc(ibmvfc_fpin->fpin_status, ibmvfc_fpin->wwpn,
> +					  type, cpu_to_be16(0),

[Severity: Medium]
Does this code risk generating FPIN descriptors with a zero-initialized
attached_wwpn if the VIOS asynchronous sub-CRQ event lacks a populated WWPN?

The legacy path (ibmvfc_basic_fpin_to_desc) explicitly accepts and uses the
resolved target's WWPN, whereas this new function relies strictly on the CRQ
payload (ibmvfc_fpin->wwpn). If targets are resolved primarily via node_name
and the event payload's WWPN is zeroed, will we substitute an empty WWPN into
the FPIN descriptor instead of using the known target WWPN?

> +					  cpu_to_be32(IBMVFC_FPIN_DEFAULT_EVENT_THRESHOLD),
> +					  cpu_to_be32(1));
> +}
> +
>  /**
>   * ibmvfc_find_target - Search for a target in a target list
>   * @target_list: list head of targets to search
[ ... ]
> @@ -3463,28 +3502,39 @@ static struct ibmvfc_target *ibmvfc_find_target(struct list_head *target_list,
>   */
>  static void ibmvfc_process_async_work(struct work_struct *work)
>  {
> +	struct ibmvfc_async_sub_crq *subq = NULL;
>  	struct ibmvfc_async_work *aw;
> -	struct ibmvfc_async_crq *crq;
> +	struct ibmvfc_async_crq *crq = NULL;
>  	struct ibmvfc_target *tgt;
>  	struct ibmvfc_host *vhost;
> -	struct fc_els_fpin *fpin;
> +	struct fc_els_fpin *fpin = NULL;
>  	unsigned long flags;
> +	__be64 node_name;
> +	__be64 scsi_id;
> +	__be64 wwpn;
>  
>  	aw = container_of_const(work, struct ibmvfc_async_work, async_work_s);
>  	vhost = aw->vhost;
> -	crq = &aw->crq;
> +	if (aw->event.type == IBMVFC_ASYNC_CRQ_SUB) {
> +		subq = &aw->event.subq;
> +		scsi_id = 0;
> +		wwpn = subq->wwpn;
> +		node_name = (subq->flags & IBMVFC_ASYNC_ID_IS_ASSOC_ID) ? 0 : subq->id.node_name;
> +	} else {
> +		crq = &aw->event.async_crq;
> +		scsi_id = crq->scsi_id;
> +		wwpn = crq->wwpn;
> +		node_name = crq->node_name;
> +	}
>  
> -	if (!crq->scsi_id && !crq->wwpn && !crq->node_name)
> +	if (!scsi_id && !wwpn && !node_name)
>  		goto free;
>  
>  	spin_lock_irqsave(vhost->host->host_lock, flags);
> -	tgt = ibmvfc_find_target(&vhost->scsi_scrqs.targets, crq->scsi_id,
> -				 crq->wwpn, crq->node_name);
> +	tgt = ibmvfc_find_target(&vhost->scsi_scrqs.targets, scsi_id, wwpn, 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);
> +		tgt = ibmvfc_find_target(&vhost->nvme_scrqs.targets, scsi_id, wwpn, node_name);
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but its presence completely breaks
the FPIN functionality being extended here.

The ibmvfc_find_target() function drops all valid targets because it checks:

    if (!tgt->rport || !tgt->nvme_remote_port)
        continue;

Since FCP and NVMe targets are kept in completely separate lists (scsi_scrqs
vs nvme_scrqs), no target will ever have both an rport and an nvme_remote_port
simultaneously. Will this unconditionally return NULL for every async event
and fail with "Invalid target for FPIN", dropping the notifications?

>  
>  	if (tgt) {
> @@ -3496,7 +3546,11 @@ static void ibmvfc_process_async_work(struct work_struct *work)
>  		goto free;
>  	}
>  
> -	fpin = ibmvfc_basic_fpin_to_desc(crq, tgt->wwpn);
> +	if (crq)
> +		fpin = ibmvfc_basic_fpin_to_desc(crq, tgt->wwpn);
> +	else
> +		fpin = ibmvfc_full_fpin_to_desc(subq);
> +
>  	if (fpin) {
>  		fc_host_fpin_rcv(tgt->vhost->host,
>  				 sizeof(*fpin) + be32_to_cpu(fpin->desc_len),

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-ibmvfc-fpin-support-v8-0-b27183b055af@linux.ibm.com?part=4