Re: [PATCH v4 4/8] ibmvfc: define asynchronous sub-queue

[sashiko-bot@kernel.org]

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

New issues:
- [High] The driver unloads without explicitly flushing or canceling `fpin_workq`, leading to a Use-After-Free.
- [High] The error path in `ibmvfc_probe()` manually destroys a device-managed workqueue, resulting in a double-free.
- [High] The `ibmvfc_process_async_work()` function accesses a target object outside of the host lock without holding a reference count, leading to a TOCTOU Use-After-Free.
- [High] The driver fails to clear the `valid` byte of asynchronous CRQ entries after processing them, which will permanently stall asynchronous events.

Pre-existing issues:
- [High] Out-of-bounds array write when parsing channel setup completion from VIOS.
--

--- Patch [4]: [PATCH v4 4/8] ibmvfc: define asynchronous sub-queue ---
commit 9bd2cdf24ef9c308bf4803492cdf8f30fbef4057
Author: Dave Marquardt <davemarq@linux.ibm.com>

ibmvfc: define asynchronous sub-queue
    
Define data structures for asynchronous sub-queue support required for full and
extended FPIN functionality. Introduce the ibmvfc_async_subq structure for
async events, update channel setup capabilities, and negotiate async support
with VIOS during login.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
> index 3dd2adda195ee..c95e78d729ed7 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c

[ ... ]

> @@ -3185,16 +3191,228 @@ static const char *ibmvfc_get_link_state(enum ibmvfc_ae_link_state state)
[ ... ]
> +static void ibmvfc_process_async_work(struct work_struct *work)
> +{
> +	struct ibmvfc_target *tgt, *next;
> +	struct ibmvfc_async_work *aw;
> +	struct ibmvfc_async_crq *crq;
> +	struct ibmvfc_host *vhost;
> +	struct fc_els_fpin *fpin;
> +	unsigned long flags;
> +
> +	aw = container_of(work, struct ibmvfc_async_work, async_work_s);
> +	crq = &aw->crq;
> +	vhost = aw->vhost;
> +
> +	if (!crq->scsi_id && !crq->wwpn && !crq->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)
> +			continue;
> +		if (crq->wwpn && cpu_to_be64(tgt->ids.port_name) != crq->wwpn)
> +			continue;
> +		if (crq->node_name && cpu_to_be64(tgt->ids.node_name) != crq->node_name)
> +			continue;
> +		if (!tgt->rport)
> +			continue;
> +		break;
> +	}
> +	spin_unlock_irqrestore(vhost->host->host_lock, flags);
> +
> +	if (list_entry_is_head(tgt, &vhost->targets, queue) || !tgt->rport) {
> +		dev_err_ratelimited(vhost->dev, "Invalid target for FPIN\n");
> +		goto end;
> +	}
> +
> +	fpin = ibmvfc_basic_fpin_to_desc(crq, tgt->wwpn);

[Severity: High]
What prevents a use-after-free here if a concurrent device deletion or logout
frees tgt immediately after the lock is dropped?

Since the lock is dropped without acquiring a reference to tgt (via kref_get),
is it safe to access tgt->wwpn or pass tgt->vhost->host to fc_host_fpin_rcv()
outside of the lock?

[ ... ]

> @@ -3803,8 +4041,6 @@ static void ibmvfc_tasklet(void *data)
>  		/* Pull all the valid messages off the async CRQ */
>  		while ((async = ibmvfc_next_async_crq(vhost)) != NULL) {
>  			ibmvfc_handle_async(async, vhost);
> -			async->valid = 0;
> -			wmb();
>  		}

[Severity: High]
Is it intentional to remove the clearing of the valid byte here?

The VIO CRQ protocol normally requires clearing the valid byte after consuming
an entry. Without this, it seems the consumed entries are never freed, which
could cause the async CRQ ring buffer to permanently fill up and stall
asynchronous events.

>  
>  		/* Pull all the valid messages off the CRQ */
> @@ -3818,8 +4054,6 @@ static void ibmvfc_tasklet(void *data)
>  		if ((async = ibmvfc_next_async_crq(vhost)) != NULL) {
>  			vio_disable_interrupts(vdev);
>  			ibmvfc_handle_async(async, vhost);
> -			async->valid = 0;
> -			wmb();
>  		} else if ((crq = ibmvfc_next_crq(vhost)) != NULL) {

[ ... ]

> @@ -5069,8 +5393,12 @@ static void ibmvfc_channel_setup_done(struct ibmvfc_event *evt)
>  		return;
>  	}
>  
> -	ibmvfc_set_host_action(vhost, IBMVFC_HOST_ACTION_QUERY);
> -	wake_up(&vhost->work_wait_q);
> +	if (ibmvfc_check_caps(vhost, IBMVFC_SUPPORT_SCSI)) {
> +		ibmvfc_fabric_login(vhost);
> +	} else {
> +		ibmvfc_set_host_action(vhost, IBMVFC_HOST_ACTION_QUERY);
> +		wake_up(&vhost->work_wait_q);
> +	}
>  }

[Severity: High]
This isn't a bug introduced by this patch, but is there a bounds check
missing for active_queues in this function?

Looking earlier in ibmvfc_channel_setup_done(), active_queues is read from
the VIOS hardware response and used as a loop bound:

ibmvfc_channel_setup_done() {
    ...
    active_queues = be32_to_cpu(setup->num_scsi_subq_channels);
    scrqs->active_queues = active_queues;
    ...
    for (i = 0; i < active_queues; i++)
        scrqs->scrqs[i].vios_cookie = be64_to_cpu(setup->channel_handles[i]);
    ...
}

Could this loop write past the end of the scrqs->scrqs array if the VIOS
returns a value greater than scrqs->max_queues?

[ ... ]

> @@ -6364,9 +6694,15 @@ static int ibmvfc_probe(struct vio_dev *vdev, const struct vio_device_id *id)
>  	INIT_WORK(&vhost->rport_add_work_q, ibmvfc_rport_add_thread);
>  	mutex_init(&vhost->passthru_mutex);
>  
> -	if ((rc = ibmvfc_alloc_mem(vhost)))
> +	vhost->fpin_workq = devm_alloc_workqueue(vhost->dev, "%s-fpin-workq-%u", 0, 0,
> +						 IBMVFC_NAME, shost->host_no);
> +	if (vhost->fpin_workq == NULL)
>  		goto free_scsi_host;
>  

[Severity: High]
Could this devm_alloc_workqueue() allocation lead to a use-after-free during
driver unload?

During driver unload, ibmvfc_remove() calls scsi_host_put() which frees the
Scsi_Host and vhost structures. Since devres callbacks run after the remove
function returns, the workqueue will be destroyed after vhost is already
freed. If any pending ibmvfc_process_async_work() items execute, they would
dereference the freed vhost structure.

> +	rc = ibmvfc_alloc_mem(vhost);
> +	if (rc)
> +		goto free_workq;
> +
>  	vhost->work_thread = kthread_run(ibmvfc_work, vhost, "%s_%d", IBMVFC_NAME,
>  					 shost->host_no);
>  
> @@ -6412,6 +6748,9 @@ static int ibmvfc_probe(struct vio_dev *vdev, const struct vio_device_id *id)
>  	kthread_stop(vhost->work_thread);
>  free_host_mem:
>  	ibmvfc_free_mem(vhost);
> +free_workq:
> +	destroy_workqueue(vhost->fpin_workq);
> +	vhost->fpin_workq = NULL;

[Severity: High]
Will manually destroying a devm-managed workqueue here cause a double-free?

Because fpin_workq was allocated with devm_alloc_workqueue(), devres
automatically registers an action to destroy it. If this error path manually
calls destroy_workqueue(), it appears devres will try to destroy it a second
time during automatic cleanup.

>  free_scsi_host:
>  	scsi_host_put(shost);
>  out:

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