| Message ID | 20220810102848.282778-1-jakub@cloudflare.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] l2tp: Serialize access to sk_user_data with sock lock | expand |
On Wed, 10 Aug 2022 12:28:48 +0200 Jakub Sitnicki wrote: > Fixes: fd558d186df2 ("l2tp: Split pppol2tp patch into separate l2tp and ppp parts") That tag immediately sets off red flags. Please find the commit where to code originates, not where it was last moved. > Reported-by: van fantasy <g1042620637@gmail.com> > Tested-by: van fantasy <g1042620637@gmail.com> Can we get real names? Otherwise let's just drop those tags. I know that the legal name requirement is only for S-o-b tags, technically, but it feels silly.
On Thu, Aug 11, 2022 at 10:23 AM -07, Jakub Kicinski wrote: > On Wed, 10 Aug 2022 12:28:48 +0200 Jakub Sitnicki wrote: >> Fixes: fd558d186df2 ("l2tp: Split pppol2tp patch into separate l2tp and ppp parts") > > That tag immediately sets off red flags. Please find the commit where > to code originates, not where it was last moved. The code move happened in v2.6.35. There's no point in digging further, IMHO. > >> Reported-by: van fantasy <g1042620637@gmail.com> >> Tested-by: van fantasy <g1042620637@gmail.com> > > Can we get real names? Otherwise let's just drop those tags. > I know that the legal name requirement is only for S-o-b tags, > technically, but it feels silly. I don't make the rules. There is already a precendent in the git log: commit 5c835bb142d4013c2ab24bff5ae9f6709a39cbcf Author: Paolo Abeni <pabeni@redhat.com> Date: Fri Jul 8 16:36:09 2022 -0700 mptcp: fix subflow traversal at disconnect time At disconnect time the MPTCP protocol traverse the subflows list closing each of them. In some circumstances - MPJ subflow, passive MPTCP socket, the latter operation can remove the subflow from the list, invalidating the current iterator. Address the issue using the safe list traversing helper variant. Reported-by: van fantasy <g1042620637@gmail.com> Fixes: b29fcfb54cd7 ("mptcp: full disconnect implementation") Tested-by: Matthieu Baerts <matthieu.baerts@tessares.net> Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com> Signed-off-by: Paolo Abeni <pabeni@redhat.com> Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com> Signed-off-by: David S. Miller <davem@davemloft.net>
On Fri, 12 Aug 2022 11:54:43 +0200 Jakub Sitnicki wrote: > On Thu, Aug 11, 2022 at 10:23 AM -07, Jakub Kicinski wrote: > > On Wed, 10 Aug 2022 12:28:48 +0200 Jakub Sitnicki wrote: > >> Fixes: fd558d186df2 ("l2tp: Split pppol2tp patch into separate l2tp and ppp parts") > > > > That tag immediately sets off red flags. Please find the commit where > > to code originates, not where it was last moved. > > The code move happened in v2.6.35. There's no point in digging further, IMHO. We can discuss a new "fixes-all-stable-trees" tag but until then let's just stick to the existing rules. As luck would have it in this case I think the tag is actually correct, AFAICT the socket _was_ locked before the code move / refactoring? > >> Reported-by: van fantasy <g1042620637@gmail.com> > >> Tested-by: van fantasy <g1042620637@gmail.com> > > > > Can we get real names? Otherwise let's just drop those tags. > > I know that the legal name requirement is only for S-o-b tags, > > technically, but it feels silly. > > I don't make the rules. There is already a precendent in the git log: Ack, I'm aware, that's why I complained. If it's a single case meh, but Haowei Yan seems to be quite prolific in finding bugs so switching to the real name is preferable. Thanks!
On Fri, Aug 12, 2022 at 11:54:43 +0200, Jakub Sitnicki wrote: > On Thu, Aug 11, 2022 at 10:23 AM -07, Jakub Kicinski wrote: > > On Wed, 10 Aug 2022 12:28:48 +0200 Jakub Sitnicki wrote: > >> Fixes: fd558d186df2 ("l2tp: Split pppol2tp patch into separate l2tp and ppp parts") > > > > That tag immediately sets off red flags. Please find the commit where > > to code originates, not where it was last moved. > > The code move happened in v2.6.35. There's no point in digging further, IMHO. At the time of fd558d186df2, sk_user_data was checked/set by the newly added function l2tp_tunnel_create. The only callpath for l2tp_tunnel_create was via. pppol2tp_connect which called l2tp_tunnel_create with lock_sock held (and indeed still does). I think the addition of the netlink API (which added a new callpath for l2tp_tunnel_create via. l2tp_nl_cmd_tunnel_create which was *not* lock_sock-protected) is perhaps the right commit to point to? 309795f4bec2 ("l2tp: Add netlink control API for L2TP")
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. Fixes: fd558d186df2 ("l2tp: Split pppol2tp patch into separate l2tp and ppp parts") Reported-by: van fantasy <g1042620637@gmail.com> Tested-by: van fantasy <g1042620637@gmail.com> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com> --- net/l2tp/l2tp_core.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)