[RFC PATCH 0/3] net namespace support in l2tp_core

[RFC PATCH 0/3] net namespace support in l2tp_core

[Tom Parkin]

This exploratory patchset is an attempt to fix up l2tp_core to behave properly
with network namespaces, based on issues raised previously on netdev[1].

The reason I'm posting it as an RFC is that I'm seeing some bugs that I'm
having trouble tracking down, and I'd appreciate any suggestions as to what
I'm doing wrong.

During stress testing I see kernel oopses after a few hours.  My stress test is 
a script which spawns lots of sub-processes which create and delete multiple 
namespaces containing multiple l2tp tunnels.  I've seen oopses in various 
locations -- for example, ndist_net_exit, dst_ifdown, 
netlink_broadcast_filtered, dev_change_net_namespace.  I'm assuming my changes 
are causing some race condition which might bite in multiple places depending 
on timing.

So far I'm guessing that the method I'm using to clean up l2tp tunnel sockets
in the net namespace callback may be to blame.  In the case of "unmanaged"
sockets (see patch 3 for details) the socket is created using:

	sock_create_kern
	sk_change_net

to create a socket without a namespace reference.  When the namespace exits, I
walk the per-namespace list of tunnels, calling sk_release_kernel to release
the socket.  The l2tp-specific resources are freed in the socket destructor
function.

This general pattern of socket creation and destruction
(sock_create_kern/sk_change_net/sk_release_kernel) seems to be used
elsewhere, for example inet_ctl_sock_create and inet_ctl_sock_destroy.  So
this seems reasonable.  I'm not sure if hanging resource deallocation off
sk_destruct in this context is OK, though.  Any comments welcome!

Thanks,
Tom

[1].
	http://www.spinics.net/lists/netdev/msg214776.html
	http://www.spinics.net/lists/netdev/msg212234.html

Tom Parkin (3):
  l2tp: set netnsok flag for netlink messages
  l2tp: prevent tunnel creation on netns mismatch
  l2tp: fix network namespace use for unmanaged tunnel sockets

 net/l2tp/l2tp_core.c    |   94 ++++++++++++++++++++++++++++++-----------------
 net/l2tp/l2tp_netlink.c |    1 +
 2 files changed, 62 insertions(+), 33 deletions(-)

-- 
1.7.9.5

[RFC PATCH 1/3] l2tp: set netnsok flag for netlink messages

[Tom Parkin]

The L2TP netlink code can run in namespaces.  Set the netnsok flag in
genl_family to true to reflect that fact.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_netlink.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c
index bbba3a1..c1bab22 100644
--- a/net/l2tp/l2tp_netlink.c
+++ b/net/l2tp/l2tp_netlink.c
@@ -37,6 +37,7 @@ static struct genl_family l2tp_nl_family = {
 	.version	= L2TP_GENL_VERSION,
 	.hdrsize	= 0,
 	.maxattr	= L2TP_ATTR_MAX,
+	.netnsok	= true,
 };
 
 /* Accessed under genl lock */
-- 
1.7.9.5

[RFC 2/3] l2tp: prevent tunnel creation on netns mismatch

[Tom Parkin]

l2tp_tunnel_create is passed a pointer to the network namespace for the
tunnel, along with an optional file descriptor for the tunnel which may
be passed in from userspace via. netlink.

In the case where the file descriptor is defined, ensure that the namespace
associated with that socket matches the namespace explicitly passed to
l2tp_tunnel_create.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_core.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 1a9f372..75b347b 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1521,11 +1521,18 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
 		if (err < 0)
 			goto err;
 	} else {
-		err = -EBADF;
 		sock = sockfd_lookup(fd, &err);
 		if (!sock) {
-			pr_err("tunl %hu: sockfd_lookup(fd=%d) returned %d\n",
+			pr_err("tunl %u: sockfd_lookup(fd=%d) returned %d\n",
 			       tunnel_id, fd, err);
+			err = -EBADF;
+			goto err;
+		}
+
+		/* Reject namespace mismatches */
+		if (!net_eq(sock_net(sock->sk), net)) {
+			pr_err("tunl %u: netns mismatch\n", tunnel_id);
+			err = -EINVAL;
 			goto err;
 		}
 	}
-- 
1.7.9.5

[RFC PATCH 3/3] l2tp: fix network namespace use for unmanaged tunnel sockets

[Tom Parkin]

L2TP supports two types of tunnel: managed and unmanaged.  Managed tunnels are
dealt with by a userspace daemon, with the kernel providing user data
encapsulation and de-encapsulation for efficiency.  Unmanaged tunnels provide
a simpler interface for configuring kernelspace data encap/de-encap only.  In
the unmanaged tunnel case the kernel creates a tunnel socket which is not
exposed to userspace.

Unmanaged tunnels can be created using iproute2's "l2tp" command.

This patch addresses the following:

 * Unmanaged tunnel sockets should honour the network namespace passed to
   l2tp_tunnel_create.  To achieve this, l2tp_tunnel_sock_create now takes a
   struct net pointer rather than using the namespace of the current process.

 * Unmanaged tunnel sockets should not hold a reference to the netns, as this
   creates a reference loop which can prevent the netns correctly freeing.  We
   now drop the socket's netns reference using sk_change_net.  To prevent
   leaking sockets when the netns exits, we now implement a pernet exit hook
   to free the tunnels and sessions in the netns before it is freed.  This
   clears up any unmanaged tunnels still alive when the netns exits.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
---
 net/l2tp/l2tp_core.c |   83 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 52 insertions(+), 31 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 75b347b..2812666 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1356,29 +1356,37 @@ static void l2tp_tunnel_free(struct l2tp_tunnel *tunnel)
 /* Create a socket for the tunnel, if one isn't set up by
  * userspace. This is used for static tunnels where there is no
  * managing L2TP daemon.
+ *
+ * Since we don't want these sockets to keep a namespace alive by
+ * themselves, we drop the socket's namespace refcount after creation.
+ * These sockets are freed when the namespace exits using the pernet
+ * exit hook.
  */
-static int l2tp_tunnel_sock_create(u32 tunnel_id, u32 peer_tunnel_id, struct l2tp_tunnel_cfg *cfg, struct socket **sockp)
+static int l2tp_tunnel_sock_create(struct net *net,
+				u32 tunnel_id,
+				u32 peer_tunnel_id,
+				struct l2tp_tunnel_cfg *cfg,
+				struct socket **sockp)
 {
 	int err = -EINVAL;
-	struct sockaddr_in udp_addr;
+	struct socket *sock = NULL;
+	struct sockaddr_in udp_addr = {0};
+	struct sockaddr_l2tpip ip_addr = {0};
 #if IS_ENABLED(CONFIG_IPV6)
-	struct sockaddr_in6 udp6_addr;
-	struct sockaddr_l2tpip6 ip6_addr;
+	struct sockaddr_in6 udp6_addr = {0};
+	struct sockaddr_l2tpip6 ip6_addr = {0};
 #endif
-	struct sockaddr_l2tpip ip_addr;
-	struct socket *sock = NULL;
 
 	switch (cfg->encap) {
 	case L2TP_ENCAPTYPE_UDP:
 #if IS_ENABLED(CONFIG_IPV6)
 		if (cfg->local_ip6 && cfg->peer_ip6) {
-			err = sock_create(AF_INET6, SOCK_DGRAM, 0, sockp);
+			err = sock_create_kern(AF_INET6, SOCK_DGRAM, 0, &sock);
 			if (err < 0)
 				goto out;
 
-			sock = *sockp;
+			sk_change_net(sock->sk, net);
 
-			memset(&udp6_addr, 0, sizeof(udp6_addr));
 			udp6_addr.sin6_family = AF_INET6;
 			memcpy(&udp6_addr.sin6_addr, cfg->local_ip6,
 			       sizeof(udp6_addr.sin6_addr));
@@ -1400,13 +1408,12 @@ static int l2tp_tunnel_sock_create(u32 tunnel_id, u32 peer_tunnel_id, struct l2t
 		} else
 #endif
 		{
-			err = sock_create(AF_INET, SOCK_DGRAM, 0, sockp);
+			err = sock_create_kern(AF_INET, SOCK_DGRAM, 0, &sock);
 			if (err < 0)
 				goto out;
 
-			sock = *sockp;
+			sk_change_net(sock->sk, net);
 
-			memset(&udp_addr, 0, sizeof(udp_addr));
 			udp_addr.sin_family = AF_INET;
 			udp_addr.sin_addr = cfg->local_ip;
 			udp_addr.sin_port = htons(cfg->local_udp_port);
@@ -1433,14 +1440,13 @@ static int l2tp_tunnel_sock_create(u32 tunnel_id, u32 peer_tunnel_id, struct l2t
 	case L2TP_ENCAPTYPE_IP:
 #if IS_ENABLED(CONFIG_IPV6)
 		if (cfg->local_ip6 && cfg->peer_ip6) {
-			err = sock_create(AF_INET6, SOCK_DGRAM, IPPROTO_L2TP,
-					  sockp);
+			err = sock_create_kern(AF_INET6, SOCK_DGRAM,
+					IPPROTO_L2TP, &sock);
 			if (err < 0)
 				goto out;
 
-			sock = *sockp;
+			sk_change_net(sock->sk, net);
 
-			memset(&ip6_addr, 0, sizeof(ip6_addr));
 			ip6_addr.l2tp_family = AF_INET6;
 			memcpy(&ip6_addr.l2tp_addr, cfg->local_ip6,
 			       sizeof(ip6_addr.l2tp_addr));
@@ -1462,14 +1468,13 @@ static int l2tp_tunnel_sock_create(u32 tunnel_id, u32 peer_tunnel_id, struct l2t
 		} else
 #endif
 		{
-			err = sock_create(AF_INET, SOCK_DGRAM, IPPROTO_L2TP,
-					  sockp);
+			err = sock_create_kern(AF_INET, SOCK_DGRAM,
+					IPPROTO_L2TP, &sock);
 			if (err < 0)
 				goto out;
 
-			sock = *sockp;
+			sk_change_net(sock->sk, net);
 
-			memset(&ip_addr, 0, sizeof(ip_addr));
 			ip_addr.l2tp_family = AF_INET;
 			ip_addr.l2tp_addr = cfg->local_ip;
 			ip_addr.l2tp_conn_id = tunnel_id;
@@ -1493,6 +1498,7 @@ static int l2tp_tunnel_sock_create(u32 tunnel_id, u32 peer_tunnel_id, struct l2t
 	}
 
 out:
+	*sockp = sock;
 	if ((err < 0) && sock) {
 		sock_release(sock);
 		*sockp = NULL;
@@ -1517,7 +1523,8 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
 	 * kernel socket.
 	 */
 	if (fd < 0) {
-		err = l2tp_tunnel_sock_create(tunnel_id, peer_tunnel_id, cfg, &sock);
+		err = l2tp_tunnel_sock_create(net, tunnel_id, peer_tunnel_id,
+				cfg, &sock);
 		if (err < 0)
 			goto err;
 	} else {
@@ -1631,6 +1638,7 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
 	spin_unlock_bh(&pn->l2tp_tunnel_list_lock);
 
 	err = 0;
+
 err:
 	if (tunnelp)
 		*tunnelp = tunnel;
@@ -1652,19 +1660,19 @@ int l2tp_tunnel_delete(struct l2tp_tunnel *tunnel)
 	int err = 0;
 	struct socket *sock = tunnel->sock ? tunnel->sock->sk_socket : NULL;
 
-	/* Force the tunnel socket to close. This will eventually
-	 * cause the tunnel to be deleted via the normal socket close
-	 * mechanisms when userspace closes the tunnel socket.
+	/* If the tunnel socket was created directly by the kernel, use the
+	 * sk_* API to release the socket now.  Otherwise go through the
+	 * inet_* layer.
+	 * In either case, when the socket goes away the tunnel and the
+	 * sessions are removed via. the socket destructor.
 	 */
 	if (sock != NULL) {
-		err = inet_shutdown(sock, 2);
-
-		/* If the tunnel's socket was created by the kernel,
-		 * close the socket here since the socket was not
-		 * created by userspace.
-		 */
-		if (sock->file == NULL)
+		if (sock->file == NULL) {
+			sk_release_kernel(tunnel->sock);
+		} else {
+			err = inet_shutdown(sock, 2);
 			err = inet_release(sock);
+		}
 	}
 
 	return err;
@@ -1851,8 +1859,21 @@ static __net_init int l2tp_init_net(struct net *net)
 	return 0;
 }
 
+static __net_exit void l2tp_exit_net(struct net *net)
+{
+	struct l2tp_tunnel *tunnel = NULL;
+	struct l2tp_net *pn = l2tp_pernet(net);
+
+	list_for_each_entry(tunnel, &pn->l2tp_tunnel_list, list) {
+		int err = l2tp_tunnel_delete(tunnel);
+		if (err)
+			pr_err("l2tp_tunnel_delete() returned %d\n", err);
+	}
+}
+
 static struct pernet_operations l2tp_net_ops = {
 	.init = l2tp_init_net,
+	.exit = l2tp_exit_net,
 	.id   = &l2tp_net_id,
 	.size = sizeof(struct l2tp_net),
 };
-- 
1.7.9.5