[PATCH 3.16 00/10] Fix possible crash on L2CAP socket shutdown

[PATCH 3.16 00/10] Fix possible crash on L2CAP socket shutdown

[Denis Grigorev]

This series of commits fixes a problem with closing l2cap connection
if socket has unACKed frames. Due an to an infinite loop in l2cap_wait_ack
the userspace process gets stuck in close() and then the kernel crashes
with the following report:

Call trace:
[<ffffffc000ace0b4>] l2cap_do_send+0x2c/0xec
[<ffffffc000acf5f8>] l2cap_send_sframe+0x178/0x260
[<ffffffc000acf740>] l2cap_send_rr_or_rnr+0x60/0x84
[<ffffffc000acf980>] l2cap_ack_timeout+0x60/0xac
[<ffffffc0000b35b8>] process_one_work+0x140/0x384
[<ffffffc0000b393c>] worker_thread+0x140/0x4e4
[<ffffffc0000b8c48>] kthread+0xdc/0xf0

All kernels below v4.3 are affected.

-------------------------

Commit log:

Alexey Dobriyan (1):
  Bluetooth: Stop sabotaging list poisoning

Dean Jenkins (8):
  Bluetooth: L2CAP ERTM shutdown protect sk and chan
  Bluetooth: Make __l2cap_wait_ack more efficient
  Bluetooth: Add BT_DBG to l2cap_sock_shutdown()
  Bluetooth: __l2cap_wait_ack() use msecs_to_jiffies()
  Bluetooth: __l2cap_wait_ack() add defensive timeout
  Bluetooth: Unwind l2cap_sock_shutdown()
  Bluetooth: Reorganize mutex lock in l2cap_sock_shutdown()
  Bluetooth: l2cap_disconnection_req priority over shutdown

Tedd Ho-Jeong An (1):
  Bluetooth: Reinitialize the list after deletion for session user list

 include/net/bluetooth/l2cap.h |  2 +
 net/bluetooth/l2cap_core.c    | 12 ++---
 net/bluetooth/l2cap_sock.c    | 94 +++++++++++++++++++++++++++--------
 3 files changed, 78 insertions(+), 30 deletions(-)

-- 
2.17.1

[PATCH 3.16 01/10] Bluetooth: Stop sabotaging list poisoning

[Denis Grigorev]

From: Alexey Dobriyan <adobriyan@gmail.com>

list_del() poisons pointers with special values, no need to overwrite them.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
---
 net/bluetooth/l2cap_core.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 35c41fdbde16..1700c3dc3fe1 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1616,7 +1616,7 @@ int l2cap_register_user(struct l2cap_conn *conn, struct l2cap_user *user)
 
 	hci_dev_lock(hdev);
 
-	if (user->list.next || user->list.prev) {
+	if (!list_empty(&user->list)) {
 		ret = -EINVAL;
 		goto out_unlock;
 	}
@@ -1646,12 +1646,10 @@ void l2cap_unregister_user(struct l2cap_conn *conn, struct l2cap_user *user)
 
 	hci_dev_lock(hdev);
 
-	if (!user->list.next || !user->list.prev)
+	if (list_empty(&user->list))
 		goto out_unlock;
 
 	list_del(&user->list);
-	user->list.next = NULL;
-	user->list.prev = NULL;
 	user->remove(conn, user);
 
 out_unlock:
@@ -1666,8 +1664,6 @@ static void l2cap_unregister_all_users(struct l2cap_conn *conn)
 	while (!list_empty(&conn->users)) {
 		user = list_first_entry(&conn->users, struct l2cap_user, list);
 		list_del(&user->list);
-		user->list.next = NULL;
-		user->list.prev = NULL;
 		user->remove(conn, user);
 	}
 }
-- 
2.17.1

[PATCH 3.16 02/10] Bluetooth: Reinitialize the list after deletion for session user list

[Denis Grigorev]

From: Tedd Ho-Jeong An <tedd.an@intel.com>

If the user->list is deleted with list_del(), it doesn't initialize the
entry which can cause the issue with list_empty(). According to the
comment from the list.h, list_empty() returns false even if the list is
empty and put the entry in an undefined state.

/**
 * list_del - deletes entry from list.
 * @entry: the element to delete from the list.
 * Note: list_empty() on entry does not return true after this, the entry is
 * in an undefined state.
 */

Because of this behavior, list_empty() returns false even if list is empty
when the device is reconnected.

So, user->list needs to be re-initialized after list_del(). list.h already
have a macro list_del_init() which deletes the entry and initailze it again.

Signed-off-by: Tedd Ho-Jeong An <tedd.an@intel.com>
Tested-by: Jörg Otte <jrg.otte@gmail.com>
Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
---
 net/bluetooth/l2cap_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 1700c3dc3fe1..eb0125d1451d 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1649,7 +1649,7 @@ void l2cap_unregister_user(struct l2cap_conn *conn, struct l2cap_user *user)
 	if (list_empty(&user->list))
 		goto out_unlock;
 
-	list_del(&user->list);
+	list_del_init(&user->list);
 	user->remove(conn, user);
 
 out_unlock:
@@ -1663,7 +1663,7 @@ static void l2cap_unregister_all_users(struct l2cap_conn *conn)
 
 	while (!list_empty(&conn->users)) {
 		user = list_first_entry(&conn->users, struct l2cap_user, list);
-		list_del(&user->list);
+		list_del_init(&user->list);
 		user->remove(conn, user);
 	}
 }
-- 
2.17.1

[PATCH 3.16 03/10] Bluetooth: L2CAP ERTM shutdown protect sk and chan

[Denis Grigorev]

From: Dean Jenkins <Dean_Jenkins@mentor.com>

During execution of l2cap_sock_shutdown() which might
sleep, the sk and chan structures can be in an unlocked
condition which potentially allows the structures to be
freed by other running threads. Therefore, there is a
possibility of a malfunction or memory reuse after being
freed.

Keep the sk and chan structures alive during the
execution of l2cap_sock_shutdown() by using their
respective hold and put functions. This allows the structures
to be freeable at the end of l2cap_sock_shutdown().

Signed-off-by: Kautuk Consul <Kautuk_Consul@mentor.com>
Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
---
 net/bluetooth/l2cap_sock.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 071d35c9f3b4..e56d34f027dd 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1092,7 +1092,12 @@ static int l2cap_sock_shutdown(struct socket *sock, int how)
 	if (!sk)
 		return 0;
 
+	/* prevent sk structure from being freed whilst unlocked */
+	sock_hold(sk);
+
 	chan = l2cap_pi(sk)->chan;
+	/* prevent chan structure from being freed whilst unlocked */
+	l2cap_chan_hold(chan);
 	conn = chan->conn;
 
 	if (conn)
@@ -1126,6 +1131,9 @@ static int l2cap_sock_shutdown(struct socket *sock, int how)
 	if (conn)
 		mutex_unlock(&conn->chan_lock);
 
+	l2cap_chan_put(chan);
+	sock_put(sk);
+
 	return err;
 }
 
-- 
2.17.1

[PATCH 3.16 04/10] Bluetooth: Make __l2cap_wait_ack more efficient

[Denis Grigorev]

From: Dean Jenkins <Dean_Jenkins@mentor.com>

Use chan->state instead of chan->conn because waiting
for ACK's is only possible in the BT_CONNECTED state.
Also avoids reference to the conn structure so makes
locking easier.

Only call __l2cap_wait_ack() when the needed condition
of chan->unacked_frames > 0 && chan->state == BT_CONNECTED
is true and convert the while loop to a do while loop.

__l2cap_wait_ack() change the function prototype to
pass in the chan variable as chan is already available
in the calling function l2cap_sock_shutdown(). Avoids
locking issues.

Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
---
 net/bluetooth/l2cap_sock.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index e56d34f027dd..6c351b43bb42 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1048,16 +1048,15 @@ static void l2cap_sock_kill(struct sock *sk)
 	sock_put(sk);
 }
 
-static int __l2cap_wait_ack(struct sock *sk)
+static int __l2cap_wait_ack(struct sock *sk, struct l2cap_chan *chan)
 {
-	struct l2cap_chan *chan = l2cap_pi(sk)->chan;
 	DECLARE_WAITQUEUE(wait, current);
 	int err = 0;
 	int timeo = HZ/5;
 
 	add_wait_queue(sk_sleep(sk), &wait);
 	set_current_state(TASK_INTERRUPTIBLE);
-	while (chan->unacked_frames > 0 && chan->conn) {
+	do {
 		if (!timeo)
 			timeo = HZ/5;
 
@@ -1074,7 +1073,10 @@ static int __l2cap_wait_ack(struct sock *sk)
 		err = sock_error(sk);
 		if (err)
 			break;
-	}
+
+	} while (chan->unacked_frames > 0 &&
+		 chan->state == BT_CONNECTED);
+
 	set_current_state(TASK_RUNNING);
 	remove_wait_queue(sk_sleep(sk), &wait);
 	return err;
@@ -1107,8 +1109,10 @@ static int l2cap_sock_shutdown(struct socket *sock, int how)
 	lock_sock(sk);
 
 	if (!sk->sk_shutdown) {
-		if (chan->mode == L2CAP_MODE_ERTM)
-			err = __l2cap_wait_ack(sk);
+		if (chan->mode == L2CAP_MODE_ERTM &&
+		    chan->unacked_frames > 0 &&
+		    chan->state == BT_CONNECTED)
+			err = __l2cap_wait_ack(sk, chan);
 
 		sk->sk_shutdown = SHUTDOWN_MASK;
 
-- 
2.17.1

[PATCH 3.16 05/10] Bluetooth: Add BT_DBG to l2cap_sock_shutdown()

[Denis Grigorev]

From: Dean Jenkins <Dean_Jenkins@mentor.com>

Add helpful BT_DBG debug to l2cap_sock_shutdown()
and __l2cap_wait_ack() so that the code flow can
be analysed.

Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
---
 net/bluetooth/l2cap_sock.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 6c351b43bb42..662835fe0e95 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1057,6 +1057,8 @@ static int __l2cap_wait_ack(struct sock *sk, struct l2cap_chan *chan)
 	add_wait_queue(sk_sleep(sk), &wait);
 	set_current_state(TASK_INTERRUPTIBLE);
 	do {
+		BT_DBG("Waiting for %d ACKs", chan->unacked_frames);
+
 		if (!timeo)
 			timeo = HZ/5;
 
@@ -1138,6 +1140,8 @@ static int l2cap_sock_shutdown(struct socket *sock, int how)
 	l2cap_chan_put(chan);
 	sock_put(sk);
 
+	BT_DBG("err: %d", err);
+
 	return err;
 }
 
-- 
2.17.1

[PATCH 3.16 06/10] Bluetooth: __l2cap_wait_ack() use msecs_to_jiffies()

[Denis Grigorev]

From: Dean Jenkins <Dean_Jenkins@mentor.com>

Use msecs_to_jiffies() instead of using HZ so that it
is easier to specify the time in milliseconds.

Also add a #define L2CAP_WAIT_ACK_POLL_PERIOD to specify the 200ms
polling period so that it is defined in a single place.

Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
---
 include/net/bluetooth/l2cap.h | 1 +
 net/bluetooth/l2cap_sock.c    | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 4abdcb220e3a..430381ba7e5e 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -54,6 +54,7 @@
 #define L2CAP_INFO_TIMEOUT		msecs_to_jiffies(4000)
 #define L2CAP_MOVE_TIMEOUT		msecs_to_jiffies(4000)
 #define L2CAP_MOVE_ERTX_TIMEOUT		msecs_to_jiffies(60000)
+#define L2CAP_WAIT_ACK_POLL_PERIOD	msecs_to_jiffies(200)
 
 #define L2CAP_A2MP_DEFAULT_MTU		670
 
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 662835fe0e95..0bd277fba24e 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1052,7 +1052,7 @@ static int __l2cap_wait_ack(struct sock *sk, struct l2cap_chan *chan)
 {
 	DECLARE_WAITQUEUE(wait, current);
 	int err = 0;
-	int timeo = HZ/5;
+	int timeo = L2CAP_WAIT_ACK_POLL_PERIOD;
 
 	add_wait_queue(sk_sleep(sk), &wait);
 	set_current_state(TASK_INTERRUPTIBLE);
@@ -1060,7 +1060,7 @@ static int __l2cap_wait_ack(struct sock *sk, struct l2cap_chan *chan)
 		BT_DBG("Waiting for %d ACKs", chan->unacked_frames);
 
 		if (!timeo)
-			timeo = HZ/5;
+			timeo = L2CAP_WAIT_ACK_POLL_PERIOD;
 
 		if (signal_pending(current)) {
 			err = sock_intr_errno(timeo);
-- 
2.17.1

[PATCH 3.16 07/10] Bluetooth: __l2cap_wait_ack() add defensive timeout

[Denis Grigorev]

From: Dean Jenkins <Dean_Jenkins@mentor.com>

Add a timeout to prevent the do while loop running in an
infinite loop. This ensures that the channel will be
instructed to close within 10 seconds so prevents
l2cap_sock_shutdown() getting stuck forever.

Returns -ENOLINK when the timeout is reached. The channel
will be subequently closed and not all data will be ACK'ed.

Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
---
 include/net/bluetooth/l2cap.h |  1 +
 net/bluetooth/l2cap_sock.c    | 11 ++++++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 430381ba7e5e..7bdf16944df3 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -55,6 +55,7 @@
 #define L2CAP_MOVE_TIMEOUT		msecs_to_jiffies(4000)
 #define L2CAP_MOVE_ERTX_TIMEOUT		msecs_to_jiffies(60000)
 #define L2CAP_WAIT_ACK_POLL_PERIOD	msecs_to_jiffies(200)
+#define L2CAP_WAIT_ACK_TIMEOUT		msecs_to_jiffies(10000)
 
 #define L2CAP_A2MP_DEFAULT_MTU		670
 
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 0bd277fba24e..00b43f008941 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1053,11 +1053,15 @@ static int __l2cap_wait_ack(struct sock *sk, struct l2cap_chan *chan)
 	DECLARE_WAITQUEUE(wait, current);
 	int err = 0;
 	int timeo = L2CAP_WAIT_ACK_POLL_PERIOD;
+	/* Timeout to prevent infinite loop */
+	unsigned long timeout = jiffies + L2CAP_WAIT_ACK_TIMEOUT;
 
 	add_wait_queue(sk_sleep(sk), &wait);
 	set_current_state(TASK_INTERRUPTIBLE);
 	do {
-		BT_DBG("Waiting for %d ACKs", chan->unacked_frames);
+		BT_DBG("Waiting for %d ACKs, timeout %04d ms",
+		       chan->unacked_frames, time_after(jiffies, timeout) ? 0 :
+		       jiffies_to_msecs(timeout - jiffies));
 
 		if (!timeo)
 			timeo = L2CAP_WAIT_ACK_POLL_PERIOD;
@@ -1076,6 +1080,11 @@ static int __l2cap_wait_ack(struct sock *sk, struct l2cap_chan *chan)
 		if (err)
 			break;
 
+		if (time_after(jiffies, timeout)) {
+			err = -ENOLINK;
+			break;
+		}
+
 	} while (chan->unacked_frames > 0 &&
 		 chan->state == BT_CONNECTED);
 
-- 
2.17.1

[PATCH 3.16 08/10] Bluetooth: Unwind l2cap_sock_shutdown()

[Denis Grigorev]

From: Dean Jenkins <Dean_Jenkins@mentor.com>

l2cap_sock_shutdown() is designed to only action shutdown
of the channel when shutdown is not already in progress.
Therefore, reorganise the code flow by adding a goto
to jump to the end of function handling when shutdown is
already being actioned. This removes one level of code
indentation and make the code more readable.

Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>
Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
---
 net/bluetooth/l2cap_sock.c | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 00b43f008941..01e52bea698c 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1105,6 +1105,11 @@ static int l2cap_sock_shutdown(struct socket *sock, int how)
 	if (!sk)
 		return 0;
 
+	if (sk->sk_shutdown)
+		goto shutdown_already;
+
+	BT_DBG("Handling sock shutdown");
+
 	/* prevent sk structure from being freed whilst unlocked */
 	sock_hold(sk);
 
@@ -1119,23 +1124,21 @@ static int l2cap_sock_shutdown(struct socket *sock, int how)
 	l2cap_chan_lock(chan);
 	lock_sock(sk);
 
-	if (!sk->sk_shutdown) {
-		if (chan->mode == L2CAP_MODE_ERTM &&
-		    chan->unacked_frames > 0 &&
-		    chan->state == BT_CONNECTED)
-			err = __l2cap_wait_ack(sk, chan);
+	if (chan->mode == L2CAP_MODE_ERTM &&
+	    chan->unacked_frames > 0 &&
+	    chan->state == BT_CONNECTED)
+		err = __l2cap_wait_ack(sk, chan);
 
-		sk->sk_shutdown = SHUTDOWN_MASK;
+	sk->sk_shutdown = SHUTDOWN_MASK;
 
-		release_sock(sk);
-		l2cap_chan_close(chan, 0);
-		lock_sock(sk);
+	release_sock(sk);
+	l2cap_chan_close(chan, 0);
+	lock_sock(sk);
 
-		if (sock_flag(sk, SOCK_LINGER) && sk->sk_lingertime &&
-		    !(current->flags & PF_EXITING))
-			err = bt_sock_wait_state(sk, BT_CLOSED,
-						 sk->sk_lingertime);
-	}
+	if (sock_flag(sk, SOCK_LINGER) && sk->sk_lingertime &&
+	    !(current->flags & PF_EXITING))
+		err = bt_sock_wait_state(sk, BT_CLOSED,
+					 sk->sk_lingertime);
 
 	if (!err && sk->sk_err)
 		err = -sk->sk_err;
@@ -1149,6 +1152,7 @@ static int l2cap_sock_shutdown(struct socket *sock, int how)
 	l2cap_chan_put(chan);
 	sock_put(sk);
 
+shutdown_already:
 	BT_DBG("err: %d", err);
 
 	return err;
-- 
2.17.1

[PATCH 3.16 09/10] Bluetooth: Reorganize mutex lock in l2cap_sock_shutdown()

[Denis Grigorev]

From: Dean Jenkins <Dean_Jenkins@mentor.com>

This commit reorganizes the mutex lock and is now
only protecting l2cap_chan_close(). This is now consistent
with other places where l2cap_chan_close() is called.

If a conn connection exists, call
mutex_lock(&conn->chan_lock) before calling l2cap_chan_close()
to ensure other L2CAP protocol operations do not interfere.

Note that the conn structure has to be protected from being
freed as it is possible for the connection to be disconnected
whilst the locks are not held. This solution allows the mutex
lock to be used even when the connection has just been
disconnected.

This commit also reduces the scope of chan locking.

The only place where chan locking is needed is the call to
l2cap_chan_close(chan, 0) which if necessary closes the channel.
Therefore, move the l2cap_chan_lock(chan) and
l2cap_chan_lock(chan) locking calls to around
l2cap_chan_close(chan, 0).

This allows __l2cap_wait_ack(sk, chan) to be called with no
chan locks being held so L2CAP messaging over the ACL link
can be done unimpaired.

Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>
Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
---
 net/bluetooth/l2cap_sock.c | 44 ++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 16 deletions(-)

diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 01e52bea698c..efc0326f6823 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1105,6 +1105,8 @@ static int l2cap_sock_shutdown(struct socket *sock, int how)
 	if (!sk)
 		return 0;
 
+	lock_sock(sk);
+
 	if (sk->sk_shutdown)
 		goto shutdown_already;
 
@@ -1116,13 +1118,8 @@ static int l2cap_sock_shutdown(struct socket *sock, int how)
 	chan = l2cap_pi(sk)->chan;
 	/* prevent chan structure from being freed whilst unlocked */
 	l2cap_chan_hold(chan);
-	conn = chan->conn;
 
-	if (conn)
-		mutex_lock(&conn->chan_lock);
-
-	l2cap_chan_lock(chan);
-	lock_sock(sk);
+	BT_DBG("chan %p state %s", chan, state_to_string(chan->state));
 
 	if (chan->mode == L2CAP_MODE_ERTM &&
 	    chan->unacked_frames > 0 &&
@@ -1130,9 +1127,28 @@ static int l2cap_sock_shutdown(struct socket *sock, int how)
 		err = __l2cap_wait_ack(sk, chan);
 
 	sk->sk_shutdown = SHUTDOWN_MASK;
-
 	release_sock(sk);
+
+	l2cap_chan_lock(chan);
+	conn = chan->conn;
+	if (conn)
+		/* prevent conn structure from being freed */
+		l2cap_conn_get(conn);
+	l2cap_chan_unlock(chan);
+
+	if (conn)
+		/* mutex lock must be taken before l2cap_chan_lock() */
+		mutex_lock(&conn->chan_lock);
+
+	l2cap_chan_lock(chan);
 	l2cap_chan_close(chan, 0);
+	l2cap_chan_unlock(chan);
+
+	if (conn) {
+		mutex_unlock(&conn->chan_lock);
+		l2cap_conn_put(conn);
+	}
+
 	lock_sock(sk);
 
 	if (sock_flag(sk, SOCK_LINGER) && sk->sk_lingertime &&
@@ -1140,20 +1156,16 @@ static int l2cap_sock_shutdown(struct socket *sock, int how)
 		err = bt_sock_wait_state(sk, BT_CLOSED,
 					 sk->sk_lingertime);
 
+	l2cap_chan_put(chan);
+	sock_put(sk);
+
+shutdown_already:
 	if (!err && sk->sk_err)
 		err = -sk->sk_err;
 
 	release_sock(sk);
-	l2cap_chan_unlock(chan);
-
-	if (conn)
-		mutex_unlock(&conn->chan_lock);
 
-	l2cap_chan_put(chan);
-	sock_put(sk);
-
-shutdown_already:
-	BT_DBG("err: %d", err);
+	BT_DBG("Sock shutdown complete err: %d", err);
 
 	return err;
 }
-- 
2.17.1

[PATCH 3.16 10/10] Bluetooth: l2cap_disconnection_req priority over shutdown

[Denis Grigorev]

From: Dean Jenkins <Dean_Jenkins@mentor.com>

There is a L2CAP protocol race between the local peer and
the remote peer demanding disconnection of the L2CAP link.

When L2CAP ERTM is used, l2cap_sock_shutdown() can be called
from userland to disconnect L2CAP. However, there can be a
delay introduced by waiting for ACKs. During this waiting
period, the remote peer may have sent a Disconnection Request.
Therefore, recheck the shutdown status of the socket
after waiting for ACKs because there is no need to do
further processing if the connection has gone.

Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>
Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
---
 net/bluetooth/l2cap_sock.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index efc0326f6823..e01f1557e1cc 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1123,9 +1123,17 @@ static int l2cap_sock_shutdown(struct socket *sock, int how)
 
 	if (chan->mode == L2CAP_MODE_ERTM &&
 	    chan->unacked_frames > 0 &&
-	    chan->state == BT_CONNECTED)
+	    chan->state == BT_CONNECTED) {
 		err = __l2cap_wait_ack(sk, chan);
 
+		/* After waiting for ACKs, check whether shutdown
+		 * has already been actioned to close the L2CAP
+		 * link such as by l2cap_disconnection_req().
+		 */
+		if (sk->sk_shutdown)
+			goto has_shutdown;
+	}
+
 	sk->sk_shutdown = SHUTDOWN_MASK;
 	release_sock(sk);
 
@@ -1156,6 +1164,7 @@ static int l2cap_sock_shutdown(struct socket *sock, int how)
 		err = bt_sock_wait_state(sk, BT_CLOSED,
 					 sk->sk_lingertime);
 
+has_shutdown:
 	l2cap_chan_put(chan);
 	sock_put(sk);
 
-- 
2.17.1

Re: [PATCH 3.16 00/10] Fix possible crash on L2CAP socket shutdown

[Ben Hutchings]

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

On Tue, 2020-06-30 at 18:36 +0300, Denis Grigorev wrote:
> This series of commits fixes a problem with closing l2cap connection
> if socket has unACKed frames. Due an to an infinite loop in l2cap_wait_ack
> the userspace process gets stuck in close() and then the kernel crashes
> with the following report:
> 
> Call trace:
> [<ffffffc000ace0b4>] l2cap_do_send+0x2c/0xec
> [<ffffffc000acf5f8>] l2cap_send_sframe+0x178/0x260
> [<ffffffc000acf740>] l2cap_send_rr_or_rnr+0x60/0x84
> [<ffffffc000acf980>] l2cap_ack_timeout+0x60/0xac
> [<ffffffc0000b35b8>] process_one_work+0x140/0x384
> [<ffffffc0000b393c>] worker_thread+0x140/0x4e4
> [<ffffffc0000b8c48>] kthread+0xdc/0xf0
> 
> All kernels below v4.3 are affected.
[...]

Thanks for your work, but I'm afraid the 3.16-stable branch is no
longer being maintained (as of today).

Ben.

-- 
Ben Hutchings
It is a miracle that curiosity survives formal education.
                                                      - Albert Einstein



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3.16 00/10] Fix possible crash on L2CAP socket shutdown

[Greg KH]

On Thu, Jul 02, 2020 at 12:31:01AM +0100, Ben Hutchings wrote:
> On Tue, 2020-06-30 at 18:36 +0300, Denis Grigorev wrote:
> > This series of commits fixes a problem with closing l2cap connection
> > if socket has unACKed frames. Due an to an infinite loop in l2cap_wait_ack
> > the userspace process gets stuck in close() and then the kernel crashes
> > with the following report:
> > 
> > Call trace:
> > [<ffffffc000ace0b4>] l2cap_do_send+0x2c/0xec
> > [<ffffffc000acf5f8>] l2cap_send_sframe+0x178/0x260
> > [<ffffffc000acf740>] l2cap_send_rr_or_rnr+0x60/0x84
> > [<ffffffc000acf980>] l2cap_ack_timeout+0x60/0xac
> > [<ffffffc0000b35b8>] process_one_work+0x140/0x384
> > [<ffffffc0000b393c>] worker_thread+0x140/0x4e4
> > [<ffffffc0000b8c48>] kthread+0xdc/0xf0
> > 
> > All kernels below v4.3 are affected.
> [...]
> 
> Thanks for your work, but I'm afraid the 3.16-stable branch is no
> longer being maintained (as of today).

Want me to mark it EOL and remove it from the release table on
kernel.org now?

thanks,

greg k-h

3.16 EOL

[Ben Hutchings]

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

On Thu, 2020-07-02 at 09:39 +0200, Greg KH wrote:
> On Thu, Jul 02, 2020 at 12:31:01AM +0100, Ben Hutchings wrote:
> > On Tue, 2020-06-30 at 18:36 +0300, Denis Grigorev wrote:
> > > This series of commits fixes a problem with closing l2cap connection
> > > if socket has unACKed frames. Due an to an infinite loop in l2cap_wait_ack
> > > the userspace process gets stuck in close() and then the kernel crashes
> > > with the following report:
> > > 
> > > Call trace:
> > > [<ffffffc000ace0b4>] l2cap_do_send+0x2c/0xec
> > > [<ffffffc000acf5f8>] l2cap_send_sframe+0x178/0x260
> > > [<ffffffc000acf740>] l2cap_send_rr_or_rnr+0x60/0x84
> > > [<ffffffc000acf980>] l2cap_ack_timeout+0x60/0xac
> > > [<ffffffc0000b35b8>] process_one_work+0x140/0x384
> > > [<ffffffc0000b393c>] worker_thread+0x140/0x4e4
> > > [<ffffffc0000b8c48>] kthread+0xdc/0xf0
> > > 
> > > All kernels below v4.3 are affected.
> > [...]
> > 
> > Thanks for your work, but I'm afraid the 3.16-stable branch is no
> > longer being maintained (as of today).
> 
> Want me to mark it EOL and remove it from the release table on
> kernel.org now?

I was about to send that patch myself, but you're welcome to do so.

Thanks again for all your assistance with releasing updates for 3.2 and
3.16 over the past 8 years.

Ben.

-- 
Ben Hutchings
It is a miracle that curiosity survives formal education.
                                                      - Albert Einstein



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: 3.16 EOL

[Greg KH]

On Thu, Jul 02, 2020 at 03:55:04PM +0100, Ben Hutchings wrote:
> On Thu, 2020-07-02 at 09:39 +0200, Greg KH wrote:
> > On Thu, Jul 02, 2020 at 12:31:01AM +0100, Ben Hutchings wrote:
> > > On Tue, 2020-06-30 at 18:36 +0300, Denis Grigorev wrote:
> > > > This series of commits fixes a problem with closing l2cap connection
> > > > if socket has unACKed frames. Due an to an infinite loop in l2cap_wait_ack
> > > > the userspace process gets stuck in close() and then the kernel crashes
> > > > with the following report:
> > > > 
> > > > Call trace:
> > > > [<ffffffc000ace0b4>] l2cap_do_send+0x2c/0xec
> > > > [<ffffffc000acf5f8>] l2cap_send_sframe+0x178/0x260
> > > > [<ffffffc000acf740>] l2cap_send_rr_or_rnr+0x60/0x84
> > > > [<ffffffc000acf980>] l2cap_ack_timeout+0x60/0xac
> > > > [<ffffffc0000b35b8>] process_one_work+0x140/0x384
> > > > [<ffffffc0000b393c>] worker_thread+0x140/0x4e4
> > > > [<ffffffc0000b8c48>] kthread+0xdc/0xf0
> > > > 
> > > > All kernels below v4.3 are affected.
> > > [...]
> > > 
> > > Thanks for your work, but I'm afraid the 3.16-stable branch is no
> > > longer being maintained (as of today).
> > 
> > Want me to mark it EOL and remove it from the release table on
> > kernel.org now?
> 
> I was about to send that patch myself, but you're welcome to do so.

Now done.

> Thanks again for all your assistance with releasing updates for 3.2 and
> 3.16 over the past 8 years.

Hey, thanks for doing all the real work there, that was a lot!

greg k-h