Re: [PATCH] scsi: qla2xxx: wait for the EDIF RX timer before freeing entries

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

New issues:
- [High] The patch queues a self-destructing work item on the global `system_wq` which unlinks itself from tracking structures, leading to a kernel oops on module unload.

Pre-existing issues:
- [High] Lockless traversal in `qla_edif_list_find_sa_index` results in a Time-of-Check to Time-of-Use (TOCTOU) Use-After-Free and list corruption.
--

--- Patch [1]: [PATCH] scsi: qla2xxx: wait for the EDIF RX timer before freeing entries ---
commit f05ca5fac489b747acb7b49fe085de217cd65141
Author: Runyu Xiao <[email protected]>

scsi: qla2xxx: wait for the EDIF RX timer before freeing entries

This commit addresses a race condition where the EDIF RX timer callback might
run concurrently with entry removal and session teardown. It defers the freeing
of the entry using a work item on the global workqueue, which waits for the
timer to finish using timer_shutdown_sync().

> diff --git a/drivers/scsi/qla2xxx/qla_edif.c b/drivers/scsi/qla2xxx/qla_edif.c
> index bfa520f936a24..43c7da40a8169 100644
> --- a/drivers/scsi/qla2xxx/qla_edif.c
> +++ b/drivers/scsi/qla2xxx/qla_edif.c

[ ... ]

> @@ -85,7 +87,8 @@ static struct edif_list_entry *qla_edif_list_find_sa_index(fc_port_t *fcport,
>  	struct list_head *indx_list = &fcport->edif.edif_indx_list;
>  
>  	list_for_each_entry_safe(entry, tentry, indx_list, next) {
> -		if (entry->handle == handle)
> +		if (entry->handle == handle &&
> +		    !(READ_ONCE(entry->flags) & EDIF_ENTRY_FLAGS_FREE_PENDING))
>  			return entry;
>  	}

[Severity: High]
This is a pre-existing issue, but does this lockless traversal risk a
time-of-check to time-of-use use-after-free?

The list is iterated without holding indx_list_lock or using RCU. A caller
like qla24xx_sadb_update() receives the raw pointer and later uses it for
deletion, oblivious to the fact that it might have already been freed by a
concurrent worker thread or IO completion.

[ ... ]

> @@ -197,7 +201,51 @@ static void qla_edif_list_delete_sa_index(fc_port_t *fcport, struct edif_list_en
>  	unsigned long flags = 0;
>  
>  	spin_lock_irqsave(&fcport->edif.indx_list_lock, flags);
> -	list_del(&entry->next);
> +	if (!list_empty(&entry->next))
> +		list_del_init(&entry->next);
> +	spin_unlock_irqrestore(&fcport->edif.indx_list_lock, flags);
> +}
> +
> +static void qla_edif_list_free_sa_index(struct edif_list_entry *entry)
> +{
> +	cancel_work_sync(&entry->free_work);
> +	timer_shutdown_sync(&entry->timer);
> +	kfree(entry);
> +}
> +
> +static void qla_edif_list_free_sa_index_work(struct work_struct *work)
> +{
> +	struct edif_list_entry *entry = container_of(work,
> +			struct edif_list_entry, free_work);
> +	fc_port_t *fcport = entry->fcport;
> +	unsigned long flags = 0;
> +	bool free_entry = false;
> +
> +	timer_shutdown_sync(&entry->timer);
> +
> +	spin_lock_irqsave(&fcport->edif.indx_list_lock, flags);
> +	if (!list_empty(&entry->next)) {
> +		list_del_init(&entry->next);
> +		free_entry = true;
> +	}

[Severity: High]
If the driver module is unloaded while this work is pending, is there a
risk of a kernel oops?

The work item unlinks the entry here, making it untrackable by
qla_edif_list_del() during module teardown.

> +	spin_unlock_irqrestore(&fcport->edif.indx_list_lock, flags);
> +
> +	if (free_entry)
> +		kfree(entry);
> +}
> +
> +static void qla_edif_list_schedule_free_sa_index(fc_port_t *fcport,
> +						 struct edif_list_entry *entry)
> +{
> +	unsigned long flags = 0;
> +
> +	spin_lock_irqsave(&fcport->edif.indx_list_lock, flags);
> +	if (!list_empty(&entry->next) &&
> +	    !(entry->flags & EDIF_ENTRY_FLAGS_FREE_PENDING)) {
> +		entry->flags |= EDIF_ENTRY_FLAGS_FREE_PENDING;
> +		timer_shutdown(&entry->timer);
> +		schedule_work(&entry->free_work);

[Severity: High]
Since this schedules the work on the global system_wq rather than a
driver-specific workqueue that gets flushed during module teardown, couldn't
the module be unloaded before this global work completes?

Because the work unlinks the entry as noted above, the standard module
teardown sequence won't see it to cancel the work, potentially leaving
the worker thread to execute unloaded module code.

> +	}
>  	spin_unlock_irqrestore(&fcport->edif.indx_list_lock, flags);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1