[RFC PATCH 0/6] udp: add encapsulation socket destroy hook

[RFC PATCH 0/6] udp: add encapsulation socket destroy hook

[Tom Parkin]

While working on an issue in l2tp relating to tunnel cleanup I discovered a
problem which I think may require a new hook in udp core to resolve.  The l2tp
code uses encapsulated udp sockets, and it needs to know when a socket is
being closed in order to clean up.  Since sessions hold a reference to the
tunnel socket, hooking sk_destruct isn't sufficient: any tunnel with sessions
running in it will be pinned open by the sessions.

To resolve this, the first patch in this series adds a .destroy hook to udp, 
while the following patches show how I'd use this feature in l2tp.  Essentially 
the .destroy callback is used to close the sessions in the tunnel, thereby 
dropping the socket references.

Does this seem like a reasonable approach?  I'm reluctant to modify udp, but
I've not managed to discover a safe alternative...

The rest of this email goes into some more detail about the problem I'm trying
to solve.

A bit of background is probably useful for context.

The kernel l2tp code supports two flavours of tunnel socket: managed and
unmanaged.  The managed tunnel socket is created by a userspace daemon which
handles the L2TP control protocol, while the kernel handles L2TP data
encapsulation.  The unmanaged tunnel socket is created by the kernel
and just provides L2TP data encapsulation with no control protocol.

These two flavours of tunnel socket make for a fairly complex set of tunnel
lifetime requirements:

A managed tunnel may be deleted by:

 * userspace using a netlink "delete" command
 * userspace calling close(2) on the tunnel socket
 * userspace dying and the tunnel socket being released as a part of
   the normal tidyup after a process terminates

An unmanaged tunnel may be deleted by:

 * userspace using a netlink "delete" command

In order to satisfy all these deletion mechanisms, l2tp uses sk_destruct to
free tunnel resources when the tunnel socket is being destroyed.  That way we
can catch userspace closing the socket as well as explicit deletion using
netlink.

The problem comes when sessions are added to the tunnel.  Each session holds a
reference to the tunnel socket, in order to prevent the tunnel socket
unexpectedly going away while the session is still active and passing data.
But of course this also keeps the tunnel socket alive even if userspace closes
it!

This is where the protocol .destroy hook comes in.  Because .destroy is called
by sk_common_release before sock_put, the l2tp code is able to use the hook to
close the sessions in the tunnel.  Now that all the session references to the
socket have been dropped, when sk_common_release makes its final sock_put
call, the socket is actually freed and sk_destruct is called.

Tom Parkin (6):
  udp: add encap_destroy callback
  l2tp: add udp encap socket destroy handler
  l2tp: drop session refs in l2tp_tunnel_closeall
  l2tp: export l2tp_tunnel_closeall
  l2tp: close sessions in ip socket destroy callback
  l2tp: close sessions before initiating tunnel delete

 include/linux/udp.h  |    1 +
 net/ipv4/udp.c       |    7 +++++++
 net/ipv6/udp.c       |    8 ++++++++
 net/l2tp/l2tp_core.c |   19 +++++++++++++++++--
 net/l2tp/l2tp_core.h |    1 +
 net/l2tp/l2tp_ip.c   |    6 ++++++
 net/l2tp/l2tp_ip6.c  |    7 +++++++
 7 files changed, 47 insertions(+), 2 deletions(-)

-- 
1.7.9.5

[RFC PATCH 1/6] udp: add encap_destroy callback

[Tom Parkin]

Users of udp encapsulation currently have an encap_rcv callback which they can
use to hook into the udp receive path.

In situations where a encapsulation user allocates resources associated with a
udp encap socket, it may be convenient to be able to also hook the proto
.destroy operation.  For example, if an encap user holds a reference to the
udp socket, the destroy hook might be used to relinquish this reference.

This patch adds a socket destroy hook into udp, which is set and enabled
in the same way as the existing encap_rcv hook.

---
 include/linux/udp.h |    1 +
 net/ipv4/udp.c      |    7 +++++++
 net/ipv6/udp.c      |    8 ++++++++
 3 files changed, 16 insertions(+)

diff --git a/include/linux/udp.h b/include/linux/udp.h
index 9d81de1..42278bb 100644
--- a/include/linux/udp.h
+++ b/include/linux/udp.h
@@ -68,6 +68,7 @@ struct udp_sock {
 	 * For encapsulation sockets.
 	 */
 	int (*encap_rcv)(struct sock *sk, struct sk_buff *skb);
+	void (*encap_destroy)(struct sock *sk);
 };
 
 static inline struct udp_sock *udp_sk(const struct sock *sk)
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 6791aac..48e2366 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1762,9 +1762,16 @@ int udp_rcv(struct sk_buff *skb)
 
 void udp_destroy_sock(struct sock *sk)
 {
+	struct udp_sock *up = udp_sk(sk);
 	bool slow = lock_sock_fast(sk);
 	udp_flush_pending_frames(sk);
 	unlock_sock_fast(sk, slow);
+	if (static_key_false(&udp_encap_needed) && up->encap_type) {
+		void (*encap_destroy)(struct sock *sk);
+		encap_destroy = ACCESS_ONCE(up->encap_destroy);
+		if (encap_destroy)
+			encap_destroy(sk);
+	}
 }
 
 /*
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 599e1ba6..d8e5e85 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -1285,10 +1285,18 @@ do_confirm:
 
 void udpv6_destroy_sock(struct sock *sk)
 {
+	struct udp_sock *up = udp_sk(sk);
 	lock_sock(sk);
 	udp_v6_flush_pending_frames(sk);
 	release_sock(sk);
 
+	if (static_key_false(&udpv6_encap_needed) && up->encap_type) {
+		void (*encap_destroy)(struct sock *sk);
+		encap_destroy = ACCESS_ONCE(up->encap_destroy);
+		if (encap_destroy)
+			encap_destroy(sk);
+	}
+
 	inet6_destroy_sock(sk);
 }
 
-- 
1.7.9.5

[RFC PATCH 2/6] l2tp: add udp encap socket destroy handler

[Tom Parkin]

L2TP sessions hold a reference to the tunnel socket to prevent it going away
while sessions are still active.  However, since tunnel destruction is handled
by the sock sk_destruct callback there is a catch-22: a tunnel with sessions
cannot be deleted since each session holds a reference to the tunnel socket.
If userspace closes a managed tunnel socket, or dies, the tunnel will persist
and it will be neccessary to individually delete the sessions using netlink
commands.  This is ugly.

To prevent this occuring, this patch leverages the udp encapsulation socket
destroy callback to gain early notification when the tunnel socket is closed.
This allows us to safely close the sessions running in the tunnel, dropping
the tunnel socket references in the process.  The tunnel socket is then
destroyed as normal, and the tunnel resources deallocated in sk_destruct.

---
 net/l2tp/l2tp_core.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 5edf1e2..3842f67 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1249,6 +1249,7 @@ static void l2tp_tunnel_destruct(struct sock *sk)
 		/* No longer an encapsulation socket. See net/ipv4/udp.c */
 		(udp_sk(sk))->encap_type = 0;
 		(udp_sk(sk))->encap_rcv = NULL;
+		(udp_sk(sk))->encap_destroy = NULL;
 		break;
 	case L2TP_ENCAPTYPE_IP:
 		break;
@@ -1340,6 +1341,16 @@ again:
 	write_unlock_bh(&tunnel->hlist_lock);
 }
 
+/* Tunnel socket destroy hook for UDP encapsulation */
+static void l2tp_udp_encap_destroy(struct sock *sk)
+{
+	struct l2tp_tunnel *tunnel = l2tp_sock_to_tunnel(sk);
+	if (tunnel) {
+		l2tp_tunnel_closeall(tunnel);
+		sock_put(sk);
+	}
+}
+
 /* Really kill the tunnel.
  * Come here only when all sessions have been cleared from the tunnel.
  */
@@ -1635,6 +1646,7 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
 		/* Mark socket as an encapsulation socket. See net/ipv4/udp.c */
 		udp_sk(sk)->encap_type = UDP_ENCAP_L2TPINUDP;
 		udp_sk(sk)->encap_rcv = l2tp_udp_encap_recv;
+		udp_sk(sk)->encap_destroy = l2tp_udp_encap_destroy;
 #if IS_ENABLED(CONFIG_IPV6)
 		if (sk->sk_family == PF_INET6)
 			udpv6_encap_enable();
-- 
1.7.9.5

[RFC PATCH 3/6] l2tp: drop session refs in l2tp_tunnel_closeall

[Tom Parkin]

l2tp_tunnel_closeall is intended to close and free all sessions running in a
given tunnel.  It lacked a session dereference, however, meaning these
sessions would all leak rather than being properly freed.

Add a session deref call in l2tp_tunnel_closeall to prevent this leak.

---
 net/l2tp/l2tp_core.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 3842f67..7f3ab65 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1328,6 +1328,8 @@ again:
 			if (session->deref != NULL)
 				(*session->deref)(session);
 
+			l2tp_session_dec_refcount(session);
+
 			write_lock_bh(&tunnel->hlist_lock);
 
 			/* Now restart from the beginning of this hash
-- 
1.7.9.5

[RFC PATCH 4/6] l2tp: export l2tp_tunnel_closeall

[Tom Parkin]

l2tp_core internally uses l2tp_tunnel_closeall to close all sessions in a
tunnel when a UDP-encapsualtion socket is destroyed.  We need to do something
similar for IP-encapsualtion sockets.

Export l2tp_tunnel_closeall as a GPL symbol to enable l2tp_ip and l2tp_ip6 to
call it from their .destroy handlers.

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

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 7f3ab65..26b5f00 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -114,7 +114,6 @@ struct l2tp_net {
 
 static void l2tp_session_set_header_len(struct l2tp_session *session, int version);
 static void l2tp_tunnel_free(struct l2tp_tunnel *tunnel);
-static void l2tp_tunnel_closeall(struct l2tp_tunnel *tunnel);
 
 static inline struct l2tp_net *l2tp_pernet(struct net *net)
 {
@@ -1279,7 +1278,7 @@ end:
 
 /* When the tunnel is closed, all the attached sessions need to go too.
  */
-static void l2tp_tunnel_closeall(struct l2tp_tunnel *tunnel)
+void l2tp_tunnel_closeall(struct l2tp_tunnel *tunnel)
 {
 	int hash;
 	struct hlist_node *walk;
@@ -1342,6 +1341,7 @@ again:
 	}
 	write_unlock_bh(&tunnel->hlist_lock);
 }
+EXPORT_SYMBOL_GPL(l2tp_tunnel_closeall);
 
 /* Tunnel socket destroy hook for UDP encapsulation */
 static void l2tp_udp_encap_destroy(struct sock *sk)
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index a6d9c5d..d1b37c1 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -239,6 +239,7 @@ extern struct l2tp_tunnel *l2tp_tunnel_find(struct net *net, u32 tunnel_id);
 extern struct l2tp_tunnel *l2tp_tunnel_find_nth(struct net *net, int nth);
 
 extern int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32 peer_tunnel_id, struct l2tp_tunnel_cfg *cfg, struct l2tp_tunnel **tunnelp);
+extern void l2tp_tunnel_closeall(struct l2tp_tunnel *tunnel);
 extern int l2tp_tunnel_delete(struct l2tp_tunnel *tunnel);
 extern 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);
 extern int l2tp_session_delete(struct l2tp_session *session);
-- 
1.7.9.5

[RFC PATCH 5/6] l2tp: close sessions in ip socket destroy callback

[Tom Parkin]

l2tp_core hooks UDP's .destroy handler to gain advance warning of a tunnel
socket being closed from userspace.  We need to do the same thing for
IP-encapsualtion sockets.

---
 net/l2tp/l2tp_ip.c  |    6 ++++++
 net/l2tp/l2tp_ip6.c |    7 +++++++
 2 files changed, 13 insertions(+)

diff --git a/net/l2tp/l2tp_ip.c b/net/l2tp/l2tp_ip.c
index f7ac8f4..338262d 100644
--- a/net/l2tp/l2tp_ip.c
+++ b/net/l2tp/l2tp_ip.c
@@ -229,10 +229,16 @@ static void l2tp_ip_close(struct sock *sk, long timeout)
 static void l2tp_ip_destroy_sock(struct sock *sk)
 {
 	struct sk_buff *skb;
+	struct l2tp_tunnel *tunnel = l2tp_sock_to_tunnel(sk);
 
 	while ((skb = __skb_dequeue_tail(&sk->sk_write_queue)) != NULL)
 		kfree_skb(skb);
 
+	if (tunnel) {
+		l2tp_tunnel_closeall(tunnel);
+		sock_put(sk);
+	}
+
 	sk_refcnt_debug_dec(sk);
 }
 
diff --git a/net/l2tp/l2tp_ip6.c b/net/l2tp/l2tp_ip6.c
index 8ee4a86..b745a40 100644
--- a/net/l2tp/l2tp_ip6.c
+++ b/net/l2tp/l2tp_ip6.c
@@ -242,10 +242,17 @@ static void l2tp_ip6_close(struct sock *sk, long timeout)
 
 static void l2tp_ip6_destroy_sock(struct sock *sk)
 {
+	struct l2tp_tunnel *tunnel = l2tp_sock_to_tunnel(sk);
+
 	lock_sock(sk);
 	ip6_flush_pending_frames(sk);
 	release_sock(sk);
 
+	if (tunnel) {
+		l2tp_tunnel_closeall(tunnel);
+		sock_put(sk);
+	}
+
 	inet6_destroy_sock(sk);
 }
 
-- 
1.7.9.5

[RFC PATCH 6/6] l2tp: close sessions before initiating tunnel delete

[Tom Parkin]

When a user deletes a tunnel using netlink, all the sessions in the tunnel
should also be deleted.  Since running sessions will pin the tunnel socket
with the references they hold, have the l2tp_tunnel_delete close all sessions
in a tunnel before finally closing the tunnel socket.

---
 net/l2tp/l2tp_core.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 26b5f00..f3735d6 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1704,6 +1704,7 @@ EXPORT_SYMBOL_GPL(l2tp_tunnel_create);
  */
 int l2tp_tunnel_delete(struct l2tp_tunnel *tunnel)
 {
+	l2tp_tunnel_closeall(tunnel);
 	return (false == queue_work(l2tp_wq, &tunnel->del_work));
 }
 EXPORT_SYMBOL_GPL(l2tp_tunnel_delete);
-- 
1.7.9.5

Re: [RFC PATCH 0/6] udp: add encapsulation socket destroy hook

[David Miller]

From: Tom Parkin <tparkin@katalix.com>
Date: Fri, 15 Feb 2013 10:25:13 +0000

> While working on an issue in l2tp relating to tunnel cleanup I discovered a
> problem which I think may require a new hook in udp core to resolve.  The l2tp
> code uses encapsulated udp sockets, and it needs to know when a socket is
> being closed in order to clean up.  Since sessions hold a reference to the
> tunnel socket, hooking sk_destruct isn't sufficient: any tunnel with sessions
> running in it will be pinned open by the sessions.
> 
> To resolve this, the first patch in this series adds a .destroy hook to udp, 
> while the following patches show how I'd use this feature in l2tp.  Essentially 
> the .destroy callback is used to close the sessions in the tunnel, thereby 
> dropping the socket references.
> 
> Does this seem like a reasonable approach?  I'm reluctant to modify udp, but
> I've not managed to discover a safe alternative...

For now this is fine by me.

If we commonly start to encapsulate in other datagram protocols we'll
need to facilitate this more generically, perhaps by having something
that hooks in near the sk_prot->destroy() in sk_release_common().