Re: [PATCH 01/29] ibmvfc: move target list from host to protocol specific channel groups

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver schedules a background work item (`vhost->rport_add_work_q`) but never explicitly flushes or cancels it during host removal or module unloading, leading to a leaked work struct and a potential use-after-free.
--

commit 455b96a57ea40cd18201b8e8603264b5cfb84e0c
Author: Tyrel Datwyler <tyreld@linux.ibm.com>

ibmvfc: move target list from host to protocol specific channel groups

Prepare the driver for protocol-specific target management by moving
the target list and target count out of struct ibmvfc_host and into
struct ibmvfc_channels.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
> index 3dd2adda195ee..9129014364423 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c
[ ... ]
> @@ -6276,7 +6276,7 @@ static void ibmvfc_rport_add_thread(struct work_struct *work)
>  		if (vhost->state != IBMVFC_ACTIVE)
>  			break;
>  
> -		list_for_each_entry(tgt, &vhost->targets, queue) {
> +		list_for_each_entry(tgt, &vhost->scsi_scrqs.targets, queue) {

[Severity: High]
This isn't a bug introduced by this patch, but does the driver leak the
rport_add_work_q on module removal?

In ibmvfc_remove(), the vhost structure is freed via scsi_host_put() after
teardown:

drivers/scsi/ibmvscsi/ibmvfc.c:ibmvfc_remove() {
    ...
	kthread_stop(vhost->work_thread);
	fc_remove_host(vhost->host);
	scsi_remove_host(vhost->host);
    ...
	scsi_host_put(vhost->host);
}

However, the teardown sequence never explicitly flushes or cancels
vhost->rport_add_work_q.

If the work thread is actively running or pending on the system workqueue:

drivers/scsi/ibmvscsi/ibmvfc.c:ibmvfc_rport_add_thread() {
	struct ibmvfc_host *vhost = container_of(work, struct ibmvfc_host,
						 rport_add_work_q);
	...
	spin_lock_irqsave(vhost->host->host_lock, flags);
    ...
}

Could it execute after the vhost is deallocated, leading to a use-after-free
when accessing the lock or other memory?

>  			if (tgt->add_rport) {
>  				did_work = 1;
>  				tgt->add_rport = 0;

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