Re: [net-next,v3,4/6] net: mana: Use GIC functions to allocate global EQs

[Paolo Abeni <pabeni@redhat.com>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: mana: Use GIC functions to allocate global EQs

Replace the GDMA global interrupt setup code with the new GIC allocation
and release functions for managing interrupt contexts.

> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index c43fd8089e77..bdc9dc437fb7 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c

[ ... ]

> @@ -2052,26 +1998,17 @@ static int mana_gd_setup_remaining_irqs(struct pci_dev *pdev)
>  static void mana_gd_remove_irqs(struct pci_dev *pdev)
>  {
>  	struct gdma_context *gc = pci_get_drvdata(pdev);
> -	struct gdma_irq_context *gic;
>  	int irq, i;
>
>  	if (gc->max_num_msix < 1)
>  		return;
>
> -	for (i = 0; i < gc->max_num_msix; i++) {
> -		irq = pci_irq_vector(pdev, i);
> -		if (irq < 0)
> -			continue;
> -
> -		gic = xa_load(&gc->irq_contexts, i);
> -		if (WARN_ON(!gic))
> -			continue;
> -
> +	for (i = 0; i < (gc->msi_sharing ? gc->max_num_msix : 1); i++) {
                                                               ^^^
Does this loop condition leak GIC references when msi_sharing is false?

When msi_sharing is false, the loop only releases the GIC at index 0.
However, mana_gd_setup_dyn_irqs allocates GICs at indices 1 through nvec:

drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_setup_dyn_irqs() {
    for (i = 1; i <= nvec; i++) {
        gic = mana_gd_get_gic(gc, false, &i);
        ...
    }
}

Each mana_gd_get_gic call initializes the GIC with refcount=1. These GICs
at indices 1..nvec are never released because mana_gd_remove_irqs only
processes index 0 when msi_sharing is false.

The old code iterated over all gc->max_num_msix entries unconditionally:

    for (i = 0; i < gc->max_num_msix; i++) {
        ...
        gic = xa_load(&gc->irq_contexts, i);
        ...
        free_irq(irq, gic);
        xa_erase(&gc->irq_contexts, i);
        kfree(gic);
    }

This ensured every allocated GIC was freed. The new code with the
conditional loop breaks this, resulting in:

1. Leaked IRQ handlers - request_irq without matching free_irq for indices
   1..nvec
2. Leaked gdma_irq_context memory - the GIC structures are never freed
3. Leaked xarray entries - the entries remain in gc->irq_contexts

Additionally, pci_free_irq_vectors is called immediately after the loop,
which attempts to tear down MSI-X vectors while IRQ handlers at indices
1..nvec are still registered. Can this trigger warnings or use-after-free
if an interrupt fires during teardown?

The next commit in the series (4475f13983ab "net: mana: Allocate interrupt
context for each EQ when creating vPort") adds per-EQ get/put of GICs,
but the EQ teardown only drops the EQ's reference (refcount goes from 2 to
1). The base reference from mana_gd_setup_dyn_irqs remains unreleased.

>  		/* Need to clear the hint before free_irq */
> +		irq = pci_irq_vector(pdev, i);
>  		irq_update_affinity_hint(irq, NULL);
> -		free_irq(irq, gic);
> -		xa_erase(&gc->irq_contexts, i);
> -		kfree(gic);
> +
> +		mana_gd_put_gic(gc, false, i);
>  	}
>
>  	pci_free_irq_vectors(pdev);