| Message ID | 20241128194300.87605-1-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: mana: Fix memory leak in mana_gd_setup_irqs | expand |
From: Maxim Levitsky <mlevitsk@redhat.com> Sent: Thursday, November 28, 2024 11:43 AM > > Commit 8afefc361209 ("net: mana: Assigning IRQ affinity on HT cores") > added memory allocation in mana_gd_setup_irqs of 'irqs' but the code > doesn't free this temporary array in the success path. > > This was caught by kmemleak. > > Fixes: 8afefc361209 ("net: mana: Assigning IRQ affinity on HT cores") > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > --- > drivers/net/ethernet/microsoft/mana/gdma_main.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c > b/drivers/net/ethernet/microsoft/mana/gdma_main.c > index e97af7ac2bb2..aba188f9f10f 100644 > --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c > +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c > @@ -1375,6 +1375,7 @@ static int mana_gd_setup_irqs(struct pci_dev *pdev) > gc->max_num_msix = nvec; > gc->num_msix_usable = nvec; > cpus_read_unlock(); > + kfree(irqs); > return 0; > > free_irq: FWIW, there's a related error path leak. If the kcalloc() to populate gc->irq_contexts fails, the irqs array is not freed. Probably could extend this patch to fix that leak as well. Michael
On Thu, Nov 28, 2024 at 09:49:35PM +0000, Michael Kelley wrote: > From: Maxim Levitsky <mlevitsk@redhat.com> Sent: Thursday, November 28, 2024 11:43 AM > > > > Commit 8afefc361209 ("net: mana: Assigning IRQ affinity on HT cores") > > added memory allocation in mana_gd_setup_irqs of 'irqs' but the code > > doesn't free this temporary array in the success path. > > > > This was caught by kmemleak. > > > > Fixes: 8afefc361209 ("net: mana: Assigning IRQ affinity on HT cores") > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > --- > > drivers/net/ethernet/microsoft/mana/gdma_main.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c > > b/drivers/net/ethernet/microsoft/mana/gdma_main.c > > index e97af7ac2bb2..aba188f9f10f 100644 > > --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c > > +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c > > @@ -1375,6 +1375,7 @@ static int mana_gd_setup_irqs(struct pci_dev *pdev) > > gc->max_num_msix = nvec; > > gc->num_msix_usable = nvec; > > cpus_read_unlock(); > > + kfree(irqs); > > return 0; > > > > free_irq: > > FWIW, there's a related error path leak. If the kcalloc() to populate > gc->irq_contexts fails, the irqs array is not freed. Probably could > extend this patch to fix that leak as well. > > Michael That's why we've got a __free() macro in include/linux/cleanup.h
On Thu, 28 Nov 2024 18:13:25 -0800 Yury Norov wrote: > > FWIW, there's a related error path leak. If the kcalloc() to populate > > gc->irq_contexts fails, the irqs array is not freed. Probably could > > extend this patch to fix that leak as well. > > > > Michael > > That's why we've got a __free() macro in include/linux/cleanup.h Quoting documentation: Using device-managed and cleanup.h constructs ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Netdev remains skeptical about promises of all "auto-cleanup" APIs, including even ``devm_`` helpers, historically. They are not the preferred style of implementation, merely an acceptable one. Use of ``guard()`` is discouraged within any function longer than 20 lines, ``scoped_guard()`` is considered more readable. Using normal lock/unlock is still (weakly) preferred. Low level cleanup constructs (such as ``__free()``) can be used when building APIs and helpers, especially scoped iterators. However, direct use of ``__free()`` within networking core and drivers is discouraged. Similar guidance applies to declaring variables mid-function. See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#using-device-managed-and-cleanup-h-constructs
On Thu, Nov 28, 2024 at 09:49:35PM +0000, Michael Kelley wrote: > From: Maxim Levitsky <mlevitsk@redhat.com> Sent: Thursday, November 28, 2024 11:43 AM > > > > Commit 8afefc361209 ("net: mana: Assigning IRQ affinity on HT cores") > > added memory allocation in mana_gd_setup_irqs of 'irqs' but the code > > doesn't free this temporary array in the success path. > > > > This was caught by kmemleak. > > > > Fixes: 8afefc361209 ("net: mana: Assigning IRQ affinity on HT cores") > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > --- > > drivers/net/ethernet/microsoft/mana/gdma_main.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c > > b/drivers/net/ethernet/microsoft/mana/gdma_main.c > > index e97af7ac2bb2..aba188f9f10f 100644 > > --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c > > +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c > > @@ -1375,6 +1375,7 @@ static int mana_gd_setup_irqs(struct pci_dev *pdev) > > gc->max_num_msix = nvec; > > gc->num_msix_usable = nvec; > > cpus_read_unlock(); > > + kfree(irqs); > > return 0; > > > > free_irq: > > FWIW, there's a related error path leak. If the kcalloc() to populate > gc->irq_contexts fails, the irqs array is not freed. Probably could > extend this patch to fix that leak as well. Yes, as that problem also appears to be introduced by the cited commit I agree it would be good to fix them both in one patch.
On Tue, 2024-12-03 at 16:21 +0000, Simon Horman wrote: > On Thu, Nov 28, 2024 at 09:49:35PM +0000, Michael Kelley wrote: > > From: Maxim Levitsky <mlevitsk@redhat.com> Sent: Thursday, November 28, 2024 11:43 AM > > > Commit 8afefc361209 ("net: mana: Assigning IRQ affinity on HT cores") > > > added memory allocation in mana_gd_setup_irqs of 'irqs' but the code > > > doesn't free this temporary array in the success path. > > > > > > This was caught by kmemleak. > > > > > > Fixes: 8afefc361209 ("net: mana: Assigning IRQ affinity on HT cores") > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > > --- > > > drivers/net/ethernet/microsoft/mana/gdma_main.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c > > > b/drivers/net/ethernet/microsoft/mana/gdma_main.c > > > index e97af7ac2bb2..aba188f9f10f 100644 > > > --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c > > > +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c > > > @@ -1375,6 +1375,7 @@ static int mana_gd_setup_irqs(struct pci_dev *pdev) > > > gc->max_num_msix = nvec; > > > gc->num_msix_usable = nvec; > > > cpus_read_unlock(); > > > + kfree(irqs); > > > return 0; > > > > > > free_irq: > > > > FWIW, there's a related error path leak. If the kcalloc() to populate > > gc->irq_contexts fails, the irqs array is not freed. Probably could > > extend this patch to fix that leak as well. > > Yes, as that problem also appears to be introduced by the cited commit > I agree it would be good to fix them both in one patch. > I'll send a v2 tomorrow. Thanks for the review! Best regards, Maxim Levitsky
diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c index e97af7ac2bb2..aba188f9f10f 100644 --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c @@ -1375,6 +1375,7 @@ static int mana_gd_setup_irqs(struct pci_dev *pdev) gc->max_num_msix = nvec; gc->num_msix_usable = nvec; cpus_read_unlock(); + kfree(irqs); return 0; free_irq:
Commit 8afefc361209 ("net: mana: Assigning IRQ affinity on HT cores") added memory allocation in mana_gd_setup_irqs of 'irqs' but the code doesn't free this temporary array in the success path. This was caught by kmemleak. Fixes: 8afefc361209 ("net: mana: Assigning IRQ affinity on HT cores") Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- drivers/net/ethernet/microsoft/mana/gdma_main.c | 1 + 1 file changed, 1 insertion(+)