Re: [PATCH v2] scsi: megaraid_sas: Protect megasas_get_ctrl_info() in megasas_resume()

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Mixing goto-based error handling with scoped_guard() in megasas_resume() violates subsystem cleanup guidelines.

Pre-existing issues:
- [High] Missing IRQ teardown in megasas_resume error path leads to ISR Use-After-Free and memory leak.
--

commit aca55e5d3d8f6827c1bf9b8345c147c29fda17ef
Author: Bart Van Assche <bvanassche@acm.org>
scsi: megaraid_sas: Protect megasas_get_ctrl_info() in megasas_resume()

This commit protects the megasas_get_ctrl_info() call within
megasas_resume() by wrapping it with scoped_guard() using
instance->reset_mutex. This ensures the function is called with the
mutex held, preventing unsafe releases and reacquisitions.

> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
> index f0152b043e18c..d4cbc0643eaf1 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_base.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
> @@ -7875,7 +7875,9 @@ megasas_resume(struct device *dev)
>  			goto fail_init_mfi;
>  	}
>  
> -	if (megasas_get_ctrl_info(instance) != DCMD_SUCCESS)
> +	scoped_guard(mutex, &instance->reset_mutex)
> +		rval = megasas_get_ctrl_info(instance);

[Severity: Medium]
Does mixing scoped_guard() with goto-based error handling in megasas_resume()
violate subsystem cleanup guidelines?

The subsystem documentation explicitly forbids mixing scope-based cleanup
helpers with goto-based error handling in the same function, requiring an
all-or-nothing conversion to avoid confusing ownership semantics.

Since megasas_resume() heavily relies on the goto fail_init_mfi style,
should this function be fully converted, or should a traditional
mutex_lock() and mutex_unlock() be used here instead?

> +	if (rval != DCMD_SUCCESS)
>  		goto fail_init_mfi;

[Severity: High]
This is a pre-existing issue, but does the fail_init_mfi error path lack
proper IRQ teardown, creating a potential Use-After-Free?

If a failure occurs later in megasas_resume() after interrupts are enabled
(for example, if megasas_fusion_start_watchdog() fails), the code jumps to
fail_init_mfi and calls megasas_free_ctrl_mem(instance). This frees
instance->reply_map and the fusion context, but it does not call
megasas_destroy_irqs(instance) or disable the interrupts.

Since instance->mask_interrupts would be 0 at that point, any subsequent
shared or spurious interrupt could invoke megasas_isr_fusion(), accessing
the freed reply_map and fusion context.

Does this error path need to unregister interrupts before freeing the memory?

>  
>  	tasklet_init(&instance->isr_tasklet, instance->instancet->tasklet,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/f06b5ee432b21cf293f0663e15b64f75a84b9fd5.1788204406.git.bvanassche@acm.org?part=1