Re: [PATCH 6.1-rc6] l2tp: call udp_tunnel_encap_enable() and sock_release() without sk_callback_lock
From: Jakub Sitnicki <jakub@cloudflare.com>
Date: 2022-11-18 22:13:42
On Fri, Nov 18, 2022 at 06:50 PM +01, Jakub Sitnicki wrote: [...]
But, I also have an alternative idea - instead of pulling the function call that might sleep out of the critical section, I think we could make the critical section much shorter by rearranging the tunnel initialization code slightly. That is, a change like below.
[...]
quoted hunk
--8<--diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c index 754fdda8a5f5..07454c0418e3 100644 --- a/net/l2tp/l2tp_core.c +++ b/net/l2tp/l2tp_core.c@@ -1474,11 +1474,15 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net, } sk = sock->sk; - write_lock(&sk->sk_callback_lock); + write_lock_bh(&sk->sk_callback_lock); ret = l2tp_validate_socket(sk, net, tunnel->encap); if (ret < 0) goto err_sock; + if (tunnel->encap != L2TP_ENCAPTYPE_UDP) + rcu_assign_sk_user_data(sk, tunnel);
sk_user_data needs to be reset back to NULL if we bail out when the tunnel already exists. Will add that and turn it into a patch tomorrow.
quoted hunk
+ + write_unlock_bh(&sk->sk_callback_lock); tunnel->l2tp_net = net; pn = l2tp_pernet(net);@@ -1507,8 +1511,6 @@ int l2tp_tunnel_register(struct l2tp_tunnel *tunnel, struct net *net, }; setup_udp_tunnel_sock(net, sock, &udp_cfg); - } else { - rcu_assign_sk_user_data(sk, tunnel); } tunnel->old_sk_destruct = sk->sk_destruct;@@ -1522,7 +1524,6 @@ 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:@@ -1530,8 +1531,6 @@ 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; }