Re: [PATCH v2] scsi: qla2xxx: synchronize EDIF entry teardown with timer

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Uninitialized `fcport` pointer in `edif_list_entry` causes a NULL pointer dereference in the deferred freeing workqueue.
--

--- Patch [1]: [PATCH v2] scsi: qla2xxx: synchronize EDIF entry teardown with timer ---
commit 79fc4cdf869d75c3e6f24287c79efbb79add0bf0
Author: Runyu Xiao <[email protected]>

scsi: qla2xxx: synchronize EDIF entry teardown with timer

This patch introduces a deferred freeing mechanism for EDIF entries to
prevent teardown races with running timer callbacks. It keeps the entry
linked while teardown is pending and queues a work item to safely free
the entry on the adapter workqueue.

Link: https://lore.kernel.org/r/[email protected]

> diff --git a/drivers/scsi/qla2xxx/qla_edif.c b/drivers/scsi/qla2xxx/qla_edif.c
> --- a/drivers/scsi/qla2xxx/qla_edif.c
> +++ b/drivers/scsi/qla2xxx/qla_edif.c
> @@ -163,16 +163,19 @@ static int qla_edif_list_add_sa_update_index(fc_port_t *fcport,

[ ... ]

> -	entry = kzalloc_obj(struct edif_list_entry, GFP_ATOMIC);
> -	if (!entry)
> +	new_entry = kzalloc_obj(struct edif_list_entry, GFP_ATOMIC);
> +	if (!new_entry)
>  		return -ENOMEM;
>  
> -	INIT_LIST_HEAD(&entry->next);
> -	entry->handle = handle;
> -	entry->update_sa_index = sa_index;
> -	entry->delete_sa_index = INVALID_EDIF_SA_INDEX;
> -	entry->count = 0;
> -	entry->flags = 0;
> -	timer_setup(&entry->timer, qla2x00_sa_replace_iocb_timeout, 0);
> +	INIT_LIST_HEAD(&new_entry->next);
> +	new_entry->handle = handle;
> +	new_entry->update_sa_index = sa_index;
> +	new_entry->delete_sa_index = INVALID_EDIF_SA_INDEX;
> +	new_entry->count = 0;
> +	new_entry->flags = 0;
> +	timer_setup(&new_entry->timer, qla2x00_sa_replace_iocb_timeout, 0);
> +	INIT_WORK(&new_entry->free_work, qla_edif_list_free_sa_index_work);

[Severity: High]
Does this code leave new_entry->fcport uninitialized? Looking at
qla_edif_list_add_sa_update_index(), the kzalloc_obj call zero-initializes
the memory, but there doesn't appear to be an assignment to set
new_entry->fcport to the passed-in fcport pointer.

[ ... ]

> +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);

[Severity: High]
If entry->fcport is not initialized when the entry is created, will this
result in a null pointer dereference?

It looks like the worker thread in qla_edif_list_free_sa_index_work()
extracts the uninitialized fcport and attempts to acquire its spinlock,
which could lead to a crash here.

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