| Message ID | 20240415122346.26503-1-fw@strlen.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 86600ea11dc18ecd10110c13148d0fb04a80ceea |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] ip6_vti: fix memleak on netns dismantle | expand |
+ Steffen Klassert, Herbert Xu, David Ahern On Mon, Apr 15, 2024 at 02:23:44PM +0200, Florian Westphal wrote: > kmemleak reports net_device resources are no longer released, restore > needs_free_netdev toggle. Sample backtrace: > > unreferenced object 0xffff88810874f000 (size 4096): [..] > [<00000000a2b8af8b>] __kmalloc_node+0x209/0x290 > [<0000000040b0a1a9>] alloc_netdev_mqs+0x58/0x470 > [<00000000b4be1e78>] vti6_init_net+0x94/0x230 > [<000000008830c1ea>] ops_init+0x32/0xc0 > [<000000006a26fa8f>] setup_net+0x134/0x2e0 > [..] > > Cc: Breno Leitao <leitao@debian.org> > Fixes: a9b2d55a8f1e ("ip6_vti: Do not use custom stat allocator") > Signed-off-by: Florian Westphal <fw@strlen.de> Reviewed-by: Simon Horman <horms@kernel.org> > --- > net/ipv6/ip6_vti.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/net/ipv6/ip6_vti.c b/net/ipv6/ip6_vti.c > index 4d68a0777b0c..78344cf3867e 100644 > --- a/net/ipv6/ip6_vti.c > +++ b/net/ipv6/ip6_vti.c > @@ -901,6 +901,7 @@ static void vti6_dev_setup(struct net_device *dev) > { > dev->netdev_ops = &vti6_netdev_ops; > dev->header_ops = &ip_tunnel_header_ops; > + dev->needs_free_netdev = true; > > dev->pcpu_stat_type = NETDEV_PCPU_STAT_TSTATS; > dev->type = ARPHRD_TUNNEL6; > -- > 2.43.2 > >
Hello: This patch was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Mon, 15 Apr 2024 14:23:44 +0200 you wrote: > kmemleak reports net_device resources are no longer released, restore > needs_free_netdev toggle. Sample backtrace: > > unreferenced object 0xffff88810874f000 (size 4096): [..] > [<00000000a2b8af8b>] __kmalloc_node+0x209/0x290 > [<0000000040b0a1a9>] alloc_netdev_mqs+0x58/0x470 > [<00000000b4be1e78>] vti6_init_net+0x94/0x230 > [<000000008830c1ea>] ops_init+0x32/0xc0 > [<000000006a26fa8f>] setup_net+0x134/0x2e0 > [..] > > [...] Here is the summary with links: - [net-next] ip6_vti: fix memleak on netns dismantle https://git.kernel.org/netdev/net-next/c/86600ea11dc1 You are awesome, thank you!
Hello Florian, On Mon, Apr 15, 2024 at 02:23:44PM +0200, Florian Westphal wrote: > kmemleak reports net_device resources are no longer released, restore > needs_free_netdev toggle. Sample backtrace: > > unreferenced object 0xffff88810874f000 (size 4096): [..] > [<00000000a2b8af8b>] __kmalloc_node+0x209/0x290 > [<0000000040b0a1a9>] alloc_netdev_mqs+0x58/0x470 > [<00000000b4be1e78>] vti6_init_net+0x94/0x230 > [<000000008830c1ea>] ops_init+0x32/0xc0 > [<000000006a26fa8f>] setup_net+0x134/0x2e0 > [..] > > Cc: Breno Leitao <leitao@debian.org> > Fixes: a9b2d55a8f1e ("ip6_vti: Do not use custom stat allocator") > Signed-off-by: Florian Westphal <fw@strlen.de> > --- > net/ipv6/ip6_vti.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/net/ipv6/ip6_vti.c b/net/ipv6/ip6_vti.c > index 4d68a0777b0c..78344cf3867e 100644 > --- a/net/ipv6/ip6_vti.c > +++ b/net/ipv6/ip6_vti.c > @@ -901,6 +901,7 @@ static void vti6_dev_setup(struct net_device *dev) > { > dev->netdev_ops = &vti6_netdev_ops; > dev->header_ops = &ip_tunnel_header_ops; > + dev->needs_free_netdev = true; Thanks for the fix! Could you help me to understand how needs_free_netdev will trigger the free()here? I _though_ that any device that is being unregistered would have the stats freed. This is the flow I am reading: 1) When the device is unregistered, then it is marked as todo: unregister_netdevice_many_notify() { list_for_each_entry(dev, head, unreg_list) { net_set_todo(dev); } } 2) Then, "run_todo" will run later, and it does: netdev_run_todo() { list_for_each_entry_safe(dev, tmp, &list, todo_list) { if (unlikely(dev->reg_state != NETREG_UNREGISTERING)) { netdev_WARN(dev, "run_todo but not unregistering\n"); list_del(&dev->todo_list); continue; } while (!list_empty(&list)) { netdev_do_free_pcpu_stats(dev); } } Thank you!
On Tue, Apr 23, 2024 at 11:46 AM Breno Leitao <leitao@debian.org> wrote: > > Hello Florian, > > On Mon, Apr 15, 2024 at 02:23:44PM +0200, Florian Westphal wrote: > > kmemleak reports net_device resources are no longer released, restore > > needs_free_netdev toggle. Sample backtrace: > > > > unreferenced object 0xffff88810874f000 (size 4096): [..] > > [<00000000a2b8af8b>] __kmalloc_node+0x209/0x290 > > [<0000000040b0a1a9>] alloc_netdev_mqs+0x58/0x470 > > [<00000000b4be1e78>] vti6_init_net+0x94/0x230 > > [<000000008830c1ea>] ops_init+0x32/0xc0 > > [<000000006a26fa8f>] setup_net+0x134/0x2e0 > > [..] > > > > Cc: Breno Leitao <leitao@debian.org> > > Fixes: a9b2d55a8f1e ("ip6_vti: Do not use custom stat allocator") > > Signed-off-by: Florian Westphal <fw@strlen.de> > > --- > > net/ipv6/ip6_vti.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/net/ipv6/ip6_vti.c b/net/ipv6/ip6_vti.c > > index 4d68a0777b0c..78344cf3867e 100644 > > --- a/net/ipv6/ip6_vti.c > > +++ b/net/ipv6/ip6_vti.c > > @@ -901,6 +901,7 @@ static void vti6_dev_setup(struct net_device *dev) > > { > > dev->netdev_ops = &vti6_netdev_ops; > > dev->header_ops = &ip_tunnel_header_ops; > > + dev->needs_free_netdev = true; > > Thanks for the fix! > > Could you help me to understand how needs_free_netdev will trigger the > free()here? > > I _though_ that any device that is being unregistered would have the stats > freed. > > This is the flow I am reading: > > 1) When the device is unregistered, then it is marked as todo: > > unregister_netdevice_many_notify() { > list_for_each_entry(dev, head, unreg_list) { > net_set_todo(dev); > } > } > > 2) Then, "run_todo" will run later, and it does: > netdev_run_todo() { > list_for_each_entry_safe(dev, tmp, &list, todo_list) { > if (unlikely(dev->reg_state != NETREG_UNREGISTERING)) { > netdev_WARN(dev, "run_todo but not unregistering\n"); > list_del(&dev->todo_list); > continue; > } > > while (!list_empty(&list)) { > netdev_do_free_pcpu_stats(dev); The relevant part is missing: if (dev->needs_free_netdev) free_netdev(dev); > } > > } > > Thank you! I do not think Florian patch has anything to do with free_pcpu_stats() Please take a look at git show cf124db566e -- net/ipv6/ip6_vti.c
Hello Eric, On Tue, Apr 23, 2024 at 09:53:45PM +0200, Eric Dumazet wrote: > On Tue, Apr 23, 2024 at 11:46 AM Breno Leitao <leitao@debian.org> wrote: > > > > Hello Florian, > > > > On Mon, Apr 15, 2024 at 02:23:44PM +0200, Florian Westphal wrote: > > > kmemleak reports net_device resources are no longer released, restore > > > needs_free_netdev toggle. Sample backtrace: > > > > > > unreferenced object 0xffff88810874f000 (size 4096): [..] > > > [<00000000a2b8af8b>] __kmalloc_node+0x209/0x290 > > > [<0000000040b0a1a9>] alloc_netdev_mqs+0x58/0x470 > > > [<00000000b4be1e78>] vti6_init_net+0x94/0x230 > > > [<000000008830c1ea>] ops_init+0x32/0xc0 > > > [<000000006a26fa8f>] setup_net+0x134/0x2e0 > > > [..] > > > > > > Cc: Breno Leitao <leitao@debian.org> > > > Fixes: a9b2d55a8f1e ("ip6_vti: Do not use custom stat allocator") > > > Signed-off-by: Florian Westphal <fw@strlen.de> > > > --- > > > net/ipv6/ip6_vti.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/net/ipv6/ip6_vti.c b/net/ipv6/ip6_vti.c > > > index 4d68a0777b0c..78344cf3867e 100644 > > > --- a/net/ipv6/ip6_vti.c > > > +++ b/net/ipv6/ip6_vti.c > > > @@ -901,6 +901,7 @@ static void vti6_dev_setup(struct net_device *dev) > > > { > > > dev->netdev_ops = &vti6_netdev_ops; > > > dev->header_ops = &ip_tunnel_header_ops; > > > + dev->needs_free_netdev = true; > > > > Thanks for the fix! > > > > Could you help me to understand how needs_free_netdev will trigger the > > free()here? > > > > I _though_ that any device that is being unregistered would have the stats > > freed. > > > > This is the flow I am reading: > > > > 1) When the device is unregistered, then it is marked as todo: > > > > unregister_netdevice_many_notify() { > > list_for_each_entry(dev, head, unreg_list) { > > net_set_todo(dev); > > } > > } > > > > 2) Then, "run_todo" will run later, and it does: > > netdev_run_todo() { > > list_for_each_entry_safe(dev, tmp, &list, todo_list) { > > if (unlikely(dev->reg_state != NETREG_UNREGISTERING)) { > > netdev_WARN(dev, "run_todo but not unregistering\n"); > > list_del(&dev->todo_list); > > continue; > > } > > > > while (!list_empty(&list)) { > > netdev_do_free_pcpu_stats(dev); > > The relevant part is missing: > > if (dev->needs_free_netdev) > free_netdev(dev); > > > } > > > > } > > > > Thank you! > > I do not think Florian patch has anything to do with free_pcpu_stats() That makes total sense now. I though the memory that was being leaked was related to pcpu stats, since this is was I was supposedly changing, but, it is not. The regression is not related to the fact that a9b2d55a8f1e ("ip6_vti: Do not use custom stat allocator") removed the following line wrongly: -dev->needs_free_netdev = true; Everything is clear now. Thanks!
diff --git a/net/ipv6/ip6_vti.c b/net/ipv6/ip6_vti.c index 4d68a0777b0c..78344cf3867e 100644 --- a/net/ipv6/ip6_vti.c +++ b/net/ipv6/ip6_vti.c @@ -901,6 +901,7 @@ static void vti6_dev_setup(struct net_device *dev) { dev->netdev_ops = &vti6_netdev_ops; dev->header_ops = &ip_tunnel_header_ops; + dev->needs_free_netdev = true; dev->pcpu_stat_type = NETDEV_PCPU_STAT_TSTATS; dev->type = ARPHRD_TUNNEL6;
kmemleak reports net_device resources are no longer released, restore needs_free_netdev toggle. Sample backtrace: unreferenced object 0xffff88810874f000 (size 4096): [..] [<00000000a2b8af8b>] __kmalloc_node+0x209/0x290 [<0000000040b0a1a9>] alloc_netdev_mqs+0x58/0x470 [<00000000b4be1e78>] vti6_init_net+0x94/0x230 [<000000008830c1ea>] ops_init+0x32/0xc0 [<000000006a26fa8f>] setup_net+0x134/0x2e0 [..] Cc: Breno Leitao <leitao@debian.org> Fixes: a9b2d55a8f1e ("ip6_vti: Do not use custom stat allocator") Signed-off-by: Florian Westphal <fw@strlen.de> --- net/ipv6/ip6_vti.c | 1 + 1 file changed, 1 insertion(+)