[PATCH net-next v2 00/16] l2tp: fix API races discovered by syzbot

[PATCH net-next v2 00/16] l2tp: fix API races discovered by syzbot

[James Chapman]

This patch series addresses several races with L2TP APIs discovered by
syzbot. While working on this, it became clear that the L2TP code
needed some work to address object lifetime issues. There are no
functional changes.

The set of patches 1-13 in combination fix the following syzbot reports.

9df43faf0 KASAN: use-after-free Read in pppol2tp_connect
6e6a5ec8d general protection fault in pppol2tp_connect
347bd5acd KASAN: use-after-free Read in inet_shutdown
19c09769f WARNING in debug_print_object

In detail:-

 1. Add RCU protection of sk_user_data. Since L2TP hooks on sockets
    opened by userspace, we may race with other socket families that
    attempt to use the same socket. (patches 1-2)

 2. Fix inet_shutdown races when L2TP tunnels close. (patch 3)

 3. Refactor code to address internal object lifetime
    issues. Previously internal refcounts and socket refcounts were
    used inconsistently and led to workarounds to fix specific
    bugs. With the changes made here, we can now fetch the
    tunnel/session context from its socket sk_user_data and fetch the
    socket from the tunnel/session without using other APIs such as
    sockfd_lookup. (patches 4-8)

 4. Refactor pppol2tp_connect to fix several races and split it up to
    improve readability. (patch 9)

 5. Refactor session destroy paths to use a workqueue such that all
    session cleanup is done using common code, regardless of whether
    the session is closed by netlink request or (in the case of ppp)
    its socket closed. (patches 10-13)

 6. Misc cleanups made possible by the refactoring done in this
    series. (patches 14-16)

Changes in v2:-

    Fix compile error that would have broken bisect.

James Chapman (16):
  l2tp: update sk_user_data while holding sk_callback_lock
  l2tp: add RCU read lock to protect tunnel ptr in ip socket destroy
  l2tp: don't use inet_shutdown on tunnel destroy
  l2tp: refactor tunnel lifetime handling wrt its socket
  l2tp: use tunnel closing flag
  l2tp: refactor session lifetime handling
  l2tp: hide sessions if they are closing
  l2tp: hide session from pppol2tp_sock_to_session if it is closing
  l2tp: refactor pppol2tp_connect
  l2tp: add session_free callback
  l2tp: do session destroy using a workqueue
  l2tp: simplify l2tp_tunnel_closeall
  l2tp: refactor ppp session cleanup paths
  l2tp: remove redundant sk_user_data check when creating tunnels
  l2tp: remove unwanted error message
  l2tp: make __l2tp_session_unhash internal

 net/l2tp/l2tp_core.c | 310 ++++++++++++++++++------------------
 net/l2tp/l2tp_core.h |  37 ++---
 net/l2tp/l2tp_ip.c   |  10 +-
 net/l2tp/l2tp_ip6.c  |   8 +-
 net/l2tp/l2tp_ppp.c  | 434 ++++++++++++++++++++++++++++++---------------------
 5 files changed, 434 insertions(+), 365 deletions(-)

-- 
1.9.1

[PATCH net-next v2 01/16] l2tp: update sk_user_data while holding sk_callback_lock

[James Chapman]

Since L2TP hooks on sockets opened by userspace using sk_user_data, we
may race with other socket families that attempt to use the same
socket.

This problem was discovered by syzbot using AF_KCM. KCM has since been
modified to use only TCP sockets to avoid hitting this issue but we
should prevent such races in L2TP anyway.

Fixes: c8fffcea0a079 ("l2tp: Refactor l2tp core driver to make use of the common UDP tunnel function")
Reported-by: syzbot+8865eaff7f9acd593945@syzkaller.appspotmail.com

Kernel BUG at net/l2tp/l2tp_ppp.c:176!
invalid opcode: 0000 [#1] SMP KASAN
Dumping ftrace buffer:
   (ftrace buffer empty)
Modules linked in:
CPU: 1 PID: 3503 Comm: syzkaller938388 Not tainted 4.15.0-rc7+ #181
Hardware name: Google Google Compute Engine/Google Compute Engine
RIP: 0010:pppol2tp_sock_to_session net/l2tp/l2tp_ppp.c:176 [inline]
RIP: 0010:pppol2tp_sendmsg+0x512/0x670 net/l2tp/l2tp_ppp.c:304
RSP: 0018:ffff8801d4887438 EFLAGS: 00010293
RAX: ffff8801bfef2180 RBX: ffff8801bff88440 RCX: ffffffff84ffbca2
RDX: 0000000000000000 RSI: ffff8801d4887598 RDI: ffff8801bff88820
RBP: ffff8801d48874a8 R08: 0000000000000000 R09: 1ffff1003a910e17
R10: 0000000000000003 R11: 0000000000000001 R12: ffff8801bfff9bc0
R13: 0000000000000000 R14: 0000000000008000 R15: 0000000000000000
FS:  0000000001194880(0000) GS:ffff8801db300000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000020ea0000 CR3: 00000001bfecf001 CR4: 00000000001606e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 sock_sendmsg_nosec net/socket.c:628 [inline]
 sock_sendmsg+0xca/0x110 net/socket.c:638
 kernel_sendmsg+0x47/0x60 net/socket.c:646
 sock_no_sendpage+0x1cc/0x280 net/core/sock.c:2581
 kernel_sendpage+0xbf/0xe0 net/socket.c:3349
 kcm_write_msgs+0x404/0x1b80 net/kcm/kcmsock.c:646
 kcm_sendmsg+0x148d/0x22d0 net/kcm/kcmsock.c:1035
 sock_sendmsg_nosec net/socket.c:628 [inline]
 sock_sendmsg+0xca/0x110 net/socket.c:638
 ___sys_sendmsg+0x767/0x8b0 net/socket.c:2018
 __sys_sendmsg+0xe5/0x210 net/socket.c:2052
 SYSC_sendmsg net/socket.c:2063 [inline]
 SyS_sendmsg+0x2d/0x50 net/socket.c:2059
 entry_SYSCALL_64_fastpath+0x23/0x9a
RIP: 0033:0x440159
RSP: 002b:00007ffe74df8288 EFLAGS: 00000217 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: ffffffffffffffff RCX: 0000000000440159
RDX: 0000000000000000 RSI: 00000000201fcfc8 RDI: 0000000000000005
RBP: 00000000006ca018 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000217 R12: 0000000000401ac0
R13: 0000000000401b50 R14: 0000000000000000 R15: 0000000000000000
Code: c5 61 70 fc 48 8b 7d d0 e8 7c c2 5b fd 84 c0 74 0d e8 b3 61 70 fc 48 89 df e8 3b 49 2f ff 41 bd f7 ff ff ff eb 86 e8 9e 61 70 fc <0f> 0b 41 bd 95 ff ff ff e9 74 ff ff ff e8 ec 32 a8 fc e9 77 fb
RIP: pppol2tp_sock_to_session net/l2tp/l2tp_ppp.c:176 [inline] RSP: ffff8801d4887438
RIP: pppol2tp_sendmsg+0x512/0x670 net/l2tp/l2tp_ppp.c:304 RSP: ffff8801d4887438
---
 net/l2tp/l2tp_core.c | 21 ++++++++++++++++++---
 net/l2tp/l2tp_ppp.c  |  8 ++++++--
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 194a7483bb93..de7dce64173f 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1216,6 +1216,7 @@ static void l2tp_tunnel_destruct(struct sock *sk)
 
 
 	/* Disable udp encapsulation */
+	write_lock_bh(&sk->sk_callback_lock);
 	switch (tunnel->encap) {
 	case L2TP_ENCAPTYPE_UDP:
 		/* No longer an encapsulation socket. See net/ipv4/udp.c */
@@ -1229,7 +1230,8 @@ 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;
+	rcu_assign_sk_user_data(sk, NULL);
+	write_unlock_bh(&sk->sk_callback_lock);
 
 	/* Remove the tunnel struct from the tunnel list */
 	pn = l2tp_pernet(tunnel->l2tp_net);
@@ -1583,6 +1585,20 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
 	}
 #endif
 
+	/* Assign socket sk_user_data. Must be done with
+	 * sk_callback_lock. Bail if sk_user_data is already assigned.
+	 */
+	write_lock_bh(&sk->sk_callback_lock);
+	if (sk->sk_user_data) {
+		err = -EALREADY;
+		write_unlock_bh(&sk->sk_callback_lock);
+		kfree(tunnel);
+		tunnel = NULL;
+		goto err;
+	}
+	rcu_assign_sk_user_data(sk, tunnel);
+	write_unlock_bh(&sk->sk_callback_lock);
+
 	/* Mark socket as an encapsulation socket. See net/ipv4/udp.c */
 	tunnel->encap = encap;
 	if (encap == L2TP_ENCAPTYPE_UDP) {
@@ -1594,8 +1610,6 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
 		udp_cfg.encap_destroy = l2tp_udp_encap_destroy;
 
 		setup_udp_tunnel_sock(net, sock, &udp_cfg);
-	} else {
-		sk->sk_user_data = tunnel;
 	}
 
 	/* Hook on the tunnel socket destructor so that we can cleanup
@@ -1603,6 +1617,7 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
 	 */
 	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");
diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index 59f246d7b290..fe5a0043dd32 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -443,7 +443,9 @@ static void pppol2tp_session_destruct(struct sock *sk)
 	skb_queue_purge(&sk->sk_write_queue);
 
 	if (session) {
-		sk->sk_user_data = NULL;
+		write_lock_bh(&sk->sk_callback_lock);
+		rcu_assign_sk_user_data(sk, NULL);
+		write_unlock_bh(&sk->sk_callback_lock);
 		BUG_ON(session->magic != L2TP_SESSION_MAGIC);
 		l2tp_session_dec_refcount(session);
 	}
@@ -796,7 +798,9 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
 
 out_no_ppp:
 	/* This is how we get the session context from the socket. */
-	sk->sk_user_data = session;
+	write_lock_bh(&sk->sk_callback_lock);
+	rcu_assign_sk_user_data(sk, session);
+	write_unlock_bh(&sk->sk_callback_lock);
 	rcu_assign_pointer(ps->sk, sk);
 	mutex_unlock(&ps->sk_lock);
 
-- 
1.9.1

[PATCH net-next v2 02/16] l2tp: add RCU read lock to protect tunnel ptr in ip socket destroy

[James Chapman]

If an L2TPIP socket is closed, add RCU protection when we deref
sk_user_data to prevent races with another thread closing the same
tunnel.

Fixes: 0d76751fad ("l2tp: Add L2TPv3 IP encapsulation (no UDP) support")

 refcount_t: increment on 0; use-after-free.
 WARNING: CPU: 2 PID: 2892 at lib/refcount.c:153 refcount_inc+0x2b/0x30
 Modules linked in:
 CPU: 2 PID: 2892 Comm: pppol2tp_chaos Not tainted 4.15.0-rc9+ #1
 Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
 RIP: 0010:refcount_inc+0x2b/0x30
 RSP: 0018:ffff880014147b50 EFLAGS: 00010282
 RAX: 000000000000002b RBX: ffff8800194785c0 RCX: 0000000000000000
 RDX: 0000000000000001 RSI: ffff88001ad1f758 RDI: ffff88001ad1f758
 RBP: ffff880014147b50 R08: 0000000000000000 R09: 0000000000000000
 R10: 0000000000000001 R11: 0000000000000001 R12: ffff8800194785c8
 R13: ffff8800194785c0 R14: ffff88001a2c6c20 R15: ffff880013a9d580
 FS:  0000000000000000(0000) GS:ffff88001ad00000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: 00007fbc17ea7000 CR3: 0000000003022000 CR4: 00000000000006e0
 Call Trace:
  l2tp_tunnel_delete+0x34/0x60
  l2tp_ip_destroy_sock+0x6d/0x80
  sk_common_release+0x19/0xd0
  l2tp_ip_close+0x89/0xa0
  inet_release+0x37/0x60
  sock_release+0x1a/0x70
  sock_close+0xd/0x20
  __fput+0xed/0x1f0
  ____fput+0x9/0x10
  task_work_run+0x77/0xb0
  do_exit+0x311/0xcf0
  do_group_exit+0x47/0xc0
  get_signal+0x343/0x8e0
  do_signal+0x23/0x780
  ? find_held_lock+0x39/0xb0
  exit_to_usermode_loop+0x4a/0x93
  syscall_return_slowpath+0x102/0x150
  entry_SYSCALL_64_fastpath+0x98/0x9a
 RIP: 0033:0x7fbc172d7fcf
 RSP: 002b:00007ffd524cd4d8 EFLAGS: 00000246 ORIG_RAX: 000000000000000e
 RAX: 0000000000000000 RBX: 0000000000000006 RCX: 00007fbc172d7fcf
 RDX: 0000000000000000 RSI: 00007ffd524cd460 RDI: 0000000000000002
 RBP: 0000000000404930 R08: 0000000000000000 R09: 00007ffd524cd460
 R10: 0000000000000008 R11: 0000000000000246 R12: 000000000000001e
 R13: 0000000000404a03 R14: 0000000000000000 R15: 0000000000000000
 Code: 55 48 89 e5 e8 87 ff ff ff 84 c0 74 02 5d c3 80 3d 97 87 bb 01 00 75 f5 48 c7 c7 58 3e cc 82 c6 05 87 87 bb 01
---
 net/l2tp/l2tp_ip.c  | 5 ++++-
 net/l2tp/l2tp_ip6.c | 5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/l2tp/l2tp_ip.c b/net/l2tp/l2tp_ip.c
index ff61124fdf59..42f3c2f72bf4 100644
--- a/net/l2tp/l2tp_ip.c
+++ b/net/l2tp/l2tp_ip.c
@@ -234,15 +234,18 @@ 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);
+	struct l2tp_tunnel *tunnel;
 
 	while ((skb = __skb_dequeue_tail(&sk->sk_write_queue)) != NULL)
 		kfree_skb(skb);
 
+	rcu_read_lock();
+	tunnel = rcu_dereference_sk_user_data(sk);
 	if (tunnel) {
 		l2tp_tunnel_closeall(tunnel);
 		sock_put(sk);
 	}
+	rcu_read_unlock();
 
 	sk_refcnt_debug_dec(sk);
 }
diff --git a/net/l2tp/l2tp_ip6.c b/net/l2tp/l2tp_ip6.c
index 192344688c06..be4a3eee85a9 100644
--- a/net/l2tp/l2tp_ip6.c
+++ b/net/l2tp/l2tp_ip6.c
@@ -248,16 +248,19 @@ 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);
+	struct l2tp_tunnel *tunnel;
 
 	lock_sock(sk);
 	ip6_flush_pending_frames(sk);
 	release_sock(sk);
 
+	rcu_read_lock();
+	tunnel = rcu_dereference_sk_user_data(sk);
 	if (tunnel) {
 		l2tp_tunnel_closeall(tunnel);
 		sock_put(sk);
 	}
+	rcu_read_unlock();
 
 	inet6_destroy_sock(sk);
 }
-- 
1.9.1

[PATCH net-next v2 03/16] l2tp: don't use inet_shutdown on tunnel destroy

[James Chapman]

Previously, if a tunnel was closed, we called inet_shutdown to mark
the socket as unconnected such that userspace would get errors and
then close the socket. This could race with userspace closing the
socket. Instead, leave userspace to close the socket in its own time
(our tunnel will be detached anyway).

Fixes: 309795f4be ("l2tp: Add netlink control API for L2TP")

 BUG: unable to handle kernel NULL pointer dereference at 00000000000000a0
 IP: __lock_acquire+0x263/0x1630
 PGD 0 P4D 0
 Oops: 0000 [#1] SMP KASAN
 Modules linked in:
 CPU: 2 PID: 42 Comm: kworker/u8:2 Not tainted 4.15.0-rc7+ #129
 Workqueue: l2tp l2tp_tunnel_del_work
 RIP: 0010:__lock_acquire+0x263/0x1630
 RSP: 0018:ffff88001a37fc70 EFLAGS: 00010002
 RAX: 0000000000000001 RBX: 0000000000000088 RCX: 0000000000000000
 RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
 RBP: ffff88001a37fd18 R08: 0000000000000001 R09: 0000000000000000
 R10: 0000000000000000 R11: 00000000000076fd R12: 00000000000000a0
 R13: ffff88001a3722c0 R14: 0000000000000001 R15: 0000000000000000
 FS:  0000000000000000(0000) GS:ffff88001ad00000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: 00000000000000a0 CR3: 000000001730b000 CR4: 00000000000006e0
 Call Trace:
  ? __lock_acquire+0xc77/0x1630
  ? console_trylock+0x11/0xa0
  lock_acquire+0x117/0x230
  ? lock_sock_nested+0x3a/0xa0
  _raw_spin_lock_bh+0x3a/0x50
  ? lock_sock_nested+0x3a/0xa0
  lock_sock_nested+0x3a/0xa0
  inet_shutdown+0x33/0xf0
  l2tp_tunnel_del_work+0x60/0xef
  process_one_work+0x1ea/0x5f0
  ? process_one_work+0x162/0x5f0
  worker_thread+0x48/0x3e0
  ? trace_hardirqs_on+0xd/0x10
  kthread+0x108/0x140
  ? process_one_work+0x5f0/0x5f0
  ? kthread_stop+0x2a0/0x2a0
  ret_from_fork+0x24/0x30
 Code: 00 41 81 ff ff 1f 00 00 0f 87 7a 13 00 00 45 85 f6 49 8b 85
 68 08 00 00 0f 84 ae 03 00 00 c7 44 24 18 00 00 00 00 e9 f0 00 00 00 <49> 81 3c
 24 80 93 3f 83 b8 00 00 00 00 44 0f 44 c0 83 fe 01 0f
 RIP: __lock_acquire+0x263/0x1630 RSP: ffff88001a37fc70
 CR2: 00000000000000a0
---
 net/l2tp/l2tp_core.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index de7dce64173f..b68ae77e021e 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1329,17 +1329,10 @@ static void l2tp_tunnel_del_work(struct work_struct *work)
 
 	sock = sk->sk_socket;
 
-	/* If the tunnel socket was created by userspace, then go through the
-	 * inet layer to shut the socket down, and let userspace close it.
-	 * Otherwise, if we created the socket directly within the kernel, use
+	/* If the tunnel socket was created within the kernel, use
 	 * the sk API to release it here.
-	 * In either case the tunnel resources are freed in the socket
-	 * destructor when the tunnel socket goes away.
 	 */
-	if (tunnel->fd >= 0) {
-		if (sock)
-			inet_shutdown(sock, 2);
-	} else {
+	if (tunnel->fd < 0) {
 		if (sock) {
 			kernel_sock_shutdown(sock, SHUT_RDWR);
 			sock_release(sock);
-- 
1.9.1

[PATCH net-next v2 04/16] l2tp: refactor tunnel lifetime handling wrt its socket

[James Chapman]

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")
---
 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

[PATCH net-next v2 05/16] l2tp: use tunnel closing flag

[James Chapman]

The tunnel's closing flag is set when the tunnel is being
destroyed. Use it to reject new sessions and remove acpt_newsess which
was doing the same thing. Also prevent the tunnel being seen in
l2tp_tunnel_get lookups.
---
 net/l2tp/l2tp_core.c | 27 +++++++++++++++++++++------
 net/l2tp/l2tp_core.h |  4 ----
 2 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 49d6e06099ec..691fe9368d91 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -163,6 +163,13 @@ struct l2tp_tunnel *l2tp_tunnel_get(const struct net *net, u32 tunnel_id)
 	rcu_read_lock_bh();
 	list_for_each_entry_rcu(tunnel, &pn->l2tp_tunnel_list, list) {
 		if (tunnel->tunnel_id == tunnel_id) {
+			spin_lock_bh(&tunnel->lock);
+			if (tunnel->closing) {
+				spin_unlock_bh(&tunnel->lock);
+				rcu_read_unlock_bh();
+				return NULL;
+			}
+			spin_unlock_bh(&tunnel->lock);
 			l2tp_tunnel_inc_refcount(tunnel);
 			rcu_read_unlock_bh();
 
@@ -278,13 +285,16 @@ int l2tp_session_register(struct l2tp_session *session,
 	struct l2tp_net *pn;
 	int err;
 
+	spin_lock_bh(&tunnel->lock);
+	if (tunnel->closing) {
+		spin_unlock_bh(&tunnel->lock);
+		return -ENODEV;
+	}
+	spin_unlock_bh(&tunnel->lock);
+
 	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) {
@@ -1220,7 +1230,6 @@ 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]) {
@@ -1506,7 +1515,6 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
 	sprintf(&tunnel->name[0], "tunl %u", tunnel_id);
 	spin_lock_init(&tunnel->lock);
 	rwlock_init(&tunnel->hlist_lock);
-	tunnel->acpt_newsess = true;
 
 	/* The net we belong to */
 	tunnel->l2tp_net = net;
@@ -1710,6 +1718,13 @@ struct l2tp_session *l2tp_session_create(int priv_size, struct l2tp_tunnel *tunn
 {
 	struct l2tp_session *session;
 
+	spin_lock_bh(&tunnel->lock);
+	if (tunnel->closing) {
+		spin_unlock_bh(&tunnel->lock);
+		return ERR_PTR(-ENODEV);
+	}
+	spin_unlock_bh(&tunnel->lock);
+
 	session = kzalloc(sizeof(struct l2tp_session) + priv_size, GFP_KERNEL);
 	if (session != NULL) {
 		session->magic = L2TP_SESSION_MAGIC;
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index e88ff7895ccb..4e098c822cd1 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -160,10 +160,6 @@ 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 */
-- 
1.9.1

[PATCH net-next v2 06/16] l2tp: refactor session lifetime handling

[James Chapman]

Simplify relationship with tunnel such that the session holds a ref on
the tunnel, not its socket. This guarantees that the tunnel is always
extant if one or more sessions exists on the tunnel. If the session
has a socket (ppp), have it hold a ref on the socket until the session
is destroyed.

Since pppol2tp_sock_to_session returns a session and the session now
holds a sock ref, have it return with a ref on the session.

Fixes: fd558d186df2c ("l2tp: Split pppol2tp patch into separate l2tp and ppp parts")
Fixes: f3c66d4e144a0 ("l2tp: prevent creation of sessions on terminated tunnels")
---
 net/l2tp/l2tp_core.c |  7 ++-----
 net/l2tp/l2tp_ppp.c  | 36 ++++++++++++++++++------------------
 2 files changed, 20 insertions(+), 23 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 691fe9368d91..477b96cf8ab3 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -290,6 +290,7 @@ int l2tp_session_register(struct l2tp_session *session,
 		spin_unlock_bh(&tunnel->lock);
 		return -ENODEV;
 	}
+	l2tp_tunnel_inc_refcount(tunnel);
 	spin_unlock_bh(&tunnel->lock);
 
 	head = l2tp_session_id_hash(tunnel, session->session_id);
@@ -315,14 +316,9 @@ int l2tp_session_register(struct l2tp_session *session,
 				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);
@@ -334,6 +330,7 @@ int l2tp_session_register(struct l2tp_session *session,
 	spin_unlock_bh(&pn->l2tp_session_hlist_lock);
 err_tlock:
 	write_unlock_bh(&tunnel->hlist_lock);
+	l2tp_tunnel_dec_refcount(tunnel);
 
 	return err;
 }
diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index fe5a0043dd32..ff95a4d4eac5 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -166,16 +166,17 @@ static inline struct l2tp_session *pppol2tp_sock_to_session(struct sock *sk)
 	if (sk == NULL)
 		return NULL;
 
-	sock_hold(sk);
-	session = (struct l2tp_session *)(sk->sk_user_data);
+	rcu_read_lock_bh();
+	session = rcu_dereference_bh(__sk_user_data((sk)));
 	if (session == NULL) {
-		sock_put(sk);
-		goto out;
+		rcu_read_unlock_bh();
+		return NULL;
 	}
+	l2tp_session_inc_refcount(session);
+	rcu_read_unlock();
 
 	BUG_ON(session->magic != L2TP_SESSION_MAGIC);
 
-out:
 	return session;
 }
 
@@ -243,8 +244,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.
 	 */
-	rcu_read_lock();
-	sk = rcu_dereference(ps->sk);
+	rcu_read_lock_bh();
+	sk = rcu_dereference_bh(ps->sk);
 	if (sk == NULL)
 		goto no_sock;
 
@@ -267,12 +268,12 @@ static void pppol2tp_recv(struct l2tp_session *session, struct sk_buff *skb, int
 			kfree_skb(skb);
 		}
 	}
-	rcu_read_unlock();
+	rcu_read_unlock_bh();
 
 	return;
 
 no_sock:
-	rcu_read_unlock();
+	rcu_read_unlock_bh();
 	l2tp_info(session, L2TP_MSG_DATA, "%s: no socket\n", session->name);
 	kfree_skb(skb);
 }
@@ -341,12 +342,12 @@ static int pppol2tp_sendmsg(struct socket *sock, struct msghdr *m,
 	l2tp_xmit_skb(session, skb, session->hdr_len);
 	local_bh_enable();
 
-	sock_put(sk);
+	l2tp_session_dec_refcount(session);
 
 	return total_len;
 
 error_put_sess:
-	sock_put(sk);
+	l2tp_session_dec_refcount(session);
 error:
 	return error;
 }
@@ -400,12 +401,12 @@ static int pppol2tp_xmit(struct ppp_channel *chan, struct sk_buff *skb)
 	l2tp_xmit_skb(session, skb, session->hdr_len);
 	local_bh_enable();
 
-	sock_put(sk);
+	l2tp_session_dec_refcount(session);
 
 	return 1;
 
 abort_put_sess:
-	sock_put(sk);
+	l2tp_session_dec_refcount(session);
 abort:
 	/* Free the original skb */
 	kfree_skb(skb);
@@ -483,7 +484,6 @@ static int pppol2tp_release(struct socket *sock)
 	sock->sk = NULL;
 
 	session = pppol2tp_sock_to_session(sk);
-
 	if (session != NULL) {
 		struct pppol2tp_session *ps;
 
@@ -976,7 +976,7 @@ static int pppol2tp_getname(struct socket *sock, struct sockaddr *uaddr,
 	*usockaddr_len = len;
 	error = 0;
 
-	sock_put(sk);
+	l2tp_session_dec_refcount(session);
 end:
 	return error;
 }
@@ -1247,7 +1247,7 @@ static int pppol2tp_ioctl(struct socket *sock, unsigned int cmd,
 	err = pppol2tp_session_ioctl(session, cmd, arg);
 
 end_put_sess:
-	sock_put(sk);
+	l2tp_session_dec_refcount(session);
 end:
 	return err;
 }
@@ -1398,7 +1398,7 @@ static int pppol2tp_setsockopt(struct socket *sock, int level, int optname,
 		err = pppol2tp_session_setsockopt(sk, session, optname, val);
 	}
 
-	sock_put(sk);
+	l2tp_session_dec_refcount(session);
 end:
 	return err;
 }
@@ -1530,7 +1530,7 @@ static int pppol2tp_getsockopt(struct socket *sock, int level, int optname,
 	err = 0;
 
 end_put_sess:
-	sock_put(sk);
+	l2tp_session_dec_refcount(session);
 end:
 	return err;
 }
-- 
1.9.1

[PATCH net-next v2 07/16] l2tp: hide sessions if they are closing

[James Chapman]

Replace the dead flag in the session context with a closing flag and
spinlock. Check it in session lookup functions such that we don't try
to access session data while it is being destroyed.
---
 net/l2tp/l2tp_core.c | 34 +++++++++++++++++++++++++++++++++-
 net/l2tp/l2tp_core.h |  2 ++
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 477b96cf8ab3..869dec89ff0f 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -198,7 +198,14 @@ struct l2tp_session *l2tp_session_get(const struct net *net,
 		rcu_read_lock_bh();
 		hlist_for_each_entry_rcu(session, session_list, global_hlist) {
 			if (session->session_id == session_id) {
+				spin_lock_bh(&session->lock);
+				if (session->closing) {
+					spin_unlock_bh(&session->lock);
+					rcu_read_unlock_bh();
+					return NULL;
+				}
 				l2tp_session_inc_refcount(session);
+				spin_unlock_bh(&session->lock);
 				rcu_read_unlock_bh();
 
 				return session;
@@ -213,7 +220,14 @@ struct l2tp_session *l2tp_session_get(const struct net *net,
 	read_lock_bh(&tunnel->hlist_lock);
 	hlist_for_each_entry(session, session_list, hlist) {
 		if (session->session_id == session_id) {
+			spin_lock_bh(&session->lock);
+			if (session->closing) {
+				spin_unlock_bh(&session->lock);
+				read_unlock_bh(&tunnel->hlist_lock);
+				return NULL;
+			}
 			l2tp_session_inc_refcount(session);
+			spin_unlock_bh(&session->lock);
 			read_unlock_bh(&tunnel->hlist_lock);
 
 			return session;
@@ -234,6 +248,12 @@ struct l2tp_session *l2tp_session_get_nth(struct l2tp_tunnel *tunnel, int nth)
 	read_lock_bh(&tunnel->hlist_lock);
 	for (hash = 0; hash < L2TP_HASH_SIZE; hash++) {
 		hlist_for_each_entry(session, &tunnel->session_hlist[hash], hlist) {
+			spin_lock_bh(&session->lock);
+			if (session->closing) {
+				spin_unlock_bh(&session->lock);
+				continue;
+			}
+			spin_unlock_bh(&session->lock);
 			if (++count > nth) {
 				l2tp_session_inc_refcount(session);
 				read_unlock_bh(&tunnel->hlist_lock);
@@ -261,6 +281,12 @@ struct l2tp_session *l2tp_session_get_by_ifname(const struct net *net,
 	rcu_read_lock_bh();
 	for (hash = 0; hash < L2TP_HASH_SIZE_2; hash++) {
 		hlist_for_each_entry_rcu(session, &pn->l2tp_session_hlist[hash], global_hlist) {
+			spin_lock_bh(&session->lock);
+			if (session->closing) {
+				spin_unlock_bh(&session->lock);
+				continue;
+			}
+			spin_unlock_bh(&session->lock);
 			if (!strcmp(session->ifname, ifname)) {
 				l2tp_session_inc_refcount(session);
 				rcu_read_unlock_bh();
@@ -1678,8 +1704,13 @@ void __l2tp_session_unhash(struct l2tp_session *session)
  */
 int l2tp_session_delete(struct l2tp_session *session)
 {
-	if (test_and_set_bit(0, &session->dead))
+	spin_lock_bh(&session->lock);
+	if (session->closing) {
+		spin_unlock_bh(&session->lock);
 		return 0;
+	}
+	session->closing = true;
+	spin_unlock_bh(&session->lock);
 
 	__l2tp_session_unhash(session);
 	l2tp_session_queue_purge(session);
@@ -1747,6 +1778,7 @@ struct l2tp_session *l2tp_session_create(int priv_size, struct l2tp_tunnel *tunn
 
 		INIT_HLIST_NODE(&session->hlist);
 		INIT_HLIST_NODE(&session->global_hlist);
+		spin_lock_init(&session->lock);
 
 		/* Inherit debug options from tunnel */
 		session->debug = tunnel->debug;
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index 4e098c822cd1..d28d91600ad5 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -75,6 +75,8 @@ struct l2tp_session {
 	int			magic;		/* should be
 						 * L2TP_SESSION_MAGIC */
 	long			dead;
+	bool                    closing;
+	spinlock_t              lock;		/* protect closing */
 
 	struct l2tp_tunnel	*tunnel;	/* back pointer to tunnel
 						 * context */
-- 
1.9.1

[PATCH net-next v2 08/16] l2tp: hide session from pppol2tp_sock_to_session if it is closing

[James Chapman]

---
 net/l2tp/l2tp_ppp.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index ff95a4d4eac5..947066b3d6d8 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -172,8 +172,16 @@ static inline struct l2tp_session *pppol2tp_sock_to_session(struct sock *sk)
 		rcu_read_unlock_bh();
 		return NULL;
 	}
+
+	spin_lock_bh(&session->lock);
+	if (session->closing) {
+		spin_unlock_bh(&session->lock);
+		rcu_read_unlock_bh();
+		return NULL;
+	}
 	l2tp_session_inc_refcount(session);
-	rcu_read_unlock();
+	spin_unlock_bh(&session->lock);
+	rcu_read_unlock_bh();
 
 	BUG_ON(session->magic != L2TP_SESSION_MAGIC);
 
-- 
1.9.1

[PATCH net-next v2 09/16] l2tp: refactor pppol2tp_connect

[James Chapman]

It's hard to understand pppol2tp_connect so split it up into separate
functions and document it better.

Fixes: fd558d186d ("l2tp: Split pppol2tp patch into separate l2tp and ppp parts")
---
 net/l2tp/l2tp_ppp.c | 307 +++++++++++++++++++++++++++++++---------------------
 1 file changed, 185 insertions(+), 122 deletions(-)

diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index 947066b3d6d8..19efb56620ab 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -425,6 +425,17 @@ static int pppol2tp_xmit(struct ppp_channel *chan, struct sk_buff *skb)
  * Session (and tunnel control) socket create/destroy.
  *****************************************************************************/
 
+/* called with ps->sk_lock */
+static void pppol2tp_attach(struct l2tp_session *session, struct sock *sk)
+{
+	struct pppol2tp_session *ps = l2tp_session_priv(session);
+
+	write_lock_bh(&sk->sk_callback_lock);
+	rcu_assign_sk_user_data(sk, session);
+	write_unlock_bh(&sk->sk_callback_lock);
+	rcu_assign_pointer(ps->sk, sk);
+}
+
 /* Called by l2tp_core when a session socket is being closed.
  */
 static void pppol2tp_session_close(struct l2tp_session *session)
@@ -462,10 +473,18 @@ static void pppol2tp_session_destruct(struct sock *sk)
 
 static void pppol2tp_put_sk(struct rcu_head *head)
 {
-	struct pppol2tp_session *ps;
+	struct pppol2tp_session *ps = container_of(head, typeof(*ps), rcu);
+	struct l2tp_session *session = container_of((void *)ps,
+						    typeof(*session), priv);
+	struct sock *sk = ps->__sk;
 
-	ps = container_of(head, typeof(*ps), rcu);
-	sock_put(ps->__sk);
+	/* If session is invalid, something serious is wrong. Warn and
+	 * leak the socket.
+	 */
+	WARN_ON(session->magic != L2TP_SESSION_MAGIC);
+	if (session->magic != L2TP_SESSION_MAGIC)
+		return;
+	sock_put(sk);
 }
 
 /* Called when the PPPoX socket (session) is closed.
@@ -615,25 +634,147 @@ static void pppol2tp_session_init(struct l2tp_session *session)
 	}
 }
 
-/* connect() handler. Attach a PPPoX socket to a tunnel UDP socket
+/* Prepare a tunnel. If a tunnel instance doesn't already exist,
+ * optionally create it. Return with a ref on the tunnel instance.
+ */
+static int pppol2tp_tunnel_prep(struct net *net, int fd, int ver,
+				u32 tunnel_id, u32 peer_tunnel_id,
+				bool can_create, struct l2tp_tunnel **tunnelp)
+{
+	struct l2tp_tunnel *tunnel;
+	int error;
+
+	tunnel = l2tp_tunnel_get(net, tunnel_id);
+	if (!tunnel && can_create) {
+		struct l2tp_tunnel_cfg tcfg = {
+			.encap = L2TP_ENCAPTYPE_UDP,
+			.debug = 0,
+		};
+		error = l2tp_tunnel_create(net, fd, ver, tunnel_id,
+					   peer_tunnel_id, &tcfg, &tunnel);
+		if (error < 0)
+			return error;
+
+		l2tp_tunnel_inc_refcount(tunnel);
+	}
+
+	/* Error if we can't find the tunnel */
+	if (!tunnel)
+		return -ENOENT;
+
+	if (!tunnel->recv_payload_hook)
+		tunnel->recv_payload_hook = pppol2tp_recv_payload_hook;
+
+	if (tunnel->peer_tunnel_id == 0)
+		tunnel->peer_tunnel_id = peer_tunnel_id;
+
+	*tunnelp = tunnel;
+	return 0;
+
+	l2tp_tunnel_dec_refcount(tunnel);
+	return error;
+}
+
+/* Prepare a session in a tunnel. If the session doesn't already
+ * exist, create it and add it to the tunnel's session list. Return
+ * with a ref on the session instance and its sk_lock held.
+ */
+static int pppol2tp_session_prep(struct sock *sk, struct l2tp_tunnel *tunnel,
+				 u32 session_id, u32 peer_session_id,
+				 struct l2tp_session **sessionp)
+{
+	struct l2tp_session *session;
+	struct pppol2tp_session *ps;
+	int error;
+	struct l2tp_session_cfg cfg = {};
+
+	session = l2tp_session_get(sock_net(sk), tunnel, session_id);
+	if (session) {
+		ps = l2tp_session_priv(session);
+
+		/* Using a pre-existing session is fine as long as it hasn't
+		 * been connected yet.
+		 */
+		mutex_lock(&ps->sk_lock);
+		if (rcu_dereference_protected(ps->sk,
+					      lockdep_is_held(&ps->sk_lock))) {
+			mutex_unlock(&ps->sk_lock);
+			l2tp_session_dec_refcount(session);
+			return -EEXIST;
+		}
+	} else {
+		/* Default MTU must allow space for UDP/L2TP/PPP headers */
+		cfg.mtu = 1500 - PPPOL2TP_HEADER_OVERHEAD;
+		cfg.mru = cfg.mtu;
+
+		session = l2tp_session_create(sizeof(struct pppol2tp_session),
+					      tunnel, session_id,
+					      peer_session_id, &cfg);
+		if (IS_ERR(session)) {
+			error = PTR_ERR(session);
+			return error;
+		}
+
+		pppol2tp_session_init(session);
+		ps = l2tp_session_priv(session);
+
+		mutex_lock(&ps->sk_lock);
+		error = l2tp_session_register(session, tunnel);
+		if (error < 0) {
+			mutex_unlock(&ps->sk_lock);
+			kfree(session);
+			return error;
+		}
+		l2tp_session_inc_refcount(session);
+	}
+
+	*sessionp = session;
+	return 0;
+}
+
+static int pppol2tp_setup_ppp(struct l2tp_session *session, struct sock *sk)
+{
+	struct pppox_sock *po = pppox_sk(sk);
+
+	/* The only header we need to worry about is the L2TP
+	 * header. This size is different depending on whether
+	 * sequence numbers are enabled for the data channel.
+	 */
+	po->chan.hdrlen = PPPOL2TP_L2TP_HDR_SIZE_NOSEQ;
+
+	po->chan.private = sk;
+	po->chan.ops	 = &pppol2tp_chan_ops;
+	po->chan.mtu	 = session->mtu;
+
+	return ppp_register_net_channel(sock_net(sk), &po->chan);
+}
+
+/* connect() handler. Attach a PPPoX socket to a tunnel socket.
+ * The PPPoX socket is associated with an l2tp_session and the tunnel
+ * socket is associated with an l2tp_tunnel. The l2tp_tunnel and
+ * l2tp_session are usually created by netlink before the PPPoX socket
+ * is connected. However, for L2TPv2 we support a legacy mode where
+ * netlink is not used and we create the l2tp_tunnel and l2tp_session
+ * when the PPPoX sockets are connected. In legacy mode, a per-tunnel
+ * PPPoX socket is used as a control socket for the tunnel and is
+ * identified by session_id 0. An l2tp_session is created to manage
+ * the control socket and an l2tp_tunnel is created for the tunnel if
+ * it doesn't already exist.
  */
 static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
 			    int sockaddr_len, int flags)
 {
 	struct sock *sk = sock->sk;
 	struct sockaddr_pppol2tp *sp = (struct sockaddr_pppol2tp *) uservaddr;
-	struct pppox_sock *po = pppox_sk(sk);
 	struct l2tp_session *session = NULL;
-	struct l2tp_tunnel *tunnel;
+	struct l2tp_tunnel *tunnel = NULL;
 	struct pppol2tp_session *ps;
-	struct l2tp_session_cfg cfg = { 0, };
 	int error = 0;
 	u32 tunnel_id, peer_tunnel_id;
 	u32 session_id, peer_session_id;
-	bool drop_refcnt = false;
-	bool drop_tunnel = false;
 	int ver = 2;
 	int fd;
+	bool is_ctrl_skt;
 
 	lock_sock(sk);
 
@@ -695,139 +836,56 @@ static int pppol2tp_connect(struct socket *sock, struct sockaddr *uservaddr,
 		goto end; /* bad socket address */
 	}
 
-	/* Don't bind if tunnel_id is 0 */
 	error = -EINVAL;
-	if (tunnel_id == 0)
+	if (tunnel_id == 0 || peer_tunnel_id == 0)
 		goto end;
 
-	tunnel = l2tp_tunnel_get(sock_net(sk), tunnel_id);
-	if (tunnel)
-		drop_tunnel = true;
-
-	/* Special case: create tunnel context if session_id and
-	 * peer_session_id is 0. Otherwise look up tunnel using supplied
-	 * tunnel id.
+	/* The socket is a control socket if session_id is 0. There is
+	 * one control socket per tunnel. Control sockets do not have ppp.
 	 */
-	if ((session_id == 0) && (peer_session_id == 0)) {
-		if (tunnel == NULL) {
-			struct l2tp_tunnel_cfg tcfg = {
-				.encap = L2TP_ENCAPTYPE_UDP,
-				.debug = 0,
-			};
-			error = l2tp_tunnel_create(sock_net(sk), fd, ver, tunnel_id, peer_tunnel_id, &tcfg, &tunnel);
-			if (error < 0)
-				goto end;
-		}
-	} else {
-		/* Error if we can't find the tunnel */
-		error = -ENOENT;
-		if (tunnel == NULL)
-			goto end;
-
-		/* Error if socket is not prepped */
-		if (tunnel->sock == NULL)
-			goto end;
-	}
-
-	if (tunnel->recv_payload_hook == NULL)
-		tunnel->recv_payload_hook = pppol2tp_recv_payload_hook;
-
-	if (tunnel->peer_tunnel_id == 0)
-		tunnel->peer_tunnel_id = peer_tunnel_id;
-
-	session = l2tp_session_get(sock_net(sk), tunnel, session_id);
-	if (session) {
-		drop_refcnt = true;
-		ps = l2tp_session_priv(session);
+	is_ctrl_skt = (session_id == 0 && peer_session_id == 0);
 
-		/* Using a pre-existing session is fine as long as it hasn't
-		 * been connected yet.
-		 */
-		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;
-		}
-	} else {
-		/* Default MTU must allow space for UDP/L2TP/PPP headers */
-		cfg.mtu = 1500 - PPPOL2TP_HEADER_OVERHEAD;
-		cfg.mru = cfg.mtu;
-
-		session = l2tp_session_create(sizeof(struct pppol2tp_session),
-					      tunnel, session_id,
-					      peer_session_id, &cfg);
-		if (IS_ERR(session)) {
-			error = PTR_ERR(session);
-			goto end;
-		}
+	/* prep and possibly create the l2tp tunnel instance */
+	error = pppol2tp_tunnel_prep(sock_net(sk), fd, ver, tunnel_id,
+				     peer_tunnel_id, is_ctrl_skt, &tunnel);
+	if (error)
+		goto end;
 
-		pppol2tp_session_init(session);
-		ps = l2tp_session_priv(session);
-		l2tp_session_inc_refcount(session);
+	/* prep and possibly create the l2tp session instance */
+	error = pppol2tp_session_prep(sk, tunnel, session_id,
+				      peer_session_id, &session);
+	if (error)
+		goto end;
 
-		mutex_lock(&ps->sk_lock);
-		error = l2tp_session_register(session, tunnel);
-		if (error < 0) {
+	/* setup ppp unless it's a control socket */
+	ps = l2tp_session_priv(session);
+	if (!is_ctrl_skt) {
+		error = pppol2tp_setup_ppp(session, sk);
+		if (error) {
 			mutex_unlock(&ps->sk_lock);
-			kfree(session);
 			goto end;
 		}
-		drop_refcnt = true;
-	}
-
-	/* 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()
-	 * handlers.
-	 */
-	if ((session->session_id == 0) &&
-	    (session->peer_session_id == 0)) {
-		error = 0;
-		goto out_no_ppp;
 	}
 
-	/* The only header we need to worry about is the L2TP
-	 * header. This size is different depending on whether
-	 * sequence numbers are enabled for the data channel.
+	/* The session has now been added to the tunnel. Hold the
+	 * socket to prevent it going away until the session is
+	 * destroyed and attach it to the session such that we can get
+	 * the session instance from the socket and vice versa.
 	 */
-	po->chan.hdrlen = PPPOL2TP_L2TP_HDR_SIZE_NOSEQ;
-
-	po->chan.private = sk;
-	po->chan.ops	 = &pppol2tp_chan_ops;
-	po->chan.mtu	 = session->mtu;
-
-	error = ppp_register_net_channel(sock_net(sk), &po->chan);
-	if (error) {
-		mutex_unlock(&ps->sk_lock);
-		goto end;
-	}
-
-out_no_ppp:
-	/* This is how we get the session context from the socket. */
-	write_lock_bh(&sk->sk_callback_lock);
-	rcu_assign_sk_user_data(sk, session);
-	write_unlock_bh(&sk->sk_callback_lock);
-	rcu_assign_pointer(ps->sk, sk);
+	sock_hold(sk);
+	pppol2tp_attach(session, 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);
 
 end:
-	if (drop_refcnt)
+	release_sock(sk);
+	if (session)
 		l2tp_session_dec_refcount(session);
-	if (drop_tunnel)
+	if (tunnel)
 		l2tp_tunnel_dec_refcount(tunnel);
-	release_sock(sk);
 
 	return error;
 }
@@ -841,6 +899,7 @@ 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) {
@@ -864,10 +923,14 @@ static int pppol2tp_session_create(struct net *net, struct l2tp_tunnel *tunnel,
 	}
 
 	pppol2tp_session_init(session);
-
+	ps = l2tp_session_priv(session);
+	mutex_lock(&ps->sk_lock);
 	error = l2tp_session_register(session, tunnel);
-	if (error < 0)
+	if (error < 0) {
+		mutex_unlock(&ps->sk_lock);
 		goto err_sess;
+	}
+	mutex_unlock(&ps->sk_lock);
 
 	return 0;
 
-- 
1.9.1

[PATCH net-next v2 10/16] l2tp: add session_free callback

[James Chapman]

When a session refcount hits 0, the session is freed via
l2tp_session_free. Some pseudowires (ppp, eth) may have additional
resources to free when this happens. Add a session_free callback that
can be used by pseudowires to override the default kfree. The callback
is responsible for freeing the session.
---
 net/l2tp/l2tp_core.c | 7 +++++--
 net/l2tp/l2tp_core.h | 1 +
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 869dec89ff0f..d6306ba2d78e 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1662,12 +1662,15 @@ void l2tp_session_free(struct l2tp_session *session)
 
 	BUG_ON(refcount_read(&session->ref_count) != 0);
 
+	if (session->session_free)
+		session->session_free(session);
+	else
+		kfree(session);
+
 	if (tunnel) {
 		BUG_ON(tunnel->magic != L2TP_TUNNEL_MAGIC);
 		l2tp_tunnel_dec_refcount(tunnel);
 	}
-
-	kfree(session);
 }
 EXPORT_SYMBOL_GPL(l2tp_session_free);
 
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index d28d91600ad5..094b2e0dbd75 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -126,6 +126,7 @@ struct l2tp_session {
 	int (*build_header)(struct l2tp_session *session, void *buf);
 	void (*recv_skb)(struct l2tp_session *session, struct sk_buff *skb, int data_len);
 	void (*session_close)(struct l2tp_session *session);
+	void (*session_free)(struct l2tp_session *session);
 #if IS_ENABLED(CONFIG_L2TP_DEBUGFS)
 	void (*show)(struct seq_file *m, void *priv);
 #endif
-- 
1.9.1

[PATCH net-next v2 11/16] l2tp: do session destroy using a workqueue

[James Chapman]

Handle session destroy in the same way as we handle tunnel destroy -
through a workqueue. Sessions can be destroyed either because its
socket is closed (if it has a socket) or by netlink request. A
workqueue synchronises these.
---
 net/l2tp/l2tp_core.c | 30 +++++++++++++++++++++++-------
 net/l2tp/l2tp_core.h |  2 ++
 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index d6306ba2d78e..55b1f312fedc 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1702,6 +1702,24 @@ void __l2tp_session_unhash(struct l2tp_session *session)
 }
 EXPORT_SYMBOL_GPL(__l2tp_session_unhash);
 
+/* Workqueue session deletion function */
+static void l2tp_session_del_work(struct work_struct *work)
+{
+	struct l2tp_session *session = container_of(work, struct l2tp_session,
+						    del_work);
+
+	__l2tp_session_unhash(session);
+	l2tp_session_queue_purge(session);
+	if (session->session_close)
+		(*session->session_close)(session);
+
+	/* drop initial ref */
+	l2tp_session_dec_refcount(session);
+
+	/* drop workqueue ref */
+	l2tp_session_dec_refcount(session);
+}
+
 /* This function is used by the netlink SESSION_DELETE command and by
    pseudowire modules.
  */
@@ -1715,13 +1733,9 @@ int l2tp_session_delete(struct l2tp_session *session)
 	session->closing = true;
 	spin_unlock_bh(&session->lock);
 
-	__l2tp_session_unhash(session);
-	l2tp_session_queue_purge(session);
-	if (session->session_close != NULL)
-		(*session->session_close)(session);
-
-	l2tp_session_dec_refcount(session);
-
+	/* Hold session ref while queued work item is pending */
+	l2tp_session_inc_refcount(session);
+	queue_work(l2tp_wq, &session->del_work);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(l2tp_session_delete);
@@ -1783,6 +1797,8 @@ struct l2tp_session *l2tp_session_create(int priv_size, struct l2tp_tunnel *tunn
 		INIT_HLIST_NODE(&session->global_hlist);
 		spin_lock_init(&session->lock);
 
+		INIT_WORK(&session->del_work, l2tp_session_del_work);
+
 		/* Inherit debug options from tunnel */
 		session->debug = tunnel->debug;
 
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index 094b2e0dbd75..8a11badb7104 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -123,6 +123,8 @@ struct l2tp_session {
 	struct l2tp_stats	stats;
 	struct hlist_node	global_hlist;	/* Global hash list node */
 
+	struct work_struct	del_work;
+
 	int (*build_header)(struct l2tp_session *session, void *buf);
 	void (*recv_skb)(struct l2tp_session *session, struct sk_buff *skb, int data_len);
 	void (*session_close)(struct l2tp_session *session);
-- 
1.9.1

[PATCH net-next v2 12/16] l2tp: simplify l2tp_tunnel_closeall

[James Chapman]

Since session destroy now uses a workqueue, let l2tp_session_delete
handle all the work of destroying a session. Don't remove the session
from the tunnel's list immediately. The tunnel will remain extant
until all of its sessions are gone anyway.

The session's dead flag is now unused so is removed.
---
 net/l2tp/l2tp_core.c | 32 ++++----------------------------
 net/l2tp/l2tp_core.h |  1 -
 2 files changed, 4 insertions(+), 29 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 55b1f312fedc..c909fe9273c9 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1254,36 +1254,9 @@ void l2tp_tunnel_closeall(struct l2tp_tunnel *tunnel)
 
 	write_lock_bh(&tunnel->hlist_lock);
 	for (hash = 0; hash < L2TP_HASH_SIZE; hash++) {
-again:
 		hlist_for_each_safe(walk, tmp, &tunnel->session_hlist[hash]) {
 			session = hlist_entry(walk, struct l2tp_session, hlist);
-
-			l2tp_info(session, L2TP_MSG_CONTROL,
-				  "%s: closing session\n", session->name);
-
-			hlist_del_init(&session->hlist);
-
-			if (test_and_set_bit(0, &session->dead))
-				goto again;
-
-			write_unlock_bh(&tunnel->hlist_lock);
-
-			__l2tp_session_unhash(session);
-			l2tp_session_queue_purge(session);
-
-			if (session->session_close != NULL)
-				(*session->session_close)(session);
-
-			l2tp_session_dec_refcount(session);
-
-			write_lock_bh(&tunnel->hlist_lock);
-
-			/* Now restart from the beginning of this hash
-			 * chain.  We always remove a session from the
-			 * list so we are guaranteed to make forward
-			 * progress.
-			 */
-			goto again;
+			l2tp_session_delete(session);
 		}
 	}
 	write_unlock_bh(&tunnel->hlist_lock);
@@ -1708,6 +1681,9 @@ static void l2tp_session_del_work(struct work_struct *work)
 	struct l2tp_session *session = container_of(work, struct l2tp_session,
 						    del_work);
 
+	l2tp_info(session, L2TP_MSG_CONTROL,
+		  "%s: closing session\n", session->name);
+
 	__l2tp_session_unhash(session);
 	l2tp_session_queue_purge(session);
 	if (session->session_close)
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index 8a11badb7104..73c4ce79c708 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -74,7 +74,6 @@ struct l2tp_session_cfg {
 struct l2tp_session {
 	int			magic;		/* should be
 						 * L2TP_SESSION_MAGIC */
-	long			dead;
 	bool                    closing;
 	spinlock_t              lock;		/* protect closing */
 
-- 
1.9.1

[PATCH net-next v2 13/16] l2tp: refactor ppp session cleanup paths

[James Chapman]

Use l2tp core's session_free callback to drive the ppp session
cleanup. PPP sessions are cleaned up by RCU. The PPP session socket is
allowed to close only when the session is freed.

With this patch, the following syzbot bug reports are finally fixed.

Reported-by: syzbot+9df43faf09bd400f2993@syzkaller.appspotmail.com
Reported-by: syzbot+6e6a5ec8de31a94cd015@syzkaller.appspotmail.com
Reported-by: syzbot+19c09769f14b48810113@syzkaller.appspotmail.com
Reported-by: syzbot+347bd5acde002e353a36@syzkaller.appspotmail.com
---
 net/l2tp/l2tp_ppp.c | 109 +++++++++++++++++++++++++++++-----------------------
 1 file changed, 60 insertions(+), 49 deletions(-)

diff --git a/net/l2tp/l2tp_ppp.c b/net/l2tp/l2tp_ppp.c
index 19efb56620ab..e50feb1479e6 100644
--- a/net/l2tp/l2tp_ppp.c
+++ b/net/l2tp/l2tp_ppp.c
@@ -436,18 +436,68 @@ static void pppol2tp_attach(struct l2tp_session *session, struct sock *sk)
 	rcu_assign_pointer(ps->sk, sk);
 }
 
-/* Called by l2tp_core when a session socket is being closed.
+/* called with ps->sk_lock */
+static void pppol2tp_detach(struct l2tp_session *session, struct sock *sk)
+{
+	struct pppol2tp_session *ps = l2tp_session_priv(session);
+
+	rcu_assign_pointer(ps->sk, NULL);
+	write_lock_bh(&sk->sk_callback_lock);
+	rcu_assign_sk_user_data(sk, NULL);
+	write_unlock_bh(&sk->sk_callback_lock);
+}
+
+static void pppol2tp_session_free_rcu(struct rcu_head *head)
+{
+	struct pppol2tp_session *ps = container_of(head, typeof(*ps), rcu);
+	struct l2tp_session *session = container_of((void *)ps,
+						    typeof(*session), priv);
+
+	/* If session is invalid, something serious is wrong. Warn and
+	 * leak the socket and session.
+	 */
+	WARN_ON(session->magic != L2TP_SESSION_MAGIC);
+	if (session->magic != L2TP_SESSION_MAGIC)
+		return;
+
+	if (ps->__sk)
+		sock_put(ps->__sk);
+	kfree(session);
+}
+
+/* Called by l2tp_core when a session is being freed.
+ */
+static void pppol2tp_session_free(struct l2tp_session *session)
+{
+	struct pppol2tp_session *ps = l2tp_session_priv(session);
+
+	/* If session is invalid, something serious is wrong. Warn and
+	 * leak the socket and session.
+	 */
+	WARN_ON(session->magic != L2TP_SESSION_MAGIC);
+	if (session->magic != L2TP_SESSION_MAGIC)
+		return;
+
+	call_rcu(&ps->rcu, pppol2tp_session_free_rcu);
+}
+
+/* Called by l2tp_core when a session is being closed.
  */
 static void pppol2tp_session_close(struct l2tp_session *session)
 {
 	struct sock *sk;
 
 	BUG_ON(session->magic != L2TP_SESSION_MAGIC);
-
 	sk = pppol2tp_session_get_sock(session);
 	if (sk) {
-		if (sk->sk_socket)
-			inet_shutdown(sk->sk_socket, SEND_SHUTDOWN);
+		struct pppol2tp_session *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);
+		pppol2tp_detach(session, sk);
+		mutex_unlock(&ps->sk_lock);
 		sock_put(sk);
 	}
 }
@@ -457,34 +507,8 @@ static void pppol2tp_session_close(struct l2tp_session *session)
  */
 static void pppol2tp_session_destruct(struct sock *sk)
 {
-	struct l2tp_session *session = sk->sk_user_data;
-
 	skb_queue_purge(&sk->sk_receive_queue);
 	skb_queue_purge(&sk->sk_write_queue);
-
-	if (session) {
-		write_lock_bh(&sk->sk_callback_lock);
-		rcu_assign_sk_user_data(sk, NULL);
-		write_unlock_bh(&sk->sk_callback_lock);
-		BUG_ON(session->magic != L2TP_SESSION_MAGIC);
-		l2tp_session_dec_refcount(session);
-	}
-}
-
-static void pppol2tp_put_sk(struct rcu_head *head)
-{
-	struct pppol2tp_session *ps = container_of(head, typeof(*ps), rcu);
-	struct l2tp_session *session = container_of((void *)ps,
-						    typeof(*session), priv);
-	struct sock *sk = ps->__sk;
-
-	/* If session is invalid, something serious is wrong. Warn and
-	 * leak the socket.
-	 */
-	WARN_ON(session->magic != L2TP_SESSION_MAGIC);
-	if (session->magic != L2TP_SESSION_MAGIC)
-		return;
-	sock_put(sk);
 }
 
 /* Called when the PPPoX socket (session) is closed.
@@ -509,27 +533,13 @@ static int pppol2tp_release(struct socket *sock)
 	sk->sk_state = PPPOX_DEAD;
 	sock_orphan(sk);
 	sock->sk = NULL;
+	release_sock(sk);
 
-	session = pppol2tp_sock_to_session(sk);
-	if (session != NULL) {
-		struct pppol2tp_session *ps;
-
+	rcu_read_lock_bh();
+	session = rcu_dereference_bh(__sk_user_data((sk)));
+	if (session)
 		l2tp_session_delete(session);
-
-		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);
+	rcu_read_unlock_bh();
 
 	/* This will delete the session context via
 	 * pppol2tp_session_destruct() if the socket's refcnt drops to
@@ -613,6 +623,7 @@ static void pppol2tp_session_init(struct l2tp_session *session)
 
 	session->recv_skb = pppol2tp_recv;
 	session->session_close = pppol2tp_session_close;
+	session->session_free = pppol2tp_session_free;
 #if IS_ENABLED(CONFIG_L2TP_DEBUGFS)
 	session->show = pppol2tp_show;
 #endif
-- 
1.9.1

[PATCH net-next v2 14/16] l2tp: remove redundant sk_user_data check when creating tunnels

[James Chapman]

l2tp_tunnel_create now checks sk_user_data so this check is redundant
---
 net/l2tp/l2tp_core.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index c909fe9273c9..a91cd384e397 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1488,14 +1488,6 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
 		break;
 	}
 
-	/* Check if this socket has already been prepped */
-	tunnel = l2tp_tunnel(sk);
-	if (tunnel != NULL) {
-		/* This socket has already been prepped */
-		err = -EBUSY;
-		goto err;
-	}
-
 	tunnel = kzalloc(sizeof(struct l2tp_tunnel), GFP_KERNEL);
 	if (tunnel == NULL) {
 		err = -ENOMEM;
-- 
1.9.1

[PATCH net-next v2 15/16] l2tp: remove unwanted error message

[James Chapman]

If when creating a new tunnel, the indicated fd is closed by another
thread, we emit an error message about it. e.g.

  l2tp_core: tunl 4: sockfd_lookup(fd=3) returned -9

 It's not useful so remove it.
---
 net/l2tp/l2tp_core.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index a91cd384e397..28940cf9cc76 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1449,8 +1449,6 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
 	} else {
 		sock = sockfd_lookup(fd, &err);
 		if (!sock) {
-			pr_err("tunl %u: sockfd_lookup(fd=%d) returned %d\n",
-			       tunnel_id, fd, err);
 			err = -EBADF;
 			goto err;
 		}
-- 
1.9.1

[PATCH net-next v2 16/16] l2tp: make __l2tp_session_unhash internal

[James Chapman]

__l2tp_session_unhash is now only used internally so there is no
reason to expose it to other l2tp modules. Rename it
l2tp_session_unhash while we're at it.
---
 net/l2tp/l2tp_core.c | 5 ++---
 net/l2tp/l2tp_core.h | 1 -
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
index 28940cf9cc76..34221c6763d8 100644
--- a/net/l2tp/l2tp_core.c
+++ b/net/l2tp/l2tp_core.c
@@ -1642,7 +1642,7 @@ void l2tp_session_free(struct l2tp_session *session)
  * shutdown via. l2tp_session_delete and a pseudowire-specific session_close
  * callback.
  */
-void __l2tp_session_unhash(struct l2tp_session *session)
+static void l2tp_session_unhash(struct l2tp_session *session)
 {
 	struct l2tp_tunnel *tunnel = session->tunnel;
 
@@ -1663,7 +1663,6 @@ void __l2tp_session_unhash(struct l2tp_session *session)
 		}
 	}
 }
-EXPORT_SYMBOL_GPL(__l2tp_session_unhash);
 
 /* Workqueue session deletion function */
 static void l2tp_session_del_work(struct work_struct *work)
@@ -1674,7 +1673,7 @@ static void l2tp_session_del_work(struct work_struct *work)
 	l2tp_info(session, L2TP_MSG_CONTROL,
 		  "%s: closing session\n", session->name);
 
-	__l2tp_session_unhash(session);
+	l2tp_session_unhash(session);
 	l2tp_session_queue_purge(session);
 	if (session->session_close)
 		(*session->session_close)(session);
diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h
index 73c4ce79c708..faca18046518 100644
--- a/net/l2tp/l2tp_core.h
+++ b/net/l2tp/l2tp_core.h
@@ -239,7 +239,6 @@ struct l2tp_session *l2tp_session_create(int priv_size,
 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);
 void l2tp_recv_common(struct l2tp_session *session, struct sk_buff *skb,
-- 
1.9.1

Re: [PATCH net-next v2 01/16] l2tp: update sk_user_data while holding sk_callback_lock

[David Miller]

From: James Chapman <jchapman@katalix.com>
Date: Mon, 12 Feb 2018 10:11:05 +0000

> Since L2TP hooks on sockets opened by userspace using sk_user_data, we
> may race with other socket families that attempt to use the same
> socket.
> 
> This problem was discovered by syzbot using AF_KCM. KCM has since been
> modified to use only TCP sockets to avoid hitting this issue but we
> should prevent such races in L2TP anyway.
> 
> Fixes: c8fffcea0a079 ("l2tp: Refactor l2tp core driver to make use of the common UDP tunnel function")
> Reported-by: syzbot+8865eaff7f9acd593945@syzkaller.appspotmail.com

Yikes.  Where is your signoff James?

> Kernel BUG at net/l2tp/l2tp_ppp.c:176!
> invalid opcode: 0000 [#1] SMP KASAN
> Dumping ftrace buffer:
>    (ftrace buffer empty)

And this oops dump should be before the various
fixes/reported-by/signed-off-by tags.

Thanks.

Re: [PATCH net-next v2 02/16] l2tp: add RCU read lock to protect tunnel ptr in ip socket destroy

[David Miller]

From: James Chapman <jchapman@katalix.com>
Date: Mon, 12 Feb 2018 10:11:06 +0000

> If an L2TPIP socket is closed, add RCU protection when we deref
> sk_user_data to prevent races with another thread closing the same
> tunnel.
> 
> Fixes: 0d76751fad ("l2tp: Add L2TPv3 IP encapsulation (no UDP) support")

Another patch with no signoff, and:

>  refcount_t: increment on 0; use-after-free.
>  WARNING: CPU: 2 PID: 2892 at lib/refcount.c:153 refcount_inc+0x2b/0x30
>  Modules linked in:
>  CPU: 2 PID: 2892 Comm: pppol2tp_chaos Not tainted 4.15.0-rc9+ #1
 ...

a kernel log dump which should be in the commit message body.

Re: [PATCH net-next v2 03/16] l2tp: don't use inet_shutdown on tunnel destroy

[David Miller]

From: James Chapman <jchapman@katalix.com>
Date: Mon, 12 Feb 2018 10:11:07 +0000

> Previously, if a tunnel was closed, we called inet_shutdown to mark
> the socket as unconnected such that userspace would get errors and
> then close the socket. This could race with userspace closing the
> socket. Instead, leave userspace to close the socket in its own time
> (our tunnel will be detached anyway).
> 
> Fixes: 309795f4be ("l2tp: Add netlink control API for L2TP")
 ...
>  BUG: unable to handle kernel NULL pointer dereference at 00000000000000a0
>  IP: __lock_acquire+0x263/0x1630

Ok, all of these are like this.

Please fix this up, put the kernel log message into the message body
and add an appropriate signoff.

Thank you.

Re: [PATCH net-next v2 03/16] l2tp: don't use inet_shutdown on tunnel destroy

[James Chapman]

On 12/02/18 16:22, David Miller wrote:
> From: James Chapman <jchapman@katalix.com>
> Date: Mon, 12 Feb 2018 10:11:07 +0000
>
>> Previously, if a tunnel was closed, we called inet_shutdown to mark
>> the socket as unconnected such that userspace would get errors and
>> then close the socket. This could race with userspace closing the
>> socket. Instead, leave userspace to close the socket in its own time
>> (our tunnel will be detached anyway).
>>
>> Fixes: 309795f4be ("l2tp: Add netlink control API for L2TP")
>  ...
>>  BUG: unable to handle kernel NULL pointer dereference at 00000000000000a0
>>  IP: __lock_acquire+0x263/0x1630
> Ok, all of these are like this.
>
> Please fix this up, put the kernel log message into the message body
> and add an appropriate signoff.
>
> Thank you.

Oh stupid me. I'll do so straight away. Sorry!

Re: [PATCH net-next v2 01/16] l2tp: update sk_user_data while holding sk_callback_lock

[Guillaume Nault]

On Mon, Feb 12, 2018 at 10:11:05AM +0000, James Chapman wrote:
> Since L2TP hooks on sockets opened by userspace using sk_user_data, we
> may race with other socket families that attempt to use the same
> socket.
> 
> This problem was discovered by syzbot using AF_KCM. KCM has since been
> modified to use only TCP sockets to avoid hitting this issue but we
> should prevent such races in L2TP anyway.
> 
> Fixes: c8fffcea0a079 ("l2tp: Refactor l2tp core driver to make use of the common UDP tunnel function")
> Reported-by: syzbot+8865eaff7f9acd593945@syzkaller.appspotmail.com
> 
> Kernel BUG at net/l2tp/l2tp_ppp.c:176!
> invalid opcode: 0000 [#1] SMP KASAN
> Dumping ftrace buffer:
>    (ftrace buffer empty)
> Modules linked in:
> CPU: 1 PID: 3503 Comm: syzkaller938388 Not tainted 4.15.0-rc7+ #181
> Hardware name: Google Google Compute Engine/Google Compute Engine
> RIP: 0010:pppol2tp_sock_to_session net/l2tp/l2tp_ppp.c:176 [inline]
> RIP: 0010:pppol2tp_sendmsg+0x512/0x670 net/l2tp/l2tp_ppp.c:304
> RSP: 0018:ffff8801d4887438 EFLAGS: 00010293
> RAX: ffff8801bfef2180 RBX: ffff8801bff88440 RCX: ffffffff84ffbca2
> RDX: 0000000000000000 RSI: ffff8801d4887598 RDI: ffff8801bff88820
> RBP: ffff8801d48874a8 R08: 0000000000000000 R09: 1ffff1003a910e17
> R10: 0000000000000003 R11: 0000000000000001 R12: ffff8801bfff9bc0
> R13: 0000000000000000 R14: 0000000000008000 R15: 0000000000000000
> FS:  0000000001194880(0000) GS:ffff8801db300000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000020ea0000 CR3: 00000001bfecf001 CR4: 00000000001606e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
>  sock_sendmsg_nosec net/socket.c:628 [inline]
>  sock_sendmsg+0xca/0x110 net/socket.c:638
>  kernel_sendmsg+0x47/0x60 net/socket.c:646
>  sock_no_sendpage+0x1cc/0x280 net/core/sock.c:2581
>  kernel_sendpage+0xbf/0xe0 net/socket.c:3349
>  kcm_write_msgs+0x404/0x1b80 net/kcm/kcmsock.c:646
>  kcm_sendmsg+0x148d/0x22d0 net/kcm/kcmsock.c:1035
>  sock_sendmsg_nosec net/socket.c:628 [inline]
>  sock_sendmsg+0xca/0x110 net/socket.c:638
>  ___sys_sendmsg+0x767/0x8b0 net/socket.c:2018
>  __sys_sendmsg+0xe5/0x210 net/socket.c:2052
>  SYSC_sendmsg net/socket.c:2063 [inline]
>  SyS_sendmsg+0x2d/0x50 net/socket.c:2059
>  entry_SYSCALL_64_fastpath+0x23/0x9a
> RIP: 0033:0x440159
> RSP: 002b:00007ffe74df8288 EFLAGS: 00000217 ORIG_RAX: 000000000000002e
> RAX: ffffffffffffffda RBX: ffffffffffffffff RCX: 0000000000440159
> RDX: 0000000000000000 RSI: 00000000201fcfc8 RDI: 0000000000000005
> RBP: 00000000006ca018 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000217 R12: 0000000000401ac0
> R13: 0000000000401b50 R14: 0000000000000000 R15: 0000000000000000
> Code: c5 61 70 fc 48 8b 7d d0 e8 7c c2 5b fd 84 c0 74 0d e8 b3 61 70 fc 48 89 df e8 3b 49 2f ff 41 bd f7 ff ff ff eb 86 e8 9e 61 70 fc <0f> 0b 41 bd 95 ff ff ff e9 74 ff ff ff e8 ec 32 a8 fc e9 77 fb
> RIP: pppol2tp_sock_to_session net/l2tp/l2tp_ppp.c:176 [inline] RSP: ffff8801d4887438
> RIP: pppol2tp_sendmsg+0x512/0x670 net/l2tp/l2tp_ppp.c:304 RSP: ffff8801d4887438
> ---
>  net/l2tp/l2tp_core.c | 21 ++++++++++++++++++---
>  net/l2tp/l2tp_ppp.c  |  8 ++++++--
>  2 files changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c
> index 194a7483bb93..de7dce64173f 100644
> --- a/net/l2tp/l2tp_core.c
> +++ b/net/l2tp/l2tp_core.c
> @@ -1216,6 +1216,7 @@ static void l2tp_tunnel_destruct(struct sock *sk)
>  
>  
>  	/* Disable udp encapsulation */
> +	write_lock_bh(&sk->sk_callback_lock);
>  	switch (tunnel->encap) {
>  	case L2TP_ENCAPTYPE_UDP:
>  		/* No longer an encapsulation socket. See net/ipv4/udp.c */
> @@ -1229,7 +1230,8 @@ 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;
> +	rcu_assign_sk_user_data(sk, NULL);
> +	write_unlock_bh(&sk->sk_callback_lock);
>  
>  	/* Remove the tunnel struct from the tunnel list */
>  	pn = l2tp_pernet(tunnel->l2tp_net);
> @@ -1583,6 +1585,20 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
>  	}
>  #endif
>  
> +	/* Assign socket sk_user_data. Must be done with
> +	 * sk_callback_lock. Bail if sk_user_data is already assigned.
> +	 */
> +	write_lock_bh(&sk->sk_callback_lock);
> +	if (sk->sk_user_data) {
> +		err = -EALREADY;
> +		write_unlock_bh(&sk->sk_callback_lock);
> +		kfree(tunnel);
> +		tunnel = NULL;
> +		goto err;
> +	}
> +	rcu_assign_sk_user_data(sk, tunnel);
> +	write_unlock_bh(&sk->sk_callback_lock);
> +
I'd rather use this code only to replace the direct ->sk_user_data
assignment (in the 'else' branch). It looks strange to assign it here
using the new locking scheme and let setup_udp_tunnel_sock() re-assign
it later, with the same value but without locking.

Of course, if every user of ->sk_user_data was updated to the new
locking scheme, that should kill the race without having to update
setup_udp_tunnel_sock(). But only KCM locks ->sk_callback_lock for now
and, as you pointed out, it doesn't act on UDP sockets anymore.

Therefore setting ->sk_user_data here rather than at its original place
only brings confusion in my opinion.

And using rcu_assign_sk_user_data() is a bit confusing too. We never treat
->sk_user_data as RCU protected in the rest of the code.

>  	/* Mark socket as an encapsulation socket. See net/ipv4/udp.c */
>  	tunnel->encap = encap;
>  	if (encap == L2TP_ENCAPTYPE_UDP) {
> @@ -1594,8 +1610,6 @@ int l2tp_tunnel_create(struct net *net, int fd, int version, u32 tunnel_id, u32
>  		udp_cfg.encap_destroy = l2tp_udp_encap_destroy;
>  
>  		setup_udp_tunnel_sock(net, sock, &udp_cfg);
> -	} else {
> -		sk->sk_user_data = tunnel;
>  	}

Re: [PATCH net-next v2 02/16] l2tp: add RCU read lock to protect tunnel ptr in ip socket destroy

[Guillaume Nault]

On Mon, Feb 12, 2018 at 10:11:06AM +0000, James Chapman wrote:
> If an L2TPIP socket is closed, add RCU protection when we deref
> sk_user_data to prevent races with another thread closing the same
> tunnel.
> 
> Fixes: 0d76751fad ("l2tp: Add L2TPv3 IP encapsulation (no UDP) support")
> 
>  refcount_t: increment on 0; use-after-free.
>  WARNING: CPU: 2 PID: 2892 at lib/refcount.c:153 refcount_inc+0x2b/0x30
>  Modules linked in:
>  CPU: 2 PID: 2892 Comm: pppol2tp_chaos Not tainted 4.15.0-rc9+ #1
>  Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
>  RIP: 0010:refcount_inc+0x2b/0x30
>  RSP: 0018:ffff880014147b50 EFLAGS: 00010282
>  RAX: 000000000000002b RBX: ffff8800194785c0 RCX: 0000000000000000
>  RDX: 0000000000000001 RSI: ffff88001ad1f758 RDI: ffff88001ad1f758
>  RBP: ffff880014147b50 R08: 0000000000000000 R09: 0000000000000000
>  R10: 0000000000000001 R11: 0000000000000001 R12: ffff8800194785c8
>  R13: ffff8800194785c0 R14: ffff88001a2c6c20 R15: ffff880013a9d580
>  FS:  0000000000000000(0000) GS:ffff88001ad00000(0000) knlGS:0000000000000000
>  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>  CR2: 00007fbc17ea7000 CR3: 0000000003022000 CR4: 00000000000006e0
>  Call Trace:
>   l2tp_tunnel_delete+0x34/0x60
>   l2tp_ip_destroy_sock+0x6d/0x80
>   sk_common_release+0x19/0xd0
>   l2tp_ip_close+0x89/0xa0
>   inet_release+0x37/0x60
>   sock_release+0x1a/0x70
>   sock_close+0xd/0x20
>   __fput+0xed/0x1f0
>   ____fput+0x9/0x10
>   task_work_run+0x77/0xb0
>   do_exit+0x311/0xcf0
>   do_group_exit+0x47/0xc0
>   get_signal+0x343/0x8e0
>   do_signal+0x23/0x780
>   ? find_held_lock+0x39/0xb0
>   exit_to_usermode_loop+0x4a/0x93
>   syscall_return_slowpath+0x102/0x150
>   entry_SYSCALL_64_fastpath+0x98/0x9a
>  RIP: 0033:0x7fbc172d7fcf
>  RSP: 002b:00007ffd524cd4d8 EFLAGS: 00000246 ORIG_RAX: 000000000000000e
>  RAX: 0000000000000000 RBX: 0000000000000006 RCX: 00007fbc172d7fcf
>  RDX: 0000000000000000 RSI: 00007ffd524cd460 RDI: 0000000000000002
>  RBP: 0000000000404930 R08: 0000000000000000 R09: 00007ffd524cd460
>  R10: 0000000000000008 R11: 0000000000000246 R12: 000000000000001e
>  R13: 0000000000404a03 R14: 0000000000000000 R15: 0000000000000000
>  Code: 55 48 89 e5 e8 87 ff ff ff 84 c0 74 02 5d c3 80 3d 97 87 bb 01 00 75 f5 48 c7 c7 58 3e cc 82 c6 05 87 87 bb 01
> ---
>  net/l2tp/l2tp_ip.c  | 5 ++++-
>  net/l2tp/l2tp_ip6.c | 5 ++++-
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/net/l2tp/l2tp_ip.c b/net/l2tp/l2tp_ip.c
> index ff61124fdf59..42f3c2f72bf4 100644
> --- a/net/l2tp/l2tp_ip.c
> +++ b/net/l2tp/l2tp_ip.c
> @@ -234,15 +234,18 @@ 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);
> +	struct l2tp_tunnel *tunnel;
>  
>  	while ((skb = __skb_dequeue_tail(&sk->sk_write_queue)) != NULL)
>  		kfree_skb(skb);
>  
> +	rcu_read_lock();
> +	tunnel = rcu_dereference_sk_user_data(sk);
Is this safe? We don't wait for a grace period after resetting ->sk_user_data.

>  	if (tunnel) {
>  		l2tp_tunnel_closeall(tunnel);
I'm pretty sure l2tp_tunnel_closeall() can sleep and can't be called in
RCU critical sections.

>  		sock_put(sk);
>  	}
> +	rcu_read_unlock();
>  

Re: [PATCH net-next v2 03/16] l2tp: don't use inet_shutdown on tunnel destroy

[Guillaume Nault]

On Mon, Feb 12, 2018 at 10:11:07AM +0000, James Chapman wrote:
> Previously, if a tunnel was closed, we called inet_shutdown to mark
> the socket as unconnected such that userspace would get errors and
> then close the socket. This could race with userspace closing the
> socket. Instead, leave userspace to close the socket in its own time
> (our tunnel will be detached anyway).
> 
Acked-by: Guillaume Nault <g.nault@alphalink.fr>

Re: [PATCH net-next v2 04/16] l2tp: refactor tunnel lifetime handling wrt its socket

[Guillaume Nault]

On Mon, Feb 12, 2018 at 10:11:08AM +0000, James Chapman wrote:
> 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.
> 

> --- a/net/l2tp/l2tp_core.c
> +++ b/net/l2tp/l2tp_core.c
> @@ -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 */
> +}
> +
EXPORT_SYMBOL_GPL(l2tp_tunnel_free) is missing.

Re: [PATCH net-next v2 00/16] l2tp: fix API races discovered by syzbot

[Guillaume Nault]

On Mon, Feb 12, 2018 at 10:11:04AM +0000, James Chapman wrote:
> This patch series addresses several races with L2TP APIs discovered by
> syzbot. While working on this, it became clear that the L2TP code
> needed some work to address object lifetime issues. There are no
> functional changes.
> 
> The set of patches 1-13 in combination fix the following syzbot reports.
> 
Thanks for taking care of this James. I see some of my feedbacks were
already addressed later in the series. I'll take more time to review
your v3.

Re: [PATCH net-next v2 04/16] l2tp: refactor tunnel lifetime handling wrt its socket

[kbuild test robot]

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

Hi James,

I love your patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/James-Chapman/l2tp-fix-API-races-discovered-by-syzbot/20180215-060031
config: x86_64-rhel (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

>> ERROR: "l2tp_tunnel_free" [net/l2tp/l2tp_ppp.ko] undefined!
>> ERROR: "l2tp_tunnel_free" [net/l2tp/l2tp_netlink.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 40671 bytes --]