[PATCH] scsi: ibmvfc: synchronize event timer completion

[PATCH] scsi: ibmvfc: synchronize event timer completion

[Runyu Xiao]

The response and purge paths remove an event from their queue and
complete it immediately after timer_delete(). A timer callback can still
be running on another CPU and can access the event while its completion
returns it to the event pool for reuse.

Use timer_delete_sync_try() in the interrupt-side completion path and defer
the completion to workqueue context when the timer callback is active.
Flush the work before releasing the event pool so an event cannot be reused
or freed while its callback is still running.

Fixes: 072b91f9c651 ("[SCSI] ibmvfc: IBM Power Virtual Fibre Channel Adapter Client Driver")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
---
 drivers/scsi/ibmvscsi/ibmvfc-core.c | 40 ++++++++++++++++++++++-------
 drivers/scsi/ibmvscsi/ibmvfc.h      |  2 ++
 2 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
index 93b9f699c..072008411 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
+++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
@@ -811,6 +811,28 @@ static int ibmvfc_send_crq_init_complete(struct ibmvfc_host *vhost)
 	return ibmvfc_send_crq(vhost, 0xC002000000000000LL, 0);
 }
 
+static void ibmvfc_complete_event_work(struct work_struct *work)
+{
+	struct ibmvfc_event *evt = container_of(work, struct ibmvfc_event,
+						timer_work);
+
+	timer_delete_sync(&evt->timer);
+	evt->done(evt);
+}
+
+static void ibmvfc_complete_event(struct ibmvfc_event *evt)
+{
+	/* The response path can run in hardirq context, so defer only when the
+	 * timer callback is already executing on another context.
+	 */
+	if (timer_delete_sync_try(&evt->timer) < 0) {
+		schedule_work(&evt->timer_work);
+		return;
+	}
+
+	evt->done(evt);
+}
+
 /**
  * ibmvfc_init_event_pool - Allocates and initializes the event pool for a host
  * @vhost:	ibmvfc host who owns the event pool
@@ -859,6 +881,7 @@ static int ibmvfc_init_event_pool(struct ibmvfc_host *vhost,
 		 */
 		atomic_set(&evt->active, -1);
 		atomic_set(&evt->free, 1);
+		INIT_WORK(&evt->timer_work, ibmvfc_complete_event_work);
 		evt->crq.valid = 0x80;
 		evt->crq.ioba = cpu_to_be64(pool->iu_token + (sizeof(*evt->xfer_iu) * i));
 		evt->xfer_iu = pool->iu_storage + i;
@@ -886,6 +909,7 @@ static void ibmvfc_free_event_pool(struct ibmvfc_host *vhost,
 
 	ENTER;
 	for (i = 0; i < pool->size; ++i) {
+		flush_work(&pool->events[i].timer_work);
 		list_del(&pool->events[i].queue_list);
 		BUG_ON(atomic_read(&pool->events[i].free) != 1);
 		if (pool->events[i].ext_list)
@@ -1134,7 +1158,7 @@ static void ibmvfc_complete_purge(struct list_head *purge_list)
 	list_for_each_entry_safe(evt, pos, purge_list, queue_list) {
 		list_del(&evt->queue_list);
 		ibmvfc_trc_end(evt);
-		evt->done(evt);
+		ibmvfc_complete_event(evt);
 	}
 }
 
@@ -1162,7 +1186,7 @@ static void ibmvfc_fail_request(struct ibmvfc_event *evt, int error_code)
 	else
 		evt->xfer_iu->mad_common.status = cpu_to_be16(IBMVFC_MAD_DRIVER_FAILED);
 
-	timer_delete(&evt->timer);
+	timer_delete_sync_try(&evt->timer);
 }
 
 /**
@@ -1824,7 +1848,6 @@ int ibmvfc_send_event(struct ibmvfc_event *evt,
 		atomic_set(&evt->active, 0);
 		list_del(&evt->queue_list);
 		spin_unlock_irqrestore(&evt->queue->l_lock, flags);
-		timer_delete(&evt->timer);
 
 		/* If send_crq returns H_CLOSED, return SCSI_MLQUEUE_HOST_BUSY.
 		 * Firmware will send a CRQ with a transport event (0xFF) to
@@ -1836,7 +1859,8 @@ int ibmvfc_send_event(struct ibmvfc_event *evt,
 				dev_warn(vhost->dev, "Send warning. Receive queue closed, will retry.\n");
 			if (evt->cmnd)
 				scsi_dma_unmap(evt->cmnd);
-			ibmvfc_free_event(evt);
+			evt->done = ibmvfc_free_event;
+			ibmvfc_complete_event(evt);
 			return SCSI_MLQUEUE_HOST_BUSY;
 		}
 
@@ -1851,7 +1875,7 @@ int ibmvfc_send_event(struct ibmvfc_event *evt,
 			evt->done = evt->_done;
 		}
 
-		evt->done(evt);
+		ibmvfc_complete_event(evt);
 	} else {
 		spin_unlock_irqrestore(&evt->queue->l_lock, flags);
 		ibmvfc_trc_start(evt);
@@ -3925,10 +3949,9 @@ static void ibmvfc_tasklet(void *data)
 	spin_unlock_irqrestore(vhost->host->host_lock, flags);
 
 	list_for_each_entry_safe(evt, temp, &evt_doneq, queue_list) {
-		timer_delete(&evt->timer);
 		list_del(&evt->queue_list);
 		ibmvfc_trc_end(evt);
-		evt->done(evt);
+		ibmvfc_complete_event(evt);
 	}
 }
 
@@ -4031,10 +4054,9 @@ static void ibmvfc_drain_sub_crq(struct ibmvfc_queue *scrq)
 	spin_unlock_irqrestore(scrq->q_lock, flags);
 
 	list_for_each_entry_safe(evt, temp, &evt_doneq, queue_list) {
-		timer_delete(&evt->timer);
 		list_del(&evt->queue_list);
 		ibmvfc_trc_end(evt);
-		evt->done(evt);
+		ibmvfc_complete_event(evt);
 	}
 }
 
diff --git a/drivers/scsi/ibmvscsi/ibmvfc.h b/drivers/scsi/ibmvscsi/ibmvfc.h
index ca80ceffe..fba2e3a23 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc.h
+++ b/drivers/scsi/ibmvscsi/ibmvfc.h
@@ -14,6 +14,7 @@
 #include <linux/interrupt.h>
 #include <linux/list.h>
 #include <linux/types.h>
+#include <linux/workqueue.h>
 #include <scsi/scsi_device.h>
 #include <scsi/viosrp.h>
 #include <linux/nvme.h>
@@ -867,6 +868,7 @@ struct ibmvfc_event {
 	struct completion comp;
 	struct completion *eh_comp;
 	struct timer_list timer;
+	struct work_struct timer_work;
 	u16 hwq;
 	u8 reserved;
 };
-- 
2.34.1

Re: [PATCH] scsi: ibmvfc: synchronize event timer completion

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: ibmvfc: deferred MAD event completion executes without host_lock
- [High] scsi: ibmvfc: lockless list_del in ibmvfc_free_event_pool() races with workqueue
--

commit e0e195e126820c8780df5dcd52eddd1ee13268d3
Author: Runyu Xiao <runyu.xiao@seu.edu.cn>

scsi: ibmvfc: synchronize event timer completion

This commit replaces timer_delete() with timer_delete_sync_try() during
event completion to avoid races with active timer callbacks. If a timer
callback is executing, it defers the completion to a workqueue and flushes
the work before freeing the event pool.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index 3534ac45e9b87..71d9f9886ae55 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> @@ -811,6 +811,28 @@ static int ibmvfc_send_crq_init_complete(struct ibmvfc_host *vhost)
>  	return ibmvfc_send_crq(vhost, 0xC002000000000000LL, 0);
>  }
>  
> +static void ibmvfc_complete_event_work(struct work_struct *work)
> +{
> +	struct ibmvfc_event *evt = container_of(work, struct ibmvfc_event,
> +						timer_work);
> +
> +	timer_delete_sync(&evt->timer);
> +	evt->done(evt);

[Severity: High]
If the completion is deferred to this workqueue, does it execute the callback
without acquiring the host_lock?

When ibmvfc_send_event() fails, it overrides evt->done to the raw callback
(evt->_done) to avoid deadlocking inline. If that completion is then deferred
to this workqueue via ibmvfc_complete_event(), it appears it will run in
process context without the lock. Since MAD event handlers typically require
host_lock to safely modify host state machine fields, could this cause data
races or state corruption?

> +}
> +
> +static void ibmvfc_complete_event(struct ibmvfc_event *evt)
> +{
> +	/* The response path can run in hardirq context, so defer only when the
> +	 * timer callback is already executing on another context.
> +	 */
> +	if (timer_delete_sync_try(&evt->timer) < 0) {
> +		schedule_work(&evt->timer_work);
> +		return;
> +	}
> +
> +	evt->done(evt);
> +}

[ ... ]

> @@ -886,6 +909,7 @@ static void ibmvfc_free_event_pool(struct ibmvfc_host *vhost,
>  
>  	ENTER;
>  	for (i = 0; i < pool->size; ++i) {
> +		flush_work(&pool->events[i].timer_work);
>  		list_del(&pool->events[i].queue_list);

[Severity: High]
Does this lockless list_del() race with work items for other events?

This loop flushes one specific event's work and then immediately removes it
from the list locklessly. However, work items for other events in the pool
might still be completing concurrently on other CPUs.

When those concurrent work items call into ibmvfc_free_event(), they will
acquire queue->l_lock and call list_add_tail() on the queue->free list.
Since list_del() is modifying the same list without taking queue->l_lock,
could this corrupt the list pointers and lead to a crash?

>  		BUG_ON(atomic_read(&pool->events[i].free) != 1);
>  		if (pool->events[i].ext_list)

[ ... ]

> @@ -1851,7 +1875,7 @@ int ibmvfc_send_event(struct ibmvfc_event *evt,
>  			evt->done = evt->_done;
>  		}
>  
> -		evt->done(evt);
> +		ibmvfc_complete_event(evt);

[Severity: High]
If ibmvfc_complete_event() here falls back to schedule_work(), the event
completion is abandoned to the workqueue. As mentioned above, the deferred
evt->done() call will execute without the host_lock that the caller of
ibmvfc_send_event() currently holds. Is it safe for the MAD completion path
to run asynchronously without that lock?

>  	} else {
>  		spin_unlock_irqrestore(&evt->queue->l_lock, flags);
>  		ibmvfc_trc_start(evt);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930075801.2949514-1-runyu.xiao@seu.edu.cn?part=1

[PATCH v2] scsi: ibmvfc: synchronize event timer completion

[Runyu Xiao]

An event completion may be deferred when the timer callback is active. MAD
events normally use ibmvfc_locked_done(), but send failures replace the
wrapper with the raw callback because the caller may already hold
host_lock. Preserve that locking requirement when the completion runs from
workqueue context.

Flush every event completion work item before taking the event pool list
lock, then remove the free-list entries while holding queue->l_lock. Route
purge completions through the same timer synchronization helper so all
completion paths obey the same ordering.

Fixes: 072b91f9c651 ("[SCSI] ibmvfc: IBM Power Virtual Fibre Channel Adapter Client Driver")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
---
 drivers/scsi/ibmvscsi/ibmvfc-core.c | 61 ++++++++++++++++++++++++-----
 drivers/scsi/ibmvscsi/ibmvfc.h      |  3 ++
 2 files changed, 55 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
index 93b9f699c..a3d016f6e 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
+++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
@@ -811,6 +811,35 @@ static int ibmvfc_send_crq_init_complete(struct ibmvfc_host *vhost)
 	return ibmvfc_send_crq(vhost, 0xC002000000000000LL, 0);
 }
 
+static void ibmvfc_complete_event_work(struct work_struct *work)
+{
+	struct ibmvfc_event *evt = container_of(work, struct ibmvfc_event,
+						timer_work);
+	unsigned long flags;
+
+	timer_delete_sync(&evt->timer);
+	if (evt->done_needs_host_lock) {
+		spin_lock_irqsave(evt->vhost->host->host_lock, flags);
+		evt->done(evt);
+		spin_unlock_irqrestore(evt->vhost->host->host_lock, flags);
+	} else {
+		evt->done(evt);
+	}
+}
+
+static void ibmvfc_complete_event(struct ibmvfc_event *evt)
+{
+	/* The response path can run in hardirq context, so defer only when the
+	 * timer callback is already executing on another context.
+	 */
+	if (timer_delete_sync_try(&evt->timer) < 0) {
+		schedule_work(&evt->timer_work);
+		return;
+	}
+
+	evt->done(evt);
+}
+
 /**
  * ibmvfc_init_event_pool - Allocates and initializes the event pool for a host
  * @vhost:	ibmvfc host who owns the event pool
@@ -859,6 +888,7 @@ static int ibmvfc_init_event_pool(struct ibmvfc_host *vhost,
 		 */
 		atomic_set(&evt->active, -1);
 		atomic_set(&evt->free, 1);
+		INIT_WORK(&evt->timer_work, ibmvfc_complete_event_work);
 		evt->crq.valid = 0x80;
 		evt->crq.ioba = cpu_to_be64(pool->iu_token + (sizeof(*evt->xfer_iu) * i));
 		evt->xfer_iu = pool->iu_storage + i;
@@ -882,18 +912,31 @@ static void ibmvfc_free_event_pool(struct ibmvfc_host *vhost,
 				   struct ibmvfc_queue *queue)
 {
 	int i;
+	unsigned long flags;
 	struct ibmvfc_event_pool *pool = &queue->evt_pool;
 
 	ENTER;
+	if (!pool->size)
+		goto out;
+
+	for (i = 0; i < pool->size; ++i)
+		flush_work(&pool->events[i].timer_work);
+
+	spin_lock_irqsave(&queue->l_lock, flags);
 	for (i = 0; i < pool->size; ++i) {
 		list_del(&pool->events[i].queue_list);
 		BUG_ON(atomic_read(&pool->events[i].free) != 1);
+	}
+	spin_unlock_irqrestore(&queue->l_lock, flags);
+
+	for (i = 0; i < pool->size; ++i) {
 		if (pool->events[i].ext_list)
 			dma_pool_free(vhost->sg_pool,
 				      pool->events[i].ext_list,
 				      pool->events[i].ext_list_token);
 	}
 
+out:
 	kfree(pool->events);
 	dma_free_coherent(vhost->dev,
 			  pool->size * sizeof(*pool->iu_storage),
@@ -1134,7 +1177,7 @@ static void ibmvfc_complete_purge(struct list_head *purge_list)
 	list_for_each_entry_safe(evt, pos, purge_list, queue_list) {
 		list_del(&evt->queue_list);
 		ibmvfc_trc_end(evt);
-		evt->done(evt);
+		ibmvfc_complete_event(evt);
 	}
 }
 
@@ -1162,7 +1205,7 @@ static void ibmvfc_fail_request(struct ibmvfc_event *evt, int error_code)
 	else
 		evt->xfer_iu->mad_common.status = cpu_to_be16(IBMVFC_MAD_DRIVER_FAILED);
 
-	timer_delete(&evt->timer);
+	timer_delete_sync_try(&evt->timer);
 }
 
 /**
@@ -1669,6 +1712,7 @@ void ibmvfc_init_event(struct ibmvfc_event *evt,
 		evt->_done = done;
 		evt->done = ibmvfc_locked_done;
 	}
+	evt->done_needs_host_lock = false;
 	evt->hwq = 0;
 }
 
@@ -1824,7 +1868,6 @@ int ibmvfc_send_event(struct ibmvfc_event *evt,
 		atomic_set(&evt->active, 0);
 		list_del(&evt->queue_list);
 		spin_unlock_irqrestore(&evt->queue->l_lock, flags);
-		timer_delete(&evt->timer);
 
 		/* If send_crq returns H_CLOSED, return SCSI_MLQUEUE_HOST_BUSY.
 		 * Firmware will send a CRQ with a transport event (0xFF) to
@@ -1836,7 +1879,8 @@ int ibmvfc_send_event(struct ibmvfc_event *evt,
 				dev_warn(vhost->dev, "Send warning. Receive queue closed, will retry.\n");
 			if (evt->cmnd)
 				scsi_dma_unmap(evt->cmnd);
-			ibmvfc_free_event(evt);
+			evt->done = ibmvfc_free_event;
+			ibmvfc_complete_event(evt);
 			return SCSI_MLQUEUE_HOST_BUSY;
 		}
 
@@ -1849,9 +1893,10 @@ int ibmvfc_send_event(struct ibmvfc_event *evt,
 		} else {
 			evt->xfer_iu->mad_common.status = cpu_to_be16(IBMVFC_MAD_CRQ_ERROR);
 			evt->done = evt->_done;
+			evt->done_needs_host_lock = true;
 		}
 
-		evt->done(evt);
+		ibmvfc_complete_event(evt);
 	} else {
 		spin_unlock_irqrestore(&evt->queue->l_lock, flags);
 		ibmvfc_trc_start(evt);
@@ -3925,10 +3970,9 @@ static void ibmvfc_tasklet(void *data)
 	spin_unlock_irqrestore(vhost->host->host_lock, flags);
 
 	list_for_each_entry_safe(evt, temp, &evt_doneq, queue_list) {
-		timer_delete(&evt->timer);
 		list_del(&evt->queue_list);
 		ibmvfc_trc_end(evt);
-		evt->done(evt);
+		ibmvfc_complete_event(evt);
 	}
 }
 
@@ -4031,10 +4075,9 @@ static void ibmvfc_drain_sub_crq(struct ibmvfc_queue *scrq)
 	spin_unlock_irqrestore(scrq->q_lock, flags);
 
 	list_for_each_entry_safe(evt, temp, &evt_doneq, queue_list) {
-		timer_delete(&evt->timer);
 		list_del(&evt->queue_list);
 		ibmvfc_trc_end(evt);
-		evt->done(evt);
+		ibmvfc_complete_event(evt);
 	}
 }
 
diff --git a/drivers/scsi/ibmvscsi/ibmvfc.h b/drivers/scsi/ibmvscsi/ibmvfc.h
index ca80ceffe..912d250da 100644
--- a/drivers/scsi/ibmvscsi/ibmvfc.h
+++ b/drivers/scsi/ibmvscsi/ibmvfc.h
@@ -14,6 +14,7 @@
 #include <linux/interrupt.h>
 #include <linux/list.h>
 #include <linux/types.h>
+#include <linux/workqueue.h>
 #include <scsi/scsi_device.h>
 #include <scsi/viosrp.h>
 #include <linux/nvme.h>
@@ -867,6 +868,8 @@ struct ibmvfc_event {
 	struct completion comp;
 	struct completion *eh_comp;
 	struct timer_list timer;
+	struct work_struct timer_work;
+	bool done_needs_host_lock;
 	u16 hwq;
 	u8 reserved;
 };
-- 
2.34.1