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

[Alex Elder]

On 10/28/26 5:23 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.
>
> Fixes: ad1be80d7582 ("net: ipa: introduce ipa_interrupt_init()")
> Signed-off-by: Thomas Fourier <fourier.thomas@xxxxxxxxx>

This is a good catch, something that really should have been caught
when that commit was reviewed (even by me...).

There is another problem I now spotted, and I'd like to have it
corrected when you submit version 2, because it's really related
to the original bug.

At the end of ipa_interrupt_config(), we (still) see this line:

        ipa->interrupt = interrupt;

That now happens in ipa_probe() (which calls ipa_interrupt_init()),
so shouldn't be repeated in ipa_interrupt_config().  Similarly,
ipa_interrupt_deconfig() does this:

        ipa->interrupt = NULL;

That's meant to be the inverse of what was being done in the
config function, but it should no longer be done there.

(There is no corresponding ipa->interrupt = NULL assignment
in ipa_interrupt_exit(), nor in ipa_remove().  There is no
real need, because the ipa structure will never be referenced
again after that.)

Thanks.

					-Alex


> ---
>   drivers/net/ipa/ipa_interrupt.c | 8 ++------
>   1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ipa/ipa_interrupt.c b/drivers/net/ipa/ipa_interrupt.c
> index 131b9cc94201..17cefd6a0b18 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);
> @@ -299,8 +297,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;
>   }