> 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
