[PATCH 0/4] l2tp: fix namespace support in l2tp_core

[PATCH 0/4] l2tp: fix namespace support in l2tp_core

[Tom Parkin]

As previously reported on netdev, l2tp_core has a number of issues with network 
namespaces which this patchset seeks to address:

	* Create unmanaged tunnel sockets[1] in the namespace passed to
	  l2tp_tunnel_create, rather than defaulting to using the namespace
	  of the current process.

	* Drop namespace references for unmanaged tunnel sockets.  This is to
	  prevent such a socket keeping an otherwise inaccessible namespace
	  alive.  Unmanaged sockets are freed in a namespace net_exit
	  callback.

	* Push tunnel socket release onto a workqueue to allow
	  l2tp_tunnel_delete to be called from an atomic context.  This is to
	  allow the l2tp net_exit callback to walk the tunnel list in an RCU
	  critical section.

There are also minor changes to sanity check namespaces for managed tunnel
sockets, and to flag to netlink that l2tp can run in a namespace other than
default.

Tested on AMD64 and armv6l, under preempt and non-prempt configurations.

[1]. An "unmanaged" tunnel socket is created by the kernel and not exposed to
     userspace.  It is used to perform data encapsulation and de-encapsulation
     at the kernel level without incurring the overhead of the L2TP control
     protocol.  There is code in iproute2 to create unmanaged l2tp tunnels.

Tom Parkin (4):
  l2tp: put tunnel socket release on a workqueue
  l2tp: set netnsok flag for netlink messages
  l2tp: prevent tunnel creation on netns mismatch
  l2tp: create tunnel sockets in the right namespace

 net/l2tp/l2tp_core.c    |  179 +++++++++++++++++++++++++++++------------------
 net/l2tp/l2tp_core.h    |    2 +
 net/l2tp/l2tp_netlink.c |    1 +
 3 files changed, 114 insertions(+), 68 deletions(-)

-- 
1.7.9.5

[PATCH 1/4] l2tp: put tunnel socket release on a workqueue

[Tom Parkin]

To allow l2tp_tunnel_delete to be called from an atomic context, place the
tunnel socket release calls on a workqueue for asynchronous execution.

Tunnel memory is eventually freed in the tunnel socket destructor.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
Signed-off-by: James Chapman <jchapman@katalix.com>
---
 net/l2tp/l2tp_core.c |  103 +++++++++++++++++++++++++++++---------------------
 net/l2tp/l2tp_core.h |    2 +
 2 files changed, 61 insertions(+), 44 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 06389d5..73988c0 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -101,6 +101,7 @@ struct l2tp_skb_cb {
 
 static atomic_t l2tp_tunnel_count;
 static atomic_t l2tp_session_count;
+static struct workqueue_struct *l2tp_wq;
 
 /* per-net private data for this module */
 static unsigned int l2tp_net_id;
@@ -122,7 +123,6 @@ static inline struct l2tp_net *l2tp_pernet(struct net *net)
 	return net_generic(net, l2tp_net_id);
 }
 
-
 /* Tunnel reference counts. Incremented per session that is added to
  * the tunnel.
  */
@@ -1277,6 +1277,7 @@ EXPORT_SYMBOL_GPL(l2tp_xmit_skb);
 static void l2tp_tunnel_destruct(struct sock *sk)
 {
 	struct l2tp_tunnel *tunnel;
+	struct l2tp_net *pn;
 
 	tunnel = sk->sk_user_data;
 	if (tunnel == NULL)
@@ -1284,9 +1285,8 @@ static void l2tp_tunnel_destruct(struct sock *sk)
 
 	l2tp_info(tunnel, L2TP_MSG_CONTROL, "%s: closing...\n", tunnel->name);
 
-	/* Close all sessions */
-	l2tp_tunnel_closeall(tunnel);
 
+	/* Disable udp encapsulation */
 	switch (tunnel->encap) {
 	case L2TP_ENCAPTYPE_UDP:
 		/* No longer an encapsulation socket. See net/ipv4/udp.c */
@@ -1298,17 +1298,23 @@ static void l2tp_tunnel_destruct(struct sock *sk)
 	}
 
 	/* Remove hooks into tunnel socket */
-	tunnel->sock = NULL;
 	sk->sk_destruct = tunnel->old_sk_destruct;
 	sk->sk_user_data = NULL;
+	tunnel->sock = NULL;
 
-	/* Call the original destructor */
-	if (sk->sk_destruct)
-		(*sk->sk_destruct)(sk);
+	/* Remove the tunnel struct from the tunnel list */
+	pn = l2tp_pernet(tunnel->l2tp_net);
+	spin_lock_bh(&pn->l2tp_tunnel_list_lock);
+	list_del_rcu(&tunnel->list);
+	spin_unlock_bh(&pn->l2tp_tunnel_list_lock);
+	atomic_dec(&l2tp_tunnel_count);
 
-	/* We're finished with the socket */
+	l2tp_tunnel_closeall(tunnel);
 	l2tp_tunnel_dec_refcount(tunnel);
 
+	/* Call the original destructor */
+	if (sk->sk_destruct)
+		(*sk->sk_destruct)(sk);
 end:
 	return;
 }
@@ -1382,20 +1388,41 @@ again:
  */
 static void l2tp_tunnel_free(struct l2tp_tunnel *tunnel)
 {
-	struct l2tp_net *pn = l2tp_pernet(tunnel->l2tp_net);
-
 	BUG_ON(atomic_read(&tunnel->ref_count) != 0);
 	BUG_ON(tunnel->sock != NULL);
-
 	l2tp_info(tunnel, L2TP_MSG_CONTROL, "%s: free...\n", tunnel->name);
-
-	/* Remove from tunnel list */
-	spin_lock_bh(&pn->l2tp_tunnel_list_lock);
-	list_del_rcu(&tunnel->list);
 	kfree_rcu(tunnel, rcu);
-	spin_unlock_bh(&pn->l2tp_tunnel_list_lock);
+}
 
-	atomic_dec(&l2tp_tunnel_count);
+/* Workqueue tunnel deletion function */
+static void l2tp_tunnel_del_work(struct work_struct *work)
+{
+	struct l2tp_tunnel *tunnel = NULL;
+	struct socket *sock = NULL;
+	struct sock *sk = NULL;
+
+	tunnel = container_of(work, struct l2tp_tunnel, del_work);
+	sk = l2tp_tunnel_sock_lookup(tunnel);
+	if (!sk)
+		return;
+
+	sock = sk->sk_socket;
+	BUG_ON(!sock);
+
+	/* 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.
+	 */
+	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)
+		inet_release(sock);
+
+	l2tp_tunnel_sock_put(sk);
 }
 
 /* Create a socket for the tunnel, if one isn't set up by
@@ -1657,6 +1684,9 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
 
 	sk->sk_allocation = GFP_ATOMIC;
 
+	/* Init delete workqueue struct */
+	INIT_WORK(&tunnel->del_work, l2tp_tunnel_del_work);
+
 	/* Add tunnel to our list */
 	INIT_LIST_HEAD(&tunnel->list);
 	atomic_inc(&l2tp_tunnel_count);
@@ -1688,33 +1718,7 @@ EXPORT_SYMBOL_GPL(l2tp_tunnel_create);
  */
 int l2tp_tunnel_delete(struct l2tp_tunnel *tunnel)
 {
-	int err = -EBADF;
-	struct socket *sock = NULL;
-	struct sock *sk = NULL;
-
-	sk = l2tp_tunnel_sock_lookup(tunnel);
-	if (!sk)
-		goto out;
-
-	sock = sk->sk_socket;
-	BUG_ON(!sock);
-
-	/* 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.
-	 */
-	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)
-		err = inet_release(sock);
-
-	l2tp_tunnel_sock_put(sk);
-out:
-	return err;
+	return (false == queue_work(l2tp_wq, &tunnel->del_work));
 }
 EXPORT_SYMBOL_GPL(l2tp_tunnel_delete);
 
@@ -1912,6 +1916,13 @@ static int __init l2tp_init(void)
 	if (rc)
 		goto out;
 
+	l2tp_wq = alloc_workqueue("l2tp", WQ_NON_REENTRANT | WQ_UNBOUND, 0);
+	if (!l2tp_wq) {
+		pr_err("alloc_workqueue failed\n");
+		rc = -ENOMEM;
+		goto out;
+	}
+
 	pr_info("L2TP core driver, %s\n", L2TP_DRV_VERSION);
 
 out:
@@ -1921,6 +1932,10 @@ out:
 static void __exit l2tp_exit(void)
 {
 	unregister_pernet_device(&l2tp_net_ops);
+	if (l2tp_wq) {
+		destroy_workqueue(l2tp_wq);
+		l2tp_wq = NULL;
+	}
 }
 
 module_init(l2tp_init);
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index e62204c..8eb8f1d 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -191,6 +191,8 @@ struct l2tp_tunnel {
 	int			fd;		/* Parent fd, if tunnel socket
 						 * was created by userspace */
 
+	struct work_struct	del_work;
+
 	uint8_t			priv[0];	/* private data */
 };
 
-- 
1.7.9.5

[PATCH 2/4] 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>
Signed-off-by: James Chapman <jchapman@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

[PATCH 3/4] 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>
Signed-off-by: James Chapman <jchapman@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 73988c0..60a498a 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1593,11 +1593,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

[PATCH 4/4] l2tp: create tunnel sockets in the right namespace

[Tom Parkin]

When creating unmanaged tunnel sockets we should honour the network namespace
passed to l2tp_tunnel_create.  Furthermore, unmanaged tunnel sockets should
not hold a reference to the network namespace lest they accidentally keep
alive a namespace which should otherwise have been released.

Unmanaged tunnel sockets now drop their namespace reference via sk_change_net,
and are released in a new pernet exit callback, l2tp_exit_net.

Signed-off-by: Tom Parkin <tparkin@katalix.com>
Signed-off-by: James Chapman <jchapman@katalix.com>
---
 net/l2tp/l2tp_core.c |   87 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 54 insertions(+), 33 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 60a498a..b6ba65f 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1409,18 +1409,18 @@ static void l2tp_tunnel_del_work(struct work_struct *work)
 	sock = sk->sk_socket;
 	BUG_ON(!sock);
 
-	/* 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 to shut the socket down, and let userspace close it.
+	 * In either case the tunnel resources are freed in the socket
+	 * destructor when the tunnel socket goes away.
 	 */
-	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)
-		inet_release(sock);
+	if (sock->file == NULL) {
+		kernel_sock_shutdown(sock, SHUT_RDWR);
+		sk_release_kernel(sk);
+	} else {
+		inet_shutdown(sock, 2);
+	}
 
 	l2tp_tunnel_sock_put(sk);
 }
@@ -1428,29 +1428,37 @@ static void l2tp_tunnel_del_work(struct work_struct *work)
 /* 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));
@@ -1472,13 +1480,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);
@@ -1505,14 +1512,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));
@@ -1534,14 +1540,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;
@@ -1565,8 +1570,10 @@ 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);
+		kernel_sock_shutdown(sock, SHUT_RDWR);
+		sk_release_kernel(sock->sk);
 		*sockp = NULL;
 	}
 
@@ -1589,7 +1596,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 {
@@ -1909,8 +1917,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_net *pn = l2tp_pernet(net);
+	struct l2tp_tunnel *tunnel = NULL;
+
+	rcu_read_lock_bh();
+	list_for_each_entry_rcu(tunnel, &pn->l2tp_tunnel_list, list) {
+		(void)l2tp_tunnel_delete(tunnel);
+	}
+	rcu_read_unlock_bh();
+}
+
 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

Re: [PATCH 0/4] l2tp: fix namespace support in l2tp_core

[David Miller]

These patches don't apply cleanly to net-next, please respin.

Re: [PATCH 0/4] l2tp: fix namespace support in l2tp_core

[Tom Parkin]

[-- Attachment #1: Type: text/plain, Size: 769 bytes --]

On Mon, Feb 04, 2013 at 01:21:07PM -0500, David Miller wrote:
> 
> These patches don't apply cleanly to net-next, please respin.

Hmm.  I think the problem is that this patchset builds on the bug-fix
work of this previous submission:

  * l2tp: prevent l2tp_tunnel_delete racing with userspace close

The bug fix patch went to the net tree, not net-next, hence the
problem.  I should have made the dependency between the two patches
more explicit -- apologies for not having done so!

What's the right thing for me to do in a situation like this?  Should
I wait and resubmit when net-next and net are next synchronised?

Thanks,
Tom
-- 
Tom Parkin
Katalix Systems Ltd
http://www.katalix.com
Catalysts for your Embedded Linux software development

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH 0/4] l2tp: fix namespace support in l2tp_core

[David Miller]

From: Tom Parkin <tparkin@katalix.com>
Date: Tue, 5 Feb 2013 09:27:09 +0000

> On Mon, Feb 04, 2013 at 01:21:07PM -0500, David Miller wrote:
>> 
>> These patches don't apply cleanly to net-next, please respin.
> 
> Hmm.  I think the problem is that this patchset builds on the bug-fix
> work of this previous submission:
> 
>   * l2tp: prevent l2tp_tunnel_delete racing with userspace close
> 
> The bug fix patch went to the net tree, not net-next, hence the
> problem.  I should have made the dependency between the two patches
> more explicit -- apologies for not having done so!
> 
> What's the right thing for me to do in a situation like this?  Should
> I wait and resubmit when net-next and net are next synchronised?

The best thing to do is tell me about the dependency, then I can
pull 'net' into 'net-next' before applying your patches.

I'll do that right now.