Re: [PATCH v3 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 5 potential issue(s) to consider:

New issues:
- [Critical] Out-of-bounds Read and Type Confusion due to missing loop exit check.
- [High] Use-after-free race condition due to unlocked access without reference counting.
- [Medium] Unconditional read of `subq->id.node_name` without validating the `IBMVFC_ASYNC_ID_IS_ASSOC_ID` flag.
- [Low] Dead code due to impossible null check `(!crq && !subq)`.

Pre-existing issues:
- [High] Missing read-before-write memory barrier when yielding asynchronous queue descriptors.
--

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

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

This patch extends the ibmvfc driver to process asynchronous sub-queue CRQs 
alongside the existing async CRQs. It modifies ibmvfc_handle_async and 
ibmvfc_process_async_work to handle the new sub-queue event structures.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
> index 804887fb56357..4678d76c84fe2 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c
[ ... ]
> @@ -3356,26 +3377,51 @@ static void ibmvfc_process_async_work(struct work_struct *work)
>  {
[ ... ]
>  	aw = container_of(work, struct ibmvfc_async_work, async_work_s);
>  	vhost = aw->vhost;
> +	is_subq = aw->is_subq;
> +	if (is_subq)
> +		subq = &aw->crq.subq;
> +	else
> +		crq = &aw->crq.async_crq;
>  
> -	if (!crq->scsi_id && !crq->wwpn && !crq->node_name)
> +	if (!crq && !subq) {

[Severity: Low]
Is this null check reachable? Since crq and subq are assigned the addresses
of fields inside the aw struct, it appears they can never evaluate to NULL,
making this check and the following error branch dead code.

> +		dev_err_ratelimited(vhost->dev,
> +				    "FPIN event received, unable to process\n");
> +		goto end;
> +	}
> +
> +	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;

[Severity: Medium]
Does this code safely read the node_name field? 

The id field is a union of node_name and assoc_id. If the firmware sets the 
IBMVFC_ASYNC_ID_IS_ASSOC_ID flag on this sub-queue event, wouldn't this 
unconditional read of node_name result in comparing an association ID against 
tgt->ids.node_name later on, potentially causing valid events to be discarded?

> +		scsi_id = 0;
> +	}
> +
> +	if (!scsi_id && !wwpn && !node_name)
>  		goto end;
>  
>  	spin_lock_irqsave(vhost->host->host_lock, flags);
>  	list_for_each_entry_safe(tgt, next, &vhost->targets, queue) {
> -		if (crq->scsi_id && cpu_to_be64(tgt->scsi_id) != crq->scsi_id)
> +		if (scsi_id && cpu_to_be64(tgt->scsi_id) != scsi_id)
>  			continue;
> -		if (crq->wwpn && cpu_to_be64(tgt->ids.port_name) != crq->wwpn)
> +		if (wwpn && cpu_to_be64(tgt->ids.port_name) != wwpn)
>  			continue;
> -		if (crq->node_name && cpu_to_be64(tgt->ids.node_name) != crq->node_name)
> +		if (node_name && cpu_to_be64(tgt->ids.node_name) != node_name)
>  			continue;
>  		if (!tgt->rport)
>  			continue;
> @@ -3388,41 +3434,71 @@ static void ibmvfc_process_async_work(struct work_struct *work)
>  		goto end;
>  	}

[Severity: Critical]
If the list iteration finishes without finding a match, doesn't tgt end up
pointing to the list head (&vhost->targets) rather than a valid target? 

If so, the subsequent check if (!tgt || !tgt->rport) operates on this
invalid pointer. Since rport happens to be at an offset that might overlap
with valid data in the host struct, could this bypass the safety check
and lead to memory corruption when accessing tgt->wwpn or tgt->vhost->host 
when creating the FPIN descriptor?

> -	fpin = ibmvfc_basic_fpin_to_desc(crq, tgt->wwpn);
> +	spin_unlock_irqrestore(vhost->host->host_lock, flags);

[Severity: High]
Is it safe to drop the vhost->host->host_lock here without first acquiring
a reference to tgt via kref_get(&tgt->kref)? 

Could a concurrent thread processing a logout execute ibmvfc_del_tgt and free 
the target memory before this async work thread finishes accessing tgt->wwpn 
and tgt->vhost->host?

[ ... ]
> @@ -3388,41 +3434,71 @@ 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: Medium]
Similar to the question above in ibmvfc_process_async_work, is it safe to 
read node_name here without first verifying the IBMVFC_ASYNC_ID_IS_ASSOC_ID 
flag in subq->flags?

[ ... ]
> @@ -3492,16 +3568,26 @@ VISIBLE_IF_KUNIT void ibmvfc_handle_async(void *crq,
>  	default:
> -		dev_err(vhost->dev, "Unknown async event received: %lld\n", crq->event);
> +		dev_err(vhost->dev, "Unknown async event received: %llu\n", event);
>  		break;
>  	}
> +
> +	if (is_subq)
> +		subq->valid = 0;
> +	else
> +		async_crq->valid = 0;
> +	wmb();

[Severity: High]
This is a pre-existing issue, but does this code need a full memory barrier
before clearing the valid bit? 

The driver reads the DMA-mapped payload earlier in ibmvfc_handle_async, and 
setting valid = 0 yields the slot back to the hardware. On architectures like 
PowerPC, without a read-before-write mb() before the valid assignment, could 
the CPU reorder the write before the payload reads, allowing hardware to 
overwrite the event data while the driver is still processing it?

>  }
>  EXPORT_SYMBOL_IF_KUNIT(ibmvfc_handle_async);

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