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

[Long Li]

> Subject: [EXTERNAL] Re: [net-next,v3,4/6] net: mana: Use GIC functions to
> allocate global EQs
> 
> 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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fnetdev-
> ai.bots.linux.dev%2Fai-
> local.html&data=05%7C02%7Clongli%40microsoft.com%7Cb43e1b10bd564c760
> a4d08de7eb17c82%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C6390
> 87497958656662%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWU
> sIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%
> 7C0%7C%7C%7C&sdata=r63AcJ7Cd3APZSSpl4SPBcGxSdosinxkIurfTp9yZmc%3D&
> reserved=0
> ---
> 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.

Thank you, I'm sending v4 to fix this.

Long