[PATCHv2 0/3] Bluetooth: Change socket lock to l2cap_chan lock

[PATCHv2 0/3] Bluetooth: Change socket lock to l2cap_chan lock

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Changing socket lock to L2CAP chan lock in L2CAP code. Needed for implementing
protocols above L2CAP without creating sockets.

Changes:
	* PATCHv2: Rebase remaining parts against latest tree and merge all
	patches dealing with converting sk lock to chan lock together following
	recommendation from Ulisses and Gustavo.
	* PATCHv1: Added extra line (per Gustavo comment)
	* RFCv6: Same code but patches 2,3 and 4 from RFCv5 are merged together
	following recommendations from review.
	* RFCv5: Fixed locking bug in l2cap_data_channel, added locks in 
	l2cap_sock_shutdown function, fixed several styles issues.
	* RFCv4: Better split patches so they looks more clear and obvious,
	taking coments about naming change and delete unused vars. See diff change
	from the previous version below:
	* RFCv3: Split the big patch to several small (I believe logical) chunks,
	remove unneded locks from cleanup_listen, use the same arguments for
	locked/unlocked socket error functions.
	* RFCv2: Convert l2cap channel list back to mutex from RCU list.

Andrei Emeltchenko (3):
  Bluetooth: Add unlocked __l2cap_chan_add function
  Bluetooth: Change sk lock to chan lock in L2CAP core
  Bluetooth: Remove socket lock check

 net/bluetooth/l2cap_core.c |  173 +++++++++++++++++++++++++++-----------------
 net/bluetooth/l2cap_sock.c |   28 +++++--
 2 files changed, 125 insertions(+), 76 deletions(-)

-- 
1.7.9

[PATCHv2 1/3] Bluetooth: Add unlocked __l2cap_chan_add function

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Add unlocked L2CAP channel add function. Unlocked version will
be used in later patches.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Reviewed-by: Ulisses Furquim <ulisses@profusion.mobi>
Acked-by: Gustavo F. Padovan <padovan@profusion.mobi>
---
 net/bluetooth/l2cap_core.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index c0640b7..0e4f4cb 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -300,7 +300,7 @@ void l2cap_chan_destroy(struct l2cap_chan *chan)
 	l2cap_chan_put(chan);
 }
 
-static void l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
+void __l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
 {
 	BT_DBG("conn %p, psm 0x%2.2x, dcid 0x%4.4x", conn,
 			chan->psm, chan->dcid);
@@ -346,8 +346,13 @@ static void l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
 
 	l2cap_chan_hold(chan);
 
-	mutex_lock(&conn->chan_lock);
 	list_add(&chan->list, &conn->chan_l);
+}
+
+void l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
+{
+	mutex_lock(&conn->chan_lock);
+	__l2cap_chan_add(conn, chan);
 	mutex_unlock(&conn->chan_lock);
 }
 
-- 
1.7.9

[PATCHv2 2/3] Bluetooth: Change sk lock to chan lock in L2CAP core

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Change sk lock to chan lock in l2cap core and move sk locks
to l2cap sock code. bh_locks were used because of being RCU
critical section. When needed use explicit socket locks.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 net/bluetooth/l2cap_core.c |  164 ++++++++++++++++++++++++++------------------
 net/bluetooth/l2cap_sock.c |   20 ++++-
 2 files changed, 114 insertions(+), 70 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 0e4f4cb..5f9ce44 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -238,13 +238,12 @@ static void l2cap_chan_timeout(struct work_struct *work)
 	struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
 							chan_timer.work);
 	struct l2cap_conn *conn = chan->conn;
-	struct sock *sk = chan->sk;
 	int reason;
 
 	BT_DBG("chan %p state %s", chan, state_to_string(chan->state));
 
 	mutex_lock(&conn->chan_lock);
-	lock_sock(sk);
+	l2cap_chan_lock(chan);
 
 	if (chan->state == BT_CONNECTED || chan->state == BT_CONFIG)
 		reason = ECONNREFUSED;
@@ -256,7 +255,7 @@ static void l2cap_chan_timeout(struct work_struct *work)
 
 	l2cap_chan_close(chan, reason);
 
-	release_sock(sk);
+	l2cap_chan_unlock(chan);
 
 	chan->ops->close(chan->data);
 	mutex_unlock(&conn->chan_lock);
@@ -378,6 +377,8 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
 		hci_conn_put(conn->hcon);
 	}
 
+	lock_sock(sk);
+
 	__l2cap_state_change(chan, BT_CLOSED);
 	sock_set_flag(sk, SOCK_ZAPPED);
 
@@ -390,6 +391,8 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
 	} else
 		sk->sk_state_change(sk);
 
+	release_sock(sk);
+
 	if (!(test_bit(CONF_OUTPUT_DONE, &chan->conf_state) &&
 			test_bit(CONF_INPUT_DONE, &chan->conf_state)))
 		return;
@@ -422,10 +425,10 @@ static void l2cap_chan_cleanup_listen(struct sock *parent)
 	while ((sk = bt_accept_dequeue(parent, NULL))) {
 		struct l2cap_chan *chan = l2cap_pi(sk)->chan;
 
+		l2cap_chan_lock(chan);
 		__clear_chan_timer(chan);
-		lock_sock(sk);
 		l2cap_chan_close(chan, ECONNRESET);
-		release_sock(sk);
+		l2cap_chan_unlock(chan);
 
 		chan->ops->close(chan->data);
 	}
@@ -441,10 +444,12 @@ void l2cap_chan_close(struct l2cap_chan *chan, int reason)
 
 	switch (chan->state) {
 	case BT_LISTEN:
+		lock_sock(sk);
 		l2cap_chan_cleanup_listen(sk);
 
 		__l2cap_state_change(chan, BT_CLOSED);
 		sock_set_flag(sk, SOCK_ZAPPED);
+		release_sock(sk);
 		break;
 
 	case BT_CONNECTED:
@@ -487,7 +492,9 @@ void l2cap_chan_close(struct l2cap_chan *chan, int reason)
 		break;
 
 	default:
+		lock_sock(sk);
 		sock_set_flag(sk, SOCK_ZAPPED);
+		release_sock(sk);
 		break;
 	}
 }
@@ -715,6 +722,7 @@ static inline int l2cap_mode_supported(__u8 mode, __u32 feat_mask)
 
 static void l2cap_send_disconn_req(struct l2cap_conn *conn, struct l2cap_chan *chan, int err)
 {
+	struct sock *sk = chan->sk;
 	struct l2cap_disconn_req req;
 
 	if (!conn)
@@ -731,8 +739,10 @@ static void l2cap_send_disconn_req(struct l2cap_conn *conn, struct l2cap_chan *c
 	l2cap_send_cmd(conn, l2cap_get_ident(conn),
 			L2CAP_DISCONN_REQ, sizeof(req), &req);
 
+	lock_sock(sk);
 	__l2cap_state_change(chan, BT_DISCONN);
 	__l2cap_chan_set_err(chan, err);
+	release_sock(sk);
 }
 
 /* ---- L2CAP connections ---- */
@@ -747,10 +757,10 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
 	list_for_each_entry_safe(chan, tmp, &conn->chan_l, list) {
 		struct sock *sk = chan->sk;
 
-		bh_lock_sock(sk);
+		l2cap_chan_lock(chan);
 
 		if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
-			bh_unlock_sock(sk);
+			l2cap_chan_unlock(chan);
 			continue;
 		}
 
@@ -759,17 +769,15 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
 
 			if (!l2cap_chan_check_security(chan) ||
 					!__l2cap_no_conn_pending(chan)) {
-				bh_unlock_sock(sk);
+				l2cap_chan_unlock(chan);
 				continue;
 			}
 
 			if (!l2cap_mode_supported(chan->mode, conn->feat_mask)
 					&& test_bit(CONF_STATE2_DEVICE,
 					&chan->conf_state)) {
-				/* l2cap_chan_close() calls list_del(chan)
-				 * so release the lock */
 				l2cap_chan_close(chan, ECONNRESET);
-				bh_unlock_sock(sk);
+				l2cap_chan_unlock(chan);
 				continue;
 			}
 
@@ -789,6 +797,7 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
 			rsp.dcid = cpu_to_le16(chan->scid);
 
 			if (l2cap_chan_check_security(chan)) {
+				lock_sock(sk);
 				if (bt_sk(sk)->defer_setup) {
 					struct sock *parent = bt_sk(sk)->parent;
 					rsp.result = cpu_to_le16(L2CAP_CR_PEND);
@@ -801,6 +810,7 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
 					rsp.result = cpu_to_le16(L2CAP_CR_SUCCESS);
 					rsp.status = cpu_to_le16(L2CAP_CS_NO_INFO);
 				}
+				release_sock(sk);
 			} else {
 				rsp.result = cpu_to_le16(L2CAP_CR_PEND);
 				rsp.status = cpu_to_le16(L2CAP_CS_AUTHEN_PEND);
@@ -811,7 +821,7 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
 
 			if (test_bit(CONF_REQ_SENT, &chan->conf_state) ||
 					rsp.result != L2CAP_CR_SUCCESS) {
-				bh_unlock_sock(sk);
+				l2cap_chan_unlock(chan);
 				continue;
 			}
 
@@ -821,7 +831,7 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
 			chan->num_conf_req++;
 		}
 
-		bh_unlock_sock(sk);
+		l2cap_chan_unlock(chan);
 	}
 
 	mutex_unlock(&conn->chan_lock);
@@ -910,7 +920,11 @@ clean:
 static void l2cap_chan_ready(struct l2cap_chan *chan)
 {
 	struct sock *sk = chan->sk;
-	struct sock *parent = bt_sk(sk)->parent;
+	struct sock *parent;
+
+	lock_sock(sk);
+
+	parent = bt_sk(sk)->parent;
 
 	BT_DBG("sk %p, parent %p", sk, parent);
 
@@ -922,6 +936,8 @@ static void l2cap_chan_ready(struct l2cap_chan *chan)
 
 	if (parent)
 		parent->sk_data_ready(parent, 0);
+
+	release_sock(sk);
 }
 
 static void l2cap_conn_ready(struct l2cap_conn *conn)
@@ -939,23 +955,25 @@ static void l2cap_conn_ready(struct l2cap_conn *conn)
 	mutex_lock(&conn->chan_lock);
 
 	list_for_each_entry(chan, &conn->chan_l, list) {
-		struct sock *sk = chan->sk;
 
-		bh_lock_sock(sk);
+		l2cap_chan_lock(chan);
 
 		if (conn->hcon->type == LE_LINK) {
 			if (smp_conn_security(conn, chan->sec_level))
 				l2cap_chan_ready(chan);
 
 		} else if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
+			struct sock *sk = chan->sk;
 			__clear_chan_timer(chan);
+			lock_sock(sk);
 			__l2cap_state_change(chan, BT_CONNECTED);
 			sk->sk_state_change(sk);
+			release_sock(sk);
 
 		} else if (chan->state == BT_CONNECT)
 			l2cap_do_start(chan);
 
-		bh_unlock_sock(sk);
+		l2cap_chan_unlock(chan);
 	}
 
 	mutex_unlock(&conn->chan_lock);
@@ -993,7 +1011,6 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
 {
 	struct l2cap_conn *conn = hcon->l2cap_data;
 	struct l2cap_chan *chan, *l;
-	struct sock *sk;
 
 	if (!conn)
 		return;
@@ -1006,10 +1023,12 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
 
 	/* Kill channels */
 	list_for_each_entry_safe(chan, l, &conn->chan_l, list) {
-		sk = chan->sk;
-		lock_sock(sk);
+		l2cap_chan_lock(chan);
+
 		l2cap_chan_del(chan, err);
-		release_sock(sk);
+
+		l2cap_chan_unlock(chan);
+
 		chan->ops->close(chan->data);
 	}
 
@@ -1140,7 +1159,7 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid, bdaddr_t *d
 
 	hci_dev_lock(hdev);
 
-	lock_sock(sk);
+	l2cap_chan_lock(chan);
 
 	/* PSM must be odd and lsb of upper byte must be 0 */
 	if ((__le16_to_cpu(psm) & 0x0101) != 0x0001 && !cid &&
@@ -1167,17 +1186,21 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid, bdaddr_t *d
 		goto done;
 	}
 
+	lock_sock(sk);
+
 	switch (sk->sk_state) {
 	case BT_CONNECT:
 	case BT_CONNECT2:
 	case BT_CONFIG:
 		/* Already connecting */
 		err = 0;
+		release_sock(sk);
 		goto done;
 
 	case BT_CONNECTED:
 		/* Already connected */
 		err = -EISCONN;
+		release_sock(sk);
 		goto done;
 
 	case BT_OPEN:
@@ -1187,11 +1210,15 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid, bdaddr_t *d
 
 	default:
 		err = -EBADFD;
+		release_sock(sk);
 		goto done;
 	}
 
 	/* Set destination address and psm */
 	bacpy(&bt_sk(sk)->dst, dst);
+
+	release_sock(sk);
+
 	chan->psm = psm;
 	chan->dcid = cid;
 
@@ -1219,16 +1246,18 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid, bdaddr_t *d
 	/* Update source addr of the socket */
 	bacpy(src, conn->src);
 
+	l2cap_chan_unlock(chan);
 	l2cap_chan_add(conn, chan);
+	l2cap_chan_lock(chan);
 
-	__l2cap_state_change(chan, BT_CONNECT);
+	l2cap_state_change(chan, BT_CONNECT);
 	__set_chan_timer(chan, sk->sk_sndtimeo);
 
 	if (hcon->state == BT_CONNECTED) {
 		if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
 			__clear_chan_timer(chan);
 			if (l2cap_chan_check_security(chan))
-				__l2cap_state_change(chan, BT_CONNECTED);
+				l2cap_state_change(chan, BT_CONNECTED);
 		} else
 			l2cap_do_start(chan);
 	}
@@ -1236,6 +1265,7 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid, bdaddr_t *d
 	err = 0;
 
 done:
+	l2cap_chan_unlock(chan);
 	hci_dev_unlock(hdev);
 	hci_dev_put(hdev);
 	return err;
@@ -1277,14 +1307,14 @@ static void l2cap_monitor_timeout(struct work_struct *work)
 {
 	struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
 							monitor_timer.work);
-	struct sock *sk = chan->sk;
 
 	BT_DBG("chan %p", chan);
 
-	lock_sock(sk);
+	l2cap_chan_lock(chan);
+
 	if (chan->retry_count >= chan->remote_max_tx) {
 		l2cap_send_disconn_req(chan->conn, chan, ECONNABORTED);
-		release_sock(sk);
+		l2cap_chan_unlock(chan);
 		return;
 	}
 
@@ -1292,25 +1322,26 @@ static void l2cap_monitor_timeout(struct work_struct *work)
 	__set_monitor_timer(chan);
 
 	l2cap_send_rr_or_rnr(chan, L2CAP_CTRL_POLL);
-	release_sock(sk);
+	l2cap_chan_unlock(chan);
 }
 
 static void l2cap_retrans_timeout(struct work_struct *work)
 {
 	struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
 							retrans_timer.work);
-	struct sock *sk = chan->sk;
 
 	BT_DBG("chan %p", chan);
 
-	lock_sock(sk);
+	l2cap_chan_lock(chan);
+
 	chan->retry_count = 1;
 	__set_monitor_timer(chan);
 
 	set_bit(CONN_WAIT_F, &chan->conn_state);
 
 	l2cap_send_rr_or_rnr(chan, L2CAP_CTRL_POLL);
-	release_sock(sk);
+
+	l2cap_chan_unlock(chan);
 }
 
 static void l2cap_drop_acked_frames(struct l2cap_chan *chan)
@@ -2001,9 +2032,11 @@ static void l2cap_ack_timeout(struct work_struct *work)
 
 	BT_DBG("chan %p", chan);
 
-	lock_sock(chan->sk);
+	l2cap_chan_lock(chan);
+
 	__l2cap_send_ack(chan);
-	release_sock(chan->sk);
+
+	l2cap_chan_unlock(chan);
 
 	l2cap_chan_put(chan);
 }
@@ -2664,7 +2697,7 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
 
 	bt_accept_enqueue(parent, sk);
 
-	l2cap_chan_add(conn, chan);
+	__l2cap_chan_add(conn, chan);
 
 	dcid = chan->scid;
 
@@ -2737,7 +2770,6 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
 	struct l2cap_conn_rsp *rsp = (struct l2cap_conn_rsp *) data;
 	u16 scid, dcid, result, status;
 	struct l2cap_chan *chan;
-	struct sock *sk;
 	u8 req[128];
 	int err;
 
@@ -2767,8 +2799,7 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
 
 	err = 0;
 
-	sk = chan->sk;
-	lock_sock(sk);
+	l2cap_chan_lock(chan);
 
 	switch (result) {
 	case L2CAP_CR_SUCCESS:
@@ -2794,7 +2825,7 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
 		break;
 	}
 
-	release_sock(sk);
+	l2cap_chan_unlock(chan);
 
 unlock:
 	mutex_unlock(&conn->chan_lock);
@@ -2819,7 +2850,6 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 	u16 dcid, flags;
 	u8 rsp[64];
 	struct l2cap_chan *chan;
-	struct sock *sk;
 	int len;
 
 	dcid  = __le16_to_cpu(req->dcid);
@@ -2831,8 +2861,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 	if (!chan)
 		return -ENOENT;
 
-	sk = chan->sk;
-	lock_sock(sk);
+	l2cap_chan_lock(chan);
 
 	if (chan->state != BT_CONFIG && chan->state != BT_CONNECT2) {
 		struct l2cap_cmd_rej_cid rej;
@@ -2921,7 +2950,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 	}
 
 unlock:
-	release_sock(sk);
+	l2cap_chan_unlock(chan);
 	return 0;
 }
 
@@ -2930,7 +2959,6 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 	struct l2cap_conf_rsp *rsp = (struct l2cap_conf_rsp *)data;
 	u16 scid, flags, result;
 	struct l2cap_chan *chan;
-	struct sock *sk;
 	int len = cmd->len - sizeof(*rsp);
 
 	scid   = __le16_to_cpu(rsp->scid);
@@ -2944,8 +2972,7 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 	if (!chan)
 		return 0;
 
-	sk = chan->sk;
-	lock_sock(sk);
+	l2cap_chan_lock(chan);
 
 	switch (result) {
 	case L2CAP_CONF_SUCCESS:
@@ -3004,7 +3031,7 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 		}
 
 	default:
-		__l2cap_chan_set_err(chan, ECONNRESET);
+		l2cap_chan_set_err(chan, ECONNRESET);
 
 		__set_chan_timer(chan,
 				msecs_to_jiffies(L2CAP_DISC_REJ_TIMEOUT));
@@ -3031,7 +3058,7 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 	}
 
 done:
-	release_sock(sk);
+	l2cap_chan_unlock(chan);
 	return 0;
 }
 
@@ -3056,17 +3083,21 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn, struct l2cap_cmd
 		return 0;
 	}
 
+	l2cap_chan_lock(chan);
+
 	sk = chan->sk;
-	lock_sock(sk);
 
 	rsp.dcid = cpu_to_le16(chan->scid);
 	rsp.scid = cpu_to_le16(chan->dcid);
 	l2cap_send_cmd(conn, cmd->ident, L2CAP_DISCONN_RSP, sizeof(rsp), &rsp);
 
+	lock_sock(sk);
 	sk->sk_shutdown = SHUTDOWN_MASK;
+	release_sock(sk);
 
 	l2cap_chan_del(chan, ECONNRESET);
-	release_sock(sk);
+
+	l2cap_chan_unlock(chan);
 
 	chan->ops->close(chan->data);
 
@@ -3080,7 +3111,6 @@ static inline int l2cap_disconnect_rsp(struct l2cap_conn *conn, struct l2cap_cmd
 	struct l2cap_disconn_rsp *rsp = (struct l2cap_disconn_rsp *) data;
 	u16 dcid, scid;
 	struct l2cap_chan *chan;
-	struct sock *sk;
 
 	scid = __le16_to_cpu(rsp->scid);
 	dcid = __le16_to_cpu(rsp->dcid);
@@ -3095,11 +3125,11 @@ static inline int l2cap_disconnect_rsp(struct l2cap_conn *conn, struct l2cap_cmd
 		return 0;
 	}
 
-	sk = chan->sk;
-	lock_sock(sk);
+	l2cap_chan_lock(chan);
 
 	l2cap_chan_del(chan, 0);
-	release_sock(sk);
+
+	l2cap_chan_unlock(chan);
 
 	chan->ops->close(chan->data);
 
@@ -4251,7 +4281,6 @@ drop:
 static inline int l2cap_data_channel(struct l2cap_conn *conn, u16 cid, struct sk_buff *skb)
 {
 	struct l2cap_chan *chan;
-	struct sock *sk = NULL;
 	u32 control;
 	u16 tx_seq;
 	int len;
@@ -4259,11 +4288,12 @@ static inline int l2cap_data_channel(struct l2cap_conn *conn, u16 cid, struct sk
 	chan = l2cap_get_chan_by_scid(conn, cid);
 	if (!chan) {
 		BT_DBG("unknown cid 0x%4.4x", cid);
-		goto drop;
+		/* Drop packet and return */
+		kfree(skb);
+		return 0;
 	}
 
-	sk = chan->sk;
-	lock_sock(sk);
+	l2cap_chan_lock(chan);
 
 	BT_DBG("chan %p, len %d", chan, skb->len);
 
@@ -4334,8 +4364,7 @@ drop:
 	kfree_skb(skb);
 
 done:
-	if (sk)
-		release_sock(sk);
+	l2cap_chan_unlock(chan);
 
 	return 0;
 }
@@ -4554,9 +4583,7 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
 	mutex_lock(&conn->chan_lock);
 
 	list_for_each_entry(chan, &conn->chan_l, list) {
-		struct sock *sk = chan->sk;
-
-		bh_lock_sock(sk);
+		l2cap_chan_lock(chan);
 
 		BT_DBG("chan->scid %d", chan->scid);
 
@@ -4566,19 +4593,19 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
 				l2cap_chan_ready(chan);
 			}
 
-			bh_unlock_sock(sk);
+			l2cap_chan_unlock(chan);
 			continue;
 		}
 
 		if (test_bit(CONF_CONNECT_PEND, &chan->conf_state)) {
-			bh_unlock_sock(sk);
+			l2cap_chan_unlock(chan);
 			continue;
 		}
 
 		if (!status && (chan->state == BT_CONNECTED ||
 						chan->state == BT_CONFIG)) {
 			l2cap_check_encryption(chan, encrypt);
-			bh_unlock_sock(sk);
+			l2cap_chan_unlock(chan);
 			continue;
 		}
 
@@ -4599,9 +4626,12 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
 					msecs_to_jiffies(L2CAP_DISC_TIMEOUT));
 			}
 		} else if (chan->state == BT_CONNECT2) {
+			struct sock *sk = chan->sk;
 			struct l2cap_conn_rsp rsp;
 			__u16 res, stat;
 
+			lock_sock(sk);
+
 			if (!status) {
 				if (bt_sk(sk)->defer_setup) {
 					struct sock *parent = bt_sk(sk)->parent;
@@ -4622,6 +4652,8 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
 				stat = L2CAP_CS_NO_INFO;
 			}
 
+			release_sock(sk);
+
 			rsp.scid   = cpu_to_le16(chan->dcid);
 			rsp.dcid   = cpu_to_le16(chan->scid);
 			rsp.result = cpu_to_le16(res);
@@ -4630,7 +4662,7 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
 							sizeof(rsp), &rsp);
 		}
 
-		bh_unlock_sock(sk);
+		l2cap_chan_unlock(chan);
 	}
 
 	mutex_unlock(&conn->chan_lock);
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 1273fcb..bbc1747 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -127,6 +127,8 @@ static int l2cap_sock_connect(struct socket *sock, struct sockaddr *addr, int al
 	if (err)
 		goto done;
 
+	lock_sock(sk);
+
 	err = bt_sock_wait_state(sk, BT_CONNECTED,
 			sock_sndtimeo(sk, flags & O_NONBLOCK));
 done:
@@ -809,15 +811,18 @@ static int l2cap_sock_shutdown(struct socket *sock, int how)
 
 	if (conn)
 		mutex_lock(&conn->chan_lock);
-
+	l2cap_chan_lock(chan);
 	lock_sock(sk);
+
 	if (!sk->sk_shutdown) {
 		if (chan->mode == L2CAP_MODE_ERTM)
 			err = __l2cap_wait_ack(sk);
 
 		sk->sk_shutdown = SHUTDOWN_MASK;
 
+		release_sock(sk);
 		l2cap_chan_close(chan, 0);
+		lock_sock(sk);
 
 		if (sock_flag(sk, SOCK_LINGER) && sk->sk_lingertime)
 			err = bt_sock_wait_state(sk, BT_CLOSED,
@@ -828,7 +833,7 @@ static int l2cap_sock_shutdown(struct socket *sock, int how)
 		err = -sk->sk_err;
 
 	release_sock(sk);
-
+	l2cap_chan_unlock(chan);
 	if (conn)
 		mutex_unlock(&conn->chan_lock);
 
@@ -874,8 +879,12 @@ static int l2cap_sock_recv_cb(void *data, struct sk_buff *skb)
 	struct sock *sk = data;
 	struct l2cap_pinfo *pi = l2cap_pi(sk);
 
-	if (pi->rx_busy_skb)
-		return -ENOMEM;
+	lock_sock(sk);
+
+	if (pi->rx_busy_skb) {
+		err = -ENOMEM;
+		goto done;
+	}
 
 	err = sock_queue_rcv_skb(sk, skb);
 
@@ -894,6 +903,9 @@ static int l2cap_sock_recv_cb(void *data, struct sk_buff *skb)
 		err = 0;
 	}
 
+done:
+	release_sock(sk);
+
 	return err;
 }
 
-- 
1.7.9

[PATCHv2 3/3] Bluetooth: Remove socket lock check

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Simplify code so that we do not need to check whether socket is locked.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Acked-by: Gustavo F. Padovan <padovan@profusion.mobi>
---
 net/bluetooth/l2cap_sock.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index bbc1747..e2fc24b 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -125,15 +125,15 @@ static int l2cap_sock_connect(struct socket *sock, struct sockaddr *addr, int al
 
 	err = l2cap_chan_connect(chan, la.l2_psm, la.l2_cid, &la.l2_bdaddr);
 	if (err)
-		goto done;
+		return err;
 
 	lock_sock(sk);
 
 	err = bt_sock_wait_state(sk, BT_CONNECTED,
 			sock_sndtimeo(sk, flags & O_NONBLOCK));
-done:
-	if (sock_owned_by_user(sk))
-		release_sock(sk);
+
+	release_sock(sk);
+
 	return err;
 }
 
-- 
1.7.9

Re: [PATCHv2 0/3] Bluetooth: Change socket lock to l2cap_chan lock

[Ulisses Furquim]

Hi Andrei,

On Wed, Feb 22, 2012 at 8:43 AM, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> Changing socket lock to L2CAP chan lock in L2CAP code. Needed for implementing
> protocols above L2CAP without creating sockets.
>
> Changes:
>        * PATCHv2: Rebase remaining parts against latest tree and merge all
>        patches dealing with converting sk lock to chan lock together following
>        recommendation from Ulisses and Gustavo.
>        * PATCHv1: Added extra line (per Gustavo comment)
>        * RFCv6: Same code but patches 2,3 and 4 from RFCv5 are merged together
>        following recommendations from review.
>        * RFCv5: Fixed locking bug in l2cap_data_channel, added locks in
>        l2cap_sock_shutdown function, fixed several styles issues.
>        * RFCv4: Better split patches so they looks more clear and obvious,
>        taking coments about naming change and delete unused vars. See diff change
>        from the previous version below:
>        * RFCv3: Split the big patch to several small (I believe logical) chunks,
>        remove unneded locks from cleanup_listen, use the same arguments for
>        locked/unlocked socket error functions.
>        * RFCv2: Convert l2cap channel list back to mutex from RCU list.
>
> Andrei Emeltchenko (3):
>  Bluetooth: Add unlocked __l2cap_chan_add function
>  Bluetooth: Change sk lock to chan lock in L2CAP core
>  Bluetooth: Remove socket lock check
>
>  net/bluetooth/l2cap_core.c |  173 +++++++++++++++++++++++++++-----------------
>  net/bluetooth/l2cap_sock.c |   28 +++++--
>  2 files changed, 125 insertions(+), 76 deletions(-)

I'm mostly ok with the patches. However, have you seen my questions on
reply to PATCHv1 08/14? Please, check that. Thanks.

Regards,

-- 
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs

Re: [PATCHv2 0/3] Bluetooth: Change socket lock to l2cap_chan lock

[Andrei Emeltchenko]

Hi Ulisses,

On Wed, Feb 22, 2012 at 10:30:40AM -0200, Ulisses Furquim wrote:
> Hi Andrei,
> 
> On Wed, Feb 22, 2012 at 8:43 AM, Andrei Emeltchenko
> <Andrei.Emeltchenko.news@gmail.com> wrote:
> > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> >
> > Changing socket lock to L2CAP chan lock in L2CAP code. Needed for implementing
> > protocols above L2CAP without creating sockets.
> >
> > Changes:
> >        * PATCHv2: Rebase remaining parts against latest tree and merge all
> >        patches dealing with converting sk lock to chan lock together following
> >        recommendation from Ulisses and Gustavo.
> >        * PATCHv1: Added extra line (per Gustavo comment)
> >        * RFCv6: Same code but patches 2,3 and 4 from RFCv5 are merged together
> >        following recommendations from review.
> >        * RFCv5: Fixed locking bug in l2cap_data_channel, added locks in
> >        l2cap_sock_shutdown function, fixed several styles issues.
> >        * RFCv4: Better split patches so they looks more clear and obvious,
> >        taking coments about naming change and delete unused vars. See diff change
> >        from the previous version below:
> >        * RFCv3: Split the big patch to several small (I believe logical) chunks,
> >        remove unneded locks from cleanup_listen, use the same arguments for
> >        locked/unlocked socket error functions.
> >        * RFCv2: Convert l2cap channel list back to mutex from RCU list.
> >
> > Andrei Emeltchenko (3):
> >  Bluetooth: Add unlocked __l2cap_chan_add function
> >  Bluetooth: Change sk lock to chan lock in L2CAP core
> >  Bluetooth: Remove socket lock check
> >
> >  net/bluetooth/l2cap_core.c |  173 +++++++++++++++++++++++++++-----------------
> >  net/bluetooth/l2cap_sock.c |   28 +++++--
> >  2 files changed, 125 insertions(+), 76 deletions(-)
> 
> I'm mostly ok with the patches. However, have you seen my questions on
> reply to PATCHv1 08/14? Please, check that. Thanks.

Sorry missed those comments.

In a case of:
<------8<---------------------------------------------------------------------
|  > @@ -440,10 +444,12 @@ void l2cap_chan_close(struct l2cap_chan *chan, int
|  > reason)
|  >
|  >        switch (chan->state) {
|  >        case BT_LISTEN:
|  > +               lock_sock(sk);
|  >                l2cap_chan_cleanup_listen(sk);
|  >
|  >                __l2cap_state_change(chan, BT_CLOSED);
|  >                sock_set_flag(sk, SOCK_ZAPPED);
|  > +               release_sock(sk);
|  >                break;
|  >
|  >        case BT_CONNECTED:
|
|  Do we need to lock sock around l2cap_chan_cleanup_listen() as well?
|  l2cap_chan_close() will call l2cap_chan_del() which now does
|  lock_sock/release_sock, right?
<------8<---------------------------------------------------------------------

Those are different socks. We take parent sk lock here.

concerning:

<------8<------------------------------------------------------------------
|  > @@ -714,6 +722,7 @@ static inline int l2cap_mode_supported(__u8 mode,
|  > __u32 feat_mask)
|  >
|  >  static void l2cap_send_disconn_req(struct l2cap_conn *conn, struct
|  >  l2cap_chan *chan, int err)
|  >  {
|  > +       struct sock *sk = chan->sk;
|  >        struct l2cap_disconn_req req;
|  >
|  >        if (!conn)
|  > @@ -730,8 +739,10 @@ static void l2cap_send_disconn_req(struct
|  > l2cap_conn *conn, struct l2cap_chan *c
|  >        l2cap_send_cmd(conn, l2cap_get_ident(conn),
|  >                        L2CAP_DISCONN_REQ, sizeof(req), &req);
|  >
|  > +       lock_sock(sk);
|  >        __l2cap_state_change(chan, BT_DISCONN);
|  >        __l2cap_chan_set_err(chan, err);
|  > +       release_sock(sk);
|  >  }
|  >
|  >  /* ---- L2CAP connections ---- */
|
|  It seems we didn't have any locking around these before. Why? Do we
|  really need it now?
<------8<------------------------------------------------------------------

The locks were in the functions invoking l2cap_send_disconn_req.

Otherwise would following changes be OK?

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 60d3276..56a85c0 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -363,8 +363,6 @@ void l2cap_chan_add(struct l2cap_conn *conn, struct
l2cap_chan *chan)
        mutex_unlock(&conn->chan_lock);
 }
 
-/* Delete channel.
- * Must be called on the locked socket. */
 static void l2cap_chan_del(struct l2cap_chan *chan, int err)
 {
        struct sock *sk = chan->sk;
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index ae15d7a..efe391e 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -812,6 +812,7 @@ static int l2cap_sock_shutdown(struct socket *sock,
int how)
 
        if (conn)
                mutex_lock(&conn->chan_lock);
+
        l2cap_chan_lock(chan);
        lock_sock(sk);
 
@@ -835,6 +836,7 @@ static int l2cap_sock_shutdown(struct socket *sock,
int how)
 
        release_sock(sk);
        l2cap_chan_unlock(chan);
+
        if (conn)
                mutex_unlock(&conn->chan_lock);
 

Best regards 
Andrei Emeltchenko 

Re: [PATCHv2 0/3] Bluetooth: Change socket lock to l2cap_chan lock

[Ulisses Furquim]

Hi Andrei,

On Wed, Feb 22, 2012 at 11:21 AM, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> Hi Ulisses,
>
> On Wed, Feb 22, 2012 at 10:30:40AM -0200, Ulisses Furquim wrote:
>> Hi Andrei,
>>
>> On Wed, Feb 22, 2012 at 8:43 AM, Andrei Emeltchenko
>> <Andrei.Emeltchenko.news@gmail.com> wrote:
>> > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>> >
>> > Changing socket lock to L2CAP chan lock in L2CAP code. Needed for implementing
>> > protocols above L2CAP without creating sockets.
>> >
>> > Changes:
>> >        * PATCHv2: Rebase remaining parts against latest tree and merge all
>> >        patches dealing with converting sk lock to chan lock together following
>> >        recommendation from Ulisses and Gustavo.
>> >        * PATCHv1: Added extra line (per Gustavo comment)
>> >        * RFCv6: Same code but patches 2,3 and 4 from RFCv5 are merged together
>> >        following recommendations from review.
>> >        * RFCv5: Fixed locking bug in l2cap_data_channel, added locks in
>> >        l2cap_sock_shutdown function, fixed several styles issues.
>> >        * RFCv4: Better split patches so they looks more clear and obvious,
>> >        taking coments about naming change and delete unused vars. See diff change
>> >        from the previous version below:
>> >        * RFCv3: Split the big patch to several small (I believe logical) chunks,
>> >        remove unneded locks from cleanup_listen, use the same arguments for
>> >        locked/unlocked socket error functions.
>> >        * RFCv2: Convert l2cap channel list back to mutex from RCU list.
>> >
>> > Andrei Emeltchenko (3):
>> >  Bluetooth: Add unlocked __l2cap_chan_add function
>> >  Bluetooth: Change sk lock to chan lock in L2CAP core
>> >  Bluetooth: Remove socket lock check
>> >
>> >  net/bluetooth/l2cap_core.c |  173 +++++++++++++++++++++++++++-----------------
>> >  net/bluetooth/l2cap_sock.c |   28 +++++--
>> >  2 files changed, 125 insertions(+), 76 deletions(-)
>>
>> I'm mostly ok with the patches. However, have you seen my questions on
>> reply to PATCHv1 08/14? Please, check that. Thanks.
>
> Sorry missed those comments.
>
> In a case of:
> <------8<---------------------------------------------------------------------
> |  > @@ -440,10 +444,12 @@ void l2cap_chan_close(struct l2cap_chan *chan, int
> |  > reason)
> |  >
> |  >        switch (chan->state) {
> |  >        case BT_LISTEN:
> |  > +               lock_sock(sk);
> |  >                l2cap_chan_cleanup_listen(sk);
> |  >
> |  >                __l2cap_state_change(chan, BT_CLOSED);
> |  >                sock_set_flag(sk, SOCK_ZAPPED);
> |  > +               release_sock(sk);
> |  >                break;
> |  >
> |  >        case BT_CONNECTED:
> |
> |  Do we need to lock sock around l2cap_chan_cleanup_listen() as well?
> |  l2cap_chan_close() will call l2cap_chan_del() which now does
> |  lock_sock/release_sock, right?
> <------8<---------------------------------------------------------------------
>
> Those are different socks. We take parent sk lock here.

Hmm, ok, I missed that.

> concerning:
>
> <------8<------------------------------------------------------------------
> |  > @@ -714,6 +722,7 @@ static inline int l2cap_mode_supported(__u8 mode,
> |  > __u32 feat_mask)
> |  >
> |  >  static void l2cap_send_disconn_req(struct l2cap_conn *conn, struct
> |  >  l2cap_chan *chan, int err)
> |  >  {
> |  > +       struct sock *sk = chan->sk;
> |  >        struct l2cap_disconn_req req;
> |  >
> |  >        if (!conn)
> |  > @@ -730,8 +739,10 @@ static void l2cap_send_disconn_req(struct
> |  > l2cap_conn *conn, struct l2cap_chan *c
> |  >        l2cap_send_cmd(conn, l2cap_get_ident(conn),
> |  >                        L2CAP_DISCONN_REQ, sizeof(req), &req);
> |  >
> |  > +       lock_sock(sk);
> |  >        __l2cap_state_change(chan, BT_DISCONN);
> |  >        __l2cap_chan_set_err(chan, err);
> |  > +       release_sock(sk);
> |  >  }
> |  >
> |  >  /* ---- L2CAP connections ---- */
> |
> |  It seems we didn't have any locking around these before. Why? Do we
> |  really need it now?
> <------8<------------------------------------------------------------------
>
> The locks were in the functions invoking l2cap_send_disconn_req.

Ok.

> Otherwise would following changes be OK?

Sure, I'm ok with them if you only squashed them and added these tiny
changes below.

Reviewed-by: Ulisses Furquim <ulisses@profusion.mobi>

Regards,

-- Ulisses

> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 60d3276..56a85c0 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -363,8 +363,6 @@ void l2cap_chan_add(struct l2cap_conn *conn, struct
> l2cap_chan *chan)
>        mutex_unlock(&conn->chan_lock);
>  }
>
> -/* Delete channel.
> - * Must be called on the locked socket. */
>  static void l2cap_chan_del(struct l2cap_chan *chan, int err)
>  {
>        struct sock *sk = chan->sk;
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index ae15d7a..efe391e 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -812,6 +812,7 @@ static int l2cap_sock_shutdown(struct socket *sock,
> int how)
>
>        if (conn)
>                mutex_lock(&conn->chan_lock);
> +
>        l2cap_chan_lock(chan);
>        lock_sock(sk);
>
> @@ -835,6 +836,7 @@ static int l2cap_sock_shutdown(struct socket *sock,
> int how)
>
>        release_sock(sk);
>        l2cap_chan_unlock(chan);
> +
>        if (conn)
>                mutex_unlock(&conn->chan_lock);
>
>
> Best regards
> Andrei Emeltchenko

-- 
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs