Patch "l2tp: Serialize access to sk_user_data with sk_callback_lock" has been added to the 6.0-stable tree

This is a note to let you know that I've just added the patch titled

l2tp: Serialize access to sk_user_data with sk_callback_lock

to the 6.0-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
l2tp-serialize-access-to-sk_user_data-with-sk_callba.patch
and it can be found in the queue-6.0 subdirectory.

If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> know about it.

commit 1ea60c1db42da0b5b40eb7e9bf8d5937f6f475cc

Author: Jakub Sitnicki <[email protected]>
Date:   Mon Nov 14 20:16:19 2022 +0100

l2tp: Serialize access to sk_user_data with sk_callback_lock

[ Upstream commit b68777d54fac21fc833ec26ea1a2a84f975ab035 ]

sk->sk_user_data has multiple users, which are not compatible with each other. Writers must synchronize by grabbing the sk->sk_callback_lock.

l2tp currently fails to grab the lock when modifying the underlying tunnel socket fields. Fix it by adding appropriate locking.

We err on the side of safety and grab the sk_callback_lock also inside the sk_destruct callback overridden by l2tp, even though there should be no refs allowing access to the sock at the time when sk_destruct gets called.

v4:
- serialize write to sk_user_data in l2tp sk_destruct

v3:
- switch from sock lock to sk_callback_lock
- document write-protection for sk_user_data

v2:
- update Fixes to point to origin of the bug
- use real names in Reported/Tested-by tags

Cc Tom Parkin <[email protected]>

Fixes 3557baabf280 ("[L2TP]: PPP over L2TP driver core")

Reported-by Haowei Yan <[email protected]>

Signed-off-by Jakub Sitnicki <[email protected]>

Signed-off-by David S. Miller <[email protected]>

Signed-off-by Sasha Levin <[email protected]>

include/net/sock.h +1 -1

index f6e6838c82df..03a4ebe3ccc8 100644
@@ -323,7 +323,7 @@ struct sk_filter;
   *	@sk_tskey: counter to disambiguate concurrent tstamp requests
   *	@sk_zckey: counter to order MSG_ZEROCOPY notifications
   *	@sk_socket: Identd and reporting IO signals
-  *	@sk_user_data: RPC layer private data
+  *	@sk_user_data: RPC layer private data. Write-protected by @sk_callback_lock.
   *	@sk_frag: cached page frag
   *	@sk_peek_off: current peek_offset value
   *	@sk_send_head: front of stuff to transmit

net/l2tp/l2tp_core.c +13 -6

index 7499c51b1850..754fdda8a5f5 100644
@@ -1150,8 +1150,10 @@ static void l2tp_tunnel_destruct(struct sock *sk)
 	}
 
 	/* Remove hooks into tunnel socket */
+	write_lock_bh(&sk->sk_callback_lock);
 	sk->sk_destruct = tunnel->old_sk_destruct;
 	sk->sk_user_data = NULL;
+	write_unlock_bh(&sk->sk_callback_lock);
 
 	/* Call the original destructor */
 	if (sk->sk_destruct)

@@ -1469,16 +1471,18 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
 		sock = sockfd_lookup(tunnel->fd, &ret);
 		if (!sock)
 			goto err;
-
-		ret = l2tp_validate_socket(sock->sk, net, tunnel->encap);
-		if (ret < 0)
-			goto err_sock;
 	}
 
+	sk = sock->sk;
+	write_lock(&sk->sk_callback_lock);
+
+	ret = l2tp_validate_socket(sk, net, tunnel->encap);
+	if (ret < 0)
+		goto err_sock;
+
 	tunnel->l2tp_net = net;
 	pn = l2tp_pernet(net);
 
-	sk = sock->sk;
 	sock_hold(sk);
 	tunnel->sock = sk;
 

@@ -1504,7 +1508,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
 
 		setup_udp_tunnel_sock(net, sock, &udp_cfg);
 	} else {
-		sk->sk_user_data = tunnel;
+		rcu_assign_sk_user_data(sk, tunnel);
 	}
 
 	tunnel->old_sk_destruct = sk->sk_destruct;

@@ -1518,6 +1522,7 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
 	if (tunnel->fd >= 0)
 		sockfd_put(sock);
 
+	write_unlock(&sk->sk_callback_lock);
 	return 0;
 
 err_sock:

@@ -1525,6 +1530,8 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net,
 		sock_release(sock);
 	else
 		sockfd_put(sock);
+
+	write_unlock(&sk->sk_callback_lock);
 err:
 	return ret;
 }

Hi Sasha,

Quoted 11 lines · Sasha Levin

This is a note to let you know that I've just added the patch titled

l2tp: Serialize access to sk_user_data with sk_callback_lock

to the 6.0-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
l2tp-serialize-access-to-sk_user_data-with-sk_callba.patch
and it can be found in the queue-6.0 subdirectory.

Quoted 2 lines

If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> know about it.

"Just when I thought I was out, they pull me back in!"

Greg assured me yesterday that this was dropped from stable queues:

https://lore.kernel.org/stable/[email protected]/