In some rare cases, inet_sk_rx_dst_set() may be called multiple times
on the same dst, causing reference count leakage. Eventually, it
prevents net_device to be destroyed. The bug then manifested as
unregister_netdevice: waiting for lo to become free. Usage count = 1
in the kernel log, preventing new network namespace creation.
The patch works around the issue by checking whether the socket already
has the same dst set.
Signed-off-by: Kevin Xu <redacted>
---
net/ipv4/tcp_ipv4.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
From: Jakub Sitnicki <hidden> Date: 2017-03-16 10:01:27
On Thu, Mar 16, 2017 at 08:08 AM GMT, Kevin Xu wrote:
In some rare cases, inet_sk_rx_dst_set() may be called multiple times
on the same dst, causing reference count leakage. Eventually, it
prevents net_device to be destroyed. The bug then manifested as
unregister_netdevice: waiting for lo to become free. Usage count = 1
in the kernel log, preventing new network namespace creation.
The patch works around the issue by checking whether the socket already
has the same dst set.
Signed-off-by: Kevin Xu <redacted>
---
FWIW, with this patch applied I'm still sometimes seeing:
[ 125.928095] unregister_netdevice: waiting for lo to become free. Usage count = 1
-Jakub
Do you mean the message looping endlessly?
If so, then I suppose it's a different bug.
Kevin
On Mar 16, 2017, at 3:01 AM, Jakub Sitnicki [off-list ref] wrote:
quoted
On Thu, Mar 16, 2017 at 08:08 AM GMT, Kevin Xu wrote:
In some rare cases, inet_sk_rx_dst_set() may be called multiple times
on the same dst, causing reference count leakage. Eventually, it
prevents net_device to be destroyed. The bug then manifested as
unregister_netdevice: waiting for lo to become free. Usage count = 1
in the kernel log, preventing new network namespace creation.
The patch works around the issue by checking whether the socket already
has the same dst set.
Signed-off-by: Kevin Xu <redacted>
---
FWIW, with this patch applied I'm still sometimes seeing:
[ 125.928095] unregister_netdevice: waiting for lo to become free. Usage count = 1
-Jakub
From: Jakub Sitnicki <hidden> Date: 2017-03-16 10:45:07
On Thu, Mar 16, 2017 at 10:12 AM GMT, Kevin Xu wrote:
Do you mean the message looping endlessly?
No, the message is emitted just once. Around 100 seconds after
destroying a few namespaces. Occurs not so often, maybe once per ten
runs.
-Jakub
If so, then I suppose it's a different bug.
Kevin
quoted
On Mar 16, 2017, at 3:01 AM, Jakub Sitnicki [off-list ref] wrote:
quoted
On Thu, Mar 16, 2017 at 08:08 AM GMT, Kevin Xu wrote:
In some rare cases, inet_sk_rx_dst_set() may be called multiple times
on the same dst, causing reference count leakage. Eventually, it
prevents net_device to be destroyed. The bug then manifested as
unregister_netdevice: waiting for lo to become free. Usage count = 1
in the kernel log, preventing new network namespace creation.
The patch works around the issue by checking whether the socket already
has the same dst set.
Signed-off-by: Kevin Xu <redacted>
---
FWIW, with this patch applied I'm still sometimes seeing:
[ 125.928095] unregister_netdevice: waiting for lo to become free. Usage count = 1
-Jakub
On Thu, Mar 16, 2017 at 11:45:03AM +0100, Jakub Sitnicki wrote:
On Thu, Mar 16, 2017 at 10:12 AM GMT, Kevin Xu wrote:
quoted
Do you mean the message looping endlessly?
No, the message is emitted just once. Around 100 seconds after
destroying a few namespaces. Occurs not so often, maybe once per ten
runs.
-Jakub
I saw that happening from time to time during my test as well, I suspect
it was because some TCP sockets stays in either TCP_TIME_WAIT or
TCP_FIN_WAIT1. But eventually those sockets get destroyed and lo gets
deleted as well.
The patch was fixing an issue I am seeing, that the message gets looped
forever, and causing a deadlock on new network ns creation.
Kevin
quoted
If so, then I suppose it's a different bug.
Kevin
quoted
On Mar 16, 2017, at 3:01 AM, Jakub Sitnicki [off-list ref] wrote:
quoted
On Thu, Mar 16, 2017 at 08:08 AM GMT, Kevin Xu wrote:
In some rare cases, inet_sk_rx_dst_set() may be called multiple times
on the same dst, causing reference count leakage. Eventually, it
prevents net_device to be destroyed. The bug then manifested as
unregister_netdevice: waiting for lo to become free. Usage count = 1
in the kernel log, preventing new network namespace creation.
The patch works around the issue by checking whether the socket already
has the same dst set.
Signed-off-by: Kevin Xu <redacted>
---
FWIW, with this patch applied I'm still sometimes seeing:
[ 125.928095] unregister_netdevice: waiting for lo to become free. Usage count = 1
-Jakub
From: Cong Wang <hidden> Date: 2017-03-16 18:42:32
On Thu, Mar 16, 2017 at 11:18 AM, Kaiwen Xu [off-list ref] wrote:
On Thu, Mar 16, 2017 at 11:45:03AM +0100, Jakub Sitnicki wrote:
quoted
On Thu, Mar 16, 2017 at 10:12 AM GMT, Kevin Xu wrote:
quoted
Do you mean the message looping endlessly?
No, the message is emitted just once. Around 100 seconds after
destroying a few namespaces. Occurs not so often, maybe once per ten
runs.
-Jakub
I saw that happening from time to time during my test as well, I suspect
it was because some TCP sockets stays in either TCP_TIME_WAIT or
TCP_FIN_WAIT1. But eventually those sockets get destroyed and lo gets
deleted as well.
But TIMEWAIT sockets are purged by inet_twsk_purge() during netns
destroy, apparently before lo is destroyed.
The patch was fixing an issue I am seeing, that the message gets looped
forever, and causing a deadlock on new network ns creation.
But as DaveM said, the race still could happen after your patch,
the if check you add is not atomic at all.
Also, we should already lock the sock at the time we call
inet_sk_rx_dst_set(), but perhaps not for TCP_LISTEN case...
From: David Miller <davem@davemloft.net> Date: 2017-03-16 18:16:48
From: Kevin Xu <redacted>
Date: Thu, 16 Mar 2017 01:08:30 -0700
In some rare cases, inet_sk_rx_dst_set() may be called multiple times
on the same dst, causing reference count leakage. Eventually, it
prevents net_device to be destroyed. The bug then manifested as
unregister_netdevice: waiting for lo to become free. Usage count = 1
in the kernel log, preventing new network namespace creation.
The patch works around the issue by checking whether the socket already
has the same dst set.
Signed-off-by: Kevin Xu <redacted>
You need to prevent this parallel execution of this function or use
atomic compare-and-exchange to set the rx_dst in order to prevent
the double refcounting.
This patch by itself is even worse than a workaround, because depending
upon how the compiler reloads values from memory, the problem can still
occur.
In some rare cases, inet_sk_rx_dst_set() may be called multiple times
on the same dst, causing double refcounting. Eventually, it
prevents net_device to be destroyed. The bug manifested as
unregister_netdevice: waiting for lo to become free. Usage count = 1
in the kernel log, preventing new network namespace creation.
Signed-off-by: Kevin Xu <redacted>
---
net/ipv4/tcp_ipv4.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
From: Cong Wang <hidden> Date: 2017-03-19 03:50:05
On Sat, Mar 18, 2017 at 6:48 PM, Kevin Xu [off-list ref] wrote:
In some rare cases, inet_sk_rx_dst_set() may be called multiple times
on the same dst, causing double refcounting. Eventually, it
prevents net_device to be destroyed. The bug manifested as
unregister_netdevice: waiting for lo to become free. Usage count = 1
in the kernel log, preventing new network namespace creation.
Signed-off-by: Kevin Xu <redacted>
Don't know why you don't follow the discussion on your v1...
It is protected by bh_lock_sock(), so your patch is not needed
at all.
Read net/ipv4/udp.c:
1762 /* For TCP sockets, sk_rx_dst is protected by socket lock
1763 * For UDP, we use xchg() to guard against concurrent changes.
1764 */
On Sat, Mar 18, 2017 at 08:49:43PM -0700, Cong Wang wrote:
On Sat, Mar 18, 2017 at 6:48 PM, Kevin Xu [off-list ref] wrote:
quoted
In some rare cases, inet_sk_rx_dst_set() may be called multiple times
on the same dst, causing double refcounting. Eventually, it
prevents net_device to be destroyed. The bug manifested as
unregister_netdevice: waiting for lo to become free. Usage count = 1
in the kernel log, preventing new network namespace creation.
Signed-off-by: Kevin Xu <redacted>
Don't know why you don't follow the discussion on your v1...
It is protected by bh_lock_sock(), so your patch is not needed
at all.
Read net/ipv4/udp.c:
1762 /* For TCP sockets, sk_rx_dst is protected by socket lock
1763 * For UDP, we use xchg() to guard against concurrent changes.
1764 */
I probably misunderstood. Do you mean v2 patch is actually not needed or
the whole workaround is not necessary?
From: Cong Wang <hidden> Date: 2017-03-20 04:16:42
On Sat, Mar 18, 2017 at 9:03 PM, Kaiwen Xu [off-list ref] wrote:
On Sat, Mar 18, 2017 at 08:49:43PM -0700, Cong Wang wrote:
quoted
On Sat, Mar 18, 2017 at 6:48 PM, Kevin Xu [off-list ref] wrote:
quoted
In some rare cases, inet_sk_rx_dst_set() may be called multiple times
on the same dst, causing double refcounting. Eventually, it
prevents net_device to be destroyed. The bug manifested as
unregister_netdevice: waiting for lo to become free. Usage count = 1
in the kernel log, preventing new network namespace creation.
Signed-off-by: Kevin Xu <redacted>
Don't know why you don't follow the discussion on your v1...
It is protected by bh_lock_sock(), so your patch is not needed
at all.
Read net/ipv4/udp.c:
1762 /* For TCP sockets, sk_rx_dst is protected by socket lock
1763 * For UDP, we use xchg() to guard against concurrent changes.
1764 */
I probably misunderstood. Do you mean v2 patch is actually not needed or
the whole workaround is not necessary?
Your patch, no matter v1 or v2, is not needed because we use
bh_lock_sock() to serialize inet_sk_rx_dst_set(), unless you find
a case where we miss the bh_lock_sock(), but you don't say it in
your changelog. "some rare cases" is not enough to justify this bug.
On Sun, Mar 19, 2017 at 09:09:38PM -0700, Cong Wang wrote:
On Sat, Mar 18, 2017 at 9:03 PM, Kaiwen Xu [off-list ref] wrote:
quoted
On Sat, Mar 18, 2017 at 08:49:43PM -0700, Cong Wang wrote:
quoted
On Sat, Mar 18, 2017 at 6:48 PM, Kevin Xu [off-list ref] wrote:
quoted
In some rare cases, inet_sk_rx_dst_set() may be called multiple times
on the same dst, causing double refcounting. Eventually, it
prevents net_device to be destroyed. The bug manifested as
unregister_netdevice: waiting for lo to become free. Usage count = 1
in the kernel log, preventing new network namespace creation.
Signed-off-by: Kevin Xu <redacted>
Don't know why you don't follow the discussion on your v1...
It is protected by bh_lock_sock(), so your patch is not needed
at all.
Read net/ipv4/udp.c:
1762 /* For TCP sockets, sk_rx_dst is protected by socket lock
1763 * For UDP, we use xchg() to guard against concurrent changes.
1764 */
I probably misunderstood. Do you mean v2 patch is actually not needed or
the whole workaround is not necessary?
Your patch, no matter v1 or v2, is not needed because we use
bh_lock_sock() to serialize inet_sk_rx_dst_set(), unless you find
a case where we miss the bh_lock_sock(), but you don't say it in
your changelog. "some rare cases" is not enough to justify this bug.
I see, thanks for your explanation! I will try to dig in more to see if
I can find the root cause.
From: Eric Dumazet <hidden> Date: 2017-03-16 21:34:11
On Thu, 2017-03-16 at 01:08 -0700, Kevin Xu wrote:
quoted hunk
In some rare cases, inet_sk_rx_dst_set() may be called multiple times
on the same dst, causing reference count leakage. Eventually, it
prevents net_device to be destroyed. The bug then manifested as
unregister_netdevice: waiting for lo to become free. Usage count = 1
in the kernel log, preventing new network namespace creation.
The patch works around the issue by checking whether the socket already
has the same dst set.
Signed-off-by: Kevin Xu <redacted>
---
net/ipv4/tcp_ipv4.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
That should not be needed.
Have you backported the redirect fix ?
commit 45caeaa5ac0b4b11784ac6f932c0ad4c6b67cda0
Or other fixes that went very recently (pick David Miller net tree)
From: Cong Wang <hidden> Date: 2017-03-16 23:14:30
On Thu, Mar 16, 2017 at 2:33 PM, Eric Dumazet [off-list ref] wrote:
Have you backported the redirect fix ?
commit 45caeaa5ac0b4b11784ac6f932c0ad4c6b67cda0
Or other fixes that went very recently (pick David Miller net tree)
Why the commit above is relevant here? It fixes a double-release,
while Kevin's case is a double-hold... Not to mention it sk_rx_dst
instead of sk_dst_cache, according to Kevin.
From: Eric Dumazet <hidden> Date: 2017-03-16 23:29:08
On Thu, 2017-03-16 at 16:13 -0700, Cong Wang wrote:
On Thu, Mar 16, 2017 at 2:33 PM, Eric Dumazet [off-list ref] wrote:
quoted
Have you backported the redirect fix ?
commit 45caeaa5ac0b4b11784ac6f932c0ad4c6b67cda0
Or other fixes that went very recently (pick David Miller net tree)
Why the commit above is relevant here? It fixes a double-release,
while Kevin's case is a double-hold... Not to mention it sk_rx_dst
instead of sk_dst_cache, according to Kevin.
Yes you are right.
I was lazy, the other fix I wanted to mention was
02b2faaf0af1d85585f
In any case I wanted to make sure the reported issue is not for some old
kernels.