[PATCH net] net: Unpublish sk from sk_reuseport_cb before call_rcu

[PATCH net] net: Unpublish sk from sk_reuseport_cb before call_rcu

[Martin KaFai Lau]

The "reuse->sock[]" array is shared by multiple sockets.  The going away
sk must unpublish itself from "reuse->sock[]" before making call_rcu()
call.  However, this unpublish-action is currently done after a grace
period and it may cause use-after-free.

The fix is to move reuseport_detach_sock() to sk_destruct().
Due to the above reason, any socket with sk_reuseport_cb has
to go through the rcu grace period before freeing it.

It is a rather old bug (~3 yrs).  The Fixes tag is not necessary
the right commit but it is the one that introduced the SOCK_RCU_FREE
logic and this fix is depending on it.

Fixes: a4298e4522d6 ("net: add SOCK_RCU_FREE socket flag")
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
 net/core/sock.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/net/core/sock.c b/net/core/sock.c
index 07863edbe6fc..e23ec80a67bf 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1700,8 +1700,6 @@ static void __sk_destruct(struct rcu_head *head)
 		sk_filter_uncharge(sk, filter);
 		RCU_INIT_POINTER(sk->sk_filter, NULL);
 	}
-	if (rcu_access_pointer(sk->sk_reuseport_cb))
-		reuseport_detach_sock(sk);
 
 	sock_disable_timestamp(sk, SK_FLAGS_TIMESTAMP);
 
@@ -1728,7 +1726,14 @@ static void __sk_destruct(struct rcu_head *head)
 
 void sk_destruct(struct sock *sk)
 {
-	if (sock_flag(sk, SOCK_RCU_FREE))
+	bool use_call_rcu = sock_flag(sk, SOCK_RCU_FREE);
+
+	if (rcu_access_pointer(sk->sk_reuseport_cb)) {
+		reuseport_detach_sock(sk);
+		use_call_rcu = true;
+	}
+
+	if (use_call_rcu)
 		call_rcu(&sk->sk_rcu, __sk_destruct);
 	else
 		__sk_destruct(&sk->sk_rcu);
-- 
2.17.1

Re: [PATCH net] net: Unpublish sk from sk_reuseport_cb before call_rcu

[David Miller]

From: Martin KaFai Lau <kafai@fb.com>
Date: Fri, 27 Sep 2019 16:00:31 -0700

> The "reuse->sock[]" array is shared by multiple sockets.  The going away
> sk must unpublish itself from "reuse->sock[]" before making call_rcu()
> call.  However, this unpublish-action is currently done after a grace
> period and it may cause use-after-free.
> 
> The fix is to move reuseport_detach_sock() to sk_destruct().
> Due to the above reason, any socket with sk_reuseport_cb has
> to go through the rcu grace period before freeing it.
> 
> It is a rather old bug (~3 yrs).  The Fixes tag is not necessary
> the right commit but it is the one that introduced the SOCK_RCU_FREE
> logic and this fix is depending on it.
> 
> Fixes: a4298e4522d6 ("net: add SOCK_RCU_FREE socket flag")
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>

Applied and queued up for -stable, thanks Martin.