Re: [PATCH net,v2] net: ipa: Fix double free of the interrupt structure on config error

[Alex Elder]

On 10/29/26 3:20 AM, Thomas Fourier wrote:
> Since ipa_interrupt_init() allocates struct ipa_interrupt in ipa_probe(),
> the structure is owned by ipa_probe() and released with
> ipa_interrupt_exit() on its error paths and in ipa_remove().  The error
> path of ipa_interrupt_config() still frees it with kfree().  If
> ipa_interrupt_config() fails (called from ipa_config() via ipa_setup()),
> ipa_probe() unwinds through err_interrupt_exit, which frees the same
> structure again.
>
> Drop the kfree() from ipa_interrupt_config(); it should only undo what it
> set up itself. Drop the leftover initialization and cleanup of
> ipa->interrupt now managed in ipa_proble() and ipa_remove() of ipa_main.c.
>
> Fixes: ad1be80d7582 ("net: ipa: introduce ipa_interrupt_init()")
> Signed-off-by: Thomas Fourier <fourier.thomas@xxxxxxxxx>

You should respond to the netdev-bot message asking for a better
explanation of how this was found and whether it was observed or
just found by inspection.

However I think this version does look good.

Reviewed-by: Alex Elder <elder@xxxxxxxxxxxx>

> ---
> v1->v2:
>    - Drop ipa->interrupt initialization and cleanup.
>
>   drivers/net/ipa/ipa_interrupt.c | 12 ++----------
>   1 file changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/ipa/ipa_interrupt.c b/drivers/net/ipa/ipa_interrupt.c
> index 131b9cc94201..766b094eb069 100644
> --- a/drivers/net/ipa/ipa_interrupt.c
> +++ b/drivers/net/ipa/ipa_interrupt.c
> @@ -260,10 +260,8 @@ int ipa_interrupt_config(struct ipa *ipa)
>   	interrupt->enabled = 0;
>   	interrupt->suspend_enabled = bitmap_zalloc(ipa->endpoint_count,
>   						   GFP_KERNEL);
> -	if (!interrupt->suspend_enabled) {
> -		ret = -ENOMEM;
> -		goto err_kfree;
> -	}
> +	if (!interrupt->suspend_enabled)
> +		return -ENOMEM;
>   
>   	/* Disable IPA interrupt types */
>   	reg = ipa_reg(ipa, IPA_IRQ_EN);
> @@ -289,8 +287,6 @@ int ipa_interrupt_config(struct ipa *ipa)
>   		goto err_disable_wakeup;
>   	}
>   
> -	ipa->interrupt = interrupt;
> -
>   	return 0;
>   
>   err_disable_wakeup:
> @@ -299,8 +295,6 @@ int ipa_interrupt_config(struct ipa *ipa)
>   	free_irq(interrupt->irq, interrupt);
>   err_free_bitmap:
>   	bitmap_free(interrupt->suspend_enabled);
> -err_kfree:
> -	kfree(interrupt);
>   
>   	return ret;
>   }
> @@ -311,8 +305,6 @@ void ipa_interrupt_deconfig(struct ipa *ipa)
>   	struct ipa_interrupt *interrupt = ipa->interrupt;
>   	struct device *dev = ipa->dev;
>   
> -	ipa->interrupt = NULL;
> -
>   	dev_pm_clear_wake_irq(dev);
>   	(void)device_init_wakeup(dev, false);
>   	free_irq(interrupt->irq, interrupt);