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

[sashiko-bot@kernel.org]

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