* [PATCH] net: mana: Fix memory leak in mana_gd_setup_irqs
@ 2024-11-28 19:43 Maxim Levitsky
2024-11-28 21:49 ` Michael Kelley
0 siblings, 1 reply; 6+ messages in thread
From: Maxim Levitsky @ 2024-11-28 19:43 UTC (permalink / raw)
To: kvm
Cc: Shradha Gupta, Wei Liu, Haiyang Zhang, Konstantin Taranov,
Yury Norov, K. Y. Srinivasan, Eric Dumazet, linux-hyperv, Long Li,
Jakub Kicinski, Maxim Levitsky, David S. Miller, Leon Romanovsky,
netdev, Paolo Abeni, Andrew Lunn, Souradeep Chakrabarti,
Dexuan Cui, linux-kernel
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:
--
2.26.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* RE: [PATCH] net: mana: Fix memory leak in mana_gd_setup_irqs
2024-11-28 19:43 [PATCH] net: mana: Fix memory leak in mana_gd_setup_irqs Maxim Levitsky
@ 2024-11-28 21:49 ` Michael Kelley
2024-11-29 2:13 ` Yury Norov
2024-12-03 16:21 ` Simon Horman
0 siblings, 2 replies; 6+ messages in thread
From: Michael Kelley @ 2024-11-28 21:49 UTC (permalink / raw)
To: Maxim Levitsky, kvm@vger.kernel.org
Cc: Shradha Gupta, Wei Liu, Haiyang Zhang, Konstantin Taranov,
Yury Norov, K. Y. Srinivasan, Eric Dumazet,
linux-hyperv@vger.kernel.org, Long Li, Jakub Kicinski,
David S. Miller, Leon Romanovsky, netdev@vger.kernel.org,
Paolo Abeni, Andrew Lunn, Souradeep Chakrabarti, Dexuan Cui,
linux-kernel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] net: mana: Fix memory leak in mana_gd_setup_irqs
2024-11-28 21:49 ` Michael Kelley
@ 2024-11-29 2:13 ` Yury Norov
2024-11-30 18:37 ` Jakub Kicinski
2024-12-03 16:21 ` Simon Horman
1 sibling, 1 reply; 6+ messages in thread
From: Yury Norov @ 2024-11-29 2:13 UTC (permalink / raw)
To: Michael Kelley
Cc: Maxim Levitsky, kvm@vger.kernel.org, Shradha Gupta, Wei Liu,
Haiyang Zhang, Konstantin Taranov, K. Y. Srinivasan, Eric Dumazet,
linux-hyperv@vger.kernel.org, Long Li, Jakub Kicinski,
David S. Miller, Leon Romanovsky, netdev@vger.kernel.org,
Paolo Abeni, Andrew Lunn, Souradeep Chakrabarti, Dexuan Cui,
linux-kernel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] net: mana: Fix memory leak in mana_gd_setup_irqs
2024-11-29 2:13 ` Yury Norov
@ 2024-11-30 18:37 ` Jakub Kicinski
0 siblings, 0 replies; 6+ messages in thread
From: Jakub Kicinski @ 2024-11-30 18:37 UTC (permalink / raw)
To: Yury Norov
Cc: Michael Kelley, Maxim Levitsky, kvm@vger.kernel.org,
Shradha Gupta, Wei Liu, Haiyang Zhang, Konstantin Taranov,
K. Y. Srinivasan, Eric Dumazet, linux-hyperv@vger.kernel.org,
Long Li, David S. Miller, Leon Romanovsky, netdev@vger.kernel.org,
Paolo Abeni, Andrew Lunn, Souradeep Chakrabarti, Dexuan Cui,
linux-kernel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: mana: Fix memory leak in mana_gd_setup_irqs
2024-11-28 21:49 ` Michael Kelley
2024-11-29 2:13 ` Yury Norov
@ 2024-12-03 16:21 ` Simon Horman
2024-12-04 0:25 ` Maxim Levitsky
1 sibling, 1 reply; 6+ messages in thread
From: Simon Horman @ 2024-12-03 16:21 UTC (permalink / raw)
To: Michael Kelley
Cc: Maxim Levitsky, kvm@vger.kernel.org, Shradha Gupta, Wei Liu,
Haiyang Zhang, Konstantin Taranov, Yury Norov, K. Y. Srinivasan,
Eric Dumazet, linux-hyperv@vger.kernel.org, Long Li,
Jakub Kicinski, David S. Miller, Leon Romanovsky,
netdev@vger.kernel.org, Paolo Abeni, Andrew Lunn,
Souradeep Chakrabarti, Dexuan Cui, linux-kernel@vger.kernel.org
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.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] net: mana: Fix memory leak in mana_gd_setup_irqs
2024-12-03 16:21 ` Simon Horman
@ 2024-12-04 0:25 ` Maxim Levitsky
0 siblings, 0 replies; 6+ messages in thread
From: Maxim Levitsky @ 2024-12-04 0:25 UTC (permalink / raw)
To: Simon Horman, Michael Kelley
Cc: kvm@vger.kernel.org, Shradha Gupta, Wei Liu, Haiyang Zhang,
Konstantin Taranov, Yury Norov, K. Y. Srinivasan, Eric Dumazet,
linux-hyperv@vger.kernel.org, Long Li, Jakub Kicinski,
David S. Miller, Leon Romanovsky, netdev@vger.kernel.org,
Paolo Abeni, Andrew Lunn, Souradeep Chakrabarti, Dexuan Cui,
linux-kernel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-12-04 0:25 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-28 19:43 [PATCH] net: mana: Fix memory leak in mana_gd_setup_irqs Maxim Levitsky
2024-11-28 21:49 ` Michael Kelley
2024-11-29 2:13 ` Yury Norov
2024-11-30 18:37 ` Jakub Kicinski
2024-12-03 16:21 ` Simon Horman
2024-12-04 0:25 ` Maxim Levitsky
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).