| Message ID | 20230208215332.1336500-1-jan@3e8.eu |
|---|---|
| State | Accepted |
| Delegated to: | Sander Vanheule |
| Headers | show |
| Series | realtek: fix memory leak in netevent handler | expand |
Hi Jan, On Wed, 2023-02-08 at 22:53 +0100, Jan Hoffmann wrote: > The net_event_work struct is allocated, but only freed in a single case. > Move the allocation to the branch where it is actually needed, and free > it after the work has been done. > > Fixes: 03e1d93e0779 ("realtek: add driver support for routing offload") > Signed-off-by: Jan Hoffmann <jan@3e8.eu> > --- > I noticed this issue due to increasing memory usage on a HPE 1920-16G. > While I haven't done any long-term testing with this patch yet, using > kmemleak on a HPE 1920-8G no longer shows any leaks in the realtek > drivers. Thanks for noticing and taking care of fixing it! > > .../files-5.10/drivers/net/dsa/rtl83xx/common.c | 17 +++++++++-------- > .../files-5.15/drivers/net/dsa/rtl83xx/common.c | 17 +++++++++-------- > 2 files changed, 18 insertions(+), 16 deletions(-) > > diff --git a/target/linux/realtek/files-5.10/drivers/net/dsa/rtl83xx/common.c > b/target/linux/realtek/files-5.10/drivers/net/dsa/rtl83xx/common.c > index 15e6ed092696..d2d677230010 100644 > --- a/target/linux/realtek/files-5.10/drivers/net/dsa/rtl83xx/common.c > +++ b/target/linux/realtek/files-5.10/drivers/net/dsa/rtl83xx/common.c > @@ -1286,6 +1286,8 @@ static void rtl83xx_net_event_work_do(struct work_struct *work) > struct rtl838x_switch_priv *priv = net_work->priv; > > rtl83xx_l3_nexthop_update(priv, net_work->gw_addr, net_work->mac); > + > + kfree(net_work); > } > > static int rtl83xx_netevent_event(struct notifier_block *this, > @@ -1299,13 +1301,6 @@ static int rtl83xx_netevent_event(struct notifier_block *this, > > priv = container_of(this, struct rtl838x_switch_priv, ne_nb); > > - net_work = kzalloc(sizeof(*net_work), GFP_ATOMIC); > - if (!net_work) > - return NOTIFY_BAD; > - > - INIT_WORK(&net_work->work, rtl83xx_net_event_work_do); > - net_work->priv = priv; > - > switch (event) { > case NETEVENT_NEIGH_UPDATE: > if (n->tbl != &arp_tbl) > @@ -1314,10 +1309,16 @@ static int rtl83xx_netevent_event(struct notifier_block *this, > port = rtl83xx_port_dev_lower_find(dev, priv); > if (port < 0 || !(n->nud_state & NUD_VALID)) { > pr_debug("%s: Neigbour invalid, not updating\n", __func__); > - kfree(net_work); > return NOTIFY_DONE; > } > > + net_work = kzalloc(sizeof(*net_work), GFP_ATOMIC); > + if (!net_work) > + return NOTIFY_BAD; > + > + INIT_WORK(&net_work->work, rtl83xx_net_event_work_do); > + net_work->priv = priv; > + > net_work->mac = ether_addr_to_u64(n->ha); > net_work->gw_addr = *(__be32 *) n->primary_key; rtl83xx_netevent_event() has a variable err which isn't ever assigned, so I think there's a bit of dead code at the end. Otherwise I think this change is good, and err could be removed in another patch. Best, Sander
diff --git a/target/linux/realtek/files-5.10/drivers/net/dsa/rtl83xx/common.c b/target/linux/realtek/files-5.10/drivers/net/dsa/rtl83xx/common.c index 15e6ed092696..d2d677230010 100644 --- a/target/linux/realtek/files-5.10/drivers/net/dsa/rtl83xx/common.c +++ b/target/linux/realtek/files-5.10/drivers/net/dsa/rtl83xx/common.c @@ -1286,6 +1286,8 @@ static void rtl83xx_net_event_work_do(struct work_struct *work) struct rtl838x_switch_priv *priv = net_work->priv; rtl83xx_l3_nexthop_update(priv, net_work->gw_addr, net_work->mac); + + kfree(net_work); } static int rtl83xx_netevent_event(struct notifier_block *this, @@ -1299,13 +1301,6 @@ static int rtl83xx_netevent_event(struct notifier_block *this, priv = container_of(this, struct rtl838x_switch_priv, ne_nb); - net_work = kzalloc(sizeof(*net_work), GFP_ATOMIC); - if (!net_work) - return NOTIFY_BAD; - - INIT_WORK(&net_work->work, rtl83xx_net_event_work_do); - net_work->priv = priv; - switch (event) { case NETEVENT_NEIGH_UPDATE: if (n->tbl != &arp_tbl) @@ -1314,10 +1309,16 @@ static int rtl83xx_netevent_event(struct notifier_block *this, port = rtl83xx_port_dev_lower_find(dev, priv); if (port < 0 || !(n->nud_state & NUD_VALID)) { pr_debug("%s: Neigbour invalid, not updating\n", __func__); - kfree(net_work); return NOTIFY_DONE; } + net_work = kzalloc(sizeof(*net_work), GFP_ATOMIC); + if (!net_work) + return NOTIFY_BAD; + + INIT_WORK(&net_work->work, rtl83xx_net_event_work_do); + net_work->priv = priv; + net_work->mac = ether_addr_to_u64(n->ha); net_work->gw_addr = *(__be32 *) n->primary_key; diff --git a/target/linux/realtek/files-5.15/drivers/net/dsa/rtl83xx/common.c b/target/linux/realtek/files-5.15/drivers/net/dsa/rtl83xx/common.c index 1fa92ae220e0..3216d7eb835f 100644 --- a/target/linux/realtek/files-5.15/drivers/net/dsa/rtl83xx/common.c +++ b/target/linux/realtek/files-5.15/drivers/net/dsa/rtl83xx/common.c @@ -1282,6 +1282,8 @@ static void rtl83xx_net_event_work_do(struct work_struct *work) struct rtl838x_switch_priv *priv = net_work->priv; rtl83xx_l3_nexthop_update(priv, net_work->gw_addr, net_work->mac); + + kfree(net_work); } static int rtl83xx_netevent_event(struct notifier_block *this, @@ -1295,13 +1297,6 @@ static int rtl83xx_netevent_event(struct notifier_block *this, priv = container_of(this, struct rtl838x_switch_priv, ne_nb); - net_work = kzalloc(sizeof(*net_work), GFP_ATOMIC); - if (!net_work) - return NOTIFY_BAD; - - INIT_WORK(&net_work->work, rtl83xx_net_event_work_do); - net_work->priv = priv; - switch (event) { case NETEVENT_NEIGH_UPDATE: if (n->tbl != &arp_tbl) @@ -1310,10 +1305,16 @@ static int rtl83xx_netevent_event(struct notifier_block *this, port = rtl83xx_port_dev_lower_find(dev, priv); if (port < 0 || !(n->nud_state & NUD_VALID)) { pr_debug("%s: Neigbour invalid, not updating\n", __func__); - kfree(net_work); return NOTIFY_DONE; } + net_work = kzalloc(sizeof(*net_work), GFP_ATOMIC); + if (!net_work) + return NOTIFY_BAD; + + INIT_WORK(&net_work->work, rtl83xx_net_event_work_do); + net_work->priv = priv; + net_work->mac = ether_addr_to_u64(n->ha); net_work->gw_addr = *(__be32 *) n->primary_key;
The net_event_work struct is allocated, but only freed in a single case. Move the allocation to the branch where it is actually needed, and free it after the work has been done. Fixes: 03e1d93e0779 ("realtek: add driver support for routing offload") Signed-off-by: Jan Hoffmann <jan@3e8.eu> --- I noticed this issue due to increasing memory usage on a HPE 1920-16G. While I haven't done any long-term testing with this patch yet, using kmemleak on a HPE 1920-8G no longer shows any leaks in the realtek drivers. .../files-5.10/drivers/net/dsa/rtl83xx/common.c | 17 +++++++++-------- .../files-5.15/drivers/net/dsa/rtl83xx/common.c | 17 +++++++++-------- 2 files changed, 18 insertions(+), 16 deletions(-)