drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c | 1 + 1 file changed, 1 insertion(+)
In the for loop used to allocate the loc_array and bmap for each port, a
memory leak is possible when the allocation for loc_array succeeds,
but the allocation for bmap fails. This is because when the control flow
goes to the label free_eth_finfo, only the allocations starting from
(i-1)th iteration are freed.
Fix that by freeing the loc_array in the bmap allocation error path.
Fixes: d915c299f1da ("cxgb4: add skeleton for ethtool n-tuple filters")
Signed-off-by: Abdun Nihaal <abdun.nihaal@gmail.com>
---
drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
index 7f3f5afa864f..1546c3db08f0 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
@@ -2270,6 +2270,7 @@ int cxgb4_init_ethtool_filters(struct adapter *adap)
eth_filter->port[i].bmap = bitmap_zalloc(nentries, GFP_KERNEL);
if (!eth_filter->port[i].bmap) {
ret = -ENOMEM;
+ kvfree(eth_filter->port[i].loc_array);
goto free_eth_finfo;
}
}
--
2.47.2…
> +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
> @@ -2270,6 +2270,7 @@ int cxgb4_init_ethtool_filters(struct adapter *adap)
> eth_filter->port[i].bmap = bitmap_zalloc(nentries, GFP_KERNEL);
> if (!eth_filter->port[i].bmap) {
> ret = -ENOMEM;
> + kvfree(eth_filter->port[i].loc_array);
> goto free_eth_finfo;
> }
> }
How do you think about to move the shown error code assignment behind the mentioned label
(so that another bit of duplicate source code could be avoided)?
Regards,
Markus
On Wed, Apr 09, 2025 at 05:47:46PM +0200, Markus Elfring wrote:
> …
> > +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
> > @@ -2270,6 +2270,7 @@ int cxgb4_init_ethtool_filters(struct adapter *adap)
> > eth_filter->port[i].bmap = bitmap_zalloc(nentries, GFP_KERNEL);
> > if (!eth_filter->port[i].bmap) {
> > ret = -ENOMEM;
> > + kvfree(eth_filter->port[i].loc_array);
> > goto free_eth_finfo;
> > }
> > }
>
> How do you think about to move the shown error code assignment behind the mentioned label
> (so that another bit of duplicate source code could be avoided)?
Hi Markus,
If you mean something like the following. Then I agree that it
is both in keeping with the existing error handling in this function
and addresses the problem at hand.
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
index 7f3f5afa864f..df26d3388c00 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
@@ -2270,13 +2270,15 @@ int cxgb4_init_ethtool_filters(struct adapter *adap)
eth_filter->port[i].bmap = bitmap_zalloc(nentries, GFP_KERNEL);
if (!eth_filter->port[i].bmap) {
ret = -ENOMEM;
- goto free_eth_finfo;
+ goto free_eth_finfo_loc_array;
}
}
adap->ethtool_filters = eth_filter;
return 0;
+free_eth_finfo_loc_array:
+ kvfree(eth_filter->port[i].loc_array);
free_eth_finfo:
while (i-- > 0) {
bitmap_free(eth_filter->port[i].bmap);
>
> Regards,
> Markus
>
On Fri, Apr 11, 2025 at 03:57:34PM +0100, Simon Horman wrote:
> On Wed, Apr 09, 2025 at 05:47:46PM +0200, Markus Elfring wrote:
> > …
> > > +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
> > > @@ -2270,6 +2270,7 @@ int cxgb4_init_ethtool_filters(struct adapter *adap)
> > > eth_filter->port[i].bmap = bitmap_zalloc(nentries, GFP_KERNEL);
> > > if (!eth_filter->port[i].bmap) {
> > > ret = -ENOMEM;
> > > + kvfree(eth_filter->port[i].loc_array);
> > > goto free_eth_finfo;
> > > }
> > > }
> >
> > How do you think about to move the shown error code assignment behind the mentioned label
> > (so that another bit of duplicate source code could be avoided)?
>
> Hi Markus,
>
> If you mean something like the following. Then I agree that it
> is both in keeping with the existing error handling in this function
> and addresses the problem at hand.
>
> diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
> index 7f3f5afa864f..df26d3388c00 100644
> --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
> +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
> @@ -2270,13 +2270,15 @@ int cxgb4_init_ethtool_filters(struct adapter *adap)
> eth_filter->port[i].bmap = bitmap_zalloc(nentries, GFP_KERNEL);
> if (!eth_filter->port[i].bmap) {
> ret = -ENOMEM;
> - goto free_eth_finfo;
> + goto free_eth_finfo_loc_array;
> }
> }
>
> adap->ethtool_filters = eth_filter;
> return 0;
>
> +free_eth_finfo_loc_array:
> + kvfree(eth_filter->port[i].loc_array);
> free_eth_finfo:
> while (i-- > 0) {
> bitmap_free(eth_filter->port[i].bmap);
>
I think what Markus meant, was to move the ret = -ENOMEM from both the
allocations in the loop, to after the free_eth_finfo label because it is
-ENOMEM on both goto jumps.
But personally I would prefer having the ret code right after the call
that is failing. Also I would avoid creating new goto labels unless
necessary, because it is easier to see the kvfree in context inside the
loop, than to put it in a separate label.
I just tried to make the most minimal code change to fix the memory leak.
Regards,
Nihaal
On Fri, Apr 11, 2025 at 09:52:29PM +0530, Abdun Nihaal wrote:
> On Fri, Apr 11, 2025 at 03:57:34PM +0100, Simon Horman wrote:
> > On Wed, Apr 09, 2025 at 05:47:46PM +0200, Markus Elfring wrote:
> > > …
> > > > +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
> > > > @@ -2270,6 +2270,7 @@ int cxgb4_init_ethtool_filters(struct adapter *adap)
> > > > eth_filter->port[i].bmap = bitmap_zalloc(nentries, GFP_KERNEL);
> > > > if (!eth_filter->port[i].bmap) {
> > > > ret = -ENOMEM;
> > > > + kvfree(eth_filter->port[i].loc_array);
> > > > goto free_eth_finfo;
> > > > }
> > > > }
> > >
> > > How do you think about to move the shown error code assignment behind the mentioned label
> > > (so that another bit of duplicate source code could be avoided)?
> >
> > Hi Markus,
> >
> > If you mean something like the following. Then I agree that it
> > is both in keeping with the existing error handling in this function
> > and addresses the problem at hand.
> >
> > diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
> > index 7f3f5afa864f..df26d3388c00 100644
> > --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
> > +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
> > @@ -2270,13 +2270,15 @@ int cxgb4_init_ethtool_filters(struct adapter *adap)
> > eth_filter->port[i].bmap = bitmap_zalloc(nentries, GFP_KERNEL);
> > if (!eth_filter->port[i].bmap) {
> > ret = -ENOMEM;
> > - goto free_eth_finfo;
> > + goto free_eth_finfo_loc_array;
> > }
> > }
> >
> > adap->ethtool_filters = eth_filter;
> > return 0;
> >
> > +free_eth_finfo_loc_array:
> > + kvfree(eth_filter->port[i].loc_array);
> > free_eth_finfo:
> > while (i-- > 0) {
> > bitmap_free(eth_filter->port[i].bmap);
> >
>
> I think what Markus meant, was to move the ret = -ENOMEM from both the
> allocations in the loop, to after the free_eth_finfo label because it is
> -ENOMEM on both goto jumps.
>
> But personally I would prefer having the ret code right after the call
> that is failing. Also I would avoid creating new goto labels unless
> necessary, because it is easier to see the kvfree in context inside the
> loop, than to put it in a separate label.
>
> I just tried to make the most minimal code change to fix the memory leak.
Thanks Nihaal,
I agree that your patch is fine as-is for the reasons you describe above.
Reviewed-by: Simon Horman <horms@kernel.org>
On Mon, Apr 14, 2025 at 03:56:22PM +0100, Simon Horman wrote:
> On Fri, Apr 11, 2025 at 09:52:29PM +0530, Abdun Nihaal wrote:
> > On Fri, Apr 11, 2025 at 03:57:34PM +0100, Simon Horman wrote:
> > > On Wed, Apr 09, 2025 at 05:47:46PM +0200, Markus Elfring wrote:
> > > > …
> > > > > +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
> > > > > @@ -2270,6 +2270,7 @@ int cxgb4_init_ethtool_filters(struct adapter *adap)
> > > > > eth_filter->port[i].bmap = bitmap_zalloc(nentries, GFP_KERNEL);
> > > > > if (!eth_filter->port[i].bmap) {
> > > > > ret = -ENOMEM;
> > > > > + kvfree(eth_filter->port[i].loc_array);
> > > > > goto free_eth_finfo;
> > > > > }
> > > > > }
> > > >
> > > > How do you think about to move the shown error code assignment behind the mentioned label
> > > > (so that another bit of duplicate source code could be avoided)?
> > >
> > > Hi Markus,
> > >
> > > If you mean something like the following. Then I agree that it
> > > is both in keeping with the existing error handling in this function
> > > and addresses the problem at hand.
> > >
> > > diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
> > > index 7f3f5afa864f..df26d3388c00 100644
> > > --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
> > > +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
> > > @@ -2270,13 +2270,15 @@ int cxgb4_init_ethtool_filters(struct adapter *adap)
> > > eth_filter->port[i].bmap = bitmap_zalloc(nentries, GFP_KERNEL);
> > > if (!eth_filter->port[i].bmap) {
> > > ret = -ENOMEM;
> > > - goto free_eth_finfo;
> > > + goto free_eth_finfo_loc_array;
> > > }
> > > }
> > >
> > > adap->ethtool_filters = eth_filter;
> > > return 0;
> > >
> > > +free_eth_finfo_loc_array:
> > > + kvfree(eth_filter->port[i].loc_array);
> > > free_eth_finfo:
> > > while (i-- > 0) {
> > > bitmap_free(eth_filter->port[i].bmap);
> > >
> >
> > I think what Markus meant, was to move the ret = -ENOMEM from both the
> > allocations in the loop, to after the free_eth_finfo label because it is
> > -ENOMEM on both goto jumps.
> >
> > But personally I would prefer having the ret code right after the call
> > that is failing. Also I would avoid creating new goto labels unless
> > necessary, because it is easier to see the kvfree in context inside the
> > loop, than to put it in a separate label.
> >
> > I just tried to make the most minimal code change to fix the memory leak.
>
> Thanks Nihaal,
>
> I agree that your patch is fine as-is for the reasons you describe above.
>
> Reviewed-by: Simon Horman <horms@kernel.org>
As the patch was marked as changes requested, presumably due to earlier
discussion in this thread, could you please post a v2. You can include my
tag above. And note under the scissors ("---") that adding the tag was the
only change between v1 and v2.
On Mon, Apr 14, 2025 at 03:59:32PM +0100, Simon Horman wrote:
> As the patch was marked as changes requested, presumably due to earlier
> discussion in this thread, could you please post a v2. You can include my
> tag above. And note under the scissors ("---") that adding the tag was the
> only change between v1 and v2.
Thanks Simon, I'll post a v2.
Regards,
Nihaal
> I think what Markus meant, was to move the ret = -ENOMEM from both the > allocations in the loop, to after the free_eth_finfo label because it is > -ENOMEM on both goto jumps. Exactly, I find this a reasonable source code transformation for another update step. > But personally I would prefer having the ret code right after the call > that is failing. Also I would avoid creating new goto labels unless > necessary, because it is easier to see the kvfree in context inside the > loop, than to put it in a separate label. How will development views evolve further here? > I just tried to make the most minimal code change to fix the memory leak. This approach is generally appropriate. Regards, Markus
© 2016 - 2026 Red Hat, Inc.