Re: [PATCH v2 02/33] ibmvfc: flush rport_add_work_q during driver teardown

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch incorrectly assumes all events with `!evt->cmnd` are MAD events, assigning an uninitialized `evt->_done` pointer to `evt->done` for TMF events, causing a kernel panic or deadlock.

Pre-existing issues:
- [High] The error path in `ibmvfc_probe()` lacks a `cancel_work_sync()` or `flush_work()` for `vhost->rport_add_work_q`, leading to a workqueue leak and a use-after-free.
- [High] Target objects allocated from `vhost->tgt_pool` are improperly freed using `kfree()` instead of `mempool_free()`, permanently depleting the mempool's reserves.
- [High] In `ibmvfc_cancel_all_mq()`, the return value of `ibmvfc_send_event()` is ignored. If sending fails with `H_CLOSED`, the event is freed internally but the caller adds it to a wait list, causing a use-after-free and an indefinite hang.
--

--- Patch [2]: [PATCH v2 02/33] ibmvfc: flush rport_add_work_q during driver teardown ---
commit 17edf5c871930177f3e5dcc29059e768abdcd3dd
Author: Tyrel Datwyler <tyreld@linux.ibm.com>

    ibmvfc: flush rport_add_work_q during driver teardown
    
    INIT_WORK is called on rport_add_work_q during driver probe. When the
    adapter is removed, ibmvfc_remove() stops the work thread and proceeds
    to tear down the FC host, but any work item queued via schedule_work()
    that raced the teardown could still be running or pending in the
    system workqueue.
    
    Add a flush_work() call after kthread_stop() and before fc_remove_host()
    to drain any outstanding rport add work before the FC host and its rports
    are torn down.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
> index 0a5f73df8d7d2..72317d7840a53 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c
> @@ -6443,6 +6443,7 @@ static void ibmvfc_remove(struct vio_dev *vdev)
>  
>  	ibmvfc_wait_while_resetting(vhost);
>  	kthread_stop(vhost->work_thread);
> +	flush_work(&vhost->rport_add_work_q);
>  	fc_remove_host(vhost->host);
>  	scsi_remove_host(vhost->host);

[Severity: High]
This isn't a bug introduced by this patch, but does ibmvfc_probe() also need 
a similar flush_work() or cancel_work_sync() in its error path?

If ibmvfc_probe() fails after starting the kthread and initializing the CRQ 
(for example, if scsi_add_host() fails), it stops the kthread and frees the
host memory:

kill_kthread:
	kthread_stop(vhost->work_thread);
free_host_mem:
	ibmvfc_free_mem(vhost);
free_scsi_host:
	scsi_host_put(shost);

If the CRQ initialization has already received an interrupt, transitioned the
host state, and scheduled rport_add_work_q on the system workqueue, wouldn't
ibmvfc_rport_add_thread() execute after the vhost structure has been freed,
causing a use-after-free?

[ ... ]

> @@ -1367,8 +1367,8 @@ static void ibmvfc_set_rport_dev_loss_tmo(struct fc_rport *rport, u32 timeout)
>  static void ibmvfc_release_tgt(struct kref *kref)
>  {
>  	struct ibmvfc_target *tgt = container_of(kref, struct ibmvfc_target, kref);
>  	kfree(tgt);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does ibmvfc_release_tgt()
permanently deplete the tgt_pool mempool reserves?

Target structures are allocated from a mempool in ibmvfc_alloc_target():

	tgt = mempool_alloc(vhost->tgt_pool, GFP_NOIO);

However, ibmvfc_release_tgt() frees them directly using kfree(). While the
memory is returned to the slab allocator, bypassing mempool_free() means the
mempool's internal reserve count is never replenished.

Could this permanently deplete the mempool's preallocated reserves and lead
to allocation failures or device discovery hangs under memory pressure?

[ ... ]

> @@ -1591,8 +1591,8 @@ static void ibmvfc_init_event(struct ibmvfc_event *evt,
>  {
>  	evt->cmnd = NULL;
>  	evt->sync_iu = NULL;
>  	evt->eh_comp = NULL;
>  	evt->crq.format = format;
>  	if (format == IBMVFC_CMD_FORMAT)
>  		evt->done = done;
>  	else {
>  		evt->_done = done;

[ ... ]

> @@ -1774,9 +1774,9 @@ static int ibmvfc_send_event(struct ibmvfc_event *evt,
>  		if (evt->cmnd) {
>  			evt->cmnd->result = DID_ERROR << 16;
>  			evt->done = ibmvfc_scsi_eh_done;
>  		} else {
>  			evt->xfer_iu->mad_common.status = cpu_to_be16(IBMVFC_MAD_CRQ_ERROR);
>  			evt->done = evt->_done;
>  		}
>  
>  		evt->done(evt);

[Severity: High]
Does this else branch in ibmvfc_send_event() correctly handle Task
Management Function (TMF) events?

TMF events use the IBMVFC_CMD_FORMAT format and have evt->cmnd = NULL.
When ibmvfc_init_event() initializes a CMD format event, it explicitly leaves
evt->_done uninitialized.

If ibmvfc_send_event() encounters an error sending a TMF event, it evaluates
!evt->cmnd, enters the else branch, and assigns evt->done = evt->_done.

Could this execute an uninitialized or stale callback?

[ ... ]

> @@ -2630,8 +2630,8 @@ static int ibmvfc_cancel_all_mq(struct scsi_device *sdev, int type)
>  			}
>  			evt->sync_iu = &queues[i].cancel_rsp;
>  			ibmvfc_send_event(evt, vhost, default_timeout);
>  			list_add_tail(&evt->cancel, &cancelq);
>  		}
>  
>  		spin_unlock(queues[i].q_lock);
>  	}
>  	spin_unlock_irqrestore(vhost->host->host_lock, flags);
>  
>  	if (list_empty(&cancelq)) {
>  		if (vhost->log_level > IBMVFC_DEFAULT_LOG_LEVEL)
>  			sdev_printk(KERN_INFO, sdev, "No events found to cancel\n");
>  		return 0;
>  	}
>  
>  	sdev_printk(KERN_INFO, sdev, "Cancelling outstanding commands.\n");
>  
>  	list_for_each_entry_safe(evt, temp, &cancelq, cancel) {
>  		wait_for_completion(&evt->comp);

[Severity: High]
This isn't a bug introduced by this patch, but does ibmvfc_cancel_all_mq() 
hang indefinitely if ibmvfc_send_event() fails with H_CLOSED?

If ibmvfc_send_event() returns H_CLOSED, it returns SCSI_MLQUEUE_HOST_BUSY 
and frees the event via ibmvfc_free_event().

However, ibmvfc_cancel_all_mq() ignores the return value of 
ibmvfc_send_event(). It unconditionally proceeds to call 
list_add_tail(&evt->cancel, &cancelq) on the now-freed event and 
subsequently loops over the cancelq list calling 
wait_for_completion(&evt->comp). 

Could this cause a use-after-free and an indefinite hang since the completion
object resides in freed memory and will never be signaled?

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