Re: [PATCH v2 15/20] scsi: ibmvfc: unregister NVMe local port on adapter removal

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: ibmvfc: lockless list modification in ibmvfc_remove() races with tasklet
- [High] scsi: ibmvfc: use-after-free on rport in ibmvfc_remove()
--

commit 64ca41cc44b400a863417867ee490f75b432ff92
Author: Tyrel Datwyler <tyreld@linux.ibm.com>

scsi: ibmvfc: unregister NVMe local port on adapter removal

This commit ensures the NVMe local port is unregistered during adapter removal
to prevent a use-after-free when the transport subsequently touches the port.
It does this by adding teardown calls for targets and the local port before
host memory is freed.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index 080312ff0a93d..e68ed5e79e58c 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> @@ -6976,6 +6976,7 @@ static int ibmvfc_probe(struct vio_dev *vdev, const struct vio_device_id *id)
>  static void ibmvfc_remove(struct vio_dev *vdev)
>  {
>  	struct ibmvfc_host *vhost = dev_get_drvdata(&vdev->dev);
> +	struct ibmvfc_target *tgt, *tgt_tmp;
>  	LIST_HEAD(purge);
>  	unsigned long flags;
>  
[ ... ]
> @@ -6992,6 +6993,22 @@ static void ibmvfc_remove(struct vio_dev *vdev)
>  	fc_remove_host(vhost->host);
>  	scsi_remove_host(vhost->host);
>  
> +	list_for_each_entry_safe(tgt, tgt_tmp, &vhost->nvme_scrqs.targets, queue) {
> +		if (tgt->nvme_remote_port)
> +			ibmvfc_nvme_unregister_remoteport(tgt);
> +		list_del(&tgt->queue);

[Severity: High]
Does this list_del() race with concurrent list traversals in the tasklet?

Because this loop modifies the list locklessly without holding vhost->host_lock,
and the CRQ is not torn down until ibmvfc_release_crq_queue() is called later
in this function, an incoming asynchronous event (like IBMVFC_AE_ELS_LOGO)
could trigger the ibmvfc_tasklet.

The tasklet handles the event by locking host_lock and traversing the same list:

drivers/scsi/ibmvscsi/ibmvfc-core.c:ibmvfc_handle_async() {
    ...
    list_for_each_entry(tgt, &vhost->nvme_scrqs.targets, queue) {
    ...
}

Will this concurrent lockless modification lead to list corruption and a kernel
panic during the traversal?

> +		timer_delete_sync(&tgt->timer);
> +		kref_put(&tgt->kref, ibmvfc_release_tgt);
> +	}
> +
> +	list_for_each_entry_safe(tgt, tgt_tmp, &vhost->scsi_scrqs.targets, queue) {
> +		if (tgt->rport)
> +			fc_remote_port_delete(tgt->rport);

[Severity: High]
Is it possible this dereferences a freed rport?

Right before this loop, fc_remove_host(vhost->host) and
scsi_remove_host(vhost->host) are called. fc_remove_host() synchronously
deletes all remote ports, and scsi_remove_host() removes all SCSI targets,
dropping their final references to the rport structures and causing them to
be freed.

Will dereferencing tgt->rport here (such as reading rport->dev_loss_tmo inside
fc_remote_port_delete) result in a use-after-free?

> +		list_del(&tgt->queue);
> +		timer_delete_sync(&tgt->timer);
> +		kref_put(&tgt->kref, ibmvfc_release_tgt);
> +	}
> +
>  	spin_lock_irqsave(&vhost->host->host_lock, flags);
>  	ibmvfc_purge_requests(vhost, DID_ERROR);
>  	list_splice_init(&vhost->purge, &purge);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260919013306.2948028-1-tyreld@linux.ibm.com?part=15