* [PATCH 01/27] l2tp: lock socket before checking flags in connect()
2020-05-21 23:57 [PATCH 00/27] more l2tp locking and ordering fixes Giuliano Procida
@ 2020-05-21 23:57 ` Giuliano Procida
2020-05-21 23:57 ` [PATCH 02/27] l2tp: fix racy socket lookup in l2tp_ip and l2tp_ip6 bind() Giuliano Procida
` (26 subsequent siblings)
27 siblings, 0 replies; 30+ messages in thread
From: Giuliano Procida @ 2020-05-21 23:57 UTC (permalink / raw)
To: greg; +Cc: stable, Guillaume Nault, David S . Miller, Giuliano Procida
From: Guillaume Nault <g.nault@alphalink.fr>
commit 0382a25af3c771a8e4d5e417d1834cbe28c2aaac upstream.
Socket flags aren't updated atomically, so the socket must be locked
while reading the SOCK_ZAPPED flag.
This issue exists for both l2tp_ip and l2tp_ip6. For IPv6, this patch
also brings error handling for __ip6_datagram_connect() failures.
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
include/net/ipv6.h | 2 ++
net/ipv6/datagram.c | 4 +++-
net/l2tp/l2tp_ip.c | 19 ++++++++++++-------
net/l2tp/l2tp_ip6.c | 16 +++++++++++-----
4 files changed, 28 insertions(+), 13 deletions(-)
diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 6258264a0bf7..94880f07bc06 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -915,6 +915,8 @@ int compat_ipv6_setsockopt(struct sock *sk, int level, int optname,
int compat_ipv6_getsockopt(struct sock *sk, int level, int optname,
char __user *optval, int __user *optlen);
+int __ip6_datagram_connect(struct sock *sk, struct sockaddr *addr,
+ int addr_len);
int ip6_datagram_connect(struct sock *sk, struct sockaddr *addr, int addr_len);
int ip6_datagram_connect_v6_only(struct sock *sk, struct sockaddr *addr,
int addr_len);
diff --git a/net/ipv6/datagram.c b/net/ipv6/datagram.c
index f33154365b64..389b6367a810 100644
--- a/net/ipv6/datagram.c
+++ b/net/ipv6/datagram.c
@@ -40,7 +40,8 @@ static bool ipv6_mapped_addr_any(const struct in6_addr *a)
return ipv6_addr_v4mapped(a) && (a->s6_addr32[3] == 0);
}
-static int __ip6_datagram_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
+int __ip6_datagram_connect(struct sock *sk, struct sockaddr *uaddr,
+ int addr_len)
{
struct sockaddr_in6 *usin = (struct sockaddr_in6 *) uaddr;
struct inet_sock *inet = inet_sk(sk);
@@ -213,6 +214,7 @@ out:
fl6_sock_release(flowlabel);
return err;
}
+EXPORT_SYMBOL_GPL(__ip6_datagram_connect);
int ip6_datagram_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len)
{
diff --git a/net/l2tp/l2tp_ip.c b/net/l2tp/l2tp_ip.c
index 58f87bdd12c7..fab122cc6aac 100644
--- a/net/l2tp/l2tp_ip.c
+++ b/net/l2tp/l2tp_ip.c
@@ -321,21 +321,24 @@ static int l2tp_ip_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len
struct sockaddr_l2tpip *lsa = (struct sockaddr_l2tpip *) uaddr;
int rc;
- if (sock_flag(sk, SOCK_ZAPPED)) /* Must bind first - autobinding does not work */
- return -EINVAL;
-
if (addr_len < sizeof(*lsa))
return -EINVAL;
if (ipv4_is_multicast(lsa->l2tp_addr.s_addr))
return -EINVAL;
- rc = ip4_datagram_connect(sk, uaddr, addr_len);
- if (rc < 0)
- return rc;
-
lock_sock(sk);
+ /* Must bind first - autobinding does not work */
+ if (sock_flag(sk, SOCK_ZAPPED)) {
+ rc = -EINVAL;
+ goto out_sk;
+ }
+
+ rc = __ip4_datagram_connect(sk, uaddr, addr_len);
+ if (rc < 0)
+ goto out_sk;
+
l2tp_ip_sk(sk)->peer_conn_id = lsa->l2tp_conn_id;
write_lock_bh(&l2tp_ip_lock);
@@ -343,7 +346,9 @@ static int l2tp_ip_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len
sk_add_bind_node(sk, &l2tp_ip_bind_table);
write_unlock_bh(&l2tp_ip_lock);
+out_sk:
release_sock(sk);
+
return rc;
}
diff --git a/net/l2tp/l2tp_ip6.c b/net/l2tp/l2tp_ip6.c
index 2b5230ef8536..59e609f2db64 100644
--- a/net/l2tp/l2tp_ip6.c
+++ b/net/l2tp/l2tp_ip6.c
@@ -383,9 +383,6 @@ static int l2tp_ip6_connect(struct sock *sk, struct sockaddr *uaddr,
int addr_type;
int rc;
- if (sock_flag(sk, SOCK_ZAPPED)) /* Must bind first - autobinding does not work */
- return -EINVAL;
-
if (addr_len < sizeof(*lsa))
return -EINVAL;
@@ -402,10 +399,18 @@ static int l2tp_ip6_connect(struct sock *sk, struct sockaddr *uaddr,
return -EINVAL;
}
- rc = ip6_datagram_connect(sk, uaddr, addr_len);
-
lock_sock(sk);
+ /* Must bind first - autobinding does not work */
+ if (sock_flag(sk, SOCK_ZAPPED)) {
+ rc = -EINVAL;
+ goto out_sk;
+ }
+
+ rc = __ip6_datagram_connect(sk, uaddr, addr_len);
+ if (rc < 0)
+ goto out_sk;
+
l2tp_ip6_sk(sk)->peer_conn_id = lsa->l2tp_conn_id;
write_lock_bh(&l2tp_ip6_lock);
@@ -413,6 +418,7 @@ static int l2tp_ip6_connect(struct sock *sk, struct sockaddr *uaddr,
sk_add_bind_node(sk, &l2tp_ip6_bind_table);
write_unlock_bh(&l2tp_ip6_lock);
+out_sk:
release_sock(sk);
return rc;
--
2.27.0.rc0.183.gde8f92d652-goog
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 02/27] l2tp: fix racy socket lookup in l2tp_ip and l2tp_ip6 bind()
2020-05-21 23:57 [PATCH 00/27] more l2tp locking and ordering fixes Giuliano Procida
2020-05-21 23:57 ` [PATCH 01/27] l2tp: lock socket before checking flags in connect() Giuliano Procida
@ 2020-05-21 23:57 ` Giuliano Procida
2020-05-21 23:57 ` [PATCH 03/27] l2tp: hold session while sending creation notifications Giuliano Procida
` (25 subsequent siblings)
27 siblings, 0 replies; 30+ messages in thread
From: Giuliano Procida @ 2020-05-21 23:57 UTC (permalink / raw)
To: greg; +Cc: stable, Guillaume Nault, David S . Miller, Giuliano Procida
From: Guillaume Nault <g.nault@alphalink.fr>
commit d5e3a190937a1e386671266202c62565741f0f1a upstream.
It's not enough to check for sockets bound to same address at the
beginning of l2tp_ip{,6}_bind(): even if no socket is found at that
time, a socket with the same address could be bound before we take
the l2tp lock again.
This patch moves the lookup right before inserting the new socket, so
that no change can ever happen to the list between address lookup and
socket insertion.
Care is taken to avoid side effects on the socket in case of failure.
That is, modifications of the socket are done after the lookup, when
binding is guaranteed to succeed, and before releasing the l2tp lock,
so that concurrent lookups will always see fully initialised sockets.
For l2tp_ip, 'ret' is set to -EINVAL before checking the SOCK_ZAPPED
bit. Error code was mistakenly set to -EADDRINUSE on error by commit
32c231164b76 ("l2tp: fix racy SOCK_ZAPPED flag check in l2tp_ip{,6}_bind()").
Using -EINVAL restores original behaviour.
For l2tp_ip6, the lookup is now always done with the correct bound
device. Before this patch, when binding to a link-local address, the
lookup was done with the original sk->sk_bound_dev_if, which was later
overwritten with addr->l2tp_scope_id. Lookup is now performed with the
final sk->sk_bound_dev_if value.
Finally, the (addr_len >= sizeof(struct sockaddr_in6)) check has been
dropped: addr is a sockaddr_l2tpip6 not sockaddr_in6 and addr_len has
already been checked at this point (this part of the code seems to have
been copy-pasted from net/ipv6/raw.c).
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
net/l2tp/l2tp_ip.c | 27 ++++++++++++---------------
net/l2tp/l2tp_ip6.c | 43 ++++++++++++++++++++-----------------------
2 files changed, 32 insertions(+), 38 deletions(-)
diff --git a/net/l2tp/l2tp_ip.c b/net/l2tp/l2tp_ip.c
index fab122cc6aac..2a77732c6496 100644
--- a/net/l2tp/l2tp_ip.c
+++ b/net/l2tp/l2tp_ip.c
@@ -269,15 +269,9 @@ static int l2tp_ip_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len)
if (addr->l2tp_family != AF_INET)
return -EINVAL;
- ret = -EADDRINUSE;
- read_lock_bh(&l2tp_ip_lock);
- if (__l2tp_ip_bind_lookup(net, addr->l2tp_addr.s_addr,
- sk->sk_bound_dev_if, addr->l2tp_conn_id))
- goto out_in_use;
-
- read_unlock_bh(&l2tp_ip_lock);
-
lock_sock(sk);
+
+ ret = -EINVAL;
if (!sock_flag(sk, SOCK_ZAPPED))
goto out;
@@ -294,25 +288,28 @@ static int l2tp_ip_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len)
inet->inet_rcv_saddr = inet->inet_saddr = addr->l2tp_addr.s_addr;
if (chk_addr_ret == RTN_MULTICAST || chk_addr_ret == RTN_BROADCAST)
inet->inet_saddr = 0; /* Use device */
- sk_dst_reset(sk);
+ write_lock_bh(&l2tp_ip_lock);
+ if (__l2tp_ip_bind_lookup(net, addr->l2tp_addr.s_addr,
+ sk->sk_bound_dev_if, addr->l2tp_conn_id)) {
+ write_unlock_bh(&l2tp_ip_lock);
+ ret = -EADDRINUSE;
+ goto out;
+ }
+
+ sk_dst_reset(sk);
l2tp_ip_sk(sk)->conn_id = addr->l2tp_conn_id;
- write_lock_bh(&l2tp_ip_lock);
sk_add_bind_node(sk, &l2tp_ip_bind_table);
sk_del_node_init(sk);
write_unlock_bh(&l2tp_ip_lock);
+
ret = 0;
sock_reset_flag(sk, SOCK_ZAPPED);
out:
release_sock(sk);
- return ret;
-
-out_in_use:
- read_unlock_bh(&l2tp_ip_lock);
-
return ret;
}
diff --git a/net/l2tp/l2tp_ip6.c b/net/l2tp/l2tp_ip6.c
index 59e609f2db64..4d4561dd4023 100644
--- a/net/l2tp/l2tp_ip6.c
+++ b/net/l2tp/l2tp_ip6.c
@@ -278,6 +278,7 @@ static int l2tp_ip6_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len)
struct sockaddr_l2tpip6 *addr = (struct sockaddr_l2tpip6 *) uaddr;
struct net *net = sock_net(sk);
__be32 v4addr = 0;
+ int bound_dev_if;
int addr_type;
int err;
@@ -296,13 +297,6 @@ static int l2tp_ip6_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len)
if (addr_type & IPV6_ADDR_MULTICAST)
return -EADDRNOTAVAIL;
- err = -EADDRINUSE;
- read_lock_bh(&l2tp_ip6_lock);
- if (__l2tp_ip6_bind_lookup(net, &addr->l2tp_addr,
- sk->sk_bound_dev_if, addr->l2tp_conn_id))
- goto out_in_use;
- read_unlock_bh(&l2tp_ip6_lock);
-
lock_sock(sk);
err = -EINVAL;
@@ -312,28 +306,25 @@ static int l2tp_ip6_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len)
if (sk->sk_state != TCP_CLOSE)
goto out_unlock;
+ bound_dev_if = sk->sk_bound_dev_if;
+
/* Check if the address belongs to the host. */
rcu_read_lock();
if (addr_type != IPV6_ADDR_ANY) {
struct net_device *dev = NULL;
if (addr_type & IPV6_ADDR_LINKLOCAL) {
- if (addr_len >= sizeof(struct sockaddr_in6) &&
- addr->l2tp_scope_id) {
- /* Override any existing binding, if another
- * one is supplied by user.
- */
- sk->sk_bound_dev_if = addr->l2tp_scope_id;
- }
+ if (addr->l2tp_scope_id)
+ bound_dev_if = addr->l2tp_scope_id;
/* Binding to link-local address requires an
- interface */
- if (!sk->sk_bound_dev_if)
+ * interface.
+ */
+ if (!bound_dev_if)
goto out_unlock_rcu;
err = -ENODEV;
- dev = dev_get_by_index_rcu(sock_net(sk),
- sk->sk_bound_dev_if);
+ dev = dev_get_by_index_rcu(sock_net(sk), bound_dev_if);
if (!dev)
goto out_unlock_rcu;
}
@@ -348,13 +339,22 @@ static int l2tp_ip6_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len)
}
rcu_read_unlock();
- inet->inet_rcv_saddr = inet->inet_saddr = v4addr;
+ write_lock_bh(&l2tp_ip6_lock);
+ if (__l2tp_ip6_bind_lookup(net, &addr->l2tp_addr, bound_dev_if,
+ addr->l2tp_conn_id)) {
+ write_unlock_bh(&l2tp_ip6_lock);
+ err = -EADDRINUSE;
+ goto out_unlock;
+ }
+
+ inet->inet_saddr = v4addr;
+ inet->inet_rcv_saddr = v4addr;
+ sk->sk_bound_dev_if = bound_dev_if;
sk->sk_v6_rcv_saddr = addr->l2tp_addr;
np->saddr = addr->l2tp_addr;
l2tp_ip6_sk(sk)->conn_id = addr->l2tp_conn_id;
- write_lock_bh(&l2tp_ip6_lock);
sk_add_bind_node(sk, &l2tp_ip6_bind_table);
sk_del_node_init(sk);
write_unlock_bh(&l2tp_ip6_lock);
@@ -367,10 +367,7 @@ out_unlock_rcu:
rcu_read_unlock();
out_unlock:
release_sock(sk);
- return err;
-out_in_use:
- read_unlock_bh(&l2tp_ip6_lock);
return err;
}
--
2.27.0.rc0.183.gde8f92d652-goog
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 03/27] l2tp: hold session while sending creation notifications
2020-05-21 23:57 [PATCH 00/27] more l2tp locking and ordering fixes Giuliano Procida
2020-05-21 23:57 ` [PATCH 01/27] l2tp: lock socket before checking flags in connect() Giuliano Procida
2020-05-21 23:57 ` [PATCH 02/27] l2tp: fix racy socket lookup in l2tp_ip and l2tp_ip6 bind() Giuliano Procida
@ 2020-05-21 23:57 ` Giuliano Procida
2020-05-21 23:57 ` [PATCH 04/27] l2tp: take a reference on sessions used in genetlink handlers Giuliano Procida
` (24 subsequent siblings)
27 siblings, 0 replies; 30+ messages in thread
From: Giuliano Procida @ 2020-05-21 23:57 UTC (permalink / raw)
To: greg
Cc: stable, Guillaume Nault, David S . Miller, Amit Pundir,
Greg Kroah-Hartman, Giuliano Procida
From: Guillaume Nault <g.nault@alphalink.fr>
commit 5e6a9e5a3554a5b3db09cdc22253af1849c65dff upstream.
l2tp_session_find() doesn't take any reference on the returned session.
Therefore, the session may disappear while sending the notification.
Use l2tp_session_get() instead and decrement session's refcount once
the notification is sent.
Backporting Notes
This is a backport of a backport.
Fixes: 33f72e6f0c67 ("l2tp : multicast notification to the registered listeners")
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Amit Pundir <amit.pundir@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
net/l2tp/l2tp_netlink.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index fb3248ff8b48..43f0af1c4697 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -626,10 +626,12 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
session_id, peer_session_id, &cfg);
if (ret >= 0) {
- session = l2tp_session_find(net, tunnel, session_id);
- if (session)
+ session = l2tp_session_get(net, tunnel, session_id, false);
+ if (session) {
ret = l2tp_session_notify(&l2tp_nl_family, info, session,
L2TP_CMD_SESSION_CREATE);
+ l2tp_session_dec_refcount(session);
+ }
}
out:
--
2.27.0.rc0.183.gde8f92d652-goog
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 04/27] l2tp: take a reference on sessions used in genetlink handlers
2020-05-21 23:57 [PATCH 00/27] more l2tp locking and ordering fixes Giuliano Procida
` (2 preceding siblings ...)
2020-05-21 23:57 ` [PATCH 03/27] l2tp: hold session while sending creation notifications Giuliano Procida
@ 2020-05-21 23:57 ` Giuliano Procida
2020-05-21 23:57 ` [PATCH 05/27] l2tp: don't use l2tp_tunnel_find() in l2tp_ip and l2tp_ip6 Giuliano Procida
` (23 subsequent siblings)
27 siblings, 0 replies; 30+ messages in thread
From: Giuliano Procida @ 2020-05-21 23:57 UTC (permalink / raw)
To: greg
Cc: stable, Guillaume Nault, David S . Miller, Amit Pundir,
Greg Kroah-Hartman, Giuliano Procida
From: Guillaume Nault <g.nault@alphalink.fr>
commit 2777e2ab5a9cf2b4524486c6db1517a6ded25261 upstream.
Callers of l2tp_nl_session_find() need to hold a reference on the
returned session since there's no guarantee that it isn't going to
disappear from under them.
Relying on the fact that no l2tp netlink message may be processed
concurrently isn't enough: sessions can be deleted by other means
(e.g. by closing the PPPOL2TP socket of a ppp pseudowire).
l2tp_nl_cmd_session_delete() is a bit special: it runs a callback
function that may require a previous call to session->ref(). In
particular, for ppp pseudowires, the callback is l2tp_session_delete(),
which then calls pppol2tp_session_close() and dereferences the PPPOL2TP
socket. The socket might already be gone at the moment
l2tp_session_delete() calls session->ref(), so we need to take a
reference during the session lookup. So we need to pass the do_ref
variable down to l2tp_session_get() and l2tp_session_get_by_ifname().
Since all callers have to be updated, l2tp_session_find_by_ifname() and
l2tp_nl_session_find() are renamed to reflect their new behaviour.
Fixes: 309795f4bec2 ("l2tp: Add netlink control API for L2TP")
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Amit Pundir <amit.pundir@linaro.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
net/l2tp/l2tp_core.c | 9 +++++++--
net/l2tp/l2tp_core.h | 3 ++-
net/l2tp/l2tp_netlink.c | 39 ++++++++++++++++++++++++++-------------
3 files changed, 35 insertions(+), 16 deletions(-)
diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 8cbccddc0b1e..08377af93552 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -355,7 +355,8 @@ EXPORT_SYMBOL_GPL(l2tp_session_get_nth);
/* Lookup a session by interface name.
* This is very inefficient but is only used by management interfaces.
*/
-struct l2tp_session *l2tp_session_find_by_ifname(struct net *net, char *ifname)
+struct l2tp_session *l2tp_session_get_by_ifname(struct net *net, char *ifname,
+ bool do_ref)
{
struct l2tp_net *pn = l2tp_pernet(net);
int hash;
@@ -365,7 +366,11 @@ struct l2tp_session *l2tp_session_find_by_ifname(struct net *net, char *ifname)
for (hash = 0; hash < L2TP_HASH_SIZE_2; hash++) {
hlist_for_each_entry_rcu(session, &pn->l2tp_session_hlist[hash], global_hlist) {
if (!strcmp(session->ifname, ifname)) {
+ l2tp_session_inc_refcount(session);
+ if (do_ref && session->ref)
+ session->ref(session);
rcu_read_unlock_bh();
+
return session;
}
}
@@ -375,7 +380,7 @@ struct l2tp_session *l2tp_session_find_by_ifname(struct net *net, char *ifname)
return NULL;
}
-EXPORT_SYMBOL_GPL(l2tp_session_find_by_ifname);
+EXPORT_SYMBOL_GPL(l2tp_session_get_by_ifname);
static int l2tp_session_add_to_tunnel(struct l2tp_tunnel *tunnel,
struct l2tp_session *session)
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index 06323a12d62c..ee59d9961d1f 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -252,7 +252,8 @@ struct l2tp_session *l2tp_session_find(struct net *net,
u32 session_id);
struct l2tp_session *l2tp_session_get_nth(struct l2tp_tunnel *tunnel, int nth,
bool do_ref);
-struct l2tp_session *l2tp_session_find_by_ifname(struct net *net, char *ifname);
+struct l2tp_session *l2tp_session_get_by_ifname(struct net *net, char *ifname,
+ bool do_ref);
struct l2tp_tunnel *l2tp_tunnel_find(struct net *net, u32 tunnel_id);
struct l2tp_tunnel *l2tp_tunnel_find_nth(struct net *net, int nth);
diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index 43f0af1c4697..63a2430ff40a 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -55,7 +55,8 @@ static int l2tp_nl_session_send(struct sk_buff *skb, u32 portid, u32 seq,
/* Accessed under genl lock */
static const struct l2tp_nl_cmd_ops *l2tp_nl_cmd_ops[__L2TP_PWTYPE_MAX];
-static struct l2tp_session *l2tp_nl_session_find(struct genl_info *info)
+static struct l2tp_session *l2tp_nl_session_get(struct genl_info *info,
+ bool do_ref)
{
u32 tunnel_id;
u32 session_id;
@@ -66,14 +67,15 @@ static struct l2tp_session *l2tp_nl_session_find(struct genl_info *info)
if (info->attrs[L2TP_ATTR_IFNAME]) {
ifname = nla_data(info->attrs[L2TP_ATTR_IFNAME]);
- session = l2tp_session_find_by_ifname(net, ifname);
+ session = l2tp_session_get_by_ifname(net, ifname, do_ref);
} else if ((info->attrs[L2TP_ATTR_SESSION_ID]) &&
(info->attrs[L2TP_ATTR_CONN_ID])) {
tunnel_id = nla_get_u32(info->attrs[L2TP_ATTR_CONN_ID]);
session_id = nla_get_u32(info->attrs[L2TP_ATTR_SESSION_ID]);
tunnel = l2tp_tunnel_find(net, tunnel_id);
if (tunnel)
- session = l2tp_session_find(net, tunnel, session_id);
+ session = l2tp_session_get(net, tunnel, session_id,
+ do_ref);
}
return session;
@@ -644,7 +646,7 @@ static int l2tp_nl_cmd_session_delete(struct sk_buff *skb, struct genl_info *inf
struct l2tp_session *session;
u16 pw_type;
- session = l2tp_nl_session_find(info);
+ session = l2tp_nl_session_get(info, true);
if (session == NULL) {
ret = -ENODEV;
goto out;
@@ -658,6 +660,10 @@ static int l2tp_nl_cmd_session_delete(struct sk_buff *skb, struct genl_info *inf
if (l2tp_nl_cmd_ops[pw_type] && l2tp_nl_cmd_ops[pw_type]->session_delete)
ret = (*l2tp_nl_cmd_ops[pw_type]->session_delete)(session);
+ if (session->deref)
+ session->deref(session);
+ l2tp_session_dec_refcount(session);
+
out:
return ret;
}
@@ -667,7 +673,7 @@ static int l2tp_nl_cmd_session_modify(struct sk_buff *skb, struct genl_info *inf
int ret = 0;
struct l2tp_session *session;
- session = l2tp_nl_session_find(info);
+ session = l2tp_nl_session_get(info, false);
if (session == NULL) {
ret = -ENODEV;
goto out;
@@ -702,6 +708,8 @@ static int l2tp_nl_cmd_session_modify(struct sk_buff *skb, struct genl_info *inf
ret = l2tp_session_notify(&l2tp_nl_family, info,
session, L2TP_CMD_SESSION_MODIFY);
+ l2tp_session_dec_refcount(session);
+
out:
return ret;
}
@@ -788,29 +796,34 @@ static int l2tp_nl_cmd_session_get(struct sk_buff *skb, struct genl_info *info)
struct sk_buff *msg;
int ret;
- session = l2tp_nl_session_find(info);
+ session = l2tp_nl_session_get(info, false);
if (session == NULL) {
ret = -ENODEV;
- goto out;
+ goto err;
}
msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
if (!msg) {
ret = -ENOMEM;
- goto out;
+ goto err_ref;
}
ret = l2tp_nl_session_send(msg, info->snd_portid, info->snd_seq,
0, session, L2TP_CMD_SESSION_GET);
if (ret < 0)
- goto err_out;
+ goto err_ref_msg;
- return genlmsg_unicast(genl_info_net(info), msg, info->snd_portid);
+ ret = genlmsg_unicast(genl_info_net(info), msg, info->snd_portid);
-err_out:
- nlmsg_free(msg);
+ l2tp_session_dec_refcount(session);
-out:
+ return ret;
+
+err_ref_msg:
+ nlmsg_free(msg);
+err_ref:
+ l2tp_session_dec_refcount(session);
+err:
return ret;
}
--
2.27.0.rc0.183.gde8f92d652-goog
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 05/27] l2tp: don't use l2tp_tunnel_find() in l2tp_ip and l2tp_ip6
2020-05-21 23:57 [PATCH 00/27] more l2tp locking and ordering fixes Giuliano Procida
` (3 preceding siblings ...)
2020-05-21 23:57 ` [PATCH 04/27] l2tp: take a reference on sessions used in genetlink handlers Giuliano Procida
@ 2020-05-21 23:57 ` Giuliano Procida
2020-05-21 23:57 ` [PATCH 06/27] net: l2tp: export debug flags to UAPI Giuliano Procida
` (22 subsequent siblings)
27 siblings, 0 replies; 30+ messages in thread
From: Giuliano Procida @ 2020-05-21 23:57 UTC (permalink / raw)
To: greg
Cc: stable, Guillaume Nault, David S . Miller, Nicolas Schier,
Greg Kroah-Hartman, Giuliano Procida
From: Guillaume Nault <g.nault@alphalink.fr>
commit 8f7dc9ae4a7aece9fbc3e6637bdfa38b36bcdf09 upstream.
Using l2tp_tunnel_find() in l2tp_ip_recv() is wrong for two reasons:
* It doesn't take a reference on the returned tunnel, which makes the
call racy wrt. concurrent tunnel deletion.
* The lookup is only based on the tunnel identifier, so it can return
a tunnel that doesn't match the packet's addresses or protocol.
For example, a packet sent to an L2TPv3 over IPv6 tunnel can be
delivered to an L2TPv2 over UDPv4 tunnel. This is worse than a simple
cross-talk: when delivering the packet to an L2TP over UDP tunnel, the
corresponding socket is UDP, where ->sk_backlog_rcv() is NULL. Calling
sk_receive_skb() will then crash the kernel by trying to execute this
callback.
And l2tp_tunnel_find() isn't even needed here. __l2tp_ip_bind_lookup()
properly checks the socket binding and connection settings. It was used
as a fallback mechanism for finding tunnels that didn't have their data
path registered yet. But it's not limited to this case and can be used
to replace l2tp_tunnel_find() in the general case.
Fix l2tp_ip6 in the same way.
Fixes: 0d76751fad77 ("l2tp: Add L2TPv3 IP encapsulation (no UDP) support")
Fixes: a32e0eec7042 ("l2tp: introduce L2TPv3 IP encapsulation support for IPv6")
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
Signed-off-by: David S. Miller <davem@davemloft.net>
Cc: Nicolas Schier <n.schier@avm.de>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
net/l2tp/l2tp_ip.c | 22 ++++++++--------------
net/l2tp/l2tp_ip6.c | 23 ++++++++---------------
2 files changed, 16 insertions(+), 29 deletions(-)
diff --git a/net/l2tp/l2tp_ip.c b/net/l2tp/l2tp_ip.c
index 2a77732c6496..fd7363f8405a 100644
--- a/net/l2tp/l2tp_ip.c
+++ b/net/l2tp/l2tp_ip.c
@@ -122,6 +122,7 @@ static int l2tp_ip_recv(struct sk_buff *skb)
unsigned char *ptr, *optr;
struct l2tp_session *session;
struct l2tp_tunnel *tunnel = NULL;
+ struct iphdr *iph;
int length;
if (!pskb_may_pull(skb, 4))
@@ -180,23 +181,16 @@ pass_up:
goto discard;
tunnel_id = ntohl(*(__be32 *) &skb->data[4]);
- tunnel = l2tp_tunnel_find(net, tunnel_id);
- if (tunnel) {
- sk = tunnel->sock;
- sock_hold(sk);
- } else {
- struct iphdr *iph = (struct iphdr *) skb_network_header(skb);
+ iph = (struct iphdr *)skb_network_header(skb);
- read_lock_bh(&l2tp_ip_lock);
- sk = __l2tp_ip_bind_lookup(net, iph->daddr, 0, tunnel_id);
- if (!sk) {
- read_unlock_bh(&l2tp_ip_lock);
- goto discard;
- }
-
- sock_hold(sk);
+ read_lock_bh(&l2tp_ip_lock);
+ sk = __l2tp_ip_bind_lookup(net, iph->daddr, 0, tunnel_id);
+ if (!sk) {
read_unlock_bh(&l2tp_ip_lock);
+ goto discard;
}
+ sock_hold(sk);
+ read_unlock_bh(&l2tp_ip_lock);
if (!xfrm4_policy_check(sk, XFRM_POLICY_IN, skb))
goto discard_put;
diff --git a/net/l2tp/l2tp_ip6.c b/net/l2tp/l2tp_ip6.c
index 4d4561dd4023..5bb5337e74fc 100644
--- a/net/l2tp/l2tp_ip6.c
+++ b/net/l2tp/l2tp_ip6.c
@@ -134,6 +134,7 @@ static int l2tp_ip6_recv(struct sk_buff *skb)
unsigned char *ptr, *optr;
struct l2tp_session *session;
struct l2tp_tunnel *tunnel = NULL;
+ struct ipv6hdr *iph;
int length;
if (!pskb_may_pull(skb, 4))
@@ -193,24 +194,16 @@ pass_up:
goto discard;
tunnel_id = ntohl(*(__be32 *) &skb->data[4]);
- tunnel = l2tp_tunnel_find(net, tunnel_id);
- if (tunnel) {
- sk = tunnel->sock;
- sock_hold(sk);
- } else {
- struct ipv6hdr *iph = ipv6_hdr(skb);
-
- read_lock_bh(&l2tp_ip6_lock);
- sk = __l2tp_ip6_bind_lookup(net, &iph->daddr,
- 0, tunnel_id);
- if (!sk) {
- read_unlock_bh(&l2tp_ip6_lock);
- goto discard;
- }
+ iph = ipv6_hdr(skb);
- sock_hold(sk);
+ read_lock_bh(&l2tp_ip6_lock);
+ sk = __l2tp_ip6_bind_lookup(net, &iph->daddr, 0, tunnel_id);
+ if (!sk) {
read_unlock_bh(&l2tp_ip6_lock);
+ goto discard;
}
+ sock_hold(sk);
+ read_unlock_bh(&l2tp_ip6_lock);
if (!xfrm6_policy_check(sk, XFRM_POLICY_IN, skb))
goto discard_put;
--
2.27.0.rc0.183.gde8f92d652-goog
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 06/27] net: l2tp: export debug flags to UAPI
2020-05-21 23:57 [PATCH 00/27] more l2tp locking and ordering fixes Giuliano Procida
` (4 preceding siblings ...)
2020-05-21 23:57 ` [PATCH 05/27] l2tp: don't use l2tp_tunnel_find() in l2tp_ip and l2tp_ip6 Giuliano Procida
@ 2020-05-21 23:57 ` Giuliano Procida
2020-05-21 23:57 ` [PATCH 07/27] net: l2tp: deprecate PPPOL2TP_MSG_* in favour of L2TP_MSG_* Giuliano Procida
` (21 subsequent siblings)
27 siblings, 0 replies; 30+ messages in thread
From: Giuliano Procida @ 2020-05-21 23:57 UTC (permalink / raw)
To: greg
Cc: stable, Asbjørn Sloth Tønnesen, David S . Miller,
Giuliano Procida
From: Asbjørn Sloth Tønnesen <asbjorn@asbjorn.st>
commit 41c43fbee68f4f9a2a9675d83bca91c77862d7f0 upstream.
Move the L2TP_MSG_* definitions to UAPI, as it is part of
the netlink API.
Signed-off-by: Asbjoern Sloth Toennesen <asbjorn@asbjorn.st>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
include/uapi/linux/l2tp.h | 17 ++++++++++++++++-
net/l2tp/l2tp_core.h | 10 ----------
2 files changed, 16 insertions(+), 11 deletions(-)
diff --git a/include/uapi/linux/l2tp.h b/include/uapi/linux/l2tp.h
index 347ef22a964e..dedfb2b1832a 100644
--- a/include/uapi/linux/l2tp.h
+++ b/include/uapi/linux/l2tp.h
@@ -108,7 +108,7 @@ enum {
L2TP_ATTR_VLAN_ID, /* u16 */
L2TP_ATTR_COOKIE, /* 0, 4 or 8 bytes */
L2TP_ATTR_PEER_COOKIE, /* 0, 4 or 8 bytes */
- L2TP_ATTR_DEBUG, /* u32 */
+ L2TP_ATTR_DEBUG, /* u32, enum l2tp_debug_flags */
L2TP_ATTR_RECV_SEQ, /* u8 */
L2TP_ATTR_SEND_SEQ, /* u8 */
L2TP_ATTR_LNS_MODE, /* u8 */
@@ -173,6 +173,21 @@ enum l2tp_seqmode {
L2TP_SEQ_ALL = 2,
};
+/**
+ * enum l2tp_debug_flags - debug message categories for L2TP tunnels/sessions
+ *
+ * @L2TP_MSG_DEBUG: verbose debug (if compiled in)
+ * @L2TP_MSG_CONTROL: userspace - kernel interface
+ * @L2TP_MSG_SEQ: sequence numbers
+ * @L2TP_MSG_DATA: data packets
+ */
+enum l2tp_debug_flags {
+ L2TP_MSG_DEBUG = (1 << 0),
+ L2TP_MSG_CONTROL = (1 << 1),
+ L2TP_MSG_SEQ = (1 << 2),
+ L2TP_MSG_DATA = (1 << 3),
+};
+
/*
* NETLINK_GENERIC related info
*/
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index ee59d9961d1f..2469f63649bb 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -23,16 +23,6 @@
#define L2TP_HASH_BITS_2 8
#define L2TP_HASH_SIZE_2 (1 << L2TP_HASH_BITS_2)
-/* Debug message categories for the DEBUG socket option */
-enum {
- L2TP_MSG_DEBUG = (1 << 0), /* verbose debug (if
- * compiled in) */
- L2TP_MSG_CONTROL = (1 << 1), /* userspace - kernel
- * interface */
- L2TP_MSG_SEQ = (1 << 2), /* sequence numbers */
- L2TP_MSG_DATA = (1 << 3), /* data packets */
-};
-
struct sk_buff;
struct l2tp_stats {
--
2.27.0.rc0.183.gde8f92d652-goog
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 07/27] net: l2tp: deprecate PPPOL2TP_MSG_* in favour of L2TP_MSG_*
2020-05-21 23:57 [PATCH 00/27] more l2tp locking and ordering fixes Giuliano Procida
` (5 preceding siblings ...)
2020-05-21 23:57 ` [PATCH 06/27] net: l2tp: export debug flags to UAPI Giuliano Procida
@ 2020-05-21 23:57 ` Giuliano Procida
2020-05-26 16:17 ` Asbjørn Sloth Tønnesen
2020-05-21 23:57 ` [PATCH 08/27] net: l2tp: ppp: change PPPOL2TP_MSG_* => L2TP_MSG_* Giuliano Procida
` (20 subsequent siblings)
27 siblings, 1 reply; 30+ messages in thread
From: Giuliano Procida @ 2020-05-21 23:57 UTC (permalink / raw)
To: greg
Cc: stable, Asbjørn Sloth Tønnesen, David S . Miller,
Giuliano Procida
From: Asbjørn Sloth Tønnesen <asbjorn@asbjorn.st>
commit 47c3e7783be4e142b861d34b5c2e223330b05d8a upstream.
PPPOL2TP_MSG_* and L2TP_MSG_* are duplicates, and are being used
interchangeably in the kernel, so let's standardize on L2TP_MSG_*
internally, and keep PPPOL2TP_MSG_* defined in UAPI for compatibility.
Signed-off-by: Asbjoern Sloth Toennesen <asbjorn@asbjorn.st>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
Documentation/networking/l2tp.txt | 8 ++++----
include/uapi/linux/if_pppol2tp.h | 13 ++++++-------
2 files changed, 10 insertions(+), 11 deletions(-)
diff --git a/Documentation/networking/l2tp.txt b/Documentation/networking/l2tp.txt
index 4650a00ed012..9bc271cdc9a8 100644
--- a/Documentation/networking/l2tp.txt
+++ b/Documentation/networking/l2tp.txt
@@ -177,10 +177,10 @@ setsockopt on the PPPoX socket to set a debug mask.
The following debug mask bits are available:
-PPPOL2TP_MSG_DEBUG verbose debug (if compiled in)
-PPPOL2TP_MSG_CONTROL userspace - kernel interface
-PPPOL2TP_MSG_SEQ sequence numbers handling
-PPPOL2TP_MSG_DATA data packets
+L2TP_MSG_DEBUG verbose debug (if compiled in)
+L2TP_MSG_CONTROL userspace - kernel interface
+L2TP_MSG_SEQ sequence numbers handling
+L2TP_MSG_DATA data packets
If enabled, files under a l2tp debugfs directory can be used to dump
kernel state about L2TP tunnels and sessions. To access it, the
diff --git a/include/uapi/linux/if_pppol2tp.h b/include/uapi/linux/if_pppol2tp.h
index 163e8adac2d6..de246e9f4974 100644
--- a/include/uapi/linux/if_pppol2tp.h
+++ b/include/uapi/linux/if_pppol2tp.h
@@ -17,6 +17,7 @@
#include <linux/types.h>
+#include <linux/l2tp.h>
/* Structure used to connect() the socket to a particular tunnel UDP
* socket over IPv4.
@@ -89,14 +90,12 @@ enum {
PPPOL2TP_SO_REORDERTO = 5,
};
-/* Debug message categories for the DEBUG socket option */
+/* Debug message categories for the DEBUG socket option (deprecated) */
enum {
- PPPOL2TP_MSG_DEBUG = (1 << 0), /* verbose debug (if
- * compiled in) */
- PPPOL2TP_MSG_CONTROL = (1 << 1), /* userspace - kernel
- * interface */
- PPPOL2TP_MSG_SEQ = (1 << 2), /* sequence numbers */
- PPPOL2TP_MSG_DATA = (1 << 3), /* data packets */
+ PPPOL2TP_MSG_DEBUG = L2TP_MSG_DEBUG,
+ PPPOL2TP_MSG_CONTROL = L2TP_MSG_CONTROL,
+ PPPOL2TP_MSG_SEQ = L2TP_MSG_SEQ,
+ PPPOL2TP_MSG_DATA = L2TP_MSG_DATA,
};
--
2.27.0.rc0.183.gde8f92d652-goog
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 08/27] net: l2tp: ppp: change PPPOL2TP_MSG_* => L2TP_MSG_*
2020-05-21 23:57 [PATCH 00/27] more l2tp locking and ordering fixes Giuliano Procida
` (6 preceding siblings ...)
2020-05-21 23:57 ` [PATCH 07/27] net: l2tp: deprecate PPPOL2TP_MSG_* in favour of L2TP_MSG_* Giuliano Procida
@ 2020-05-21 23:57 ` Giuliano Procida
2020-05-21 23:57 ` [PATCH 09/27] New kernel function to get IP overhead on a socket Giuliano Procida
` (19 subsequent siblings)
27 siblings, 0 replies; 30+ messages in thread
From: Giuliano Procida @ 2020-05-21 23:57 UTC (permalink / raw)
To: greg
Cc: stable, Asbjørn Sloth Tønnesen, David S . Miller,
Giuliano Procida
From: Asbjørn Sloth Tønnesen <asbjorn@asbjorn.st>
commit fba40c632c6473fa89660e870a6042c0fe733f8c upstream.
Signed-off-by: Asbjoern Sloth Toennesen <asbjorn@asbjorn.st>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
net/l2tp/l2tp_ppp.c | 54 ++++++++++++++++++++++-----------------------
1 file changed, 27 insertions(+), 27 deletions(-)
diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index bc5d6b8f8ede..5738456b3b58 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -230,7 +230,7 @@ static void pppol2tp_recv(struct l2tp_session *session, struct sk_buff *skb, int
if (sk->sk_state & PPPOX_BOUND) {
struct pppox_sock *po;
- l2tp_dbg(session, PPPOL2TP_MSG_DATA,
+ l2tp_dbg(session, L2TP_MSG_DATA,
"%s: recv %d byte data frame, passing to ppp\n",
session->name, data_len);
@@ -253,7 +253,7 @@ static void pppol2tp_recv(struct l2tp_session *session, struct sk_buff *skb, int
po = pppox_sk(sk);
ppp_input(&po->chan, skb);
} else {
- l2tp_dbg(session, PPPOL2TP_MSG_DATA,
+ l2tp_dbg(session, L2TP_MSG_DATA,
"%s: recv %d byte data frame, passing to L2TP socket\n",
session->name, data_len);
@@ -266,7 +266,7 @@ static void pppol2tp_recv(struct l2tp_session *session, struct sk_buff *skb, int
return;
no_sock:
- l2tp_info(session, PPPOL2TP_MSG_DATA, "%s: no socket\n", session->name);
+ l2tp_info(session, L2TP_MSG_DATA, "%s: no socket\n", session->name);
kfree_skb(skb);
}
@@ -797,7 +797,7 @@ out_no_ppp:
/* This is how we get the session context from the socket. */
sk->sk_user_data = session;
sk->sk_state = PPPOX_CONNECTED;
- l2tp_info(session, PPPOL2TP_MSG_CONTROL, "%s: created\n",
+ l2tp_info(session, L2TP_MSG_CONTROL, "%s: created\n",
session->name);
end:
@@ -848,7 +848,7 @@ static int pppol2tp_session_create(struct net *net, u32 tunnel_id, u32 session_i
ps = l2tp_session_priv(session);
ps->tunnel_sock = tunnel->sock;
- l2tp_info(session, PPPOL2TP_MSG_CONTROL, "%s: created\n",
+ l2tp_info(session, L2TP_MSG_CONTROL, "%s: created\n",
session->name);
error = 0;
@@ -1010,7 +1010,7 @@ static int pppol2tp_session_ioctl(struct l2tp_session *session,
struct l2tp_tunnel *tunnel = session->tunnel;
struct pppol2tp_ioc_stats stats;
- l2tp_dbg(session, PPPOL2TP_MSG_CONTROL,
+ l2tp_dbg(session, L2TP_MSG_CONTROL,
"%s: pppol2tp_session_ioctl(cmd=%#x, arg=%#lx)\n",
session->name, cmd, arg);
@@ -1033,7 +1033,7 @@ static int pppol2tp_session_ioctl(struct l2tp_session *session,
if (copy_to_user((void __user *) arg, &ifr, sizeof(struct ifreq)))
break;
- l2tp_info(session, PPPOL2TP_MSG_CONTROL, "%s: get mtu=%d\n",
+ l2tp_info(session, L2TP_MSG_CONTROL, "%s: get mtu=%d\n",
session->name, session->mtu);
err = 0;
break;
@@ -1049,7 +1049,7 @@ static int pppol2tp_session_ioctl(struct l2tp_session *session,
session->mtu = ifr.ifr_mtu;
- l2tp_info(session, PPPOL2TP_MSG_CONTROL, "%s: set mtu=%d\n",
+ l2tp_info(session, L2TP_MSG_CONTROL, "%s: set mtu=%d\n",
session->name, session->mtu);
err = 0;
break;
@@ -1063,7 +1063,7 @@ static int pppol2tp_session_ioctl(struct l2tp_session *session,
if (put_user(session->mru, (int __user *) arg))
break;
- l2tp_info(session, PPPOL2TP_MSG_CONTROL, "%s: get mru=%d\n",
+ l2tp_info(session, L2TP_MSG_CONTROL, "%s: get mru=%d\n",
session->name, session->mru);
err = 0;
break;
@@ -1078,7 +1078,7 @@ static int pppol2tp_session_ioctl(struct l2tp_session *session,
break;
session->mru = val;
- l2tp_info(session, PPPOL2TP_MSG_CONTROL, "%s: set mru=%d\n",
+ l2tp_info(session, L2TP_MSG_CONTROL, "%s: set mru=%d\n",
session->name, session->mru);
err = 0;
break;
@@ -1088,7 +1088,7 @@ static int pppol2tp_session_ioctl(struct l2tp_session *session,
if (put_user(ps->flags, (int __user *) arg))
break;
- l2tp_info(session, PPPOL2TP_MSG_CONTROL, "%s: get flags=%d\n",
+ l2tp_info(session, L2TP_MSG_CONTROL, "%s: get flags=%d\n",
session->name, ps->flags);
err = 0;
break;
@@ -1098,7 +1098,7 @@ static int pppol2tp_session_ioctl(struct l2tp_session *session,
if (get_user(val, (int __user *) arg))
break;
ps->flags = val;
- l2tp_info(session, PPPOL2TP_MSG_CONTROL, "%s: set flags=%d\n",
+ l2tp_info(session, L2TP_MSG_CONTROL, "%s: set flags=%d\n",
session->name, ps->flags);
err = 0;
break;
@@ -1115,7 +1115,7 @@ static int pppol2tp_session_ioctl(struct l2tp_session *session,
if (copy_to_user((void __user *) arg, &stats,
sizeof(stats)))
break;
- l2tp_info(session, PPPOL2TP_MSG_CONTROL, "%s: get L2TP stats\n",
+ l2tp_info(session, L2TP_MSG_CONTROL, "%s: get L2TP stats\n",
session->name);
err = 0;
break;
@@ -1143,7 +1143,7 @@ static int pppol2tp_tunnel_ioctl(struct l2tp_tunnel *tunnel,
struct sock *sk;
struct pppol2tp_ioc_stats stats;
- l2tp_dbg(tunnel, PPPOL2TP_MSG_CONTROL,
+ l2tp_dbg(tunnel, L2TP_MSG_CONTROL,
"%s: pppol2tp_tunnel_ioctl(cmd=%#x, arg=%#lx)\n",
tunnel->name, cmd, arg);
@@ -1186,7 +1186,7 @@ static int pppol2tp_tunnel_ioctl(struct l2tp_tunnel *tunnel,
err = -EFAULT;
break;
}
- l2tp_info(tunnel, PPPOL2TP_MSG_CONTROL, "%s: get L2TP stats\n",
+ l2tp_info(tunnel, L2TP_MSG_CONTROL, "%s: get L2TP stats\n",
tunnel->name);
err = 0;
break;
@@ -1276,7 +1276,7 @@ static int pppol2tp_tunnel_setsockopt(struct sock *sk,
switch (optname) {
case PPPOL2TP_SO_DEBUG:
tunnel->debug = val;
- l2tp_info(tunnel, PPPOL2TP_MSG_CONTROL, "%s: set debug=%x\n",
+ l2tp_info(tunnel, L2TP_MSG_CONTROL, "%s: set debug=%x\n",
tunnel->name, tunnel->debug);
break;
@@ -1304,7 +1304,7 @@ static int pppol2tp_session_setsockopt(struct sock *sk,
break;
}
session->recv_seq = val ? -1 : 0;
- l2tp_info(session, PPPOL2TP_MSG_CONTROL,
+ l2tp_info(session, L2TP_MSG_CONTROL,
"%s: set recv_seq=%d\n",
session->name, session->recv_seq);
break;
@@ -1322,7 +1322,7 @@ static int pppol2tp_session_setsockopt(struct sock *sk,
PPPOL2TP_L2TP_HDR_SIZE_NOSEQ;
}
l2tp_session_set_header_len(session, session->tunnel->version);
- l2tp_info(session, PPPOL2TP_MSG_CONTROL,
+ l2tp_info(session, L2TP_MSG_CONTROL,
"%s: set send_seq=%d\n",
session->name, session->send_seq);
break;
@@ -1333,20 +1333,20 @@ static int pppol2tp_session_setsockopt(struct sock *sk,
break;
}
session->lns_mode = val ? -1 : 0;
- l2tp_info(session, PPPOL2TP_MSG_CONTROL,
+ l2tp_info(session, L2TP_MSG_CONTROL,
"%s: set lns_mode=%d\n",
session->name, session->lns_mode);
break;
case PPPOL2TP_SO_DEBUG:
session->debug = val;
- l2tp_info(session, PPPOL2TP_MSG_CONTROL, "%s: set debug=%x\n",
+ l2tp_info(session, L2TP_MSG_CONTROL, "%s: set debug=%x\n",
session->name, session->debug);
break;
case PPPOL2TP_SO_REORDERTO:
session->reorder_timeout = msecs_to_jiffies(val);
- l2tp_info(session, PPPOL2TP_MSG_CONTROL,
+ l2tp_info(session, L2TP_MSG_CONTROL,
"%s: set reorder_timeout=%d\n",
session->name, session->reorder_timeout);
break;
@@ -1427,7 +1427,7 @@ static int pppol2tp_tunnel_getsockopt(struct sock *sk,
switch (optname) {
case PPPOL2TP_SO_DEBUG:
*val = tunnel->debug;
- l2tp_info(tunnel, PPPOL2TP_MSG_CONTROL, "%s: get debug=%x\n",
+ l2tp_info(tunnel, L2TP_MSG_CONTROL, "%s: get debug=%x\n",
tunnel->name, tunnel->debug);
break;
@@ -1450,31 +1450,31 @@ static int pppol2tp_session_getsockopt(struct sock *sk,
switch (optname) {
case PPPOL2TP_SO_RECVSEQ:
*val = session->recv_seq;
- l2tp_info(session, PPPOL2TP_MSG_CONTROL,
+ l2tp_info(session, L2TP_MSG_CONTROL,
"%s: get recv_seq=%d\n", session->name, *val);
break;
case PPPOL2TP_SO_SENDSEQ:
*val = session->send_seq;
- l2tp_info(session, PPPOL2TP_MSG_CONTROL,
+ l2tp_info(session, L2TP_MSG_CONTROL,
"%s: get send_seq=%d\n", session->name, *val);
break;
case PPPOL2TP_SO_LNSMODE:
*val = session->lns_mode;
- l2tp_info(session, PPPOL2TP_MSG_CONTROL,
+ l2tp_info(session, L2TP_MSG_CONTROL,
"%s: get lns_mode=%d\n", session->name, *val);
break;
case PPPOL2TP_SO_DEBUG:
*val = session->debug;
- l2tp_info(session, PPPOL2TP_MSG_CONTROL, "%s: get debug=%d\n",
+ l2tp_info(session, L2TP_MSG_CONTROL, "%s: get debug=%d\n",
session->name, *val);
break;
case PPPOL2TP_SO_REORDERTO:
*val = (int) jiffies_to_msecs(session->reorder_timeout);
- l2tp_info(session, PPPOL2TP_MSG_CONTROL,
+ l2tp_info(session, L2TP_MSG_CONTROL,
"%s: get reorder_timeout=%d\n", session->name, *val);
break;
--
2.27.0.rc0.183.gde8f92d652-goog
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 09/27] New kernel function to get IP overhead on a socket.
2020-05-21 23:57 [PATCH 00/27] more l2tp locking and ordering fixes Giuliano Procida
` (7 preceding siblings ...)
2020-05-21 23:57 ` [PATCH 08/27] net: l2tp: ppp: change PPPOL2TP_MSG_* => L2TP_MSG_* Giuliano Procida
@ 2020-05-21 23:57 ` Giuliano Procida
2020-05-21 23:57 ` [PATCH 10/27] L2TP:Adjust intf MTU, add underlay L3, L2 hdrs Giuliano Procida
` (18 subsequent siblings)
27 siblings, 0 replies; 30+ messages in thread
From: Giuliano Procida @ 2020-05-21 23:57 UTC (permalink / raw)
To: greg
Cc: stable, R. Parameswaran, R . Parameswaran, David S . Miller,
Giuliano Procida
From: "R. Parameswaran" <parameswaran.r7@gmail.com>
commit 113c3075931a334f899008f6c753abe70a3a9323 upstream.
A new function, kernel_sock_ip_overhead(), is provided
to calculate the cumulative overhead imposed by the IP
Header and IP options, if any, on a socket's payload.
The new function returns an overhead of zero for sockets
that do not belong to the IPv4 or IPv6 address families.
This is used in the L2TP code path to compute the
total outer IP overhead on the L2TP tunnel socket when
calculating the default MTU for Ethernet pseudowires.
Signed-off-by: R. Parameswaran <rparames@brocade.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
include/linux/net.h | 3 +++
net/socket.c | 46 +++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 49 insertions(+)
diff --git a/include/linux/net.h b/include/linux/net.h
index c00b8d182226..a0c6c00ac166 100644
--- a/include/linux/net.h
+++ b/include/linux/net.h
@@ -291,6 +291,9 @@ int kernel_sendpage(struct socket *sock, struct page *page, int offset,
int kernel_sock_ioctl(struct socket *sock, int cmd, unsigned long arg);
int kernel_sock_shutdown(struct socket *sock, enum sock_shutdown_cmd how);
+/* Following routine returns the IP overhead imposed by a socket. */
+u32 kernel_sock_ip_overhead(struct sock *sk);
+
#define MODULE_ALIAS_NETPROTO(proto) \
MODULE_ALIAS("net-pf-" __stringify(proto))
diff --git a/net/socket.c b/net/socket.c
index 15bdba4211ad..1a2a7320554b 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -3304,3 +3304,49 @@ int kernel_sock_shutdown(struct socket *sock, enum sock_shutdown_cmd how)
return sock->ops->shutdown(sock, how);
}
EXPORT_SYMBOL(kernel_sock_shutdown);
+
+/* This routine returns the IP overhead imposed by a socket i.e.
+ * the length of the underlying IP header, depending on whether
+ * this is an IPv4 or IPv6 socket and the length from IP options turned
+ * on at the socket.
+ */
+u32 kernel_sock_ip_overhead(struct sock *sk)
+{
+ struct inet_sock *inet;
+ struct ip_options_rcu *opt;
+ u32 overhead = 0;
+ bool owned_by_user;
+#if IS_ENABLED(CONFIG_IPV6)
+ struct ipv6_pinfo *np;
+ struct ipv6_txoptions *optv6 = NULL;
+#endif /* IS_ENABLED(CONFIG_IPV6) */
+
+ if (!sk)
+ return overhead;
+
+ owned_by_user = sock_owned_by_user(sk);
+ switch (sk->sk_family) {
+ case AF_INET:
+ inet = inet_sk(sk);
+ overhead += sizeof(struct iphdr);
+ opt = rcu_dereference_protected(inet->inet_opt,
+ owned_by_user);
+ if (opt)
+ overhead += opt->opt.optlen;
+ return overhead;
+#if IS_ENABLED(CONFIG_IPV6)
+ case AF_INET6:
+ np = inet6_sk(sk);
+ overhead += sizeof(struct ipv6hdr);
+ if (np)
+ optv6 = rcu_dereference_protected(np->opt,
+ owned_by_user);
+ if (optv6)
+ overhead += (optv6->opt_flen + optv6->opt_nflen);
+ return overhead;
+#endif /* IS_ENABLED(CONFIG_IPV6) */
+ default: /* Returns 0 overhead if the socket is not ipv4 or ipv6 */
+ return overhead;
+ }
+}
+EXPORT_SYMBOL(kernel_sock_ip_overhead);
--
2.27.0.rc0.183.gde8f92d652-goog
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 10/27] L2TP:Adjust intf MTU, add underlay L3, L2 hdrs.
2020-05-21 23:57 [PATCH 00/27] more l2tp locking and ordering fixes Giuliano Procida
` (8 preceding siblings ...)
2020-05-21 23:57 ` [PATCH 09/27] New kernel function to get IP overhead on a socket Giuliano Procida
@ 2020-05-21 23:57 ` Giuliano Procida
2020-05-21 23:57 ` [PATCH 11/27] l2tp: remove useless duplicate session detection in l2tp_netlink Giuliano Procida
` (17 subsequent siblings)
27 siblings, 0 replies; 30+ messages in thread
From: Giuliano Procida @ 2020-05-21 23:57 UTC (permalink / raw)
To: greg
Cc: stable, R. Parameswaran, R . Parameswaran, David S . Miller,
Giuliano Procida
From: "R. Parameswaran" <parameswaran.r7@gmail.com>
commit b784e7ebfce8cfb16c6f95e14e8532d0768ab7ff upstream.
Existing L2TP kernel code does not derive the optimal MTU for Ethernet
pseudowires and instead leaves this to a userspace L2TP daemon or
operator. If an MTU is not specified, the existing kernel code chooses
an MTU that does not take account of all tunnel header overheads, which
can lead to unwanted IP fragmentation. When L2TP is used without a
control plane (userspace daemon), we would prefer that the kernel does a
better job of choosing a default pseudowire MTU, taking account of all
tunnel header overheads, including IP header options, if any. This patch
addresses this.
Change-set here uses the new kernel function, kernel_sock_ip_overhead(),
to factor the outer IP overhead on the L2TP tunnel socket (including
IP Options, if any) when calculating the default MTU for an Ethernet
pseudowire, along with consideration of the inner Ethernet header.
Signed-off-by: R. Parameswaran <rparames@brocade.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
net/l2tp/l2tp_eth.c | 55 +++++++++++++++++++++++++++++++++++++++++----
1 file changed, 51 insertions(+), 4 deletions(-)
diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
index c94160df71af..cef312da3422 100644
--- a/net/l2tp/l2tp_eth.c
+++ b/net/l2tp/l2tp_eth.c
@@ -30,6 +30,9 @@
#include <net/xfrm.h>
#include <net/net_namespace.h>
#include <net/netns/generic.h>
+#include <linux/ip.h>
+#include <linux/ipv6.h>
+#include <linux/udp.h>
#include "l2tp_core.h"
@@ -206,6 +209,53 @@ static void l2tp_eth_show(struct seq_file *m, void *arg)
}
#endif
+static void l2tp_eth_adjust_mtu(struct l2tp_tunnel *tunnel,
+ struct l2tp_session *session,
+ struct net_device *dev)
+{
+ unsigned int overhead = 0;
+ struct dst_entry *dst;
+ u32 l3_overhead = 0;
+
+ /* if the encap is UDP, account for UDP header size */
+ if (tunnel->encap == L2TP_ENCAPTYPE_UDP) {
+ overhead += sizeof(struct udphdr);
+ dev->needed_headroom += sizeof(struct udphdr);
+ }
+ if (session->mtu != 0) {
+ dev->mtu = session->mtu;
+ dev->needed_headroom += session->hdr_len;
+ return;
+ }
+ l3_overhead = kernel_sock_ip_overhead(tunnel->sock);
+ if (l3_overhead == 0) {
+ /* L3 Overhead couldn't be identified, this could be
+ * because tunnel->sock was NULL or the socket's
+ * address family was not IPv4 or IPv6,
+ * dev mtu stays at 1500.
+ */
+ return;
+ }
+ /* Adjust MTU, factor overhead - underlay L3, overlay L2 hdr
+ * UDP overhead, if any, was already factored in above.
+ */
+ overhead += session->hdr_len + ETH_HLEN + l3_overhead;
+
+ /* If PMTU discovery was enabled, use discovered MTU on L2TP device */
+ dst = sk_dst_get(tunnel->sock);
+ if (dst) {
+ /* dst_mtu will use PMTU if found, else fallback to intf MTU */
+ u32 pmtu = dst_mtu(dst);
+
+ if (pmtu != 0)
+ dev->mtu = pmtu;
+ dst_release(dst);
+ }
+ session->mtu = dev->mtu - overhead;
+ dev->mtu = session->mtu;
+ dev->needed_headroom += session->hdr_len;
+}
+
static int l2tp_eth_create(struct net *net, u32 tunnel_id, u32 session_id, u32 peer_session_id, struct l2tp_session_cfg *cfg)
{
struct net_device *dev;
@@ -249,10 +299,7 @@ static int l2tp_eth_create(struct net *net, u32 tunnel_id, u32 session_id, u32 p
}
dev_net_set(dev, net);
- if (session->mtu == 0)
- session->mtu = dev->mtu - session->hdr_len;
- dev->mtu = session->mtu;
- dev->needed_headroom += session->hdr_len;
+ l2tp_eth_adjust_mtu(tunnel, session, dev);
priv = netdev_priv(dev);
priv->dev = dev;
--
2.27.0.rc0.183.gde8f92d652-goog
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 11/27] l2tp: remove useless duplicate session detection in l2tp_netlink
2020-05-21 23:57 [PATCH 00/27] more l2tp locking and ordering fixes Giuliano Procida
` (9 preceding siblings ...)
2020-05-21 23:57 ` [PATCH 10/27] L2TP:Adjust intf MTU, add underlay L3, L2 hdrs Giuliano Procida
@ 2020-05-21 23:57 ` Giuliano Procida
2020-05-21 23:57 ` [PATCH 12/27] l2tp: remove l2tp_session_find() Giuliano Procida
` (16 subsequent siblings)
27 siblings, 0 replies; 30+ messages in thread
From: Giuliano Procida @ 2020-05-21 23:57 UTC (permalink / raw)
To: greg; +Cc: stable, Guillaume Nault, David S . Miller, Giuliano Procida
From: Guillaume Nault <g.nault@alphalink.fr>
commit af87ae465abdc070de0dc35d6c6a9e7a8cd82987 upstream.
There's no point in checking for duplicate sessions at the beginning of
l2tp_nl_cmd_session_create(); the ->session_create() callbacks already
return -EEXIST when the session already exists.
Furthermore, even if l2tp_session_find() returns NULL, a new session
might be created right after the test. So relying on ->session_create()
to avoid duplicate session is the only sane behaviour.
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
net/l2tp/l2tp_netlink.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index 63a2430ff40a..afee3d54e5ef 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -505,11 +505,6 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
goto out;
}
session_id = nla_get_u32(info->attrs[L2TP_ATTR_SESSION_ID]);
- session = l2tp_session_find(net, tunnel, session_id);
- if (session) {
- ret = -EEXIST;
- goto out;
- }
if (!info->attrs[L2TP_ATTR_PEER_SESSION_ID]) {
ret = -EINVAL;
--
2.27.0.rc0.183.gde8f92d652-goog
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 12/27] l2tp: remove l2tp_session_find()
2020-05-21 23:57 [PATCH 00/27] more l2tp locking and ordering fixes Giuliano Procida
` (10 preceding siblings ...)
2020-05-21 23:57 ` [PATCH 11/27] l2tp: remove useless duplicate session detection in l2tp_netlink Giuliano Procida
@ 2020-05-21 23:57 ` Giuliano Procida
2020-05-21 23:57 ` [PATCH 13/27] l2tp: define parameters of l2tp_session_get*() as "const" Giuliano Procida
` (15 subsequent siblings)
27 siblings, 0 replies; 30+ messages in thread
From: Giuliano Procida @ 2020-05-21 23:57 UTC (permalink / raw)
To: greg; +Cc: stable, Guillaume Nault, David S . Miller, Giuliano Procida
From: Guillaume Nault <g.nault@alphalink.fr>
commit 55a3ce3b9d98f752df9e2cfb1cba7e715522428a upstream.
This function isn't used anymore.
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
net/l2tp/l2tp_core.c | 51 +-------------------------------------------
net/l2tp/l2tp_core.h | 3 ---
2 files changed, 1 insertion(+), 53 deletions(-)
diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 08377af93552..95fa01b4edc6 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -216,27 +216,6 @@ static void l2tp_tunnel_sock_put(struct sock *sk)
sock_put(sk);
}
-/* Lookup a session by id in the global session list
- */
-static struct l2tp_session *l2tp_session_find_2(struct net *net, u32 session_id)
-{
- struct l2tp_net *pn = l2tp_pernet(net);
- struct hlist_head *session_list =
- l2tp_session_id_hash_2(pn, session_id);
- struct l2tp_session *session;
-
- rcu_read_lock_bh();
- hlist_for_each_entry_rcu(session, session_list, global_hlist) {
- if (session->session_id == session_id) {
- rcu_read_unlock_bh();
- return session;
- }
- }
- rcu_read_unlock_bh();
-
- return NULL;
-}
-
/* Session hash list.
* The session_id SHOULD be random according to RFC2661, but several
* L2TP implementations (Cisco and Microsoft) use incrementing
@@ -249,35 +228,7 @@ l2tp_session_id_hash(struct l2tp_tunnel *tunnel, u32 session_id)
return &tunnel->session_hlist[hash_32(session_id, L2TP_HASH_BITS)];
}
-/* Lookup a session by id
- */
-struct l2tp_session *l2tp_session_find(struct net *net, struct l2tp_tunnel *tunnel, u32 session_id)
-{
- struct hlist_head *session_list;
- struct l2tp_session *session;
-
- /* In L2TPv3, session_ids are unique over all tunnels and we
- * sometimes need to look them up before we know the
- * tunnel.
- */
- if (tunnel == NULL)
- return l2tp_session_find_2(net, session_id);
-
- session_list = l2tp_session_id_hash(tunnel, session_id);
- read_lock_bh(&tunnel->hlist_lock);
- hlist_for_each_entry(session, session_list, hlist) {
- if (session->session_id == session_id) {
- read_unlock_bh(&tunnel->hlist_lock);
- return session;
- }
- }
- read_unlock_bh(&tunnel->hlist_lock);
-
- return NULL;
-}
-EXPORT_SYMBOL_GPL(l2tp_session_find);
-
-/* Like l2tp_session_find() but takes a reference on the returned session.
+/* Lookup a session. A new reference is held on the returned session.
* Optionally calls session->ref() too if do_ref is true.
*/
struct l2tp_session *l2tp_session_get(struct net *net,
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index 2469f63649bb..7dc70f73a083 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -237,9 +237,6 @@ out:
struct l2tp_session *l2tp_session_get(struct net *net,
struct l2tp_tunnel *tunnel,
u32 session_id, bool do_ref);
-struct l2tp_session *l2tp_session_find(struct net *net,
- struct l2tp_tunnel *tunnel,
- u32 session_id);
struct l2tp_session *l2tp_session_get_nth(struct l2tp_tunnel *tunnel, int nth,
bool do_ref);
struct l2tp_session *l2tp_session_get_by_ifname(struct net *net, char *ifname,
--
2.27.0.rc0.183.gde8f92d652-goog
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 13/27] l2tp: define parameters of l2tp_session_get*() as "const"
2020-05-21 23:57 [PATCH 00/27] more l2tp locking and ordering fixes Giuliano Procida
` (11 preceding siblings ...)
2020-05-21 23:57 ` [PATCH 12/27] l2tp: remove l2tp_session_find() Giuliano Procida
@ 2020-05-21 23:57 ` Giuliano Procida
2020-05-21 23:57 ` [PATCH 14/27] l2tp: define parameters of l2tp_tunnel_find*() " Giuliano Procida
` (14 subsequent siblings)
27 siblings, 0 replies; 30+ messages in thread
From: Giuliano Procida @ 2020-05-21 23:57 UTC (permalink / raw)
To: greg; +Cc: stable, Guillaume Nault, David S . Miller, Giuliano Procida
From: Guillaume Nault <g.nault@alphalink.fr>
commit 9aaef50c44f132e040dcd7686c8e78a3390037c5 upstream.
Make l2tp_pernet()'s parameter constant, so that l2tp_session_get*() can
declare their "net" variable as "const".
Also constify "ifname" in l2tp_session_get_by_ifname().
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
net/l2tp/l2tp_core.c | 7 ++++---
net/l2tp/l2tp_core.h | 5 +++--
2 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 95fa01b4edc6..4e2859d72167 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -119,7 +119,7 @@ static inline struct l2tp_tunnel *l2tp_tunnel(struct sock *sk)
return sk->sk_user_data;
}
-static inline struct l2tp_net *l2tp_pernet(struct net *net)
+static inline struct l2tp_net *l2tp_pernet(const struct net *net)
{
BUG_ON(!net);
@@ -231,7 +231,7 @@ l2tp_session_id_hash(struct l2tp_tunnel *tunnel, u32 session_id)
/* Lookup a session. A new reference is held on the returned session.
* Optionally calls session->ref() too if do_ref is true.
*/
-struct l2tp_session *l2tp_session_get(struct net *net,
+struct l2tp_session *l2tp_session_get(const struct net *net,
struct l2tp_tunnel *tunnel,
u32 session_id, bool do_ref)
{
@@ -306,7 +306,8 @@ EXPORT_SYMBOL_GPL(l2tp_session_get_nth);
/* Lookup a session by interface name.
* This is very inefficient but is only used by management interfaces.
*/
-struct l2tp_session *l2tp_session_get_by_ifname(struct net *net, char *ifname,
+struct l2tp_session *l2tp_session_get_by_ifname(const struct net *net,
+ const char *ifname,
bool do_ref)
{
struct l2tp_net *pn = l2tp_pernet(net);
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index 7dc70f73a083..dab75dc4ea48 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -234,12 +234,13 @@ out:
return tunnel;
}
-struct l2tp_session *l2tp_session_get(struct net *net,
+struct l2tp_session *l2tp_session_get(const struct net *net,
struct l2tp_tunnel *tunnel,
u32 session_id, bool do_ref);
struct l2tp_session *l2tp_session_get_nth(struct l2tp_tunnel *tunnel, int nth,
bool do_ref);
-struct l2tp_session *l2tp_session_get_by_ifname(struct net *net, char *ifname,
+struct l2tp_session *l2tp_session_get_by_ifname(const struct net *net,
+ const char *ifname,
bool do_ref);
struct l2tp_tunnel *l2tp_tunnel_find(struct net *net, u32 tunnel_id);
struct l2tp_tunnel *l2tp_tunnel_find_nth(struct net *net, int nth);
--
2.27.0.rc0.183.gde8f92d652-goog
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 14/27] l2tp: define parameters of l2tp_tunnel_find*() as "const"
2020-05-21 23:57 [PATCH 00/27] more l2tp locking and ordering fixes Giuliano Procida
` (12 preceding siblings ...)
2020-05-21 23:57 ` [PATCH 13/27] l2tp: define parameters of l2tp_session_get*() as "const" Giuliano Procida
@ 2020-05-21 23:57 ` Giuliano Procida
2020-05-21 23:57 ` [PATCH 15/27] l2tp: initialise session's refcount before making it reachable Giuliano Procida
` (13 subsequent siblings)
27 siblings, 0 replies; 30+ messages in thread
From: Giuliano Procida @ 2020-05-21 23:57 UTC (permalink / raw)
To: greg; +Cc: stable, Guillaume Nault, David S . Miller, Giuliano Procida
From: Guillaume Nault <g.nault@alphalink.fr>
commit 2f858b928bf5a8174911aaec76b8b72a9ca0533d upstream.
l2tp_tunnel_find() and l2tp_tunnel_find_nth() don't modify "net".
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
net/l2tp/l2tp_core.c | 4 ++--
net/l2tp/l2tp_core.h | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 4e2859d72167..7e593e399774 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -378,7 +378,7 @@ exist:
/* Lookup a tunnel by id
*/
-struct l2tp_tunnel *l2tp_tunnel_find(struct net *net, u32 tunnel_id)
+struct l2tp_tunnel *l2tp_tunnel_find(const struct net *net, u32 tunnel_id)
{
struct l2tp_tunnel *tunnel;
struct l2tp_net *pn = l2tp_pernet(net);
@@ -396,7 +396,7 @@ struct l2tp_tunnel *l2tp_tunnel_find(struct net *net, u32 tunnel_id)
}
EXPORT_SYMBOL_GPL(l2tp_tunnel_find);
-struct l2tp_tunnel *l2tp_tunnel_find_nth(struct net *net, int nth)
+struct l2tp_tunnel *l2tp_tunnel_find_nth(const struct net *net, int nth)
{
struct l2tp_net *pn = l2tp_pernet(net);
struct l2tp_tunnel *tunnel;
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index dab75dc4ea48..8fa4ad103c25 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -242,8 +242,8 @@ struct l2tp_session *l2tp_session_get_nth(struct l2tp_tunnel *tunnel, int nth,
struct l2tp_session *l2tp_session_get_by_ifname(const struct net *net,
const char *ifname,
bool do_ref);
-struct l2tp_tunnel *l2tp_tunnel_find(struct net *net, u32 tunnel_id);
-struct l2tp_tunnel *l2tp_tunnel_find_nth(struct net *net, int nth);
+struct l2tp_tunnel *l2tp_tunnel_find(const struct net *net, u32 tunnel_id);
+struct l2tp_tunnel *l2tp_tunnel_find_nth(const struct net *net, int nth);
int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id,
u32 peer_tunnel_id, struct l2tp_tunnel_cfg *cfg,
--
2.27.0.rc0.183.gde8f92d652-goog
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 15/27] l2tp: initialise session's refcount before making it reachable
2020-05-21 23:57 [PATCH 00/27] more l2tp locking and ordering fixes Giuliano Procida
` (13 preceding siblings ...)
2020-05-21 23:57 ` [PATCH 14/27] l2tp: define parameters of l2tp_tunnel_find*() " Giuliano Procida
@ 2020-05-21 23:57 ` Giuliano Procida
2020-05-21 23:57 ` [PATCH 16/27] l2tp: hold tunnel while looking up sessions in l2tp_netlink Giuliano Procida
` (12 subsequent siblings)
27 siblings, 0 replies; 30+ messages in thread
From: Giuliano Procida @ 2020-05-21 23:57 UTC (permalink / raw)
To: greg; +Cc: stable, Guillaume Nault, David S . Miller, Giuliano Procida
From: Guillaume Nault <g.nault@alphalink.fr>
commit 9ee369a405c57613d7c83a3967780c3e30c52ecc upstream.
Sessions must be fully initialised before calling
l2tp_session_add_to_tunnel(). Otherwise, there's a short time frame
where partially initialised sessions can be accessed by external users.
Backporting Notes
l2tp_core.c: moving code that had been converted from atomic to
refcount_t by an earlier change (which isn't being included in this
patch series).
Fixes: dbdbc73b4478 ("l2tp: fix duplicate session creation")
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
net/l2tp/l2tp_core.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 7e593e399774..c0abd5efd824 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1853,6 +1853,8 @@ struct l2tp_session *l2tp_session_create(int priv_size, struct l2tp_tunnel *tunn
l2tp_session_set_header_len(session, tunnel->version);
+ l2tp_session_inc_refcount(session);
+
err = l2tp_session_add_to_tunnel(tunnel, session);
if (err) {
kfree(session);
@@ -1860,10 +1862,6 @@ struct l2tp_session *l2tp_session_create(int priv_size, struct l2tp_tunnel *tunn
return ERR_PTR(err);
}
- /* Bump the reference count. The session context is deleted
- * only when this drops to zero.
- */
- l2tp_session_inc_refcount(session);
l2tp_tunnel_inc_refcount(tunnel);
/* Ensure tunnel socket isn't deleted */
--
2.27.0.rc0.183.gde8f92d652-goog
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 16/27] l2tp: hold tunnel while looking up sessions in l2tp_netlink
2020-05-21 23:57 [PATCH 00/27] more l2tp locking and ordering fixes Giuliano Procida
` (14 preceding siblings ...)
2020-05-21 23:57 ` [PATCH 15/27] l2tp: initialise session's refcount before making it reachable Giuliano Procida
@ 2020-05-21 23:57 ` Giuliano Procida
2020-05-21 23:57 ` [PATCH 17/27] l2tp: hold tunnel while processing genl delete command Giuliano Procida
` (11 subsequent siblings)
27 siblings, 0 replies; 30+ messages in thread
From: Giuliano Procida @ 2020-05-21 23:57 UTC (permalink / raw)
To: greg; +Cc: stable, Guillaume Nault, David S . Miller, Giuliano Procida
From: Guillaume Nault <g.nault@alphalink.fr>
commit 54652eb12c1b72e9602d09cb2821d5760939190f upstream.
l2tp_tunnel_find() doesn't take a reference on the returned tunnel.
Therefore, it's unsafe to use it because the returned tunnel can go
away on us anytime.
Fix this by defining l2tp_tunnel_get(), which works like
l2tp_tunnel_find(), but takes a reference on the returned tunnel.
Caller then has to drop this reference using l2tp_tunnel_dec_refcount().
As l2tp_tunnel_dec_refcount() needs to be moved to l2tp_core.h, let's
simplify the patch and not move the L2TP_REFCNT_DEBUG part. This code
has been broken (not even compiling) in May 2012 by
commit a4ca44fa578c ("net: l2tp: Standardize logging styles")
and fixed more than two years later by
commit 29abe2fda54f ("l2tp: fix missing line continuation"). So it
doesn't appear to be used by anyone.
Same thing for l2tp_tunnel_free(); instead of moving it to l2tp_core.h,
let's just simplify things and call kfree_rcu() directly in
l2tp_tunnel_dec_refcount(). Extra assertions and debugging code
provided by l2tp_tunnel_free() didn't help catching any of the
reference counting and socket handling issues found while working on
this series.
Backporting Notes
l2tp_core.c: This patch deletes some code / moves some code to
l2tp_core.h and follows the patch (not including in this series) that
switched from atomic to refcount_t. Moved code changed back to atomic.
Fixes: 309795f4bec2 ("l2tp: Add netlink control API for L2TP")
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
net/l2tp/l2tp_core.c | 66 +++++++++++++----------------------------
net/l2tp/l2tp_core.h | 13 ++++++++
net/l2tp/l2tp_netlink.c | 6 ++--
3 files changed, 38 insertions(+), 47 deletions(-)
diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index c0abd5efd824..e5210cf8cb59 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -112,7 +112,6 @@ struct l2tp_net {
spinlock_t l2tp_session_hlist_lock;
};
-static void l2tp_tunnel_free(struct l2tp_tunnel *tunnel);
static inline struct l2tp_tunnel *l2tp_tunnel(struct sock *sk)
{
@@ -126,39 +125,6 @@ static inline struct l2tp_net *l2tp_pernet(const struct net *net)
return net_generic(net, l2tp_net_id);
}
-/* Tunnel reference counts. Incremented per session that is added to
- * the tunnel.
- */
-static inline void l2tp_tunnel_inc_refcount_1(struct l2tp_tunnel *tunnel)
-{
- atomic_inc(&tunnel->ref_count);
-}
-
-static inline void l2tp_tunnel_dec_refcount_1(struct l2tp_tunnel *tunnel)
-{
- if (atomic_dec_and_test(&tunnel->ref_count))
- l2tp_tunnel_free(tunnel);
-}
-#ifdef L2TP_REFCNT_DEBUG
-#define l2tp_tunnel_inc_refcount(_t) \
-do { \
- pr_debug("l2tp_tunnel_inc_refcount: %s:%d %s: cnt=%d\n", \
- __func__, __LINE__, (_t)->name, \
- atomic_read(&_t->ref_count)); \
- l2tp_tunnel_inc_refcount_1(_t); \
-} while (0)
-#define l2tp_tunnel_dec_refcount(_t) \
-do { \
- pr_debug("l2tp_tunnel_dec_refcount: %s:%d %s: cnt=%d\n", \
- __func__, __LINE__, (_t)->name, \
- atomic_read(&_t->ref_count)); \
- l2tp_tunnel_dec_refcount_1(_t); \
-} while (0)
-#else
-#define l2tp_tunnel_inc_refcount(t) l2tp_tunnel_inc_refcount_1(t)
-#define l2tp_tunnel_dec_refcount(t) l2tp_tunnel_dec_refcount_1(t)
-#endif
-
/* Session hash global list for L2TPv3.
* The session_id SHOULD be random according to RFC3931, but several
* L2TP implementations use incrementing session_ids. So we do a real
@@ -228,6 +194,27 @@ l2tp_session_id_hash(struct l2tp_tunnel *tunnel, u32 session_id)
return &tunnel->session_hlist[hash_32(session_id, L2TP_HASH_BITS)];
}
+/* Lookup a tunnel. A new reference is held on the returned tunnel. */
+struct l2tp_tunnel *l2tp_tunnel_get(const struct net *net, u32 tunnel_id)
+{
+ const struct l2tp_net *pn = l2tp_pernet(net);
+ struct l2tp_tunnel *tunnel;
+
+ rcu_read_lock_bh();
+ list_for_each_entry_rcu(tunnel, &pn->l2tp_tunnel_list, list) {
+ if (tunnel->tunnel_id == tunnel_id) {
+ l2tp_tunnel_inc_refcount(tunnel);
+ rcu_read_unlock_bh();
+
+ return tunnel;
+ }
+ }
+ rcu_read_unlock_bh();
+
+ return NULL;
+}
+EXPORT_SYMBOL_GPL(l2tp_tunnel_get);
+
/* Lookup a session. A new reference is held on the returned session.
* Optionally calls session->ref() too if do_ref is true.
*/
@@ -1351,17 +1338,6 @@ static void l2tp_udp_encap_destroy(struct sock *sk)
}
}
-/* Really kill the tunnel.
- * Come here only when all sessions have been cleared from the tunnel.
- */
-static void l2tp_tunnel_free(struct l2tp_tunnel *tunnel)
-{
- BUG_ON(atomic_read(&tunnel->ref_count) != 0);
- BUG_ON(tunnel->sock != NULL);
- l2tp_info(tunnel, L2TP_MSG_CONTROL, "%s: free...\n", tunnel->name);
- kfree_rcu(tunnel, rcu);
-}
-
/* Workqueue tunnel deletion function */
static void l2tp_tunnel_del_work(struct work_struct *work)
{
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index 8fa4ad103c25..0d4590b25592 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -234,6 +234,8 @@ out:
return tunnel;
}
+struct l2tp_tunnel *l2tp_tunnel_get(const struct net *net, u32 tunnel_id);
+
struct l2tp_session *l2tp_session_get(const struct net *net,
struct l2tp_tunnel *tunnel,
u32 session_id, bool do_ref);
@@ -272,6 +274,17 @@ int l2tp_nl_register_ops(enum l2tp_pwtype pw_type,
void l2tp_nl_unregister_ops(enum l2tp_pwtype pw_type);
int l2tp_ioctl(struct sock *sk, int cmd, unsigned long arg);
+static inline void l2tp_tunnel_inc_refcount(struct l2tp_tunnel *tunnel)
+{
+ atomic_inc(&tunnel->ref_count);
+}
+
+static inline void l2tp_tunnel_dec_refcount(struct l2tp_tunnel *tunnel)
+{
+ if (atomic_dec_and_test(&tunnel->ref_count))
+ kfree_rcu(tunnel, rcu);
+}
+
/* Session reference counts. Incremented when code obtains a reference
* to a session.
*/
diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index afee3d54e5ef..558327d62167 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -72,10 +72,12 @@ static struct l2tp_session *l2tp_nl_session_get(struct genl_info *info,
(info->attrs[L2TP_ATTR_CONN_ID])) {
tunnel_id = nla_get_u32(info->attrs[L2TP_ATTR_CONN_ID]);
session_id = nla_get_u32(info->attrs[L2TP_ATTR_SESSION_ID]);
- tunnel = l2tp_tunnel_find(net, tunnel_id);
- if (tunnel)
+ tunnel = l2tp_tunnel_get(net, tunnel_id);
+ if (tunnel) {
session = l2tp_session_get(net, tunnel, session_id,
do_ref);
+ l2tp_tunnel_dec_refcount(tunnel);
+ }
}
return session;
--
2.27.0.rc0.183.gde8f92d652-goog
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 17/27] l2tp: hold tunnel while processing genl delete command
2020-05-21 23:57 [PATCH 00/27] more l2tp locking and ordering fixes Giuliano Procida
` (15 preceding siblings ...)
2020-05-21 23:57 ` [PATCH 16/27] l2tp: hold tunnel while looking up sessions in l2tp_netlink Giuliano Procida
@ 2020-05-21 23:57 ` Giuliano Procida
2020-05-21 23:57 ` [PATCH 18/27] l2tp: hold tunnel while handling genl tunnel updates Giuliano Procida
` (10 subsequent siblings)
27 siblings, 0 replies; 30+ messages in thread
From: Giuliano Procida @ 2020-05-21 23:57 UTC (permalink / raw)
To: greg; +Cc: stable, Guillaume Nault, David S . Miller, Giuliano Procida
From: Guillaume Nault <g.nault@alphalink.fr>
commit bb0a32ce4389e17e47e198d2cddaf141561581ad upstream.
l2tp_nl_cmd_tunnel_delete() needs to take a reference on the tunnel, to
prevent it from being concurrently freed by l2tp_tunnel_destruct().
Fixes: 309795f4bec2 ("l2tp: Add netlink control API for L2TP")
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
net/l2tp/l2tp_netlink.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index 558327d62167..e0c3a551c9bc 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -280,8 +280,8 @@ static int l2tp_nl_cmd_tunnel_delete(struct sk_buff *skb, struct genl_info *info
}
tunnel_id = nla_get_u32(info->attrs[L2TP_ATTR_CONN_ID]);
- tunnel = l2tp_tunnel_find(net, tunnel_id);
- if (tunnel == NULL) {
+ tunnel = l2tp_tunnel_get(net, tunnel_id);
+ if (!tunnel) {
ret = -ENODEV;
goto out;
}
@@ -291,6 +291,8 @@ static int l2tp_nl_cmd_tunnel_delete(struct sk_buff *skb, struct genl_info *info
l2tp_tunnel_delete(tunnel);
+ l2tp_tunnel_dec_refcount(tunnel);
+
out:
return ret;
}
--
2.27.0.rc0.183.gde8f92d652-goog
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 18/27] l2tp: hold tunnel while handling genl tunnel updates
2020-05-21 23:57 [PATCH 00/27] more l2tp locking and ordering fixes Giuliano Procida
` (16 preceding siblings ...)
2020-05-21 23:57 ` [PATCH 17/27] l2tp: hold tunnel while processing genl delete command Giuliano Procida
@ 2020-05-21 23:57 ` Giuliano Procida
2020-05-21 23:57 ` [PATCH 19/27] l2tp: hold tunnel while handling genl TUNNEL_GET commands Giuliano Procida
` (9 subsequent siblings)
27 siblings, 0 replies; 30+ messages in thread
From: Giuliano Procida @ 2020-05-21 23:57 UTC (permalink / raw)
To: greg; +Cc: stable, Guillaume Nault, David S . Miller, Giuliano Procida
From: Guillaume Nault <g.nault@alphalink.fr>
commit 8c0e421525c9eb50d68e8f633f703ca31680b746 upstream.
We need to make sure the tunnel is not going to be destroyed by
l2tp_tunnel_destruct() concurrently.
Fixes: 309795f4bec2 ("l2tp: Add netlink control API for L2TP")
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
net/l2tp/l2tp_netlink.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index e0c3a551c9bc..71784e7542cf 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -310,8 +310,8 @@ static int l2tp_nl_cmd_tunnel_modify(struct sk_buff *skb, struct genl_info *info
}
tunnel_id = nla_get_u32(info->attrs[L2TP_ATTR_CONN_ID]);
- tunnel = l2tp_tunnel_find(net, tunnel_id);
- if (tunnel == NULL) {
+ tunnel = l2tp_tunnel_get(net, tunnel_id);
+ if (!tunnel) {
ret = -ENODEV;
goto out;
}
@@ -322,6 +322,8 @@ static int l2tp_nl_cmd_tunnel_modify(struct sk_buff *skb, struct genl_info *info
ret = l2tp_tunnel_notify(&l2tp_nl_family, info,
tunnel, L2TP_CMD_TUNNEL_MODIFY);
+ l2tp_tunnel_dec_refcount(tunnel);
+
out:
return ret;
}
--
2.27.0.rc0.183.gde8f92d652-goog
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 19/27] l2tp: hold tunnel while handling genl TUNNEL_GET commands
2020-05-21 23:57 [PATCH 00/27] more l2tp locking and ordering fixes Giuliano Procida
` (17 preceding siblings ...)
2020-05-21 23:57 ` [PATCH 18/27] l2tp: hold tunnel while handling genl tunnel updates Giuliano Procida
@ 2020-05-21 23:57 ` Giuliano Procida
2020-05-21 23:57 ` [PATCH 20/27] l2tp: hold tunnel used while creating sessions with netlink Giuliano Procida
` (8 subsequent siblings)
27 siblings, 0 replies; 30+ messages in thread
From: Giuliano Procida @ 2020-05-21 23:57 UTC (permalink / raw)
To: greg; +Cc: stable, Guillaume Nault, David S . Miller, Giuliano Procida
From: Guillaume Nault <g.nault@alphalink.fr>
commit 4e4b21da3acc68a7ea55f850cacc13706b7480e9 upstream.
Use l2tp_tunnel_get() instead of l2tp_tunnel_find() so that we get
a reference on the tunnel, preventing l2tp_tunnel_destruct() from
freeing it from under us.
Also move l2tp_tunnel_get() below nlmsg_new() so that we only take
the reference when needed.
Fixes: 309795f4bec2 ("l2tp: Add netlink control API for L2TP")
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
net/l2tp/l2tp_netlink.c | 27 +++++++++++++++------------
1 file changed, 15 insertions(+), 12 deletions(-)
diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index 71784e7542cf..b86a2caa9356 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -428,34 +428,37 @@ static int l2tp_nl_cmd_tunnel_get(struct sk_buff *skb, struct genl_info *info)
if (!info->attrs[L2TP_ATTR_CONN_ID]) {
ret = -EINVAL;
- goto out;
+ goto err;
}
tunnel_id = nla_get_u32(info->attrs[L2TP_ATTR_CONN_ID]);
- tunnel = l2tp_tunnel_find(net, tunnel_id);
- if (tunnel == NULL) {
- ret = -ENODEV;
- goto out;
- }
-
msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
if (!msg) {
ret = -ENOMEM;
- goto out;
+ goto err;
+ }
+
+ tunnel = l2tp_tunnel_get(net, tunnel_id);
+ if (!tunnel) {
+ ret = -ENODEV;
+ goto err_nlmsg;
}
ret = l2tp_nl_tunnel_send(msg, info->snd_portid, info->snd_seq,
NLM_F_ACK, tunnel, L2TP_CMD_TUNNEL_GET);
if (ret < 0)
- goto err_out;
+ goto err_nlmsg_tunnel;
+
+ l2tp_tunnel_dec_refcount(tunnel);
return genlmsg_unicast(net, msg, info->snd_portid);
-err_out:
+err_nlmsg_tunnel:
+ l2tp_tunnel_dec_refcount(tunnel);
+err_nlmsg:
nlmsg_free(msg);
-
-out:
+err:
return ret;
}
--
2.27.0.rc0.183.gde8f92d652-goog
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 20/27] l2tp: hold tunnel used while creating sessions with netlink
2020-05-21 23:57 [PATCH 00/27] more l2tp locking and ordering fixes Giuliano Procida
` (18 preceding siblings ...)
2020-05-21 23:57 ` [PATCH 19/27] l2tp: hold tunnel while handling genl TUNNEL_GET commands Giuliano Procida
@ 2020-05-21 23:57 ` Giuliano Procida
2020-05-21 23:57 ` [PATCH 21/27] l2tp: prevent creation of sessions on terminated tunnels Giuliano Procida
` (7 subsequent siblings)
27 siblings, 0 replies; 30+ messages in thread
From: Giuliano Procida @ 2020-05-21 23:57 UTC (permalink / raw)
To: greg; +Cc: stable, Guillaume Nault, David S . Miller, Giuliano Procida
From: Guillaume Nault <g.nault@alphalink.fr>
commit e702c1204eb57788ef189c839c8c779368267d70 upstream.
Use l2tp_tunnel_get() to retrieve tunnel, so that it can't go away on
us. Otherwise l2tp_tunnel_destruct() might release the last reference
count concurrently, thus freeing the tunnel while we're using it.
Fixes: 309795f4bec2 ("l2tp: Add netlink control API for L2TP")
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
net/l2tp/l2tp_netlink.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index b86a2caa9356..22917e751edf 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -502,8 +502,9 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
ret = -EINVAL;
goto out;
}
+
tunnel_id = nla_get_u32(info->attrs[L2TP_ATTR_CONN_ID]);
- tunnel = l2tp_tunnel_find(net, tunnel_id);
+ tunnel = l2tp_tunnel_get(net, tunnel_id);
if (!tunnel) {
ret = -ENODEV;
goto out;
@@ -511,24 +512,24 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
if (!info->attrs[L2TP_ATTR_SESSION_ID]) {
ret = -EINVAL;
- goto out;
+ goto out_tunnel;
}
session_id = nla_get_u32(info->attrs[L2TP_ATTR_SESSION_ID]);
if (!info->attrs[L2TP_ATTR_PEER_SESSION_ID]) {
ret = -EINVAL;
- goto out;
+ goto out_tunnel;
}
peer_session_id = nla_get_u32(info->attrs[L2TP_ATTR_PEER_SESSION_ID]);
if (!info->attrs[L2TP_ATTR_PW_TYPE]) {
ret = -EINVAL;
- goto out;
+ goto out_tunnel;
}
cfg.pw_type = nla_get_u16(info->attrs[L2TP_ATTR_PW_TYPE]);
if (cfg.pw_type >= __L2TP_PWTYPE_MAX) {
ret = -EINVAL;
- goto out;
+ goto out_tunnel;
}
if (tunnel->version > 2) {
@@ -550,7 +551,7 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
u16 len = nla_len(info->attrs[L2TP_ATTR_COOKIE]);
if (len > 8) {
ret = -EINVAL;
- goto out;
+ goto out_tunnel;
}
cfg.cookie_len = len;
memcpy(&cfg.cookie[0], nla_data(info->attrs[L2TP_ATTR_COOKIE]), len);
@@ -559,7 +560,7 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
u16 len = nla_len(info->attrs[L2TP_ATTR_PEER_COOKIE]);
if (len > 8) {
ret = -EINVAL;
- goto out;
+ goto out_tunnel;
}
cfg.peer_cookie_len = len;
memcpy(&cfg.peer_cookie[0], nla_data(info->attrs[L2TP_ATTR_PEER_COOKIE]), len);
@@ -602,7 +603,7 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
if ((l2tp_nl_cmd_ops[cfg.pw_type] == NULL) ||
(l2tp_nl_cmd_ops[cfg.pw_type]->session_create == NULL)) {
ret = -EPROTONOSUPPORT;
- goto out;
+ goto out_tunnel;
}
/* Check that pseudowire-specific params are present */
@@ -612,7 +613,7 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
case L2TP_PWTYPE_ETH_VLAN:
if (!info->attrs[L2TP_ATTR_VLAN_ID]) {
ret = -EINVAL;
- goto out;
+ goto out_tunnel;
}
break;
case L2TP_PWTYPE_ETH:
@@ -640,6 +641,8 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
}
}
+out_tunnel:
+ l2tp_tunnel_dec_refcount(tunnel);
out:
return ret;
}
--
2.27.0.rc0.183.gde8f92d652-goog
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 21/27] l2tp: prevent creation of sessions on terminated tunnels
2020-05-21 23:57 [PATCH 00/27] more l2tp locking and ordering fixes Giuliano Procida
` (19 preceding siblings ...)
2020-05-21 23:57 ` [PATCH 20/27] l2tp: hold tunnel used while creating sessions with netlink Giuliano Procida
@ 2020-05-21 23:57 ` Giuliano Procida
2020-05-21 23:57 ` [PATCH 22/27] l2tp: pass tunnel pointer to ->session_create() Giuliano Procida
` (6 subsequent siblings)
27 siblings, 0 replies; 30+ messages in thread
From: Giuliano Procida @ 2020-05-21 23:57 UTC (permalink / raw)
To: greg; +Cc: stable, Guillaume Nault, David S . Miller, Giuliano Procida
From: Guillaume Nault <g.nault@alphalink.fr>
commit f3c66d4e144a0904ea9b95d23ed9f8eb38c11bfb upstream.
l2tp_tunnel_destruct() sets tunnel->sock to NULL, then removes the
tunnel from the pernet list and finally closes all its sessions.
Therefore, it's possible to add a session to a tunnel that is still
reachable, but for which tunnel->sock has already been reset. This can
make l2tp_session_create() dereference a NULL pointer when calling
sock_hold(tunnel->sock).
This patch adds the .acpt_newsess field to struct l2tp_tunnel, which is
used by l2tp_tunnel_closeall() to prevent addition of new sessions to
tunnels. Resetting tunnel->sock is done after l2tp_tunnel_closeall()
returned, so that l2tp_session_add_to_tunnel() can safely take a
reference on it when .acpt_newsess is true.
The .acpt_newsess field is modified in l2tp_tunnel_closeall(), rather
than in l2tp_tunnel_destruct(), so that it benefits all tunnel removal
mechanisms. E.g. on UDP tunnels, a session could be added to a tunnel
after l2tp_udp_encap_destroy() proceeded. This would prevent the tunnel
from being removed because of the references held by this new session
on the tunnel and its socket. Even though the session could be removed
manually later on, this defeats the purpose of
commit 9980d001cec8 ("l2tp: add udp encap socket destroy handler").
Fixes: fd558d186df2 ("l2tp: Split pppol2tp patch into separate l2tp and ppp parts")
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
net/l2tp/l2tp_core.c | 41 ++++++++++++++++++++++++++++-------------
net/l2tp/l2tp_core.h | 4 ++++
2 files changed, 32 insertions(+), 13 deletions(-)
diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index e5210cf8cb59..ba11e72a2365 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -328,13 +328,21 @@ static int l2tp_session_add_to_tunnel(struct l2tp_tunnel *tunnel,
struct hlist_head *g_head;
struct hlist_head *head;
struct l2tp_net *pn;
+ int err;
head = l2tp_session_id_hash(tunnel, session->session_id);
write_lock_bh(&tunnel->hlist_lock);
+ if (!tunnel->acpt_newsess) {
+ err = -ENODEV;
+ goto err_tlock;
+ }
+
hlist_for_each_entry(session_walk, head, hlist)
- if (session_walk->session_id == session->session_id)
- goto exist;
+ if (session_walk->session_id == session->session_id) {
+ err = -EEXIST;
+ goto err_tlock;
+ }
if (tunnel->version == L2TP_HDR_VER_3) {
pn = l2tp_pernet(tunnel->l2tp_net);
@@ -342,12 +350,21 @@ static int l2tp_session_add_to_tunnel(struct l2tp_tunnel *tunnel,
session->session_id);
spin_lock_bh(&pn->l2tp_session_hlist_lock);
+
hlist_for_each_entry(session_walk, g_head, global_hlist)
- if (session_walk->session_id == session->session_id)
- goto exist_glob;
+ if (session_walk->session_id == session->session_id) {
+ err = -EEXIST;
+ goto err_tlock_pnlock;
+ }
+ l2tp_tunnel_inc_refcount(tunnel);
+ sock_hold(tunnel->sock);
hlist_add_head_rcu(&session->global_hlist, g_head);
+
spin_unlock_bh(&pn->l2tp_session_hlist_lock);
+ } else {
+ l2tp_tunnel_inc_refcount(tunnel);
+ sock_hold(tunnel->sock);
}
hlist_add_head(&session->hlist, head);
@@ -355,12 +372,12 @@ static int l2tp_session_add_to_tunnel(struct l2tp_tunnel *tunnel,
return 0;
-exist_glob:
+err_tlock_pnlock:
spin_unlock_bh(&pn->l2tp_session_hlist_lock);
-exist:
+err_tlock:
write_unlock_bh(&tunnel->hlist_lock);
- return -EEXIST;
+ return err;
}
/* Lookup a tunnel by id
@@ -1251,7 +1268,6 @@ static void l2tp_tunnel_destruct(struct sock *sk)
/* Remove hooks into tunnel socket */
sk->sk_destruct = tunnel->old_sk_destruct;
sk->sk_user_data = NULL;
- tunnel->sock = NULL;
/* Remove the tunnel struct from the tunnel list */
pn = l2tp_pernet(tunnel->l2tp_net);
@@ -1261,6 +1277,8 @@ static void l2tp_tunnel_destruct(struct sock *sk)
atomic_dec(&l2tp_tunnel_count);
l2tp_tunnel_closeall(tunnel);
+
+ tunnel->sock = NULL;
l2tp_tunnel_dec_refcount(tunnel);
/* Call the original destructor */
@@ -1285,6 +1303,7 @@ void l2tp_tunnel_closeall(struct l2tp_tunnel *tunnel)
tunnel->name);
write_lock_bh(&tunnel->hlist_lock);
+ tunnel->acpt_newsess = false;
for (hash = 0; hash < L2TP_HASH_SIZE; hash++) {
again:
hlist_for_each_safe(walk, tmp, &tunnel->session_hlist[hash]) {
@@ -1588,6 +1607,7 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
tunnel->magic = L2TP_TUNNEL_MAGIC;
sprintf(&tunnel->name[0], "tunl %u", tunnel_id);
rwlock_init(&tunnel->hlist_lock);
+ tunnel->acpt_newsess = true;
/* The net we belong to */
tunnel->l2tp_net = net;
@@ -1838,11 +1858,6 @@ struct l2tp_session *l2tp_session_create(int priv_size, struct l2tp_tunnel *tunn
return ERR_PTR(err);
}
- l2tp_tunnel_inc_refcount(tunnel);
-
- /* Ensure tunnel socket isn't deleted */
- sock_hold(tunnel->sock);
-
/* Ignore management session in session count value */
if (session->session_id != 0)
atomic_inc(&l2tp_session_count);
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index 0d4590b25592..8a5d51cff2f3 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -165,6 +165,10 @@ struct l2tp_tunnel {
struct rcu_head rcu;
rwlock_t hlist_lock; /* protect session_hlist */
+ bool acpt_newsess; /* Indicates whether this
+ * tunnel accepts new sessions.
+ * Protected by hlist_lock.
+ */
struct hlist_head session_hlist[L2TP_HASH_SIZE];
/* hashed list of sessions,
* hashed by id */
--
2.27.0.rc0.183.gde8f92d652-goog
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 22/27] l2tp: pass tunnel pointer to ->session_create()
2020-05-21 23:57 [PATCH 00/27] more l2tp locking and ordering fixes Giuliano Procida
` (20 preceding siblings ...)
2020-05-21 23:57 ` [PATCH 21/27] l2tp: prevent creation of sessions on terminated tunnels Giuliano Procida
@ 2020-05-21 23:57 ` Giuliano Procida
2020-05-21 23:57 ` [PATCH 23/27] l2tp: fix l2tp_eth module loading Giuliano Procida
` (5 subsequent siblings)
27 siblings, 0 replies; 30+ messages in thread
From: Giuliano Procida @ 2020-05-21 23:57 UTC (permalink / raw)
To: greg; +Cc: stable, Guillaume Nault, David S . Miller, Giuliano Procida
From: Guillaume Nault <g.nault@alphalink.fr>
commit f026bc29a8e093edfbb2a77700454b285c97e8ad upstream.
Using l2tp_tunnel_find() in pppol2tp_session_create() and
l2tp_eth_create() is racy, because no reference is held on the
returned session. These functions are only used to implement the
->session_create callback which is run by l2tp_nl_cmd_session_create().
Therefore searching for the parent tunnel isn't necessary because
l2tp_nl_cmd_session_create() already has a pointer to it and holds a
reference.
This patch modifies ->session_create()'s prototype to directly pass the
the parent tunnel as parameter, thus avoiding searching for it in
pppol2tp_session_create() and l2tp_eth_create().
Since we have to touch the ->session_create() call in
l2tp_nl_cmd_session_create(), let's also remove the useless conditional:
we know that ->session_create isn't NULL at this point because it's
already been checked earlier in this same function.
Finally, one might be tempted to think that the removed
l2tp_tunnel_find() calls were harmless because they would return the
same tunnel as the one held by l2tp_nl_cmd_session_create() anyway.
But that tunnel might be removed and a new one created with same tunnel
Id before the l2tp_tunnel_find() call. In this case l2tp_tunnel_find()
would return the new tunnel which wouldn't be protected by the
reference held by l2tp_nl_cmd_session_create().
Fixes: 309795f4bec2 ("l2tp: Add netlink control API for L2TP")
Fixes: d9e31d17ceba ("l2tp: Add L2TP ethernet pseudowire support")
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
net/l2tp/l2tp_core.h | 4 +++-
net/l2tp/l2tp_eth.c | 11 +++--------
net/l2tp/l2tp_netlink.c | 8 ++++----
net/l2tp/l2tp_ppp.c | 19 +++++++------------
4 files changed, 17 insertions(+), 25 deletions(-)
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index 8a5d51cff2f3..09cd58f03d89 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -204,7 +204,9 @@ struct l2tp_tunnel {
};
struct l2tp_nl_cmd_ops {
- int (*session_create)(struct net *net, u32 tunnel_id, u32 session_id, u32 peer_session_id, struct l2tp_session_cfg *cfg);
+ int (*session_create)(struct net *net, struct l2tp_tunnel *tunnel,
+ u32 session_id, u32 peer_session_id,
+ struct l2tp_session_cfg *cfg);
int (*session_delete)(struct l2tp_session *session);
};
diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
index cef312da3422..c785308f630b 100644
--- a/net/l2tp/l2tp_eth.c
+++ b/net/l2tp/l2tp_eth.c
@@ -256,23 +256,18 @@ static void l2tp_eth_adjust_mtu(struct l2tp_tunnel *tunnel,
dev->needed_headroom += session->hdr_len;
}
-static int l2tp_eth_create(struct net *net, u32 tunnel_id, u32 session_id, u32 peer_session_id, struct l2tp_session_cfg *cfg)
+static int l2tp_eth_create(struct net *net, struct l2tp_tunnel *tunnel,
+ u32 session_id, u32 peer_session_id,
+ struct l2tp_session_cfg *cfg)
{
struct net_device *dev;
char name[IFNAMSIZ];
- struct l2tp_tunnel *tunnel;
struct l2tp_session *session;
struct l2tp_eth *priv;
struct l2tp_eth_sess *spriv;
int rc;
struct l2tp_eth_net *pn;
- tunnel = l2tp_tunnel_find(net, tunnel_id);
- if (!tunnel) {
- rc = -ENODEV;
- goto out;
- }
-
if (cfg->ifname) {
dev = dev_get_by_name(net, cfg->ifname);
if (dev) {
diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index 22917e751edf..d3a84a181348 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -627,10 +627,10 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf
break;
}
- ret = -EPROTONOSUPPORT;
- if (l2tp_nl_cmd_ops[cfg.pw_type]->session_create)
- ret = (*l2tp_nl_cmd_ops[cfg.pw_type]->session_create)(net, tunnel_id,
- session_id, peer_session_id, &cfg);
+ ret = l2tp_nl_cmd_ops[cfg.pw_type]->session_create(net, tunnel,
+ session_id,
+ peer_session_id,
+ &cfg);
if (ret >= 0) {
session = l2tp_session_get(net, tunnel, session_id, false);
diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index 5738456b3b58..377ef5f0f39a 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -810,25 +810,20 @@ end:
#ifdef CONFIG_L2TP_V3
-/* Called when creating sessions via the netlink interface.
- */
-static int pppol2tp_session_create(struct net *net, u32 tunnel_id, u32 session_id, u32 peer_session_id, struct l2tp_session_cfg *cfg)
+/* Called when creating sessions via the netlink interface. */
+static int pppol2tp_session_create(struct net *net, struct l2tp_tunnel *tunnel,
+ u32 session_id, u32 peer_session_id,
+ struct l2tp_session_cfg *cfg)
{
int error;
- struct l2tp_tunnel *tunnel;
struct l2tp_session *session;
struct pppol2tp_session *ps;
- tunnel = l2tp_tunnel_find(net, tunnel_id);
-
- /* Error if we can't find the tunnel */
- error = -ENOENT;
- if (tunnel == NULL)
- goto out;
-
/* Error if tunnel socket is not prepped */
- if (tunnel->sock == NULL)
+ if (!tunnel->sock) {
+ error = -ENOENT;
goto out;
+ }
/* Default MTU values. */
if (cfg->mtu == 0)
--
2.27.0.rc0.183.gde8f92d652-goog
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 23/27] l2tp: fix l2tp_eth module loading
2020-05-21 23:57 [PATCH 00/27] more l2tp locking and ordering fixes Giuliano Procida
` (21 preceding siblings ...)
2020-05-21 23:57 ` [PATCH 22/27] l2tp: pass tunnel pointer to ->session_create() Giuliano Procida
@ 2020-05-21 23:57 ` Giuliano Procida
2020-05-21 23:57 ` [PATCH 24/27] l2tp: don't register sessions in l2tp_session_create() Giuliano Procida
` (4 subsequent siblings)
27 siblings, 0 replies; 30+ messages in thread
From: Giuliano Procida @ 2020-05-21 23:57 UTC (permalink / raw)
To: greg; +Cc: stable, Guillaume Nault, David S . Miller, Giuliano Procida
From: Guillaume Nault <g.nault@alphalink.fr>
commit 9f775ead5e570e7e19015b9e4e2f3dd6e71a5935 upstream.
The l2tp_eth module crashes if its netlink callbacks are run when the
pernet data aren't initialised.
We should normally register_pernet_device() before the genl callbacks.
However, the pernet data only maintain a list of l2tpeth interfaces,
and this list is never used. So let's just drop pernet handling
instead.
Fixes: d9e31d17ceba ("l2tp: Add L2TP ethernet pseudowire support")
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
net/l2tp/l2tp_eth.c | 51 ++-------------------------------------------
1 file changed, 2 insertions(+), 49 deletions(-)
diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
index c785308f630b..ab6d2152eafb 100644
--- a/net/l2tp/l2tp_eth.c
+++ b/net/l2tp/l2tp_eth.c
@@ -44,7 +44,6 @@ struct l2tp_eth {
struct net_device *dev;
struct sock *tunnel_sock;
struct l2tp_session *session;
- struct list_head list;
atomic_long_t tx_bytes;
atomic_long_t tx_packets;
atomic_long_t tx_dropped;
@@ -58,17 +57,6 @@ struct l2tp_eth_sess {
struct net_device *dev;
};
-/* per-net private data for this module */
-static unsigned int l2tp_eth_net_id;
-struct l2tp_eth_net {
- struct list_head l2tp_eth_dev_list;
- spinlock_t l2tp_eth_lock;
-};
-
-static inline struct l2tp_eth_net *l2tp_eth_pernet(struct net *net)
-{
- return net_generic(net, l2tp_eth_net_id);
-}
static struct lock_class_key l2tp_eth_tx_busylock;
static int l2tp_eth_dev_init(struct net_device *dev)
@@ -84,12 +72,6 @@ static int l2tp_eth_dev_init(struct net_device *dev)
static void l2tp_eth_dev_uninit(struct net_device *dev)
{
- struct l2tp_eth *priv = netdev_priv(dev);
- struct l2tp_eth_net *pn = l2tp_eth_pernet(dev_net(dev));
-
- spin_lock(&pn->l2tp_eth_lock);
- list_del_init(&priv->list);
- spin_unlock(&pn->l2tp_eth_lock);
dev_put(dev);
}
@@ -266,7 +248,6 @@ static int l2tp_eth_create(struct net *net, struct l2tp_tunnel *tunnel,
struct l2tp_eth *priv;
struct l2tp_eth_sess *spriv;
int rc;
- struct l2tp_eth_net *pn;
if (cfg->ifname) {
dev = dev_get_by_name(net, cfg->ifname);
@@ -299,7 +280,6 @@ static int l2tp_eth_create(struct net *net, struct l2tp_tunnel *tunnel,
priv = netdev_priv(dev);
priv->dev = dev;
priv->session = session;
- INIT_LIST_HEAD(&priv->list);
priv->tunnel_sock = tunnel->sock;
session->recv_skb = l2tp_eth_dev_recv;
@@ -320,10 +300,6 @@ static int l2tp_eth_create(struct net *net, struct l2tp_tunnel *tunnel,
strlcpy(session->ifname, dev->name, IFNAMSIZ);
dev_hold(dev);
- pn = l2tp_eth_pernet(dev_net(dev));
- spin_lock(&pn->l2tp_eth_lock);
- list_add(&priv->list, &pn->l2tp_eth_dev_list);
- spin_unlock(&pn->l2tp_eth_lock);
return 0;
@@ -336,22 +312,6 @@ out:
return rc;
}
-static __net_init int l2tp_eth_init_net(struct net *net)
-{
- struct l2tp_eth_net *pn = net_generic(net, l2tp_eth_net_id);
-
- INIT_LIST_HEAD(&pn->l2tp_eth_dev_list);
- spin_lock_init(&pn->l2tp_eth_lock);
-
- return 0;
-}
-
-static struct pernet_operations l2tp_eth_net_ops = {
- .init = l2tp_eth_init_net,
- .id = &l2tp_eth_net_id,
- .size = sizeof(struct l2tp_eth_net),
-};
-
static const struct l2tp_nl_cmd_ops l2tp_eth_nl_cmd_ops = {
.session_create = l2tp_eth_create,
@@ -365,25 +325,18 @@ static int __init l2tp_eth_init(void)
err = l2tp_nl_register_ops(L2TP_PWTYPE_ETH, &l2tp_eth_nl_cmd_ops);
if (err)
- goto out;
-
- err = register_pernet_device(&l2tp_eth_net_ops);
- if (err)
- goto out_unreg;
+ goto err;
pr_info("L2TP ethernet pseudowire support (L2TPv3)\n");
return 0;
-out_unreg:
- l2tp_nl_unregister_ops(L2TP_PWTYPE_ETH);
-out:
+err:
return err;
}
static void __exit l2tp_eth_exit(void)
{
- unregister_pernet_device(&l2tp_eth_net_ops);
l2tp_nl_unregister_ops(L2TP_PWTYPE_ETH);
}
--
2.27.0.rc0.183.gde8f92d652-goog
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 24/27] l2tp: don't register sessions in l2tp_session_create()
2020-05-21 23:57 [PATCH 00/27] more l2tp locking and ordering fixes Giuliano Procida
` (22 preceding siblings ...)
2020-05-21 23:57 ` [PATCH 23/27] l2tp: fix l2tp_eth module loading Giuliano Procida
@ 2020-05-21 23:57 ` Giuliano Procida
2020-05-21 23:57 ` [PATCH 25/27] l2tp: initialise l2tp_eth sessions before registering them Giuliano Procida
` (3 subsequent siblings)
27 siblings, 0 replies; 30+ messages in thread
From: Giuliano Procida @ 2020-05-21 23:57 UTC (permalink / raw)
To: greg; +Cc: stable, Guillaume Nault, David S . Miller, Giuliano Procida
From: Guillaume Nault <g.nault@alphalink.fr>
commit 3953ae7b218df4d1e544b98a393666f9ae58a78c upstream.
Sessions created by l2tp_session_create() aren't fully initialised:
some pseudo-wire specific operations need to be done before making the
session usable. Therefore the PPP and Ethernet pseudo-wires continue
working on the returned l2tp session while it's already been exposed to
the rest of the system.
This can lead to various issues. In particular, the session may enter
the deletion process before having been fully initialised, which will
confuse the session removal code.
This patch moves session registration out of l2tp_session_create(), so
that callers can control when the session is exposed to the rest of the
system. This is done by the new l2tp_session_register() function.
Only pppol2tp_session_create() can be easily converted to avoid
modifying its session after registration (the debug message is dropped
in order to avoid the need for holding a reference on the session).
For pppol2tp_connect() and l2tp_eth_create()), more work is needed.
That'll be done in followup patches. For now, let's just register the
session right after its creation, like it was done before. The only
difference is that we can easily take a reference on the session before
registering it, so, at least, we're sure it's not going to be freed
while we're working on it.
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
net/l2tp/l2tp_core.c | 21 +++++++--------------
net/l2tp/l2tp_core.h | 3 +++
net/l2tp/l2tp_eth.c | 9 +++++++++
net/l2tp/l2tp_ppp.c | 23 +++++++++++++++++------
4 files changed, 36 insertions(+), 20 deletions(-)
diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index ba11e72a2365..0233c496fc51 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -321,8 +321,8 @@ struct l2tp_session *l2tp_session_get_by_ifname(const struct net *net,
}
EXPORT_SYMBOL_GPL(l2tp_session_get_by_ifname);
-static int l2tp_session_add_to_tunnel(struct l2tp_tunnel *tunnel,
- struct l2tp_session *session)
+int l2tp_session_register(struct l2tp_session *session,
+ struct l2tp_tunnel *tunnel)
{
struct l2tp_session *session_walk;
struct hlist_head *g_head;
@@ -370,6 +370,10 @@ static int l2tp_session_add_to_tunnel(struct l2tp_tunnel *tunnel,
hlist_add_head(&session->hlist, head);
write_unlock_bh(&tunnel->hlist_lock);
+ /* Ignore management session in session count value */
+ if (session->session_id != 0)
+ atomic_inc(&l2tp_session_count);
+
return 0;
err_tlock_pnlock:
@@ -379,6 +383,7 @@ err_tlock:
return err;
}
+EXPORT_SYMBOL_GPL(l2tp_session_register);
/* Lookup a tunnel by id
*/
@@ -1793,7 +1798,6 @@ EXPORT_SYMBOL_GPL(l2tp_session_set_header_len);
struct l2tp_session *l2tp_session_create(int priv_size, struct l2tp_tunnel *tunnel, u32 session_id, u32 peer_session_id, struct l2tp_session_cfg *cfg)
{
struct l2tp_session *session;
- int err;
session = kzalloc(sizeof(struct l2tp_session) + priv_size, GFP_KERNEL);
if (session != NULL) {
@@ -1851,17 +1855,6 @@ struct l2tp_session *l2tp_session_create(int priv_size, struct l2tp_tunnel *tunn
l2tp_session_inc_refcount(session);
- err = l2tp_session_add_to_tunnel(tunnel, session);
- if (err) {
- kfree(session);
-
- return ERR_PTR(err);
- }
-
- /* Ignore management session in session count value */
- if (session->session_id != 0)
- atomic_inc(&l2tp_session_count);
-
return session;
}
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index 09cd58f03d89..57da0f1d62dd 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -262,6 +262,9 @@ struct l2tp_session *l2tp_session_create(int priv_size,
struct l2tp_tunnel *tunnel,
u32 session_id, u32 peer_session_id,
struct l2tp_session_cfg *cfg);
+int l2tp_session_register(struct l2tp_session *session,
+ struct l2tp_tunnel *tunnel);
+
void __l2tp_session_unhash(struct l2tp_session *session);
int l2tp_session_delete(struct l2tp_session *session);
void l2tp_session_free(struct l2tp_session *session);
diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
index ab6d2152eafb..750662de735e 100644
--- a/net/l2tp/l2tp_eth.c
+++ b/net/l2tp/l2tp_eth.c
@@ -267,6 +267,13 @@ static int l2tp_eth_create(struct net *net, struct l2tp_tunnel *tunnel,
goto out;
}
+ l2tp_session_inc_refcount(session);
+ rc = l2tp_session_register(session, tunnel);
+ if (rc < 0) {
+ kfree(session);
+ goto out;
+ }
+
dev = alloc_netdev(sizeof(*priv), name, NET_NAME_UNKNOWN,
l2tp_eth_dev_setup);
if (!dev) {
@@ -298,6 +305,7 @@ static int l2tp_eth_create(struct net *net, struct l2tp_tunnel *tunnel,
__module_get(THIS_MODULE);
/* Must be done after register_netdev() */
strlcpy(session->ifname, dev->name, IFNAMSIZ);
+ l2tp_session_dec_refcount(session);
dev_hold(dev);
@@ -308,6 +316,7 @@ out_del_dev:
spriv->dev = NULL;
out_del_session:
l2tp_session_delete(session);
+ l2tp_session_dec_refcount(session);
out:
return rc;
}
diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index 377ef5f0f39a..ebb24a7120ca 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -737,6 +737,14 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
error = PTR_ERR(session);
goto end;
}
+
+ l2tp_session_inc_refcount(session);
+ error = l2tp_session_register(session, tunnel);
+ if (error < 0) {
+ kfree(session);
+ goto end;
+ }
+ drop_refcnt = true;
}
/* Associate session with its PPPoL2TP socket */
@@ -822,7 +830,7 @@ static int pppol2tp_session_create(struct net *net, struct l2tp_tunnel *tunnel,
/* Error if tunnel socket is not prepped */
if (!tunnel->sock) {
error = -ENOENT;
- goto out;
+ goto err;
}
/* Default MTU values. */
@@ -837,18 +845,21 @@ static int pppol2tp_session_create(struct net *net, struct l2tp_tunnel *tunnel,
peer_session_id, cfg);
if (IS_ERR(session)) {
error = PTR_ERR(session);
- goto out;
+ goto err;
}
ps = l2tp_session_priv(session);
ps->tunnel_sock = tunnel->sock;
- l2tp_info(session, L2TP_MSG_CONTROL, "%s: created\n",
- session->name);
+ error = l2tp_session_register(session, tunnel);
+ if (error < 0)
+ goto err_sess;
- error = 0;
+ return 0;
-out:
+err_sess:
+ kfree(session);
+err:
return error;
}
--
2.27.0.rc0.183.gde8f92d652-goog
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 25/27] l2tp: initialise l2tp_eth sessions before registering them
2020-05-21 23:57 [PATCH 00/27] more l2tp locking and ordering fixes Giuliano Procida
` (23 preceding siblings ...)
2020-05-21 23:57 ` [PATCH 24/27] l2tp: don't register sessions in l2tp_session_create() Giuliano Procida
@ 2020-05-21 23:57 ` Giuliano Procida
2020-05-21 23:57 ` [PATCH 26/27] l2tp: protect sock pointer of struct pppol2tp_session with RCU Giuliano Procida
` (2 subsequent siblings)
27 siblings, 0 replies; 30+ messages in thread
From: Giuliano Procida @ 2020-05-21 23:57 UTC (permalink / raw)
To: greg; +Cc: stable, Guillaume Nault, David S . Miller, Giuliano Procida
From: Guillaume Nault <g.nault@alphalink.fr>
commit ee28de6bbd78c2e18111a0aef43ea746f28d2073 upstream.
Sessions must be initialised before being made externally visible by
l2tp_session_register(). Otherwise the session may be concurrently
deleted before being initialised, which can confuse the deletion path
and eventually lead to kernel oops.
Therefore, we need to move l2tp_session_register() down in
l2tp_eth_create(), but also handle the intermediate step where only the
session or the netdevice has been registered.
We can't just call l2tp_session_register() in ->ndo_init() because
we'd have no way to properly undo this operation in ->ndo_uninit().
Instead, let's register the session and the netdevice in two different
steps and protect the session's device pointer with RCU.
And now that we allow the session's .dev field to be NULL, we don't
need to prevent the netdevice from being removed anymore. So we can
drop the dev_hold() and dev_put() calls in l2tp_eth_create() and
l2tp_eth_dev_uninit().
Backporting Notes
l2tp_eth.c: In l2tp_eth_create the "out" label was renamed to "err".
There was one extra occurrence of "goto out" to update.
Fixes: d9e31d17ceba ("l2tp: Add L2TP ethernet pseudowire support")
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
net/l2tp/l2tp_eth.c | 108 +++++++++++++++++++++++++++++++-------------
1 file changed, 76 insertions(+), 32 deletions(-)
diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
index 750662de735e..facc180d1635 100644
--- a/net/l2tp/l2tp_eth.c
+++ b/net/l2tp/l2tp_eth.c
@@ -54,7 +54,7 @@ struct l2tp_eth {
/* via l2tp_session_priv() */
struct l2tp_eth_sess {
- struct net_device *dev;
+ struct net_device __rcu *dev;
};
@@ -72,7 +72,14 @@ static int l2tp_eth_dev_init(struct net_device *dev)
static void l2tp_eth_dev_uninit(struct net_device *dev)
{
- dev_put(dev);
+ struct l2tp_eth *priv = netdev_priv(dev);
+ struct l2tp_eth_sess *spriv;
+
+ spriv = l2tp_session_priv(priv->session);
+ RCU_INIT_POINTER(spriv->dev, NULL);
+ /* No need for synchronize_net() here. We're called by
+ * unregister_netdev*(), which does the synchronisation for us.
+ */
}
static int l2tp_eth_dev_xmit(struct sk_buff *skb, struct net_device *dev)
@@ -126,8 +133,8 @@ static void l2tp_eth_dev_setup(struct net_device *dev)
static void l2tp_eth_dev_recv(struct l2tp_session *session, struct sk_buff *skb, int data_len)
{
struct l2tp_eth_sess *spriv = l2tp_session_priv(session);
- struct net_device *dev = spriv->dev;
- struct l2tp_eth *priv = netdev_priv(dev);
+ struct net_device *dev;
+ struct l2tp_eth *priv;
if (session->debug & L2TP_MSG_DATA) {
unsigned int length;
@@ -151,16 +158,25 @@ static void l2tp_eth_dev_recv(struct l2tp_session *session, struct sk_buff *skb,
skb_dst_drop(skb);
nf_reset(skb);
+ rcu_read_lock();
+ dev = rcu_dereference(spriv->dev);
+ if (!dev)
+ goto error_rcu;
+
+ priv = netdev_priv(dev);
if (dev_forward_skb(dev, skb) == NET_RX_SUCCESS) {
atomic_long_inc(&priv->rx_packets);
atomic_long_add(data_len, &priv->rx_bytes);
} else {
atomic_long_inc(&priv->rx_errors);
}
+ rcu_read_unlock();
+
return;
+error_rcu:
+ rcu_read_unlock();
error:
- atomic_long_inc(&priv->rx_errors);
kfree_skb(skb);
}
@@ -171,11 +187,15 @@ static void l2tp_eth_delete(struct l2tp_session *session)
if (session) {
spriv = l2tp_session_priv(session);
- dev = spriv->dev;
+
+ rtnl_lock();
+ dev = rtnl_dereference(spriv->dev);
if (dev) {
- unregister_netdev(dev);
- spriv->dev = NULL;
+ unregister_netdevice(dev);
+ rtnl_unlock();
module_put(THIS_MODULE);
+ } else {
+ rtnl_unlock();
}
}
}
@@ -185,9 +205,20 @@ static void l2tp_eth_show(struct seq_file *m, void *arg)
{
struct l2tp_session *session = arg;
struct l2tp_eth_sess *spriv = l2tp_session_priv(session);
- struct net_device *dev = spriv->dev;
+ struct net_device *dev;
+
+ rcu_read_lock();
+ dev = rcu_dereference(spriv->dev);
+ if (!dev) {
+ rcu_read_unlock();
+ return;
+ }
+ dev_hold(dev);
+ rcu_read_unlock();
seq_printf(m, " interface %s\n", dev->name);
+
+ dev_put(dev);
}
#endif
@@ -254,7 +285,7 @@ static int l2tp_eth_create(struct net *net, struct l2tp_tunnel *tunnel,
if (dev) {
dev_put(dev);
rc = -EEXIST;
- goto out;
+ goto err;
}
strlcpy(name, cfg->ifname, IFNAMSIZ);
} else
@@ -264,21 +295,14 @@ static int l2tp_eth_create(struct net *net, struct l2tp_tunnel *tunnel,
peer_session_id, cfg);
if (IS_ERR(session)) {
rc = PTR_ERR(session);
- goto out;
- }
-
- l2tp_session_inc_refcount(session);
- rc = l2tp_session_register(session, tunnel);
- if (rc < 0) {
- kfree(session);
- goto out;
+ goto err;
}
dev = alloc_netdev(sizeof(*priv), name, NET_NAME_UNKNOWN,
l2tp_eth_dev_setup);
if (!dev) {
rc = -ENOMEM;
- goto out_del_session;
+ goto err_sess;
}
dev_net_set(dev, net);
@@ -296,28 +320,48 @@ static int l2tp_eth_create(struct net *net, struct l2tp_tunnel *tunnel,
#endif
spriv = l2tp_session_priv(session);
- spriv->dev = dev;
- rc = register_netdev(dev);
- if (rc < 0)
- goto out_del_dev;
+ l2tp_session_inc_refcount(session);
+
+ rtnl_lock();
+
+ /* Register both device and session while holding the rtnl lock. This
+ * ensures that l2tp_eth_delete() will see that there's a device to
+ * unregister, even if it happened to run before we assign spriv->dev.
+ */
+ rc = l2tp_session_register(session, tunnel);
+ if (rc < 0) {
+ rtnl_unlock();
+ goto err_sess_dev;
+ }
+
+ rc = register_netdevice(dev);
+ if (rc < 0) {
+ rtnl_unlock();
+ l2tp_session_delete(session);
+ l2tp_session_dec_refcount(session);
+ free_netdev(dev);
+
+ return rc;
+ }
- __module_get(THIS_MODULE);
- /* Must be done after register_netdev() */
strlcpy(session->ifname, dev->name, IFNAMSIZ);
+ rcu_assign_pointer(spriv->dev, dev);
+
+ rtnl_unlock();
+
l2tp_session_dec_refcount(session);
- dev_hold(dev);
+ __module_get(THIS_MODULE);
return 0;
-out_del_dev:
- free_netdev(dev);
- spriv->dev = NULL;
-out_del_session:
- l2tp_session_delete(session);
+err_sess_dev:
l2tp_session_dec_refcount(session);
-out:
+ free_netdev(dev);
+err_sess:
+ kfree(session);
+err:
return rc;
}
--
2.27.0.rc0.183.gde8f92d652-goog
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 26/27] l2tp: protect sock pointer of struct pppol2tp_session with RCU
2020-05-21 23:57 [PATCH 00/27] more l2tp locking and ordering fixes Giuliano Procida
` (24 preceding siblings ...)
2020-05-21 23:57 ` [PATCH 25/27] l2tp: initialise l2tp_eth sessions before registering them Giuliano Procida
@ 2020-05-21 23:57 ` Giuliano Procida
2020-05-21 23:57 ` [PATCH 27/27] l2tp: initialise PPP sessions before registering them Giuliano Procida
2020-05-26 10:54 ` [PATCH 00/27] more l2tp locking and ordering fixes Greg KH
27 siblings, 0 replies; 30+ messages in thread
From: Giuliano Procida @ 2020-05-21 23:57 UTC (permalink / raw)
To: greg; +Cc: stable, Guillaume Nault, David S . Miller, Giuliano Procida
From: Guillaume Nault <g.nault@alphalink.fr>
commit ee40fb2e1eb5bc0ddd3f2f83c6e39a454ef5a741 upstream.
pppol2tp_session_create() registers sessions that can't have their
corresponding socket initialised. This socket has to be created by
userspace, then connected to the session by pppol2tp_connect().
Therefore, we need to protect the pppol2tp socket pointer of L2TP
sessions, so that it can safely be updated when userspace is connecting
or closing the socket. This will eventually allow pppol2tp_connect()
to avoid generating transient states while initialising its parts of the
session.
To this end, this patch protects the pppol2tp socket pointer using RCU.
The pppol2tp socket pointer is still set in pppol2tp_connect(), but
only once we know the function isn't going to fail. It's eventually
reset by pppol2tp_release(), which now has to wait for a grace period
to elapse before it can drop the last reference on the socket. This
ensures that pppol2tp_session_get_sock() can safely grab a reference
on the socket, even after ps->sk is reset to NULL but before this
operation actually gets visible from pppol2tp_session_get_sock().
The rest is standard RCU conversion: pppol2tp_recv(), which already
runs in atomic context, is simply enclosed by rcu_read_lock() and
rcu_read_unlock(), while other functions are converted to use
pppol2tp_session_get_sock() followed by sock_put().
pppol2tp_session_setsockopt() is a special case. It used to retrieve
the pppol2tp socket from the L2TP session, which itself was retrieved
from the pppol2tp socket. Therefore we can just avoid dereferencing
ps->sk and directly use the original socket pointer instead.
With all users of ps->sk now handling NULL and concurrent updates, the
L2TP ->ref() and ->deref() callbacks aren't needed anymore. Therefore,
rather than converting pppol2tp_session_sock_hold() and
pppol2tp_session_sock_put(), we can just drop them.
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
net/l2tp/l2tp_ppp.c | 154 +++++++++++++++++++++++++++++---------------
1 file changed, 101 insertions(+), 53 deletions(-)
diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index ebb24a7120ca..4ba4546051ed 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -122,8 +122,11 @@
struct pppol2tp_session {
int owner; /* pid that opened the socket */
- struct sock *sock; /* Pointer to the session
+ struct mutex sk_lock; /* Protects .sk */
+ struct sock __rcu *sk; /* Pointer to the session
* PPPoX socket */
+ struct sock *__sk; /* Copy of .sk, for cleanup */
+ struct rcu_head rcu; /* For asynchronous release */
struct sock *tunnel_sock; /* Pointer to the tunnel UDP
* socket */
int flags; /* accessed by PPPIOCGFLAGS.
@@ -138,6 +141,24 @@ static const struct ppp_channel_ops pppol2tp_chan_ops = {
static const struct proto_ops pppol2tp_ops;
+/* Retrieves the pppol2tp socket associated to a session.
+ * A reference is held on the returned socket, so this function must be paired
+ * with sock_put().
+ */
+static struct sock *pppol2tp_session_get_sock(struct l2tp_session *session)
+{
+ struct pppol2tp_session *ps = l2tp_session_priv(session);
+ struct sock *sk;
+
+ rcu_read_lock();
+ sk = rcu_dereference(ps->sk);
+ if (sk)
+ sock_hold(sk);
+ rcu_read_unlock();
+
+ return sk;
+}
+
/* Helpers to obtain tunnel/session contexts from sockets.
*/
static inline struct l2tp_session *pppol2tp_sock_to_session(struct sock *sk)
@@ -224,7 +245,8 @@ static void pppol2tp_recv(struct l2tp_session *session, struct sk_buff *skb, int
/* If the socket is bound, send it in to PPP's input queue. Otherwise
* queue it on the session socket.
*/
- sk = ps->sock;
+ rcu_read_lock();
+ sk = rcu_dereference(ps->sk);
if (sk == NULL)
goto no_sock;
@@ -262,30 +284,16 @@ static void pppol2tp_recv(struct l2tp_session *session, struct sk_buff *skb, int
kfree_skb(skb);
}
}
+ rcu_read_unlock();
return;
no_sock:
+ rcu_read_unlock();
l2tp_info(session, L2TP_MSG_DATA, "%s: no socket\n", session->name);
kfree_skb(skb);
}
-static void pppol2tp_session_sock_hold(struct l2tp_session *session)
-{
- struct pppol2tp_session *ps = l2tp_session_priv(session);
-
- if (ps->sock)
- sock_hold(ps->sock);
-}
-
-static void pppol2tp_session_sock_put(struct l2tp_session *session)
-{
- struct pppol2tp_session *ps = l2tp_session_priv(session);
-
- if (ps->sock)
- sock_put(ps->sock);
-}
-
/************************************************************************
* Transmit handling
***********************************************************************/
@@ -446,14 +454,16 @@ abort:
*/
static void pppol2tp_session_close(struct l2tp_session *session)
{
- struct pppol2tp_session *ps = l2tp_session_priv(session);
- struct sock *sk = ps->sock;
- struct socket *sock = sk->sk_socket;
+ struct sock *sk;
BUG_ON(session->magic != L2TP_SESSION_MAGIC);
- if (sock)
- inet_shutdown(sock, SEND_SHUTDOWN);
+ sk = pppol2tp_session_get_sock(session);
+ if (sk) {
+ if (sk->sk_socket)
+ inet_shutdown(sk->sk_socket, SEND_SHUTDOWN);
+ sock_put(sk);
+ }
/* Don't let the session go away before our socket does */
l2tp_session_inc_refcount(session);
@@ -476,6 +486,14 @@ static void pppol2tp_session_destruct(struct sock *sk)
}
}
+static void pppol2tp_put_sk(struct rcu_head *head)
+{
+ struct pppol2tp_session *ps;
+
+ ps = container_of(head, typeof(*ps), rcu);
+ sock_put(ps->__sk);
+}
+
/* Called when the PPPoX socket (session) is closed.
*/
static int pppol2tp_release(struct socket *sock)
@@ -501,11 +519,24 @@ static int pppol2tp_release(struct socket *sock)
session = pppol2tp_sock_to_session(sk);
- /* Purge any queued data */
if (session != NULL) {
+ struct pppol2tp_session *ps;
+
__l2tp_session_unhash(session);
l2tp_session_queue_purge(session);
- sock_put(sk);
+
+ ps = l2tp_session_priv(session);
+ mutex_lock(&ps->sk_lock);
+ ps->__sk = rcu_dereference_protected(ps->sk,
+ lockdep_is_held(&ps->sk_lock));
+ RCU_INIT_POINTER(ps->sk, NULL);
+ mutex_unlock(&ps->sk_lock);
+ call_rcu(&ps->rcu, pppol2tp_put_sk);
+
+ /* Rely on the sock_put() call at the end of the function for
+ * dropping the reference held by pppol2tp_sock_to_session().
+ * The last reference will be dropped by pppol2tp_put_sk().
+ */
}
release_sock(sk);
@@ -572,12 +603,14 @@ out:
static void pppol2tp_show(struct seq_file *m, void *arg)
{
struct l2tp_session *session = arg;
- struct pppol2tp_session *ps = l2tp_session_priv(session);
+ struct sock *sk;
+
+ sk = pppol2tp_session_get_sock(session);
+ if (sk) {
+ struct pppox_sock *po = pppox_sk(sk);
- if (ps) {
- struct pppox_sock *po = pppox_sk(ps->sock);
- if (po)
- seq_printf(m, " interface %s\n", ppp_dev_name(&po->chan));
+ seq_printf(m, " interface %s\n", ppp_dev_name(&po->chan));
+ sock_put(sk);
}
}
#endif
@@ -715,13 +748,17 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
/* Using a pre-existing session is fine as long as it hasn't
* been connected yet.
*/
- if (ps->sock) {
+ mutex_lock(&ps->sk_lock);
+ if (rcu_dereference_protected(ps->sk,
+ lockdep_is_held(&ps->sk_lock))) {
+ mutex_unlock(&ps->sk_lock);
error = -EEXIST;
goto end;
}
/* consistency checks */
if (ps->tunnel_sock != tunnel->sock) {
+ mutex_unlock(&ps->sk_lock);
error = -EEXIST;
goto end;
}
@@ -738,19 +775,21 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
goto end;
}
+ ps = l2tp_session_priv(session);
+ mutex_init(&ps->sk_lock);
l2tp_session_inc_refcount(session);
+
+ mutex_lock(&ps->sk_lock);
error = l2tp_session_register(session, tunnel);
if (error < 0) {
+ mutex_unlock(&ps->sk_lock);
kfree(session);
goto end;
}
drop_refcnt = true;
}
- /* Associate session with its PPPoL2TP socket */
- ps = l2tp_session_priv(session);
ps->owner = current->pid;
- ps->sock = sk;
ps->tunnel_sock = tunnel->sock;
session->recv_skb = pppol2tp_recv;
@@ -759,12 +798,6 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
session->show = pppol2tp_show;
#endif
- /* We need to know each time a skb is dropped from the reorder
- * queue.
- */
- session->ref = pppol2tp_session_sock_hold;
- session->deref = pppol2tp_session_sock_put;
-
/* If PMTU discovery was enabled, use the MTU that was discovered */
dst = sk_dst_get(tunnel->sock);
if (dst != NULL) {
@@ -798,12 +831,17 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
po->chan.mtu = session->mtu;
error = ppp_register_net_channel(sock_net(sk), &po->chan);
- if (error)
+ if (error) {
+ mutex_unlock(&ps->sk_lock);
goto end;
+ }
out_no_ppp:
/* This is how we get the session context from the socket. */
sk->sk_user_data = session;
+ rcu_assign_pointer(ps->sk, sk);
+ mutex_unlock(&ps->sk_lock);
+
sk->sk_state = PPPOX_CONNECTED;
l2tp_info(session, L2TP_MSG_CONTROL, "%s: created\n",
session->name);
@@ -849,6 +887,7 @@ static int pppol2tp_session_create(struct net *net, struct l2tp_tunnel *tunnel,
}
ps = l2tp_session_priv(session);
+ mutex_init(&ps->sk_lock);
ps->tunnel_sock = tunnel->sock;
error = l2tp_session_register(session, tunnel);
@@ -1020,12 +1059,10 @@ static int pppol2tp_session_ioctl(struct l2tp_session *session,
"%s: pppol2tp_session_ioctl(cmd=%#x, arg=%#lx)\n",
session->name, cmd, arg);
- sk = ps->sock;
+ sk = pppol2tp_session_get_sock(session);
if (!sk)
return -EBADR;
- sock_hold(sk);
-
switch (cmd) {
case SIOCGIFMTU:
err = -ENXIO;
@@ -1301,7 +1338,6 @@ static int pppol2tp_session_setsockopt(struct sock *sk,
int optname, int val)
{
int err = 0;
- struct pppol2tp_session *ps = l2tp_session_priv(session);
switch (optname) {
case PPPOL2TP_SO_RECVSEQ:
@@ -1322,8 +1358,8 @@ static int pppol2tp_session_setsockopt(struct sock *sk,
}
session->send_seq = val ? -1 : 0;
{
- struct sock *ssk = ps->sock;
- struct pppox_sock *po = pppox_sk(ssk);
+ struct pppox_sock *po = pppox_sk(sk);
+
po->chan.hdrlen = val ? PPPOL2TP_L2TP_HDR_SIZE_SEQ :
PPPOL2TP_L2TP_HDR_SIZE_NOSEQ;
}
@@ -1659,8 +1695,9 @@ static void pppol2tp_seq_session_show(struct seq_file *m, void *v)
{
struct l2tp_session *session = v;
struct l2tp_tunnel *tunnel = session->tunnel;
- struct pppol2tp_session *ps = l2tp_session_priv(session);
- struct pppox_sock *po = pppox_sk(ps->sock);
+ unsigned char state;
+ char user_data_ok;
+ struct sock *sk;
u32 ip = 0;
u16 port = 0;
@@ -1670,6 +1707,15 @@ static void pppol2tp_seq_session_show(struct seq_file *m, void *v)
port = ntohs(inet->inet_sport);
}
+ sk = pppol2tp_session_get_sock(session);
+ if (sk) {
+ state = sk->sk_state;
+ user_data_ok = (session == sk->sk_user_data) ? 'Y' : 'N';
+ } else {
+ state = 0;
+ user_data_ok = 'N';
+ }
+
seq_printf(m, " SESSION '%s' %08X/%d %04X/%04X -> "
"%04X/%04X %d %c\n",
session->name, ip, port,
@@ -1677,9 +1723,7 @@ static void pppol2tp_seq_session_show(struct seq_file *m, void *v)
session->session_id,
tunnel->peer_tunnel_id,
session->peer_session_id,
- ps->sock->sk_state,
- (session == ps->sock->sk_user_data) ?
- 'Y' : 'N');
+ state, user_data_ok);
seq_printf(m, " %d/%d/%c/%c/%s %08x %u\n",
session->mtu, session->mru,
session->recv_seq ? 'R' : '-',
@@ -1696,8 +1740,12 @@ static void pppol2tp_seq_session_show(struct seq_file *m, void *v)
atomic_long_read(&session->stats.rx_bytes),
atomic_long_read(&session->stats.rx_errors));
- if (po)
+ if (sk) {
+ struct pppox_sock *po = pppox_sk(sk);
+
seq_printf(m, " interface %s\n", ppp_dev_name(&po->chan));
+ sock_put(sk);
+ }
}
static int pppol2tp_seq_show(struct seq_file *m, void *v)
--
2.27.0.rc0.183.gde8f92d652-goog
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 27/27] l2tp: initialise PPP sessions before registering them
2020-05-21 23:57 [PATCH 00/27] more l2tp locking and ordering fixes Giuliano Procida
` (25 preceding siblings ...)
2020-05-21 23:57 ` [PATCH 26/27] l2tp: protect sock pointer of struct pppol2tp_session with RCU Giuliano Procida
@ 2020-05-21 23:57 ` Giuliano Procida
2020-05-26 10:54 ` [PATCH 00/27] more l2tp locking and ordering fixes Greg KH
27 siblings, 0 replies; 30+ messages in thread
From: Giuliano Procida @ 2020-05-21 23:57 UTC (permalink / raw)
To: greg; +Cc: stable, Guillaume Nault, David S . Miller, Giuliano Procida
From: Guillaume Nault <g.nault@alphalink.fr>
commit f98be6c6359e7e4a61aaefb9964c1db31cb9ec0c upstream.
pppol2tp_connect() initialises L2TP sessions after they've been exposed
to the rest of the system by l2tp_session_register(). This puts
sessions into transient states that are the source of several races, in
particular with session's deletion path.
This patch centralises the initialisation code into
pppol2tp_session_init(), which is called before the registration phase.
The only field that can't be set before session registration is the
pppol2tp socket pointer, which has already been converted to RCU. So
pppol2tp_connect() should now be race-free.
The session's .session_close() callback is now set before registration.
Therefore, it's always called when l2tp_core deletes the session, even
if it was created by pppol2tp_session_create() and hasn't been plugged
to a pppol2tp socket yet. That'd prevent session free because the extra
reference taken by pppol2tp_session_close() wouldn't be dropped by the
socket's ->sk_destruct() callback (pppol2tp_session_destruct()).
We could set .session_close() only while connecting a session to its
pppol2tp socket, or teach pppol2tp_session_close() to avoid grabbing a
reference when the session isn't connected, but that'd require adding
some form of synchronisation to be race free.
Instead of that, we can just let the pppol2tp socket hold a reference
on the session as soon as it starts depending on it (that is, in
pppol2tp_connect()). Then we don't need to utilise
pppol2tp_session_close() to hold a reference at the last moment to
prevent l2tp_core from dropping it.
When releasing the socket, pppol2tp_release() now deletes the session
using the standard l2tp_session_delete() function, instead of merely
removing it from hash tables. l2tp_session_delete() drops the reference
the sessions holds on itself, but also makes sure it doesn't remove a
session twice. So it can safely be called, even if l2tp_core already
tried, or is concurrently trying, to remove the session.
Finally, pppol2tp_session_destruct() drops the reference held by the
socket.
Fixes: fd558d186df2 ("l2tp: Split pppol2tp patch into separate l2tp and ppp parts")
Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Giuliano Procida <gprocida@google.com>
---
net/l2tp/l2tp_ppp.c | 69 +++++++++++++++++++++++++--------------------
1 file changed, 38 insertions(+), 31 deletions(-)
diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index 4ba4546051ed..8ff5352bb0e3 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -464,9 +464,6 @@ static void pppol2tp_session_close(struct l2tp_session *session)
inet_shutdown(sk->sk_socket, SEND_SHUTDOWN);
sock_put(sk);
}
-
- /* Don't let the session go away before our socket does */
- l2tp_session_inc_refcount(session);
}
/* Really kill the session socket. (Called from sock_put() if
@@ -522,8 +519,7 @@ static int pppol2tp_release(struct socket *sock)
if (session != NULL) {
struct pppol2tp_session *ps;
- __l2tp_session_unhash(session);
- l2tp_session_queue_purge(session);
+ l2tp_session_delete(session);
ps = l2tp_session_priv(session);
mutex_lock(&ps->sk_lock);
@@ -615,6 +611,35 @@ static void pppol2tp_show(struct seq_file *m, void *arg)
}
#endif
+static void pppol2tp_session_init(struct l2tp_session *session)
+{
+ struct pppol2tp_session *ps;
+ struct dst_entry *dst;
+
+ session->recv_skb = pppol2tp_recv;
+ session->session_close = pppol2tp_session_close;
+#if defined(CONFIG_L2TP_DEBUGFS) || defined(CONFIG_L2TP_DEBUGFS_MODULE)
+ session->show = pppol2tp_show;
+#endif
+
+ ps = l2tp_session_priv(session);
+ mutex_init(&ps->sk_lock);
+ ps->tunnel_sock = session->tunnel->sock;
+ ps->owner = current->pid;
+
+ /* If PMTU discovery was enabled, use the MTU that was discovered */
+ dst = sk_dst_get(session->tunnel->sock);
+ if (dst) {
+ u32 pmtu = dst_mtu(dst);
+
+ if (pmtu) {
+ session->mtu = pmtu - PPPOL2TP_HEADER_OVERHEAD;
+ session->mru = pmtu - PPPOL2TP_HEADER_OVERHEAD;
+ }
+ dst_release(dst);
+ }
+}
+
/* connect() handler. Attach a PPPoX socket to a tunnel UDP socket
*/
static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
@@ -626,7 +651,6 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
struct l2tp_session *session = NULL;
struct l2tp_tunnel *tunnel;
struct pppol2tp_session *ps;
- struct dst_entry *dst;
struct l2tp_session_cfg cfg = { 0, };
int error = 0;
u32 tunnel_id, peer_tunnel_id;
@@ -775,8 +799,8 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
goto end;
}
+ pppol2tp_session_init(session);
ps = l2tp_session_priv(session);
- mutex_init(&ps->sk_lock);
l2tp_session_inc_refcount(session);
mutex_lock(&ps->sk_lock);
@@ -789,26 +813,6 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
drop_refcnt = true;
}
- ps->owner = current->pid;
- ps->tunnel_sock = tunnel->sock;
-
- session->recv_skb = pppol2tp_recv;
- session->session_close = pppol2tp_session_close;
-#if defined(CONFIG_L2TP_DEBUGFS) || defined(CONFIG_L2TP_DEBUGFS_MODULE)
- session->show = pppol2tp_show;
-#endif
-
- /* If PMTU discovery was enabled, use the MTU that was discovered */
- dst = sk_dst_get(tunnel->sock);
- if (dst != NULL) {
- u32 pmtu = dst_mtu(dst);
-
- if (pmtu != 0)
- session->mtu = session->mru = pmtu -
- PPPOL2TP_HEADER_OVERHEAD;
- dst_release(dst);
- }
-
/* Special case: if source & dest session_id == 0x0000, this
* socket is being created to manage the tunnel. Just set up
* the internal context for use by ioctl() and sockopt()
@@ -842,6 +846,12 @@ out_no_ppp:
rcu_assign_pointer(ps->sk, sk);
mutex_unlock(&ps->sk_lock);
+ /* Keep the reference we've grabbed on the session: sk doesn't expect
+ * the session to disappear. pppol2tp_session_destruct() is responsible
+ * for dropping it.
+ */
+ drop_refcnt = false;
+
sk->sk_state = PPPOX_CONNECTED;
l2tp_info(session, L2TP_MSG_CONTROL, "%s: created\n",
session->name);
@@ -863,7 +873,6 @@ static int pppol2tp_session_create(struct net *net, struct l2tp_tunnel *tunnel,
{
int error;
struct l2tp_session *session;
- struct pppol2tp_session *ps;
/* Error if tunnel socket is not prepped */
if (!tunnel->sock) {
@@ -886,9 +895,7 @@ static int pppol2tp_session_create(struct net *net, struct l2tp_tunnel *tunnel,
goto err;
}
- ps = l2tp_session_priv(session);
- mutex_init(&ps->sk_lock);
- ps->tunnel_sock = tunnel->sock;
+ pppol2tp_session_init(session);
error = l2tp_session_register(session, tunnel);
if (error < 0)
--
2.27.0.rc0.183.gde8f92d652-goog
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 00/27] more l2tp locking and ordering fixes
2020-05-21 23:57 [PATCH 00/27] more l2tp locking and ordering fixes Giuliano Procida
` (26 preceding siblings ...)
2020-05-21 23:57 ` [PATCH 27/27] l2tp: initialise PPP sessions before registering them Giuliano Procida
@ 2020-05-26 10:54 ` Greg KH
27 siblings, 0 replies; 30+ messages in thread
From: Greg KH @ 2020-05-26 10:54 UTC (permalink / raw)
To: Giuliano Procida; +Cc: stable
On Fri, May 22, 2020 at 12:57:13AM +0100, Giuliano Procida wrote:
> Hi Greg.
>
> This is for 4.4.
>
> This is a follow-up to "l2tp locking and ordering fixes" for 4.9.
All now queued up, thanks.
greg k-h
^ permalink raw reply [flat|nested] 30+ messages in thread