| Message ID | 20220823101459.211986-1-jakub@cloudflare.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v3] l2tp: Serialize access to sk_user_data with sk_callback_lock | expand |
hello, On Tue, 2022-08-23 at 12:14 +0200, Jakub Sitnicki wrote: > 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. Fix it by adding appropriate locking. > > We don't to grab the lock when l2tp clears sk_user_data, because it happens > only in sk->sk_destruct, when the sock is going away. l2tp can additionally clears sk_user_data in sk->sk_prot->close() via udp_lib_close() -> sk_common_release() -> sk->sk_prot->destroy() -> udp_destroy_sock() -> up->encap_destroy() -> l2tp_udp_encap_destroy(). That still happens at socket closing time, but when network has still access to the sock itself. It should be safe as the other sk_user_data users touch it only via fd, but perhaps a 'better safe the sorry' approach could be relevant there? Thanks! Paolo
On Thu, Aug 25, 2022 at 12:23 PM +02, Paolo Abeni wrote: > hello, > > On Tue, 2022-08-23 at 12:14 +0200, Jakub Sitnicki wrote: >> 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. Fix it by adding appropriate locking. >> >> We don't to grab the lock when l2tp clears sk_user_data, because it happens >> only in sk->sk_destruct, when the sock is going away. > > l2tp can additionally clears sk_user_data in sk->sk_prot->close() via > udp_lib_close() -> sk_common_release() -> sk->sk_prot->destroy() -> > udp_destroy_sock() -> up->encap_destroy() -> l2tp_udp_encap_destroy(). > > That still happens at socket closing time, but when network has still > access to the sock itself. It should be safe as the other sk_user_data > users touch it only via fd, but perhaps a 'better safe the sorry' > approach could be relevant there? Fair point. Let me add that.
diff --git a/include/net/sock.h b/include/net/sock.h index d08cfe190a78..601c48601496 100644 --- a/include/net/sock.h +++ b/include/net/sock.h @@ -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 diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c index 7499c51b1850..429ad9633f13 100644 --- a/net/l2tp/l2tp_core.c +++ b/net/l2tp/l2tp_core.c @@ -1469,16 +1469,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 +1506,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 +1520,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 +1528,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; }
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. Fix it by adding appropriate locking. We don't to grab the lock when l2tp clears sk_user_data, because it happens only in sk->sk_destruct, when the sock is going away. 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 Fixes: 3557baabf280 ("[L2TP]: PPP over L2TP driver core") Reported-by: Haowei Yan <g1042620637@gmail.com> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com> --- include/net/sock.h | 2 +- net/l2tp/l2tp_core.c | 17 +++++++++++------ 2 files changed, 12 insertions(+), 7 deletions(-)