[PATCH] scsi: megaraid_sas: kill tasklet after free_irq in teardown paths

[PATCH] scsi: megaraid_sas: kill tasklet after free_irq in teardown paths

[Fan Wu]

On MFI-series adapters the interrupt handler arms instance->isr_tasklet
for as long as the interrupt is registered, so the tasklet can be armed
again after tasklet_kill() has returned.  megasas_detach_one() and
megasas_suspend() kill the tasklet before unregistering the interrupts,
and the probe error paths skip the kill entirely, so the completion
callback can run after the teardown frees the buffers it dereferences,
or a tasklet pending across suspend is re-initialized by resume's
tasklet_init() while still queued.

Kill the tasklet after megasas_destroy_irqs(), which drains the handler
via free_irq(), and add the missing kills in the probe error paths.

This issue was found by an in-house static analysis tool.

Fixes: 5d018ad05734 ("[SCSI] megaraid_sas: adds tasklet for cmd completion")
Cc: stable@vger.kernel.org
Assisted-by: Codex:gpt-5.6
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
 drivers/scsi/megaraid/megaraid_sas_base.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
index ecd365d78ae3..627bcba720cd 100644
--- a/drivers/scsi/megaraid/megaraid_sas_base.c
+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
@@ -6536,6 +6536,7 @@ static int megasas_init_fw(struct megasas_instance *instance)
 fail_get_ld_pd_list:
 	instance->instancet->disable_intr(instance);
 	megasas_destroy_irqs(instance);
+	tasklet_kill(&instance->isr_tasklet);
 fail_init_adapter:
 	if (instance->msix_vectors)
 		pci_free_irq_vectors(instance->pdev);
@@ -7618,6 +7619,7 @@ static int megasas_probe_one(struct pci_dev *pdev,
 
 	instance->instancet->disable_intr(instance);
 	megasas_destroy_irqs(instance);
+	tasklet_kill(&instance->isr_tasklet);
 
 	if (instance->adapter_type != MFI_SERIES)
 		megasas_release_fusion(instance);
@@ -7770,12 +7772,11 @@ megasas_suspend(struct device *dev)
 		instance->ev = NULL;
 	}
 
-	tasklet_kill(&instance->isr_tasklet);
-
 	pci_set_drvdata(instance->pdev, instance);
 	instance->instancet->disable_intr(instance);
 
 	megasas_destroy_irqs(instance);
+	tasklet_kill(&instance->isr_tasklet);
 
 	if (instance->msix_vectors)
 		pci_free_irq_vectors(instance->pdev);
@@ -8010,8 +8011,6 @@ static void megasas_detach_one(struct pci_dev *pdev)
 	/* cancel all wait events */
 	wake_up_all(&instance->int_cmd_wait_q);
 
-	tasklet_kill(&instance->isr_tasklet);
-
 	/*
 	 * Take the instance off the instance array. Note that we will not
 	 * decrement the max_index. We let this array be sparse array
@@ -8028,6 +8027,7 @@ static void megasas_detach_one(struct pci_dev *pdev)
 	instance->instancet->disable_intr(instance);
 
 	megasas_destroy_irqs(instance);
+	tasklet_kill(&instance->isr_tasklet);
 
 	if (instance->msix_vectors)
 		pci_free_irq_vectors(instance->pdev);

Re: [PATCH] scsi: megaraid_sas: kill tasklet after free_irq in teardown paths

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Unprotected concurrent access to the global `megasas_mgmt_info` array during device probe and detach causes out-of-bounds writes.
- [High] The error paths in `megasas_resume` fail to disable/destroy IRQs, kill the tasklet, and free PCI IRQ vectors, causing a use-after-free.
- [High] Legacy INTx IRQ vectors are systematically leaked in teardown and error paths.
- [High] Incomplete tasklet cleanup in initialization error paths allows a pending tasklet to execute after the `instance` is freed.
- [High] The asynchronous work item `instance->work_init` is never cancelled during driver teardown, suspend, or probe error paths.
--

--- Patch [1]: [PATCH] scsi: megaraid_sas: kill tasklet after free_irq in teardown paths ---
commit 8d9eb16b77703617161e881af92f251c7fb898af
Author: Fan Wu <fanwu01@zju.edu.cn>

scsi: megaraid_sas: kill tasklet after free_irq in teardown paths

On MFI-series adapters the interrupt handler arms instance->isr_tasklet
for as long as the interrupt is registered, so the tasklet can be armed
again after tasklet_kill() has returned.  This patch moves tasklet_kill()
after megasas_destroy_irqs() to ensure tasklets are stopped after
the handler is drained.

Cc: stable@vger.kernel.org

> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
> index f0152b043e18c..e72009a587567 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_base.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
> @@ -6536,6 +6536,7 @@ static int megasas_init_fw(struct megasas_instance *instance)
>  fail_get_ld_pd_list:
>  	instance->instancet->disable_intr(instance);
>  	megasas_destroy_irqs(instance);
> +	tasklet_kill(&instance->isr_tasklet);
>  fail_init_adapter:

[Severity: High]
This isn't a bug introduced by this patch, but does the jump to
fail_init_adapter leave the tasklet pending if IRQ setup fails?

If megasas_setup_irqs_msix() partially succeeds but fails on a
subsequent vector, it returns an error and the code jumps to
fail_init_adapter, bypassing the tasklet_kill() added here.

Since the rollback loop in megasas_setup_irqs_msix() calls free_irq()
which doesn't kill scheduled tasklets, can the pending tasklet
execute and dereference the freed instance memory?

>  	if (instance->msix_vectors)
>  		pci_free_irq_vectors(instance->pdev);

[Severity: High]
This is a pre-existing issue, but does this conditional check leak
legacy INTx IRQ vectors?

When MSI-X is unavailable, the driver falls back to allocating a
legacy INTx interrupt, but instance->msix_vectors remains 0.
By wrapping pci_free_irq_vectors() in this condition across the
teardown and error paths, it appears the cleanup is skipped for
INTx vectors.

Does this permanently leak IRQ resources in the error paths?

[ ... ]

> @@ -7618,6 +7619,7 @@ static int megasas_probe_one(struct pci_dev *pdev,
>  
>  	instance->instancet->disable_intr(instance);
>  	megasas_destroy_irqs(instance);
> +	tasklet_kill(&instance->isr_tasklet);

[Severity: High]
This is a pre-existing issue, but do the error paths in megasas_probe_one()
properly clean up interrupts and tasklets before freeing the instance?

If a late step in the probe fails, such as megasas_fusion_start_watchdog(),
the code jumps to fail_init_mfi.  At this point, IRQs have already been
requested and the tasklet initialized.

However, the fail_init_mfi path skips disable_intr, megasas_destroy_irqs,
and tasklet_kill, but proceeds to free the coherent DMA buffers and the
host structure.

Can an active ISR or scheduled tasklet execute after these are freed,
resulting in a use-after-free?

[ ... ]

> @@ -8010,8 +8011,6 @@ static void megasas_detach_one(struct pci_dev *pdev)
>  	/* cancel all wait events */
>  	wake_up_all(&instance->int_cmd_wait_q);
>  

[Severity: High]
This is a pre-existing issue, but is the asynchronous work item
instance->work_init properly cancelled during detach?

The driver schedules instance->work_init on the global system
workqueue to handle firmware state changes and online controller
resets.  Neither the detach path nor the suspend and probe error
handlers appear to call cancel_work_sync(&instance->work_init).

If a controller fault triggers the work right before teardown,
will the asynchronous work dereference the freed instance memory?

> -	tasklet_kill(&instance->isr_tasklet);
> -
>  	/*
>  	 * Take the instance off the instance array. Note that we will not
>  	 * decrement the max_index. We let this array be sparse array

[Severity: Critical]
This is a pre-existing issue, but does this sparse array management
allow unbounded growth and out-of-bounds writes?

Because megasas_detach_one() intentionally does not decrement
max_index, repeated unbind and bind cycles via sysfs or hotplugs
will cause max_index to grow continuously.

Since there is no bounds checking in megasas_probe_one() when
inserting new instances:

megasas_probe_one() {
        ...
        megasas_mgmt_info.count++;
        megasas_mgmt_info.instance[megasas_mgmt_info.max_index] = instance;
        megasas_mgmt_info.max_index++;
        ...
}

Will max_index eventually exceed MAX_MGMT_ADAPTERS and overwrite
adjacent kernel memory?

> @@ -8028,6 +8027,7 @@ static void megasas_detach_one(struct pci_dev *pdev)
>  	instance->instancet->disable_intr(instance);
>  
>  	megasas_destroy_irqs(instance);
> +	tasklet_kill(&instance->isr_tasklet);
>  
>  	if (instance->msix_vectors)
>  		pci_free_irq_vectors(instance->pdev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260819005043.439436-1-fanwu01@zju.edu.cn?part=1