* [PATCH net-next] ip6_vti: fix memleak on netns dismantle
@ 2024-04-15 12:23 Florian Westphal
2024-04-16 14:03 ` Simon Horman
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Florian Westphal @ 2024-04-15 12:23 UTC (permalink / raw
To: netdev
Cc: Paolo Abeni, David S. Miller, Eric Dumazet, Jakub Kicinski,
Florian Westphal, Breno Leitao
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;
dev->pcpu_stat_type = NETDEV_PCPU_STAT_TSTATS;
dev->type = ARPHRD_TUNNEL6;
--
2.43.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] ip6_vti: fix memleak on netns dismantle
2024-04-15 12:23 [PATCH net-next] ip6_vti: fix memleak on netns dismantle Florian Westphal
@ 2024-04-16 14:03 ` Simon Horman
2024-04-17 0:40 ` patchwork-bot+netdevbpf
2024-04-23 9:46 ` Breno Leitao
2 siblings, 0 replies; 6+ messages in thread
From: Simon Horman @ 2024-04-16 14:03 UTC (permalink / raw
To: Florian Westphal
Cc: netdev, Paolo Abeni, David S. Miller, Eric Dumazet,
Jakub Kicinski, Breno Leitao, Steffen Klassert, Herbert Xu,
David Ahern
+ 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
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] ip6_vti: fix memleak on netns dismantle
2024-04-15 12:23 [PATCH net-next] ip6_vti: fix memleak on netns dismantle Florian Westphal
2024-04-16 14:03 ` Simon Horman
@ 2024-04-17 0:40 ` patchwork-bot+netdevbpf
2024-04-23 9:46 ` Breno Leitao
2 siblings, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-04-17 0:40 UTC (permalink / raw
To: Florian Westphal; +Cc: netdev, pabeni, davem, edumazet, kuba, leitao
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!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] ip6_vti: fix memleak on netns dismantle
2024-04-15 12:23 [PATCH net-next] ip6_vti: fix memleak on netns dismantle Florian Westphal
2024-04-16 14:03 ` Simon Horman
2024-04-17 0:40 ` patchwork-bot+netdevbpf
@ 2024-04-23 9:46 ` Breno Leitao
2024-04-23 19:53 ` Eric Dumazet
2 siblings, 1 reply; 6+ messages in thread
From: Breno Leitao @ 2024-04-23 9:46 UTC (permalink / raw
To: Florian Westphal
Cc: netdev, Paolo Abeni, David S. Miller, Eric Dumazet,
Jakub Kicinski
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!
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] ip6_vti: fix memleak on netns dismantle
2024-04-23 9:46 ` Breno Leitao
@ 2024-04-23 19:53 ` Eric Dumazet
2024-04-24 15:11 ` Breno Leitao
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2024-04-23 19:53 UTC (permalink / raw
To: Breno Leitao
Cc: Florian Westphal, netdev, Paolo Abeni, David S. Miller,
Jakub Kicinski
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] ip6_vti: fix memleak on netns dismantle
2024-04-23 19:53 ` Eric Dumazet
@ 2024-04-24 15:11 ` Breno Leitao
0 siblings, 0 replies; 6+ messages in thread
From: Breno Leitao @ 2024-04-24 15:11 UTC (permalink / raw
To: Eric Dumazet
Cc: Florian Westphal, netdev, Paolo Abeni, David S. Miller,
Jakub Kicinski
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!
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-04-24 15:11 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-15 12:23 [PATCH net-next] ip6_vti: fix memleak on netns dismantle Florian Westphal
2024-04-16 14:03 ` Simon Horman
2024-04-17 0:40 ` patchwork-bot+netdevbpf
2024-04-23 9:46 ` Breno Leitao
2024-04-23 19:53 ` Eric Dumazet
2024-04-24 15:11 ` Breno Leitao
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).