Re: [PATCHv3 4/6] ata: pata_mpc52xx: remove redundant irq_dispose_mapping calls

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Calling `irq_dispose_mapping()` on devres-managed IRQs destroys the IRQ descriptor before `devm_free_irq()` can execute, leading to a memory leak of `struct irqaction` and bypassing interrupt synchronization.
--

commit 22d637bd9ec3129e235484c8f8f7b01af8844e9d
Author: Rosen Penev <rosenp@gmail.com>

ata: pata_mpc52xx: remove redundant irq_dispose_mapping calls

bcom_ata_release() calls bcom_task_free(), which already calls
irq_dispose_mapping(tsk->irq).  The explicit irq_dispose_mapping()
calls in the probe error path and remove function are redundant
and cause a double-free of the same mapping.

> diff --git a/drivers/ata/pata_mpc52xx.c b/drivers/ata/pata_mpc52xx.c
> index 210a63283f62b..408b1cb75d1e7 100644
> --- a/drivers/ata/pata_mpc52xx.c
> +++ b/drivers/ata/pata_mpc52xx.c
> @@ -793,7 +793,6 @@ static int mpc52xx_ata_probe(struct platform_device *op)
>  	return 0;
>  
>   err2:
> -	irq_dispose_mapping(task_irq);
>  	bcom_ata_release(dmatsk);
>   err1:
>  	irq_dispose_mapping(ata_irq);

[Severity: High]
This is a pre-existing issue, but does calling irq_dispose_mapping() on these
IRQs bypass the devres cleanup mechanisms?

Because task_irq and ata_irq are managed via devm_request_irq(), their 
free_irq() calls are deferred to the devres cleanup phase.

If irq_dispose_mapping() (called explicitly for ata_irq, and implicitly via
bcom_ata_release() for task_irq) destroys the IRQ descriptor before devres
cleanup runs, the subsequent devm_irq_release() -> free_irq() will fail to
find the descriptor when irq_to_desc() returns NULL.

This causes free_irq() to return immediately without cleaning up, which
leaks the struct irqaction memory. 

More importantly, it skips the call to __synchronize_irq(). Could this allow
the interrupt handler to continue running concurrently while devres tears
down the driver's mapped registers, posing a use-after-free risk?

> @@ -804,14 +803,11 @@ static void mpc52xx_ata_remove(struct platform_device *op)
>  {
>  	struct ata_host *host = platform_get_drvdata(op);
>  	struct mpc52xx_ata_priv *priv = host->private_data;
[ ... ]
>  	/* Clean up DMA */
> -	task_irq = bcom_get_task_irq(priv->dmatsk);
> -	irq_dispose_mapping(task_irq);
>  	bcom_ata_release(priv->dmatsk);
>  	irq_dispose_mapping(priv->ata_irq);
>  }

[Severity: High]
This is also a pre-existing issue, but the same devres synchronization concern
applies here in mpc52xx_ata_remove() when disposing the mappings prior to the
device devres teardown.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612183227.20446-5-rosenp@gmail.com?part=1