Re: [PATCH v15 04/11] 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:
- [High] scsi: ibmvfc: wrong fpin_workq teardown order in ibmvfc_probe() error path
--

commit f3dce48358b69cb9e305579053b02a88579e0f88
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 by offloading IBMVFC_AE_FPIN async events to
a per-adapter ordered workqueue, converting VIOS FPIN messages into
fc_els_fpin structures for fc_host_fpin_rcv(), and adding KUnit test
coverage.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index 0fe6b6f4260ba..0750e722c46c5 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c

[ ... ]

> @@ -6898,9 +7169,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 = alloc_ordered_workqueue("%s-fpin-workq-%u", 0,
> +						    IBMVFC_NAME, shost->host_no);
> +	if (!vhost->fpin_workq)
>  		goto free_scsi_host;
>  
> +	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);
>  
> @@ -6946,6 +7223,8 @@ 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);
>  free_scsi_host:
>  	scsi_host_put(shost);
>  out:

[Severity: High]
Can the error unwind order in ibmvfc_probe() cause a use-after-free of
vhost->tgt_pool?

In ibmvfc_remove(), destroy_workqueue(vhost->fpin_workq) is called after
ibmvfc_release_crq_queue(vhost) and before ibmvfc_free_mem(vhost):

    ibmvfc_release_crq_queue(vhost);
    destroy_workqueue(vhost->fpin_workq);

    ibmvfc_free_mem(vhost);

As described in the commit message, this ordering ensures that queued FPIN
work items are drained before ibmvfc_free_mem() destroys vhost->tgt_pool.

In ibmvfc_probe(), however, vhost->fpin_workq is allocated before
ibmvfc_alloc_mem(vhost), so the error unwind path runs ibmvfc_free_mem()
before destroy_workqueue(vhost->fpin_workq):

    release_crq:
        ibmvfc_release_crq_queue(vhost);
    kill_kthread:
        kthread_stop(vhost->work_thread);
    free_host_mem:
        ibmvfc_free_mem(vhost);
    free_workq:
        destroy_workqueue(vhost->fpin_workq);

Because vhost->work_thread is started and ibmvfc_init_crq() registers the
CRQ and enables interrupts before scsi_add_host() and
ibmvfc_create_trace_file() are called, a partner-initiated CRQ handshake
and subsequent IBMVFC_AE_FPIN event can queue ibmvfc_process_async_work()
on vhost->fpin_workq before scsi_add_host() or ibmvfc_create_trace_file()
fails.

If that happens, ibmvfc_free_mem() will call
mempool_destroy(vhost->tgt_pool) while ibmvfc_process_async_work() is
still running or pending, and ibmvfc_process_async_work() ->
kref_put(&tgt->kref, ibmvfc_release_tgt) ->
mempool_free(tgt, tgt->vhost->tgt_pool) will access the destroyed
tgt_pool.

Should vhost->fpin_workq be allocated after ibmvfc_alloc_mem(vhost) (or
destroyed before ibmvfc_free_mem(vhost) on the error path) to match the
teardown order in ibmvfc_remove()?

[ ... ]

> @@ -6986,6 +7265,7 @@ static void ibmvfc_remove(struct vio_dev *vdev)
>  	ibmvfc_complete_purge(&purge);
>  	ibmvfc_release_sub_crqs(vhost);
>  	ibmvfc_release_crq_queue(vhost);
> +	destroy_workqueue(vhost->fpin_workq);
>  
>  	ibmvfc_free_mem(vhost);
>  	spin_lock(&ibmvfc_driver_lock);

[ ... ]

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