All of lore.kernel.org
 help / color / mirror / Atom feed
From: James Chapman <jchapman@katalix.com>
To: netdev@vger.kernel.org
Subject: [PATCH net-next 04/16] l2tp: refactor tunnel lifetime handling wrt its socket
Date: Fri,  9 Feb 2018 15:00:20 +0000	[thread overview]
Message-ID: <1518188432-9245-5-git-send-email-jchapman@katalix.com> (raw)
In-Reply-To: <1518188432-9245-1-git-send-email-jchapman@katalix.com>

Ensure that the tunnel's socket is always extant while the tunnel
object exists. Hold a ref on the socket until the tunnel is destroyed
and ensure that all tunnel destroy paths go through a common function
(l2tp_tunnel_delete).

Since the tunnel's socket is now guaranteed to exist if the tunnel
exists, we no longer need to use sockfd_lookup via l2tp_sock_to_tunnel
to derive the tunnel from the socket since this is always
sk_user_data.

The tunnel object gains a new closing flag which is protected by a
spinlock. The existing dead flag which is accessed using
test_and_set_bit APIs is no longer used so is removed.

Fixes: 80d84ef3ff1dd ("l2tp: prevent l2tp_tunnel_delete racing with userspace close")
Signed-off-by: James Chapman <jchapman@katalix.com>
---
 net/l2tp/l2tp_core.c | 128 ++++++++++++++++++---------------------------------
 net/l2tp/l2tp_core.h |  26 ++---------
 net/l2tp/l2tp_ip.c   |   5 +-
 net/l2tp/l2tp_ip6.c  |   3 +-
 4 files changed, 52 insertions(+), 110 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index b68ae77e021e..49d6e06099ec 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -136,51 +136,6 @@ static inline struct l2tp_net *l2tp_pernet(const struct net *net)
 
 }
 
-/* Lookup the tunnel socket, possibly involving the fs code if the socket is
- * owned by userspace.  A struct sock returned from this function must be
- * released using l2tp_tunnel_sock_put once you're done with it.
- */
-static struct sock *l2tp_tunnel_sock_lookup(struct l2tp_tunnel *tunnel)
-{
-	int err = 0;
-	struct socket *sock = NULL;
-	struct sock *sk = NULL;
-
-	if (!tunnel)
-		goto out;
-
-	if (tunnel->fd >= 0) {
-		/* Socket is owned by userspace, who might be in the process
-		 * of closing it.  Look the socket up using the fd to ensure
-		 * consistency.
-		 */
-		sock = sockfd_lookup(tunnel->fd, &err);
-		if (sock)
-			sk = sock->sk;
-	} else {
-		/* Socket is owned by kernelspace */
-		sk = tunnel->sock;
-		sock_hold(sk);
-	}
-
-out:
-	return sk;
-}
-
-/* Drop a reference to a tunnel socket obtained via. l2tp_tunnel_sock_put */
-static void l2tp_tunnel_sock_put(struct sock *sk)
-{
-	struct l2tp_tunnel *tunnel = l2tp_sock_to_tunnel(sk);
-	if (tunnel) {
-		if (tunnel->fd >= 0) {
-			/* Socket is owned by userspace */
-			sockfd_put(sk->sk_socket);
-		}
-		sock_put(sk);
-	}
-	sock_put(sk);
-}
-
 /* Session hash list.
  * The session_id SHOULD be random according to RFC2661, but several
  * L2TP implementations (Cisco and Microsoft) use incrementing
@@ -193,6 +148,12 @@ static void l2tp_tunnel_sock_put(struct sock *sk)
 	return &tunnel->session_hlist[hash_32(session_id, L2TP_HASH_BITS)];
 }
 
+void l2tp_tunnel_free(struct l2tp_tunnel *tunnel)
+{
+	sock_put(tunnel->sock);
+	/* the tunnel is freed in the socket destructor */
+}
+
 /* 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)
 {
@@ -969,7 +930,7 @@ int l2tp_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 {
 	struct l2tp_tunnel *tunnel;
 
-	tunnel = l2tp_sock_to_tunnel(sk);
+	tunnel = l2tp_tunnel(sk);
 	if (tunnel == NULL)
 		goto pass_up;
 
@@ -977,13 +938,10 @@ int l2tp_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 		 tunnel->name, skb->len);
 
 	if (l2tp_udp_recv_core(tunnel, skb, tunnel->recv_payload_hook))
-		goto pass_up_put;
+		goto pass_up;
 
-	sock_put(sk);
 	return 0;
 
-pass_up_put:
-	sock_put(sk);
 pass_up:
 	return 1;
 }
@@ -1214,7 +1172,6 @@ static void l2tp_tunnel_destruct(struct sock *sk)
 
 	l2tp_info(tunnel, L2TP_MSG_CONTROL, "%s: closing...\n", tunnel->name);
 
-
 	/* Disable udp encapsulation */
 	write_lock_bh(&sk->sk_callback_lock);
 	switch (tunnel->encap) {
@@ -1239,12 +1196,11 @@ static void l2tp_tunnel_destruct(struct sock *sk)
 	list_del_rcu(&tunnel->list);
 	spin_unlock_bh(&pn->l2tp_tunnel_list_lock);
 
-	tunnel->sock = NULL;
-	l2tp_tunnel_dec_refcount(tunnel);
-
 	/* Call the original destructor */
 	if (sk->sk_destruct)
 		(*sk->sk_destruct)(sk);
+
+	kfree_rcu(tunnel, rcu);
 end:
 	return;
 }
@@ -1305,30 +1261,26 @@ void l2tp_tunnel_closeall(struct l2tp_tunnel *tunnel)
 /* 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);
+	struct l2tp_tunnel *tunnel;
+
+	rcu_read_lock();
+	tunnel = rcu_dereference_sk_user_data(sk);
 	if (tunnel) {
-		l2tp_tunnel_closeall(tunnel);
-		sock_put(sk);
+		l2tp_tunnel_delete(tunnel);
 	}
+	rcu_read_unlock();
 }
 
 /* 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);
+	struct l2tp_tunnel *tunnel = container_of(work, struct l2tp_tunnel,
+						  del_work);
+	struct sock *sk = tunnel->sock;
+	struct socket *sock = sk->sk_socket;
 
 	l2tp_tunnel_closeall(tunnel);
 
-	sk = l2tp_tunnel_sock_lookup(tunnel);
-	if (!sk)
-		goto out;
-
-	sock = sk->sk_socket;
-
 	/* If the tunnel socket was created within the kernel, use
 	 * the sk API to release it here.
 	 */
@@ -1339,8 +1291,10 @@ static void l2tp_tunnel_del_work(struct work_struct *work)
 		}
 	}
 
-	l2tp_tunnel_sock_put(sk);
-out:
+	/* drop initial ref */
+	l2tp_tunnel_dec_refcount(tunnel);
+
+	/* drop workqueue ref */
 	l2tp_tunnel_dec_refcount(tunnel);
 }
 
@@ -1550,6 +1504,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);
+	spin_lock_init(&tunnel->lock);
 	rwlock_init(&tunnel->hlist_lock);
 	tunnel->acpt_newsess = true;
 
@@ -1605,14 +1560,23 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
 		setup_udp_tunnel_sock(net, sock, &udp_cfg);
 	}
 
+	/* Bump the reference count. The tunnel context is deleted
+	 * only when this drops to zero. A reference is also held on
+	 * the tunnel socket to ensure that it is not released while
+	 * the tunnel is extant. Must be done before sk_destruct is
+	 * set.
+	 */
+	refcount_set(&tunnel->ref_count, 1);
+	sock_hold(sk);
+	tunnel->sock = sk;
+	tunnel->fd = fd;
+
 	/* Hook on the tunnel socket destructor so that we can cleanup
 	 * if the tunnel socket goes away.
 	 */
 	tunnel->old_sk_destruct = sk->sk_destruct;
 	sk->sk_destruct = &l2tp_tunnel_destruct;
 
-	tunnel->sock = sk;
-	tunnel->fd = fd;
 	lockdep_set_class_and_name(&sk->sk_lock.slock, &l2tp_socket_class, "l2tp_sock");
 
 	sk->sk_allocation = GFP_ATOMIC;
@@ -1622,11 +1586,6 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
 
 	/* Add tunnel to our list */
 	INIT_LIST_HEAD(&tunnel->list);
-
-	/* Bump the reference count. The tunnel context is deleted
-	 * only when this drops to zero. Must be done before list insertion
-	 */
-	refcount_set(&tunnel->ref_count, 1);
 	spin_lock_bh(&pn->l2tp_tunnel_list_lock);
 	list_add_rcu(&tunnel->list, &pn->l2tp_tunnel_list);
 	spin_unlock_bh(&pn->l2tp_tunnel_list_lock);
@@ -1650,10 +1609,17 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
  */
 void l2tp_tunnel_delete(struct l2tp_tunnel *tunnel)
 {
-	if (!test_and_set_bit(0, &tunnel->dead)) {
-		l2tp_tunnel_inc_refcount(tunnel);
-		queue_work(l2tp_wq, &tunnel->del_work);
+	spin_lock_bh(&tunnel->lock);
+	if (tunnel->closing) {
+		spin_unlock_bh(&tunnel->lock);
+		return;
 	}
+	tunnel->closing = true;
+	spin_unlock_bh(&tunnel->lock);
+
+	/* Hold tunnel ref while queued work item is pending */
+	l2tp_tunnel_inc_refcount(tunnel);
+	queue_work(l2tp_wq, &tunnel->del_work);
 }
 EXPORT_SYMBOL_GPL(l2tp_tunnel_delete);
 
@@ -1667,8 +1633,6 @@ void l2tp_session_free(struct l2tp_session *session)
 
 	if (tunnel) {
 		BUG_ON(tunnel->magic != L2TP_TUNNEL_MAGIC);
-		sock_put(tunnel->sock);
-		session->tunnel = NULL;
 		l2tp_tunnel_dec_refcount(tunnel);
 	}
 
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index 9bbee90e9963..e88ff7895ccb 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -155,7 +155,8 @@ struct l2tp_tunnel_cfg {
 struct l2tp_tunnel {
 	int			magic;		/* Should be L2TP_TUNNEL_MAGIC */
 
-	unsigned long		dead;
+	bool                    closing;
+	spinlock_t              lock;		/* protect closing */
 
 	struct rcu_head rcu;
 	rwlock_t		hlist_lock;	/* protect session_hlist */
@@ -214,27 +215,8 @@ static inline void *l2tp_session_priv(struct l2tp_session *session)
 	return &session->priv[0];
 }
 
-static inline struct l2tp_tunnel *l2tp_sock_to_tunnel(struct sock *sk)
-{
-	struct l2tp_tunnel *tunnel;
-
-	if (sk == NULL)
-		return NULL;
-
-	sock_hold(sk);
-	tunnel = (struct l2tp_tunnel *)(sk->sk_user_data);
-	if (tunnel == NULL) {
-		sock_put(sk);
-		goto out;
-	}
-
-	BUG_ON(tunnel->magic != L2TP_TUNNEL_MAGIC);
-
-out:
-	return tunnel;
-}
-
 struct l2tp_tunnel *l2tp_tunnel_get(const struct net *net, u32 tunnel_id);
+void l2tp_tunnel_free(struct l2tp_tunnel *tunnel);
 
 struct l2tp_session *l2tp_session_get(const struct net *net,
 				      struct l2tp_tunnel *tunnel,
@@ -283,7 +265,7 @@ static inline void l2tp_tunnel_inc_refcount(struct l2tp_tunnel *tunnel)
 static inline void l2tp_tunnel_dec_refcount(struct l2tp_tunnel *tunnel)
 {
 	if (refcount_dec_and_test(&tunnel->ref_count))
-		kfree_rcu(tunnel, rcu);
+		l2tp_tunnel_free(tunnel);
 }
 
 /* Session reference counts. Incremented when code obtains a reference
diff --git a/net/l2tp/l2tp_ip.c b/net/l2tp/l2tp_ip.c
index 42f3c2f72bf4..a5591bd2fa24 100644
--- a/net/l2tp/l2tp_ip.c
+++ b/net/l2tp/l2tp_ip.c
@@ -242,12 +242,9 @@ static void l2tp_ip_destroy_sock(struct sock *sk)
 	rcu_read_lock();
 	tunnel = rcu_dereference_sk_user_data(sk);
 	if (tunnel) {
-		l2tp_tunnel_closeall(tunnel);
-		sock_put(sk);
+		l2tp_tunnel_delete(tunnel);
 	}
 	rcu_read_unlock();
-
-	sk_refcnt_debug_dec(sk);
 }
 
 static int l2tp_ip_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len)
diff --git a/net/l2tp/l2tp_ip6.c b/net/l2tp/l2tp_ip6.c
index be4a3eee85a9..de8e7eb7a638 100644
--- a/net/l2tp/l2tp_ip6.c
+++ b/net/l2tp/l2tp_ip6.c
@@ -257,8 +257,7 @@ static void l2tp_ip6_destroy_sock(struct sock *sk)
 	rcu_read_lock();
 	tunnel = rcu_dereference_sk_user_data(sk);
 	if (tunnel) {
-		l2tp_tunnel_closeall(tunnel);
-		sock_put(sk);
+		l2tp_tunnel_delete(tunnel);
 	}
 	rcu_read_unlock();
 
-- 
1.9.1

  parent reply	other threads:[~2018-02-09 15:00 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-02-09 15:00 [PATCH net-next 00/16] l2tp: fix API races discovered by syzbot James Chapman
2018-02-09 15:00 ` [PATCH net-next 01/16] l2tp: update sk_user_data while holding sk_callback_lock James Chapman
2018-02-09 15:00 ` [PATCH net-next 02/16] l2tp: add RCU read lock to protect tunnel ptr in ip socket destroy James Chapman
2018-02-09 15:00 ` [PATCH net-next 03/16] l2tp: don't use inet_shutdown on tunnel destroy James Chapman
2018-02-09 15:00 ` James Chapman [this message]
2018-02-09 15:00 ` [PATCH net-next 05/16] l2tp: use tunnel closing flag James Chapman
2018-02-09 15:00 ` [PATCH net-next 06/16] l2tp: refactor session lifetime handling James Chapman
2018-02-09 15:00 ` [PATCH net-next 07/16] l2tp: hide sessions if they are closing James Chapman
2018-02-12  3:37   ` kbuild test robot
2018-02-09 15:00 ` [PATCH net-next 08/16] l2tp: hide session from pppol2tp_sock_to_session if it is closing James Chapman
2018-02-09 15:00 ` [PATCH net-next 09/16] l2tp: refactor pppol2tp_connect James Chapman
2018-02-09 15:00 ` [PATCH net-next 10/16] l2tp: add session_free callback James Chapman
2018-02-09 15:00 ` [PATCH net-next 11/16] l2tp: do session destroy using a workqueue James Chapman
2018-02-09 15:00 ` [PATCH net-next 12/16] l2tp: simplify l2tp_tunnel_closeall James Chapman
2018-02-09 15:00 ` [PATCH net-next 13/16] l2tp: refactor ppp session cleanup paths James Chapman
2018-02-09 15:00 ` [PATCH net-next 14/16] l2tp: remove redundant sk_user_data check when creating tunnels James Chapman
2018-02-09 15:00 ` [PATCH net-next 15/16] l2tp: remove unwanted error message James Chapman
2018-02-09 15:00 ` [PATCH net-next 16/16] l2tp: make __l2tp_session_unhash internal James Chapman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1518188432-9245-5-git-send-email-jchapman@katalix.com \
    --to=jchapman@katalix.com \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.