| Message ID | 20230614073241.6382-1-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: ethernet: stmicro: stmmac: fix possible memory leak in __stmmac_open | expand |
On Wed, Jun 14, 2023 at 04:55:10PM +0200, Simon Horman wrote: > On Wed, Jun 14, 2023 at 09:32:41AM +0200, Christian Marangi wrote: > > Fix a possible memory leak in __stmmac_open when stmmac_init_phy fails. > > It's also needed to free everything allocated by stmmac_setup_dma_desc > > and not just the dma_conf struct. > > > > Correctly call free_dma_desc_resources on the new dma_conf passed to > > __stmmac_open on error. > > > > Reported-by: Jose Abreu <Jose.Abreu@synopsys.com> > > Fixes: ba39b344e924 ("net: ethernet: stmicro: stmmac: generate stmmac dma conf before open") > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > > Cc: stable@vger.kernel.org > > --- > > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > index fa07b0d50b46..0966ab86fde2 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > @@ -3877,10 +3877,10 @@ static int __stmmac_open(struct net_device *dev, > > > > stmmac_hw_teardown(dev); > > init_error: > > - free_dma_desc_resources(priv, &priv->dma_conf); > > phylink_disconnect_phy(priv->phylink); > > init_phy_error: > > pm_runtime_put(priv->device); > > + free_dma_desc_resources(priv, dma_conf); > > Hi Christian, > > Are these resources allocated by the caller? > If so, perhaps it would be clearer if a symmetric approach > was taken and the caller handled freeing them on error. > Yes, they are. Handling in the caller would require some additional delta to this and some duplicated code but if preferred I can implement it. I can provide a v2 shortly if it's ok and you prefer this implementation. > > return ret; > > } > > > > -- > > 2.40.1 > > > >
On Wed, Jun 14, 2023 at 09:32:41AM +0200, Christian Marangi wrote: > Fix a possible memory leak in __stmmac_open when stmmac_init_phy fails. > It's also needed to free everything allocated by stmmac_setup_dma_desc > and not just the dma_conf struct. > > Correctly call free_dma_desc_resources on the new dma_conf passed to > __stmmac_open on error. > > Reported-by: Jose Abreu <Jose.Abreu@synopsys.com> > Fixes: ba39b344e924 ("net: ethernet: stmicro: stmmac: generate stmmac dma conf before open") > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > Cc: stable@vger.kernel.org > --- > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index fa07b0d50b46..0966ab86fde2 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -3877,10 +3877,10 @@ static int __stmmac_open(struct net_device *dev, > > stmmac_hw_teardown(dev); > init_error: > - free_dma_desc_resources(priv, &priv->dma_conf); > phylink_disconnect_phy(priv->phylink); > init_phy_error: > pm_runtime_put(priv->device); > + free_dma_desc_resources(priv, dma_conf); Hi Christian, Are these resources allocated by the caller? If so, perhaps it would be clearer if a symmetric approach was taken and the caller handled freeing them on error. > return ret; > } > > -- > 2.40.1 > >
On Wed, Jun 14, 2023 at 10:18:20AM +0200, Christian Marangi wrote: > On Wed, Jun 14, 2023 at 04:55:10PM +0200, Simon Horman wrote: > > On Wed, Jun 14, 2023 at 09:32:41AM +0200, Christian Marangi wrote: > > > Fix a possible memory leak in __stmmac_open when stmmac_init_phy fails. > > > It's also needed to free everything allocated by stmmac_setup_dma_desc > > > and not just the dma_conf struct. > > > > > > Correctly call free_dma_desc_resources on the new dma_conf passed to > > > __stmmac_open on error. > > > > > > Reported-by: Jose Abreu <Jose.Abreu@synopsys.com> > > > Fixes: ba39b344e924 ("net: ethernet: stmicro: stmmac: generate stmmac dma conf before open") > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > > > Cc: stable@vger.kernel.org > > > --- > > > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > > index fa07b0d50b46..0966ab86fde2 100644 > > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > > @@ -3877,10 +3877,10 @@ static int __stmmac_open(struct net_device *dev, > > > > > > stmmac_hw_teardown(dev); > > > init_error: > > > - free_dma_desc_resources(priv, &priv->dma_conf); > > > phylink_disconnect_phy(priv->phylink); > > > init_phy_error: > > > pm_runtime_put(priv->device); > > > + free_dma_desc_resources(priv, dma_conf); > > > > Hi Christian, > > > > Are these resources allocated by the caller? > > If so, perhaps it would be clearer if a symmetric approach > > was taken and the caller handled freeing them on error. > > > > Yes, they are. Handling in the caller would require some additional > delta to this and some duplicated code but if preferred I can implement > it. I can provide a v2 shortly if it's ok and you prefer this > implementation. Thanks, I think that would be best, unless the new delta is enormous for some reason.
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index fa07b0d50b46..0966ab86fde2 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -3877,10 +3877,10 @@ static int __stmmac_open(struct net_device *dev, stmmac_hw_teardown(dev); init_error: - free_dma_desc_resources(priv, &priv->dma_conf); phylink_disconnect_phy(priv->phylink); init_phy_error: pm_runtime_put(priv->device); + free_dma_desc_resources(priv, dma_conf); return ret; }
Fix a possible memory leak in __stmmac_open when stmmac_init_phy fails. It's also needed to free everything allocated by stmmac_setup_dma_desc and not just the dma_conf struct. Correctly call free_dma_desc_resources on the new dma_conf passed to __stmmac_open on error. Reported-by: Jose Abreu <Jose.Abreu@synopsys.com> Fixes: ba39b344e924 ("net: ethernet: stmicro: stmmac: generate stmmac dma conf before open") Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> Cc: stable@vger.kernel.org --- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)