| Message ID | 20231208082316.3384650-1-alexious@zju.edu.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] ethernet: atheros: fix a memleak in atl1e_setup_ring_resources | expand |
>In the error handling of 'offset > adapter->ring_size', the tx_ring- >>tx_buffer allocated by kzalloc should be freed, instead of 'goto >failed' instantly. > >Fixes: a6a5325239c2 ("atl1e: Atheros L1E Gigabit Ethernet driver") >Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn> >--- Reviewed-by: Suman Ghosh <sumang@marvell.com> > >Changelog: > >v2: Setting tx_ring->tx_buffer to NULL after free.
On Fri, 8 Dec 2023 16:23:14 +0800 Zhipeng Lu wrote: > v2: Setting tx_ring->tx_buffer to NULL after free. Having closer look at this driver - it tries to free both on close and remove, so seems like we do indeed have to NULL-out the pointer, sigh. > diff --git a/drivers/net/ethernet/atheros/atl1e/atl1e_main.c b/drivers/net/ethernet/atheros/atl1e/atl1e_main.c > index 5935be190b9e..1bffe77439ac 100644 > --- a/drivers/net/ethernet/atheros/atl1e/atl1e_main.c > +++ b/drivers/net/ethernet/atheros/atl1e/atl1e_main.c > @@ -866,6 +866,8 @@ static int atl1e_setup_ring_resources(struct atl1e_adapter *adapter) > netdev_err(adapter->netdev, "offset(%d) > ring size(%d) !!\n", > offset, adapter->ring_size); > err = -1; > + kfree(tx_ring->tx_buffer); > + tx_ring->tx_buffer = NULL; > goto failed; Please add a new jump target, tho, and move the freeing there. There's a small chance someone will add more code to this function and it will need to copy / paste this unwind.
> On Fri, 8 Dec 2023 16:23:14 +0800 Zhipeng Lu wrote: > > v2: Setting tx_ring->tx_buffer to NULL after free. > > Having closer look at this driver - it tries to free both on close and > remove, so seems like we do indeed have to NULL-out the pointer, sigh. > > > diff --git a/drivers/net/ethernet/atheros/atl1e/atl1e_main.c b/drivers/net/ethernet/atheros/atl1e/atl1e_main.c > > index 5935be190b9e..1bffe77439ac 100644 > > --- a/drivers/net/ethernet/atheros/atl1e/atl1e_main.c > > +++ b/drivers/net/ethernet/atheros/atl1e/atl1e_main.c > > @@ -866,6 +866,8 @@ static int atl1e_setup_ring_resources(struct atl1e_adapter *adapter) > > netdev_err(adapter->netdev, "offset(%d) > ring size(%d) !!\n", > > offset, adapter->ring_size); > > err = -1; > > + kfree(tx_ring->tx_buffer); > > + tx_ring->tx_buffer = NULL; > > goto failed; > > Please add a new jump target, tho, and move the freeing there. > There's a small chance someone will add more code to this function > and it will need to copy / paste this unwind. > -- Thank you for your advice, I've send a v3 version of this patch.
diff --git a/drivers/net/ethernet/atheros/atl1e/atl1e_main.c b/drivers/net/ethernet/atheros/atl1e/atl1e_main.c index 5935be190b9e..1bffe77439ac 100644 --- a/drivers/net/ethernet/atheros/atl1e/atl1e_main.c +++ b/drivers/net/ethernet/atheros/atl1e/atl1e_main.c @@ -866,6 +866,8 @@ static int atl1e_setup_ring_resources(struct atl1e_adapter *adapter) netdev_err(adapter->netdev, "offset(%d) > ring size(%d) !!\n", offset, adapter->ring_size); err = -1; + kfree(tx_ring->tx_buffer); + tx_ring->tx_buffer = NULL; goto failed; }
In the error handling of 'offset > adapter->ring_size', the tx_ring->tx_buffer allocated by kzalloc should be freed, instead of 'goto failed' instantly. Fixes: a6a5325239c2 ("atl1e: Atheros L1E Gigabit Ethernet driver") Signed-off-by: Zhipeng Lu <alexious@zju.edu.cn> --- Changelog: v2: Setting tx_ring->tx_buffer to NULL after free. --- drivers/net/ethernet/atheros/atl1e/atl1e_main.c | 2 ++ 1 file changed, 2 insertions(+)