Re: [PATCH v16 05/12] 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: double free in ibmvfc_probe() when ibmvfc_alloc_mem() fails
--

commit 1bc41de0e60f763f9d00e952aa43dcb15b71cefb
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 and passing converted descriptors to
fc_host_fpin_rcv(). It also adds KUnit tests for FPIN event handling.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index b63b341efb074..1cd39d36edf5c 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c

[ ... ]

> @@ -6900,9 +7171,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);
>  
> @@ -6910,7 +7187,7 @@ static int ibmvfc_probe(struct vio_dev *vdev, const struct vio_device_id *id)
>  		dev_err(dev, "Couldn't create kernel thread: %ld\n",
>  			PTR_ERR(vhost->work_thread));
>  		rc = PTR_ERR(vhost->work_thread);
> -		goto free_host_mem;
> +		goto free_workq;
>  	}
>  
>  	if ((rc = ibmvfc_init_crq(vhost))) {
> @@ -6946,7 +7223,8 @@ static int ibmvfc_probe(struct vio_dev *vdev, const struct vio_device_id *id)
>  	ibmvfc_release_crq_queue(vhost);
>  kill_kthread:
>  	kthread_stop(vhost->work_thread);
> -free_host_mem:
> +free_workq:
> +	destroy_workqueue(vhost->fpin_workq);
>  	ibmvfc_free_mem(vhost);

[Severity: High]
Does jumping to free_workq when ibmvfc_alloc_mem(vhost) fails in
ibmvfc_probe() cause a double free in ibmvfc_free_mem(vhost)?

When an allocation fails inside ibmvfc_alloc_mem(), it already unwinds and
frees everything allocated up to that point before returning -ENOMEM, leaving
non-NULL dangling pointers in vhost:

drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_alloc_mem() {
	...
free_tgt_pool:
	mempool_destroy(vhost->tgt_pool);
free_trace:
	kfree(vhost->trace);
free_nvme_disc_buffer:
	ibmvfc_free_disc_buf(dev, &vhost->nvme_scrqs);
free_scsi_disc_buffer:
	ibmvfc_free_disc_buf(dev, &vhost->scsi_scrqs);
free_login_buffer:
	dma_free_coherent(dev, sizeof(*vhost->login_buf),
			  vhost->login_buf, vhost->login_buf_dma);
free_sg_pool:
	dma_pool_destroy(vhost->sg_pool);
unmap_async_crq:
	ibmvfc_free_queue(vhost, async_q);
nomem:
	LEAVE;
	return -ENOMEM;
}

In ibmvfc_probe(), jumping to free_workq when ibmvfc_alloc_mem() fails
destroys vhost->fpin_workq and then falls through into ibmvfc_free_mem(vhost),
which frees all of those host resources a second time and calls
dma_free_coherent() on NULL pointers such as vhost->channel_setup_buf.

Should ibmvfc_alloc_mem(vhost) be called before alloc_ordered_workqueue()
(or use separate error labels for freeing host memory and destroying
vhost->fpin_workq) so that an ibmvfc_alloc_mem() failure skips
ibmvfc_free_mem()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-ibmvfc-fpin-support-v16-0-a900aa657ed5@linux.ibm.com?part=5