Re: [PATCH v15 04/11] scsi: ibmvfc: add basic FPIN support

[Dave Marquardt <davemarq@linux.ibm.com>]

sashiko-bot@kernel.org writes:

> 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()?

Yes. I'll change the teardown order to match the teardown order in
ibmvfc_remove().

-Dave