[PATCH net] rxrpc: Fix deadlock around release of dst cached on udp tunnel

[PATCH net] rxrpc: Fix deadlock around release of dst cached on udp tunnel

[David Howells]

AF_RXRPC sockets use UDP ports in encap mode.  This causes socket and dst
from an incoming packet to get stolen and attached to the UDP socket from
whence it is leaked when that socket is closed.

When a network namespace is removed, the wait for dst records to be cleaned
up happens before the cleanup of the rxrpc and UDP socket, meaning that the
wait never finishes.

Fix this by moving the rxrpc (and, by dependence, the afs) private
per-network namespace registrations to the device group rather than subsys
group.  This allows cached rxrpc local endpoints to be cleared and their
UDP sockets closed before we try waiting for the dst records.

The symptom is that lines looking like the following:

	unregister_netdevice: waiting for lo to become free

get emitted at regular intervals after running something like the
referenced syzbot test.

Thanks to Vadim for tracking this down and work out the fix.

Reported-by: syzbot+df400f2f24a1677cd7e0@syzkaller.appspotmail.com
Reported-by: Vadim Fedorenko <vfedorenko@novek.ru>
Fixes: 5271953cad31 ("rxrpc: Use the UDP encap_rcv hook")
Signed-off-by: David Howells <dhowells@redhat.com>
---

 fs/afs/main.c        |    6 +++---
 net/rxrpc/af_rxrpc.c |    6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/afs/main.c b/fs/afs/main.c
index accdd8970e7c..b2975256dadb 100644
--- a/fs/afs/main.c
+++ b/fs/afs/main.c
@@ -193,7 +193,7 @@ static int __init afs_init(void)
 		goto error_cache;
 #endif
 
-	ret = register_pernet_subsys(&afs_net_ops);
+	ret = register_pernet_device(&afs_net_ops);
 	if (ret < 0)
 		goto error_net;
 
@@ -213,7 +213,7 @@ static int __init afs_init(void)
 error_proc:
 	afs_fs_exit();
 error_fs:
-	unregister_pernet_subsys(&afs_net_ops);
+	unregister_pernet_device(&afs_net_ops);
 error_net:
 #ifdef CONFIG_AFS_FSCACHE
 	fscache_unregister_netfs(&afs_cache_netfs);
@@ -244,7 +244,7 @@ static void __exit afs_exit(void)
 
 	proc_remove(afs_proc_symlink);
 	afs_fs_exit();
-	unregister_pernet_subsys(&afs_net_ops);
+	unregister_pernet_device(&afs_net_ops);
 #ifdef CONFIG_AFS_FSCACHE
 	fscache_unregister_netfs(&afs_cache_netfs);
 #endif
diff --git a/net/rxrpc/af_rxrpc.c b/net/rxrpc/af_rxrpc.c
index 0a2f4817ec6c..41671af6b33f 100644
--- a/net/rxrpc/af_rxrpc.c
+++ b/net/rxrpc/af_rxrpc.c
@@ -990,7 +990,7 @@ static int __init af_rxrpc_init(void)
 		goto error_security;
 	}
 
-	ret = register_pernet_subsys(&rxrpc_net_ops);
+	ret = register_pernet_device(&rxrpc_net_ops);
 	if (ret)
 		goto error_pernet;
 
@@ -1035,7 +1035,7 @@ static int __init af_rxrpc_init(void)
 error_sock:
 	proto_unregister(&rxrpc_proto);
 error_proto:
-	unregister_pernet_subsys(&rxrpc_net_ops);
+	unregister_pernet_device(&rxrpc_net_ops);
 error_pernet:
 	rxrpc_exit_security();
 error_security:
@@ -1057,7 +1057,7 @@ static void __exit af_rxrpc_exit(void)
 	unregister_key_type(&key_type_rxrpc);
 	sock_unregister(PF_RXRPC);
 	proto_unregister(&rxrpc_proto);
-	unregister_pernet_subsys(&rxrpc_net_ops);
+	unregister_pernet_device(&rxrpc_net_ops);
 	ASSERTCMP(atomic_read(&rxrpc_n_tx_skbs), ==, 0);
 	ASSERTCMP(atomic_read(&rxrpc_n_rx_skbs), ==, 0);
 

Re: [PATCH net] rxrpc: Fix deadlock around release of dst cached on udp tunnel

[Vadim Fedorenko]

On 29.01.2021 16:44, David Howells wrote:
> AF_RXRPC sockets use UDP ports in encap mode.  This causes socket and dst
> from an incoming packet to get stolen and attached to the UDP socket from
> whence it is leaked when that socket is closed.
> 
> When a network namespace is removed, the wait for dst records to be cleaned
> up happens before the cleanup of the rxrpc and UDP socket, meaning that the
> wait never finishes.
> 
> Fix this by moving the rxrpc (and, by dependence, the afs) private
> per-network namespace registrations to the device group rather than subsys
> group.  This allows cached rxrpc local endpoints to be cleared and their
> UDP sockets closed before we try waiting for the dst records.
> 
> The symptom is that lines looking like the following:
> 
> 	unregister_netdevice: waiting for lo to become free
> 
> get emitted at regular intervals after running something like the
> referenced syzbot test.
> 
> Thanks to Vadim for tracking this down and work out the fix.

You missed the call to dst_release(sk->sk_rx_dst) in rxrpc_sock_destructor. 
Without it we are still leaking the dst.

Re: [PATCH net] rxrpc: Fix deadlock around release of dst cached on udp tunnel

[Vadim Fedorenko]

On 29.01.2021 17:30, Vadim Fedorenko wrote:
> On 29.01.2021 16:44, David Howells wrote:
>> AF_RXRPC sockets use UDP ports in encap mode.  This causes socket and dst
>> from an incoming packet to get stolen and attached to the UDP socket from
>> whence it is leaked when that socket is closed.
>>
>> When a network namespace is removed, the wait for dst records to be cleaned
>> up happens before the cleanup of the rxrpc and UDP socket, meaning that the
>> wait never finishes.
>>
>> Fix this by moving the rxrpc (and, by dependence, the afs) private
>> per-network namespace registrations to the device group rather than subsys
>> group.  This allows cached rxrpc local endpoints to be cleared and their
>> UDP sockets closed before we try waiting for the dst records.
>>
>> The symptom is that lines looking like the following:
>>
>>     unregister_netdevice: waiting for lo to become free
>>
>> get emitted at regular intervals after running something like the
>> referenced syzbot test.
>>
>> Thanks to Vadim for tracking this down and work out the fix.
> 
> You missed the call to dst_release(sk->sk_rx_dst) in rxrpc_sock_destructor. 
> Without it we are still leaking the dst.
I mean this part:

diff --git a/net/rxrpc/af_rxrpc.c b/net/rxrpc/af_rxrpc.c
index 0a2f481..3c0635e 100644
--- a/net/rxrpc/af_rxrpc.c
+++ b/net/rxrpc/af_rxrpc.c
@@ -833,6 +833,7 @@ static void rxrpc_sock_destructor(struct sock *sk)
         _enter("%p", sk);

         rxrpc_purge_queue(&sk->sk_receive_queue);
+       dst_destroy(sk->sk_rx_dst);

         WARN_ON(refcount_read(&sk->sk_wmem_alloc));
         WARN_ON(!sk_unhashed(sk));

Re: [PATCH net] rxrpc: Fix deadlock around release of dst cached on udp tunnel

[David Howells]

Vadim Fedorenko <vfedorenko@novek.ru> wrote:

> You missed the call to dst_release(sk->sk_rx_dst) in
> rxrpc_sock_destructor. Without it we are still leaking the dst.

Hmmm...  I no longer get the messages appearing with this patch.  I'll have
another look.

David

Re: [PATCH net] rxrpc: Fix deadlock around release of dst cached on udp tunnel

[Vadim Fedorenko]

On 29.01.2021 17:41, David Howells wrote:
> Vadim Fedorenko <vfedorenko@novek.ru> wrote:
> 
>> You missed the call to dst_release(sk->sk_rx_dst) in
>> rxrpc_sock_destructor. Without it we are still leaking the dst.
> 
> Hmmm...  I no longer get the messages appearing with this patch.  I'll have
> another look.

Sorry, my bad. rxrpc_sock_destructor destroys AF_RXRPC sockets, which are 
totally different from the UDP kernel socket used as a transport and which are 
destroyed as usual UDP sockets.

Ack-by: Vadim Fedorenko <vfedorenko@novek.ru>

Re: [PATCH net] rxrpc: Fix deadlock around release of dst cached on udp tunnel

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Fri, 29 Jan 2021 23:53:50 +0000 you wrote:
> AF_RXRPC sockets use UDP ports in encap mode.  This causes socket and dst
> from an incoming packet to get stolen and attached to the UDP socket from
> whence it is leaked when that socket is closed.
> 
> When a network namespace is removed, the wait for dst records to be cleaned
> up happens before the cleanup of the rxrpc and UDP socket, meaning that the
> wait never finishes.
> 
> [...]

Here is the summary with links:
  - [net] rxrpc: Fix deadlock around release of dst cached on udp tunnel
    https://git.kernel.org/netdev/net/c/5399d52233c4

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

[PATCH net] rxrpc: Fix deadlock around release of dst cached on udp tunnel

[David Howells]

AF_RXRPC sockets use UDP ports in encap mode.  This causes socket and dst
from an incoming packet to get stolen and attached to the UDP socket from
whence it is leaked when that socket is closed.

When a network namespace is removed, the wait for dst records to be cleaned
up happens before the cleanup of the rxrpc and UDP socket, meaning that the
wait never finishes.

Fix this by moving the rxrpc (and, by dependence, the afs) private
per-network namespace registrations to the device group rather than subsys
group.  This allows cached rxrpc local endpoints to be cleared and their
UDP sockets closed before we try waiting for the dst records.

The symptom is that lines looking like the following:

	unregister_netdevice: waiting for lo to become free

get emitted at regular intervals after running something like the
referenced syzbot test.

Thanks to Vadim for tracking this down and work out the fix.

Reported-by: syzbot+df400f2f24a1677cd7e0@syzkaller.appspotmail.com
Reported-by: Vadim Fedorenko <vfedorenko@novek.ru>
Fixes: 5271953cad31 ("rxrpc: Use the UDP encap_rcv hook")
Signed-off-by: David Howells <dhowells@redhat.com>
Acked-by: Vadim Fedorenko <vfedorenko@novek.ru>
---

 fs/afs/main.c        |    6 +++---
 net/rxrpc/af_rxrpc.c |    6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/afs/main.c b/fs/afs/main.c
index accdd8970e7c..b2975256dadb 100644
--- a/fs/afs/main.c
+++ b/fs/afs/main.c
@@ -193,7 +193,7 @@ static int __init afs_init(void)
 		goto error_cache;
 #endif
 
-	ret = register_pernet_subsys(&afs_net_ops);
+	ret = register_pernet_device(&afs_net_ops);
 	if (ret < 0)
 		goto error_net;
 
@@ -213,7 +213,7 @@ static int __init afs_init(void)
 error_proc:
 	afs_fs_exit();
 error_fs:
-	unregister_pernet_subsys(&afs_net_ops);
+	unregister_pernet_device(&afs_net_ops);
 error_net:
 #ifdef CONFIG_AFS_FSCACHE
 	fscache_unregister_netfs(&afs_cache_netfs);
@@ -244,7 +244,7 @@ static void __exit afs_exit(void)
 
 	proc_remove(afs_proc_symlink);
 	afs_fs_exit();
-	unregister_pernet_subsys(&afs_net_ops);
+	unregister_pernet_device(&afs_net_ops);
 #ifdef CONFIG_AFS_FSCACHE
 	fscache_unregister_netfs(&afs_cache_netfs);
 #endif
diff --git a/net/rxrpc/af_rxrpc.c b/net/rxrpc/af_rxrpc.c
index 0a2f4817ec6c..41671af6b33f 100644
--- a/net/rxrpc/af_rxrpc.c
+++ b/net/rxrpc/af_rxrpc.c
@@ -990,7 +990,7 @@ static int __init af_rxrpc_init(void)
 		goto error_security;
 	}
 
-	ret = register_pernet_subsys(&rxrpc_net_ops);
+	ret = register_pernet_device(&rxrpc_net_ops);
 	if (ret)
 		goto error_pernet;
 
@@ -1035,7 +1035,7 @@ static int __init af_rxrpc_init(void)
 error_sock:
 	proto_unregister(&rxrpc_proto);
 error_proto:
-	unregister_pernet_subsys(&rxrpc_net_ops);
+	unregister_pernet_device(&rxrpc_net_ops);
 error_pernet:
 	rxrpc_exit_security();
 error_security:
@@ -1057,7 +1057,7 @@ static void __exit af_rxrpc_exit(void)
 	unregister_key_type(&key_type_rxrpc);
 	sock_unregister(PF_RXRPC);
 	proto_unregister(&rxrpc_proto);
-	unregister_pernet_subsys(&rxrpc_net_ops);
+	unregister_pernet_device(&rxrpc_net_ops);
 	ASSERTCMP(atomic_read(&rxrpc_n_tx_skbs), ==, 0);
 	ASSERTCMP(atomic_read(&rxrpc_n_rx_skbs), ==, 0);