Re: [PATCH v4 6/8] ibmvfc: extend async event handlers to handle async sub queue events

[sashiko-bot@kernel.org]

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

New issues:
- [High] The driver ignores the `IBMVFC_ASYNC_ID_IS_ASSOC_ID` flag when processing sub-queue events, incorrectly interpreting association IDs as node names.
- [High] Hardcoded event type in `ibmvfc_full_fpin_to_desc()` drops FPIN congestion event classification.

Pre-existing issues:
- [High] Double-free and use-after-free of the FPIN workqueue due to incorrect mixing of `devm_alloc_workqueue()` and manual lifecycle management.
--

--- Patch [6]: [PATCH v4 6/8] ibmvfc: extend async event handlers to handle async sub queue events ---
commit f425472451b9ed0df3bb74b15fd60bb31167f05e
Author: Dave Marquardt <davemarq@linux.ibm.com>

ibmvfc: extend async event handlers to handle async sub queue events

Refactor async event handling to support both traditional async CRQs and
new asynchronous sub-queue CRQs.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
> index 586847ff3336d..ee56f13f1a979 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c

[ ... ]

> @@ -3349,6 +3349,27 @@ 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_subq *ibmvfc_fpin)
> +{
> +	return ibmvfc_common_fpin_to_desc(ibmvfc_fpin->fpin_status, ibmvfc_fpin->wwpn,
> +					  cpu_to_be16(0), cpu_to_be16(0),
> +					  cpu_to_be32(IBMVFC_FPIN_DEFAULT_EVENT_THRESHOLD),
> +					  cpu_to_be32(1));
> +}

[Severity: High]
Is it intentional to hardcode the event type to cpu_to_be16(0) here?

Unlike ibmvfc_basic_fpin_to_desc() which maps the fpin_status to specific
types like FPIN_CONGN_DEVICE_SPEC, this appears to drop the FPIN congestion
event classification.

Could this prevent the FC transport layer from updating the storage
congestion statistics correctly?

[ ... ]

> @@ -3356,73 +3377,131 @@ static void ibmvfc_process_async_work(struct work_struct *work)

[ ... ]

> +	if (crq) {
> +		wwpn = crq->wwpn;
> +		node_name = crq->node_name;
> +		scsi_id = crq->scsi_id;
> +	} else {
> +		wwpn = subq->wwpn;
> +		node_name = subq->id.node_name;
> +		scsi_id = 0;
> +	}

[Severity: High]
Does this code need to check the IBMVFC_ASYNC_ID_IS_ASSOC_ID flag before
reading subq->id.node_name?

If the VIOS provides an association ID instead of a node name and sets the
flag in subq->flags, it seems the union would contain an assoc_id.

Could reading this unconditionally as a node_name cause target lookups to fail
and silently drop events?

>  
> -	if (!crq->scsi_id && !crq->wwpn && !crq->node_name)
> -		goto end;
> +	if (!scsi_id && !wwpn && !node_name)
> +		goto free;
>  
>  	spin_lock_irqsave(vhost->host->host_lock, flags);

[Severity: High]
This is a pre-existing issue, but is it safe to use this lock here during
driver removal?

If the vhost->fpin_workq was allocated with devm_alloc_workqueue(), it won't
be destroyed until devres runs after ibmvfc_remove() completes.

Since ibmvfc_remove() calls scsi_host_put(vhost->host), could an async FPIN
event executing concurrently access the freed host lock? Also, on probe
error paths, if destroy_workqueue() is called manually on a devm-managed
workqueue, could this lead to a double-free when devres cleans it up?

[ ... ]

> @@ -3460,18 +3539,18 @@ VISIBLE_IF_KUNIT void ibmvfc_handle_async(void *crq,

[ ... ]

> +	if (is_subq) {
> +		subq = crq;
> +		event = be16_to_cpu(subq->event);
> +		link_state = subq->link_state;
> +		scsi_id = 0;
> +		wwpn = subq->wwpn;
> +		node_name = subq->id.node_name;

[Severity: High]
Similarly, does ibmvfc_handle_async() need to verify the
IBMVFC_ASYNC_ID_IS_ASSOC_ID flag before extracting the node_name?

If the subq->id union contains an association ID instead of a WWNN, could
this result in failed target lookups for ELS_LOGO, PRLO, and PLOGI events?

> +	} else {
> +		async_crq = crq;
> +		event = be64_to_cpu(async_crq->event);
> +		link_state = async_crq->link_state;
> +		scsi_id = async_crq->scsi_id;
> +		wwpn = async_crq->wwpn;
> +		node_name = async_crq->node_name;
> +	}

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