During namespace cleanup, if ‘dst’ subsystem is holding
a reference to the loopback interface in the namespace, it does not get
released. This is because in the case where the net_device held by 'dst'
is same as the namespace's loopback net_device, current code first
does a ’dev_hold’ on the same device followed by a ‘dev_put’ on the
same device resulting in a no-op.
This change fixes this leak by assigning the initial namespace’s loopback
device to the ‘dst’’ before releasing the reference to the network device
being unregistered.
Additional reference: https://github.com/docker/docker/issues/5618
Signed-off-by: Jojy G Varghese <redacted>
---
net/core/dst.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
(Updating the patch for the case when a non-loopback dev could be unregistered)
Currently during namespace cleanup, if ‘dst’ subsystem is holding
a reference to the interface in the namespace, it does not get released.
Current code first does a ’dev_hold’ on the same device followed by a
‘dev_put’ on the same device resulting in a no-op.
This change fixes this leak by assigning the initial namespace’s loopback
device to the ‘dst’ before releasing the reference to the network
device being released.
Additional reference: https://github.com/docker/docker/issues/5618
Signed-off-by: Jojy G Varghese <redacted>
---
net/core/dst.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
During namespace cleanup, if ‘dst’ subsystem is holding
a reference to the loopback interface in the namespace, it does not get
released. This is because in the case where the net_device held by 'dst'
is same as the namespace's loopback net_device, current code first
does a ’dev_hold’ on the same device followed by a ‘dev_put’ on the
same device resulting in a no-op.
This change fixes this leak by assigning the initial namespace’s loopback
device to the ‘dst’’ before releasing the reference to the network device
being unregistered.
Additional reference: https://github.com/docker/docker/issues/5618
Signed-off-by: Jojy G Varghese <redacted>
---
net/core/dst.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: "Jojy Varghese" <redacted>
To: netdev@vger.kernel.org
Cc: davem@davemloft.net
Sent: Thursday, September 8, 2016 3:08:29 PM
Subject: Re: [PATCH] net_namespace: fixed net_device reference leak
(Updating the patch for the case when a non-loopback dev could be
unregistered)
Currently during namespace cleanup, if ‘dst’ subsystem is holding
a reference to the interface in the namespace, it does not get released.
Current code first does a ’dev_hold’ on the same device followed by a
‘dev_put’ on the same device resulting in a no-op.
This change fixes this leak by assigning the initial namespace’s loopback
device to the ‘dst’ before releasing the reference to the network
device being released.
Additional reference: https://github.com/docker/docker/issues/5618
Signed-off-by: Jojy G Varghese <redacted>
---
If the dst wasn't being cleaned up in the namespace that's being deleted,
it is unlikely it will be cleaned up after moving it to the init namespace.
I think it would be better to figure out why this dst is being leaked
in the first place, otherwise this is probably still a memory leak.
Is this easily reproducible with current net or net-next kernels?
Regards,
Lance
From: Eric Dumazet <hidden> Date: 2016-09-08 21:00:15
On Thu, 2016-09-08 at 12:08 -0700, Jojy Varghese wrote:
quoted hunk
(Updating the patch for the case when a non-loopback dev could be unregistered)
Currently during namespace cleanup, if ‘dst’ subsystem is holding
a reference to the interface in the namespace, it does not get released.
Current code first does a ’dev_hold’ on the same device followed by a
‘dev_put’ on the same device resulting in a no-op.
This change fixes this leak by assigning the initial namespace’s loopback
device to the ‘dst’ before releasing the reference to the network
device being released.
Additional reference: https://github.com/docker/docker/issues/5618
Signed-off-by: Jojy G Varghese <redacted>
---
net/core/dst.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
I understand these recurring problems are really a hassle
(unregister_netdevice: waiting for xxx to become free. Usage count = 1),
but this change simply hides the real bug ?
Pretending this dst now belongs to init_net is a bit like avoiding a
kfree(mem) : Better not free memory than risk use-after-free.
Now, try this one billion time, and I am sure host will have no memory
left.
At net namespace dismantle, all dst should be freed.
We have numerous callbacks to ensure this.
But maybe we have _one_ dst that is not properly purged.
Latest real fix was :
https://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id=751eb6b6042a596b0080967c1a529a9fe98dac1d
Hi Eric/Lance
Thanks for taking time to look at the patch. The problem in dst.c's
"dst_ifdown" is that it will never do an actual "dev_put" on the
loopback net device. This results in the loopback net device to be
alive even when the dst goes away.
Thanks
Jojy
On Thu, Sep 8, 2016 at 2:05 PM, Eric Dumazet [off-list ref] wrote:
On Thu, 2016-09-08 at 14:00 -0700, Eric Dumazet wrote:
From: Eric Dumazet <hidden> Date: 2016-09-08 22:16:55
On Thu, 2016-09-08 at 15:12 -0700, Jojy Varghese wrote:
Hi Eric/Lance
Thanks for taking time to look at the patch. The problem in dst.c's
"dst_ifdown" is that it will never do an actual "dev_put" on the
loopback net device. This results in the loopback net device to be
alive even when the dst goes away.
Sure, but the dst themselves should disappear, and thus release their
dst->dev reference.
Something is missing.
loopback device is only used when we remove one device from the system,
but namespace (and loopback device) is still around.
The dst's do disappear but the net device does not. A better solution
is to only hold reference to dst's net device ONLY if it is not a
loopback device. In the case of the loopback device, we want to do
only a "put" on it and skip the "hold".
Updating the patch below:
---
net/core/dst.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
Sorry last patch was just the delta over my original patch. Here is
the complete patch:
---
net/core/dst.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
The dst's do disappear but the net device does not. A better solution
is to only hold reference to dst's net device ONLY if it is not a
loopback device. In the case of the loopback device, we want to do
only a "put" on it and skip the "hold".
Updating the patch below:
---
net/core/dst.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
On Thu, 2016-09-08 at 15:12 -0700, Jojy Varghese wrote:
quoted
Hi Eric/Lance
Thanks for taking time to look at the patch. The problem in dst.c's
"dst_ifdown" is that it will never do an actual "dev_put" on the
loopback net device. This results in the loopback net device to be
alive even when the dst goes away.
Sure, but the dst themselves should disappear, and thus release their
dst->dev reference.
Something is missing.
loopback device is only used when we remove one device from the system,
but namespace (and loopback device) is still around.
From: Eric Dumazet <hidden> Date: 2016-09-08 23:37:43
On Thu, 2016-09-08 at 16:16 -0700, Jojy Varghese wrote:
quoted hunk
The dst's do disappear but the net device does not. A better solution
is to only hold reference to dst's net device ONLY if it is not a
loopback device. In the case of the loopback device, we want to do
only a "put" on it and skip the "hold".
Updating the patch below:
---
net/core/dst.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
From: Eric Dumazet <hidden> Date: 2016-09-08 23:51:27
On Thu, 2016-09-08 at 16:33 -0700, Jojy Varghese wrote:
quoted hunk
Sorry last patch was just the delta over my original patch. Here is
the complete patch:
---
net/core/dst.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
This is complete garbage, now we have a dst somewhere in the system,
pointing to a device that might have been freed.
Crash might happen when dst->dev is used later.
Sorry last patch was just the delta over my original patch. Here is
the complete patch:
It looks like you are grasping at straws.
Please slow down and take the time to debug this properly.
All of the mechanisms exist to make these references go away
properly, but you are seeing a leak for some reason.
Therefore, please work with Eric to discover where the real bug is
instead of posting knee-jerk reaction patches every 15 minutes.
Thank you.
Thanks for the feedback guys. I appreciate it.
I am working with 4.4.3 kernel. I will try to reproduce the issue
with the kernel tree at
https://git.kernel.org/cgit/linux/kernel/git/davem/net.git.
In the meantime, the only data I have now is :
- "dst-ifdown" does not look correct since it will never release
reference to loopback device.
- One way to fix might be to assign the 'dst''s net device to the
init net device. I thought adding reference to init net device's
loopback might be safe.
Any suggestion on debugging further is much appreciated.
-Jojy
On Thu, Sep 8, 2016 at 5:04 PM, David Miller [off-list ref] wrote:
Sorry last patch was just the delta over my original patch. Here is
the complete patch:
It looks like you are grasping at straws.
Please slow down and take the time to debug this properly.
All of the mechanisms exist to make these references go away
properly, but you are seeing a leak for some reason.
Therefore, please work with Eric to discover where the real bug is
instead of posting knee-jerk reaction patches every 15 minutes.
Thank you.
- "dst-ifdown" does not look correct since it will never release
reference to loopback device.
The routes and packets in the namespace are purged and disappear when
the namespace goes down, and that's how all of the references to the
loopback device are decremented.
Nothing dst_ifdown() does looks wrong, we are quite sure the problem
is elsewhere.
In the meantime, the only data I have now is :
- "dst-ifdown" does not look correct since it will never release
reference to loopback device.
Normally at netns dismantle, all sockets and devices holding references
to dst in this netns are closed and dst freed. Details are in various
notifiers.
At that time the dst->dev references will be released, including the
ones having dst->dev == loopback (on the soon to be freed netns)
We do not want to force in dst_ifdown() a dst->dev pointing nowhere, as
it might crash say when dst->dev is used later during a sendmsg() or
something like that.
- One way to fix might be to assign the 'dst''s net device to the
init net device. I thought adding reference to init net device's
loopback might be safe.
You only make _rebooting_ the host not possible, since the problem of
having some reference on loopback device will still be there.
We really have to find the root cause, not simply working around it.
From: David Ahern <hidden> Date: 2016-09-10 16:12:53
On 9/8/16 6:35 PM, Jojy Varghese wrote:
Thanks for the feedback guys. I appreciate it.
I am working with 4.4.3 kernel. I will try to reproduce the issue
with the kernel tree at
https://git.kernel.org/cgit/linux/kernel/git/davem/net.git.
In the meantime, the only data I have now is :
- "dst-ifdown" does not look correct since it will never release
reference to loopback device.
- One way to fix might be to assign the 'dst''s net device to the
init net device. I thought adding reference to init net device's
loopback might be safe.
Any suggestion on debugging further is much appreciated.
Last 2 times I chased down netdev refcnts I used the attach patch. It adds tracepoints to dev_hold and dev_put.
perf record -e net:dev_hold,net:dev_put -a -g -- <run your test>
You probably will need to increase memory for perf buffers (e.g., -m 8192).
It will dump a lot of data but you will find the missing put; you just have to be patient wading through the data and matching up hold and put references. python/perl script with perf script can make that easier.
---
Have you looked at whether there are open sockets? e.g., perhaps a dst reference is cached on a socket