| Message ID | 20220815130107.149345-1-jakub@cloudflare.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] l2tp: Serialize access to sk_user_data with sock lock | expand |
On Mon, Aug 15, 2022 at 15:01:07 +0200, Jakub Sitnicki wrote: > sk->sk_user_data has multiple users, which are not compatible with each > other. To synchronize the users, any check-if-unused-and-set access to the > pointer has to happen with sock lock held. > > 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. > > 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") This still seems wrong to me. In 3557baabf280 pppol2tp_connect checks/sets sk_user_data with lock_sock held. > Reported-by: Haowei Yan <g1042620637@gmail.com> > Tested-by: Haowei Yan <g1042620637@gmail.com> > Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com> > --- > Cc: Tom Parkin <tparkin@katalix.com> > > net/l2tp/l2tp_core.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c > index 7499c51b1850..9f5f86bfc395 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; > + lock_sock(sk); > + > + 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); > > + release_sock(sk); > 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); > + > + release_sock(sk); > err: > return ret; > } > -- > 2.35.3 >
On Mon, Aug 15, 2022 at 02:21 PM +01, Tom Parkin wrote: > [[PGP Signed Part:Undecided]] > On Mon, Aug 15, 2022 at 15:01:07 +0200, Jakub Sitnicki wrote: >> sk->sk_user_data has multiple users, which are not compatible with each >> other. To synchronize the users, any check-if-unused-and-set access to the >> pointer has to happen with sock lock held. >> >> 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. >> >> 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") > > This still seems wrong to me. > > In 3557baabf280 pppol2tp_connect checks/sets sk_user_data with > lock_sock held. I think you are referring to the PPP-over-L2TP socket, not the UDP socket. In pppol2tp_prepare_tunnel_socket() @ 3557baabf280 we're not holding the sock lock over the UDP socket, AFAICT. > >> Reported-by: Haowei Yan <g1042620637@gmail.com> >> Tested-by: Haowei Yan <g1042620637@gmail.com> >> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com> >> --- >> Cc: Tom Parkin <tparkin@katalix.com> >> >> net/l2tp/l2tp_core.c | 17 +++++++++++------ >> 1 file changed, 11 insertions(+), 6 deletions(-) >> >> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c >> index 7499c51b1850..9f5f86bfc395 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; >> + lock_sock(sk); >> + >> + 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); >> >> + release_sock(sk); >> 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); >> + >> + release_sock(sk); >> err: >> return ret; >> } >> -- >> 2.35.3 >>
On Mon, Aug 15, 2022 at 15:26:51 +0200, Jakub Sitnicki wrote: > On Mon, Aug 15, 2022 at 02:21 PM +01, Tom Parkin wrote: > > [[PGP Signed Part:Undecided]] > > On Mon, Aug 15, 2022 at 15:01:07 +0200, Jakub Sitnicki wrote: > >> sk->sk_user_data has multiple users, which are not compatible with each > >> other. To synchronize the users, any check-if-unused-and-set access to the > >> pointer has to happen with sock lock held. > >> > >> 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. > >> > >> 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") > > > > This still seems wrong to me. > > > > In 3557baabf280 pppol2tp_connect checks/sets sk_user_data with > > lock_sock held. > > I think you are referring to the PPP-over-L2TP socket, not the UDP > socket. In pppol2tp_prepare_tunnel_socket() @ 3557baabf280 we're not > holding the sock lock over the UDP socket, AFAICT. Yes, you're quite right -- my apologies. > > > >> Reported-by: Haowei Yan <g1042620637@gmail.com> > >> Tested-by: Haowei Yan <g1042620637@gmail.com> > >> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com> > >> --- > >> Cc: Tom Parkin <tparkin@katalix.com> > >> > >> net/l2tp/l2tp_core.c | 17 +++++++++++------ > >> 1 file changed, 11 insertions(+), 6 deletions(-) > >> > >> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c > >> index 7499c51b1850..9f5f86bfc395 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; > >> + lock_sock(sk); > >> + > >> + 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); > >> > >> + release_sock(sk); > >> 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); > >> + > >> + release_sock(sk); > >> err: > >> return ret; > >> } > >> -- > >> 2.35.3 > >> >
On Mon, 15 Aug 2022 15:01:07 +0200 Jakub Sitnicki wrote: > sk->sk_user_data has multiple users, which are not compatible with each > other. To synchronize the users, any check-if-unused-and-set access to the > pointer has to happen with sock lock held. > > 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. Note to other netdev maintainers that based on the discussion about the reuseport locking it's unclear whether we shouldn't also take the callback lock...
On Tue, Aug 16, 2022 at 06:41 PM -07, Jakub Kicinski wrote: > On Mon, 15 Aug 2022 15:01:07 +0200 Jakub Sitnicki wrote: >> sk->sk_user_data has multiple users, which are not compatible with each >> other. To synchronize the users, any check-if-unused-and-set access to the >> pointer has to happen with sock lock held. >> >> 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. > > Note to other netdev maintainers that based on the discussion about > the reuseport locking it's unclear whether we shouldn't also take > the callback lock... You're right. reuseport_array, psock, and kcm protect sk_user_data with the callback lock, not the sock lock. Need to fix it.
On Wed, 17 Aug 2022 16:33:33 +0200 Jakub Sitnicki wrote: > > Note to other netdev maintainers that based on the discussion about > > the reuseport locking it's unclear whether we shouldn't also take > > the callback lock... > > You're right. reuseport_array, psock, and kcm protect sk_user_data with > the callback lock, not the sock lock. Need to fix it. Where 'it' == current patch? Would you mind adding to the kdoc on sk_user_data that it's protected by the callback lock while at it?
On Wed, Aug 17, 2022 at 08:51 AM -07, Jakub Kicinski wrote: > On Wed, 17 Aug 2022 16:33:33 +0200 Jakub Sitnicki wrote: >> > Note to other netdev maintainers that based on the discussion about >> > the reuseport locking it's unclear whether we shouldn't also take >> > the callback lock... >> >> You're right. reuseport_array, psock, and kcm protect sk_user_data with >> the callback lock, not the sock lock. Need to fix it. > > Where 'it' == current patch? Would you mind adding to the kdoc on > sk_user_data that it's protected by the callback lock while at it? Yes, will prepare a v3 for review. Sorry, should have been explicit. Will add a kdoc. Great idea.
diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c index 7499c51b1850..9f5f86bfc395 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; + lock_sock(sk); + + 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); + release_sock(sk); 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); + + release_sock(sk); err: return ret; }
sk->sk_user_data has multiple users, which are not compatible with each other. To synchronize the users, any check-if-unused-and-set access to the pointer has to happen with sock lock held. 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. 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> Tested-by: Haowei Yan <g1042620637@gmail.com> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com> --- Cc: Tom Parkin <tparkin@katalix.com> net/l2tp/l2tp_core.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)