[PATCH 0/2] L2CAP: Locking and refcnt fixes

[PATCH 0/2] L2CAP: Locking and refcnt fixes

[Andrei Emeltchenko]

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

Add refcnt to l2cap_conn and locking to L2CAP sockopt handling.

Andrei Emeltchenko (2):
  Bluetooth: Add refcnt to l2cap_conn
  Bluetooth: Lock l2cap chan in sockopt

 include/net/bluetooth/l2cap.h |    6 ++
 net/bluetooth/l2cap_core.c    |  128 +++++++++++++++++++++++++++++++++--------
 net/bluetooth/l2cap_sock.c    |   12 ++++
 net/bluetooth/smp.c           |    7 +--
 4 files changed, 125 insertions(+), 28 deletions(-)

-- 
1.7.9.5

[PATCH 1/2] Bluetooth: Add refcnt to l2cap_conn

[Andrei Emeltchenko]

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

Adding kref to l2cap_conn helps to better handle racing when deleting
l2cap_conn. Races are created when deleting conn from timeout and from
the other execution path.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 include/net/bluetooth/l2cap.h |    6 ++
 net/bluetooth/l2cap_core.c    |  128 +++++++++++++++++++++++++++++++++--------
 net/bluetooth/smp.c           |    7 +--
 3 files changed, 113 insertions(+), 28 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index d80e3f0..6b85b00 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -572,6 +572,8 @@ struct l2cap_conn {
 
 	struct list_head	chan_l;
 	struct mutex		chan_lock;
+
+	struct kref		kref;
 };
 
 #define L2CAP_INFO_CL_MTU_REQ_SENT	0x01
@@ -777,5 +779,9 @@ void l2cap_chan_set_defaults(struct l2cap_chan *chan);
 int l2cap_ertm_init(struct l2cap_chan *chan);
 void l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan);
 void l2cap_chan_del(struct l2cap_chan *chan, int err);
+void l2cap_conn_set_timer(struct l2cap_conn *conn, struct delayed_work *work,
+			  long timeout);
+bool l2cap_conn_clear_timer(struct l2cap_conn *conn,
+			    struct delayed_work *work);
 
 #endif /* __L2CAP_H */
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 5dd7a76..2f7995e 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -57,6 +57,70 @@ static void l2cap_send_disconn_req(struct l2cap_conn *conn,
 
 static void l2cap_tx(struct l2cap_chan *chan, struct l2cap_ctrl *control,
 		    struct sk_buff_head *skbs, u8 event);
+static void l2cap_conn_del(struct hci_conn *hcon, int err);
+
+/* ---- L2CAP connections ---- */
+
+static void l2cap_conn_get(struct l2cap_conn *conn)
+{
+	BT_DBG("conn %p refcnt %d -> %d", conn,
+	       atomic_read(&conn->kref.refcount),
+	       atomic_read(&conn->kref.refcount) + 1);
+
+	kref_get(&conn->kref);
+}
+
+static void l2cap_conn_destroy(struct kref *kref)
+{
+	struct l2cap_conn *conn = container_of(kref, struct l2cap_conn, kref);
+
+	BT_DBG("conn %p", conn);
+
+	l2cap_conn_del(conn->hcon, 0);
+}
+
+static int l2cap_conn_put(struct l2cap_conn *conn)
+{
+	/* conn might be NULL, was checked before in l2cap_conn_del */
+	if (!conn) {
+		BT_DBG("conn == NULL");
+		return 1;
+	}
+
+	BT_DBG("conn %p refcnt %d -> %d", conn,
+	       atomic_read(&conn->kref.refcount),
+	       atomic_read(&conn->kref.refcount) - 1);
+
+	return kref_put(&conn->kref, &l2cap_conn_destroy);
+}
+
+void l2cap_conn_set_timer(struct l2cap_conn *conn, struct delayed_work *work,
+			  long timeout)
+{
+	BT_DBG("conn %p timeout %ld", conn, timeout);
+
+	/* If delayed work cancelled do not hold(conn)
+	   since it is already done with previous set_timer */
+	if (!cancel_delayed_work(work))
+		l2cap_conn_get(conn);
+
+	schedule_delayed_work(work, timeout);
+}
+
+bool l2cap_conn_clear_timer(struct l2cap_conn *conn, struct delayed_work *work)
+{
+	bool ret;
+
+	BT_DBG("conn %p", conn);
+
+	/* put(conn) if delayed work cancelled otherwise it
+	   is done in delayed work function */
+	ret = cancel_delayed_work(work);
+	if (ret)
+		l2cap_conn_put(conn);
+
+	return ret;
+}
 
 /* ---- L2CAP channels ---- */
 
@@ -974,7 +1038,8 @@ static void l2cap_do_start(struct l2cap_chan *chan)
 		conn->info_state |= L2CAP_INFO_FEAT_MASK_REQ_SENT;
 		conn->info_ident = l2cap_get_ident(conn);
 
-		schedule_delayed_work(&conn->info_timer, L2CAP_INFO_TIMEOUT);
+		l2cap_conn_set_timer(conn, &conn->info_timer,
+				     L2CAP_INFO_TIMEOUT);
 
 		l2cap_send_cmd(conn, conn->info_ident,
 					L2CAP_INFO_REQ, sizeof(req), &req);
@@ -1256,12 +1321,14 @@ static void l2cap_conn_unreliable(struct l2cap_conn *conn, int err)
 static void l2cap_info_timeout(struct work_struct *work)
 {
 	struct l2cap_conn *conn = container_of(work, struct l2cap_conn,
-							info_timer.work);
+					       info_timer.work);
 
 	conn->info_state |= L2CAP_INFO_FEAT_MASK_REQ_DONE;
 	conn->info_ident = 0;
 
 	l2cap_conn_start(conn);
+
+	l2cap_conn_put(conn);
 }
 
 static void l2cap_conn_del(struct hci_conn *hcon, int err)
@@ -1295,13 +1362,8 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
 
 	hci_chan_del(conn->hchan);
 
-	if (conn->info_state & L2CAP_INFO_FEAT_MASK_REQ_SENT)
-		cancel_delayed_work_sync(&conn->info_timer);
-
-	if (test_and_clear_bit(HCI_CONN_LE_SMP_PEND, &hcon->flags)) {
-		cancel_delayed_work_sync(&conn->security_timer);
+	if (test_and_clear_bit(HCI_CONN_LE_SMP_PEND, &hcon->flags))
 		smp_chan_destroy(conn);
-	}
 
 	hcon->l2cap_data = NULL;
 	kfree(conn);
@@ -1310,14 +1372,11 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
 static void security_timeout(struct work_struct *work)
 {
 	struct l2cap_conn *conn = container_of(work, struct l2cap_conn,
-						security_timer.work);
+					       security_timer.work);
 
 	BT_DBG("conn %p", conn);
 
-	if (test_and_clear_bit(HCI_CONN_LE_SMP_PEND, &conn->hcon->flags)) {
-		smp_chan_destroy(conn);
-		l2cap_conn_del(conn->hcon, ETIMEDOUT);
-	}
+	l2cap_conn_put(conn);
 }
 
 static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon, u8 status)
@@ -1359,6 +1418,8 @@ static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon, u8 status)
 
 	INIT_LIST_HEAD(&conn->chan_l);
 
+	kref_init(&conn->kref);
+
 	if (hcon->type == LE_LINK)
 		INIT_DELAYED_WORK(&conn->security_timer, security_timeout);
 	else
@@ -3316,8 +3377,8 @@ static inline int l2cap_command_rej(struct l2cap_conn *conn, struct l2cap_cmd_hd
 		return 0;
 
 	if ((conn->info_state & L2CAP_INFO_FEAT_MASK_REQ_SENT) &&
-					cmd->ident == conn->info_ident) {
-		cancel_delayed_work(&conn->info_timer);
+	    cmd->ident == conn->info_ident) {
+		l2cap_conn_clear_timer(conn, &conn->info_timer);
 
 		conn->info_state |= L2CAP_INFO_FEAT_MASK_REQ_DONE;
 		conn->info_ident = 0;
@@ -3431,10 +3492,11 @@ sendresp:
 		conn->info_state |= L2CAP_INFO_FEAT_MASK_REQ_SENT;
 		conn->info_ident = l2cap_get_ident(conn);
 
-		schedule_delayed_work(&conn->info_timer, L2CAP_INFO_TIMEOUT);
+		l2cap_conn_set_timer(conn, &conn->info_timer,
+				     L2CAP_INFO_TIMEOUT);
 
 		l2cap_send_cmd(conn, conn->info_ident,
-					L2CAP_INFO_REQ, sizeof(info), &info);
+			       L2CAP_INFO_REQ, sizeof(info), &info);
 	}
 
 	if (chan && !test_bit(CONF_REQ_SENT, &chan->conf_state) &&
@@ -3887,7 +3949,7 @@ static inline int l2cap_information_rsp(struct l2cap_conn *conn, struct l2cap_cm
 			conn->info_state & L2CAP_INFO_FEAT_MASK_REQ_DONE)
 		return 0;
 
-	cancel_delayed_work(&conn->info_timer);
+	l2cap_conn_clear_timer(conn, &conn->info_timer);
 
 	if (result != L2CAP_IR_SUCCESS) {
 		conn->info_state |= L2CAP_INFO_FEAT_MASK_REQ_DONE;
@@ -5270,8 +5332,10 @@ static void l2cap_recv_frame(struct l2cap_conn *conn, struct sk_buff *skb)
 		break;
 
 	case L2CAP_CID_SMP:
-		if (smp_sig_channel(conn, skb))
-			l2cap_conn_del(conn->hcon, EACCES);
+		if (smp_sig_channel(conn, skb)) {
+			l2cap_conn_clear_timer(conn, &conn->security_timer);
+			l2cap_conn_put(conn);
+		}
 		break;
 
 	default:
@@ -5323,8 +5387,14 @@ int l2cap_connect_cfm(struct hci_conn *hcon, u8 status)
 		conn = l2cap_conn_add(hcon, status);
 		if (conn)
 			l2cap_conn_ready(conn);
-	} else
-		l2cap_conn_del(hcon, bt_to_errno(status));
+	} else {
+		conn = hcon->l2cap_data;
+
+		if (hcon->type == LE_LINK)
+			l2cap_conn_clear_timer(conn, &conn->security_timer);
+
+		l2cap_conn_put(conn);
+	}
 
 	return 0;
 }
@@ -5342,9 +5412,14 @@ int l2cap_disconn_ind(struct hci_conn *hcon)
 
 int l2cap_disconn_cfm(struct hci_conn *hcon, u8 reason)
 {
+	struct l2cap_conn *conn = hcon->l2cap_data;
 	BT_DBG("hcon %p reason %d", hcon, reason);
 
-	l2cap_conn_del(hcon, bt_to_errno(reason));
+	if (hcon->type == LE_LINK)
+		l2cap_conn_clear_timer(conn, &conn->security_timer);
+
+	l2cap_conn_put(conn);
+
 	return 0;
 }
 
@@ -5374,10 +5449,13 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
 
 	BT_DBG("conn %p", conn);
 
+	l2cap_conn_get(conn);
+
 	if (hcon->type == LE_LINK) {
 		if (!status && encrypt)
 			smp_distribute_keys(conn, 0);
-		cancel_delayed_work(&conn->security_timer);
+
+		l2cap_conn_clear_timer(conn, &conn->security_timer);
 	}
 
 	mutex_lock(&conn->chan_lock);
@@ -5473,6 +5551,8 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
 
 	mutex_unlock(&conn->chan_lock);
 
+	l2cap_conn_put(conn);
+
 	return 0;
 }
 
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 16ef0dc..5bb5b51 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -186,8 +186,7 @@ static void smp_send_cmd(struct l2cap_conn *conn, u8 code, u16 len, void *data)
 	skb->priority = HCI_PRIO_MAX;
 	hci_send_acl(conn->hchan, skb, 0);
 
-	cancel_delayed_work_sync(&conn->security_timer);
-	schedule_delayed_work(&conn->security_timer, SMP_TIMEOUT);
+	l2cap_conn_set_timer(conn, &conn->security_timer, SMP_TIMEOUT);
 }
 
 static __u8 authreq_to_seclevel(__u8 authreq)
@@ -268,7 +267,7 @@ static void smp_failure(struct l2cap_conn *conn, u8 reason, u8 send)
 			 hcon->dst_type, reason);
 
 	if (test_and_clear_bit(HCI_CONN_LE_SMP_PEND, &conn->hcon->flags)) {
-		cancel_delayed_work_sync(&conn->security_timer);
+		l2cap_conn_clear_timer(conn, &conn->security_timer);
 		smp_chan_destroy(conn);
 	}
 }
@@ -999,7 +998,7 @@ int smp_distribute_keys(struct l2cap_conn *conn, __u8 force)
 
 	if (conn->hcon->out || force) {
 		clear_bit(HCI_CONN_LE_SMP_PEND, &conn->hcon->flags);
-		cancel_delayed_work_sync(&conn->security_timer);
+		l2cap_conn_clear_timer(conn, &conn->security_timer);
 		smp_chan_destroy(conn);
 	}
 
-- 
1.7.9.5

[PATCH 2/2] Bluetooth: Lock l2cap chan in sockopt

[Andrei Emeltchenko]

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


Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 net/bluetooth/l2cap_sock.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index a4bb27e..373ce9c 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -275,6 +275,7 @@ static int l2cap_sock_getsockopt_old(struct socket *sock, int optname, char __us
 	if (get_user(len, optlen))
 		return -EFAULT;
 
+	l2cap_chan_lock(chan);
 	lock_sock(sk);
 
 	switch (optname) {
@@ -345,6 +346,8 @@ static int l2cap_sock_getsockopt_old(struct socket *sock, int optname, char __us
 	}
 
 	release_sock(sk);
+	l2cap_chan_unlock(chan);
+
 	return err;
 }
 
@@ -367,6 +370,7 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname, ch
 	if (get_user(len, optlen))
 		return -EFAULT;
 
+	l2cap_chan_lock(chan);
 	lock_sock(sk);
 
 	switch (optname) {
@@ -442,6 +446,8 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname, ch
 	}
 
 	release_sock(sk);
+	l2cap_chan_unlock(chan);
+
 	return err;
 }
 
@@ -471,6 +477,7 @@ static int l2cap_sock_setsockopt_old(struct socket *sock, int optname, char __us
 
 	BT_DBG("sk %p", sk);
 
+	l2cap_chan_lock(chan);
 	lock_sock(sk);
 
 	switch (optname) {
@@ -556,6 +563,8 @@ static int l2cap_sock_setsockopt_old(struct socket *sock, int optname, char __us
 	}
 
 	release_sock(sk);
+	l2cap_chan_unlock(chan);
+
 	return err;
 }
 
@@ -577,6 +586,7 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname, ch
 	if (level != SOL_BLUETOOTH)
 		return -ENOPROTOOPT;
 
+	l2cap_chan_lock(chan);
 	lock_sock(sk);
 
 	switch (optname) {
@@ -729,6 +739,8 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname, ch
 	}
 
 	release_sock(sk);
+	l2cap_chan_unlock(chan);
+
 	return err;
 }
 
-- 
1.7.9.5

[PATCHv1 0/2] L2CAP: Locking and refcnt fixes

[Andrei Emeltchenko]

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

Add refcnt to l2cap_conn and locking to L2CAP sockopt handling.

Changes:
	* v1: change refcnt printing to similar used for l2cap_chan.

Andrei Emeltchenko (2):
  Bluetooth: Add refcnt to l2cap_conn
  Bluetooth: Lock l2cap chan in sockopt

 include/net/bluetooth/l2cap.h |    6 ++
 net/bluetooth/l2cap_core.c    |  126 +++++++++++++++++++++++++++++++++--------
 net/bluetooth/l2cap_sock.c    |   12 ++++
 net/bluetooth/smp.c           |    7 +--
 4 files changed, 123 insertions(+), 28 deletions(-)

-- 
1.7.9.5

[PATCHv1 1/2] Bluetooth: Add refcnt to l2cap_conn

[Andrei Emeltchenko]

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

Adding kref to l2cap_conn helps to better handle racing when deleting
l2cap_conn. Races are created when deleting conn from timeout and from
the other execution path.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 include/net/bluetooth/l2cap.h |    6 ++
 net/bluetooth/l2cap_core.c    |  126 +++++++++++++++++++++++++++++++++--------
 net/bluetooth/smp.c           |    7 +--
 3 files changed, 111 insertions(+), 28 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index e5164ff..e8b65ae 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -572,6 +572,8 @@ struct l2cap_conn {
 
 	struct list_head	chan_l;
 	struct mutex		chan_lock;
+
+	struct kref		kref;
 };
 
 #define L2CAP_INFO_CL_MTU_REQ_SENT	0x01
@@ -781,5 +783,9 @@ void l2cap_chan_set_defaults(struct l2cap_chan *chan);
 int l2cap_ertm_init(struct l2cap_chan *chan);
 void l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan);
 void l2cap_chan_del(struct l2cap_chan *chan, int err);
+void l2cap_conn_set_timer(struct l2cap_conn *conn, struct delayed_work *work,
+			  long timeout);
+bool l2cap_conn_clear_timer(struct l2cap_conn *conn,
+			    struct delayed_work *work);
 
 #endif /* __L2CAP_H */
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 9fd0599..773500b 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -57,6 +57,68 @@ static void l2cap_send_disconn_req(struct l2cap_conn *conn,
 
 static void l2cap_tx(struct l2cap_chan *chan, struct l2cap_ctrl *control,
 		    struct sk_buff_head *skbs, u8 event);
+static void l2cap_conn_del(struct hci_conn *hcon, int err);
+
+/* ---- L2CAP connections ---- */
+
+static void l2cap_conn_get(struct l2cap_conn *conn)
+{
+	BT_DBG("conn %p orig refcnt %d", conn,
+	       atomic_read(&conn->kref.refcount));
+
+	kref_get(&conn->kref);
+}
+
+static void l2cap_conn_destroy(struct kref *kref)
+{
+	struct l2cap_conn *conn = container_of(kref, struct l2cap_conn, kref);
+
+	BT_DBG("conn %p", conn);
+
+	l2cap_conn_del(conn->hcon, 0);
+}
+
+static int l2cap_conn_put(struct l2cap_conn *conn)
+{
+	/* conn might be NULL, was checked before in l2cap_conn_del */
+	if (!conn) {
+		BT_DBG("conn == NULL");
+		return 1;
+	}
+
+	BT_DBG("conn %p orig refcnt %d", conn,
+	       atomic_read(&conn->kref.refcount));
+
+	return kref_put(&conn->kref, &l2cap_conn_destroy);
+}
+
+void l2cap_conn_set_timer(struct l2cap_conn *conn, struct delayed_work *work,
+			  long timeout)
+{
+	BT_DBG("conn %p timeout %ld", conn, timeout);
+
+	/* If delayed work cancelled do not hold(conn)
+	   since it is already done with previous set_timer */
+	if (!cancel_delayed_work(work))
+		l2cap_conn_get(conn);
+
+	schedule_delayed_work(work, timeout);
+}
+
+bool l2cap_conn_clear_timer(struct l2cap_conn *conn, struct delayed_work *work)
+{
+	bool ret;
+
+	BT_DBG("conn %p", conn);
+
+	/* put(conn) if delayed work cancelled otherwise it
+	   is done in delayed work function */
+	ret = cancel_delayed_work(work);
+	if (ret)
+		l2cap_conn_put(conn);
+
+	return ret;
+}
 
 /* ---- L2CAP channels ---- */
 
@@ -976,7 +1038,8 @@ static void l2cap_do_start(struct l2cap_chan *chan)
 		conn->info_state |= L2CAP_INFO_FEAT_MASK_REQ_SENT;
 		conn->info_ident = l2cap_get_ident(conn);
 
-		schedule_delayed_work(&conn->info_timer, L2CAP_INFO_TIMEOUT);
+		l2cap_conn_set_timer(conn, &conn->info_timer,
+				     L2CAP_INFO_TIMEOUT);
 
 		l2cap_send_cmd(conn, conn->info_ident,
 					L2CAP_INFO_REQ, sizeof(req), &req);
@@ -1258,12 +1321,14 @@ static void l2cap_conn_unreliable(struct l2cap_conn *conn, int err)
 static void l2cap_info_timeout(struct work_struct *work)
 {
 	struct l2cap_conn *conn = container_of(work, struct l2cap_conn,
-							info_timer.work);
+					       info_timer.work);
 
 	conn->info_state |= L2CAP_INFO_FEAT_MASK_REQ_DONE;
 	conn->info_ident = 0;
 
 	l2cap_conn_start(conn);
+
+	l2cap_conn_put(conn);
 }
 
 static void l2cap_conn_del(struct hci_conn *hcon, int err)
@@ -1297,13 +1362,8 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
 
 	hci_chan_del(conn->hchan);
 
-	if (conn->info_state & L2CAP_INFO_FEAT_MASK_REQ_SENT)
-		cancel_delayed_work_sync(&conn->info_timer);
-
-	if (test_and_clear_bit(HCI_CONN_LE_SMP_PEND, &hcon->flags)) {
-		cancel_delayed_work_sync(&conn->security_timer);
+	if (test_and_clear_bit(HCI_CONN_LE_SMP_PEND, &hcon->flags))
 		smp_chan_destroy(conn);
-	}
 
 	hcon->l2cap_data = NULL;
 	kfree(conn);
@@ -1312,14 +1372,11 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
 static void security_timeout(struct work_struct *work)
 {
 	struct l2cap_conn *conn = container_of(work, struct l2cap_conn,
-						security_timer.work);
+					       security_timer.work);
 
 	BT_DBG("conn %p", conn);
 
-	if (test_and_clear_bit(HCI_CONN_LE_SMP_PEND, &conn->hcon->flags)) {
-		smp_chan_destroy(conn);
-		l2cap_conn_del(conn->hcon, ETIMEDOUT);
-	}
+	l2cap_conn_put(conn);
 }
 
 static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon, u8 status)
@@ -1361,6 +1418,8 @@ static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon, u8 status)
 
 	INIT_LIST_HEAD(&conn->chan_l);
 
+	kref_init(&conn->kref);
+
 	if (hcon->type == LE_LINK)
 		INIT_DELAYED_WORK(&conn->security_timer, security_timeout);
 	else
@@ -3318,8 +3377,8 @@ static inline int l2cap_command_rej(struct l2cap_conn *conn, struct l2cap_cmd_hd
 		return 0;
 
 	if ((conn->info_state & L2CAP_INFO_FEAT_MASK_REQ_SENT) &&
-					cmd->ident == conn->info_ident) {
-		cancel_delayed_work(&conn->info_timer);
+	    cmd->ident == conn->info_ident) {
+		l2cap_conn_clear_timer(conn, &conn->info_timer);
 
 		conn->info_state |= L2CAP_INFO_FEAT_MASK_REQ_DONE;
 		conn->info_ident = 0;
@@ -3433,10 +3492,11 @@ sendresp:
 		conn->info_state |= L2CAP_INFO_FEAT_MASK_REQ_SENT;
 		conn->info_ident = l2cap_get_ident(conn);
 
-		schedule_delayed_work(&conn->info_timer, L2CAP_INFO_TIMEOUT);
+		l2cap_conn_set_timer(conn, &conn->info_timer,
+				     L2CAP_INFO_TIMEOUT);
 
 		l2cap_send_cmd(conn, conn->info_ident,
-					L2CAP_INFO_REQ, sizeof(info), &info);
+			       L2CAP_INFO_REQ, sizeof(info), &info);
 	}
 
 	if (chan && !test_bit(CONF_REQ_SENT, &chan->conf_state) &&
@@ -3889,7 +3949,7 @@ static inline int l2cap_information_rsp(struct l2cap_conn *conn, struct l2cap_cm
 			conn->info_state & L2CAP_INFO_FEAT_MASK_REQ_DONE)
 		return 0;
 
-	cancel_delayed_work(&conn->info_timer);
+	l2cap_conn_clear_timer(conn, &conn->info_timer);
 
 	if (result != L2CAP_IR_SUCCESS) {
 		conn->info_state |= L2CAP_INFO_FEAT_MASK_REQ_DONE;
@@ -5277,8 +5337,10 @@ static void l2cap_recv_frame(struct l2cap_conn *conn, struct sk_buff *skb)
 		break;
 
 	case L2CAP_CID_SMP:
-		if (smp_sig_channel(conn, skb))
-			l2cap_conn_del(conn->hcon, EACCES);
+		if (smp_sig_channel(conn, skb)) {
+			l2cap_conn_clear_timer(conn, &conn->security_timer);
+			l2cap_conn_put(conn);
+		}
 		break;
 
 	default:
@@ -5330,8 +5392,14 @@ int l2cap_connect_cfm(struct hci_conn *hcon, u8 status)
 		conn = l2cap_conn_add(hcon, status);
 		if (conn)
 			l2cap_conn_ready(conn);
-	} else
-		l2cap_conn_del(hcon, bt_to_errno(status));
+	} else {
+		conn = hcon->l2cap_data;
+
+		if (hcon->type == LE_LINK)
+			l2cap_conn_clear_timer(conn, &conn->security_timer);
+
+		l2cap_conn_put(conn);
+	}
 
 	return 0;
 }
@@ -5349,9 +5417,14 @@ int l2cap_disconn_ind(struct hci_conn *hcon)
 
 int l2cap_disconn_cfm(struct hci_conn *hcon, u8 reason)
 {
+	struct l2cap_conn *conn = hcon->l2cap_data;
 	BT_DBG("hcon %p reason %d", hcon, reason);
 
-	l2cap_conn_del(hcon, bt_to_errno(reason));
+	if (hcon->type == LE_LINK)
+		l2cap_conn_clear_timer(conn, &conn->security_timer);
+
+	l2cap_conn_put(conn);
+
 	return 0;
 }
 
@@ -5381,10 +5454,13 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
 
 	BT_DBG("conn %p status 0x%2.2x encrypt %u", conn, status, encrypt);
 
+	l2cap_conn_get(conn);
+
 	if (hcon->type == LE_LINK) {
 		if (!status && encrypt)
 			smp_distribute_keys(conn, 0);
-		cancel_delayed_work(&conn->security_timer);
+
+		l2cap_conn_clear_timer(conn, &conn->security_timer);
 	}
 
 	mutex_lock(&conn->chan_lock);
@@ -5481,6 +5557,8 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
 
 	mutex_unlock(&conn->chan_lock);
 
+	l2cap_conn_put(conn);
+
 	return 0;
 }
 
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index 16ef0dc..5bb5b51 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -186,8 +186,7 @@ static void smp_send_cmd(struct l2cap_conn *conn, u8 code, u16 len, void *data)
 	skb->priority = HCI_PRIO_MAX;
 	hci_send_acl(conn->hchan, skb, 0);
 
-	cancel_delayed_work_sync(&conn->security_timer);
-	schedule_delayed_work(&conn->security_timer, SMP_TIMEOUT);
+	l2cap_conn_set_timer(conn, &conn->security_timer, SMP_TIMEOUT);
 }
 
 static __u8 authreq_to_seclevel(__u8 authreq)
@@ -268,7 +267,7 @@ static void smp_failure(struct l2cap_conn *conn, u8 reason, u8 send)
 			 hcon->dst_type, reason);
 
 	if (test_and_clear_bit(HCI_CONN_LE_SMP_PEND, &conn->hcon->flags)) {
-		cancel_delayed_work_sync(&conn->security_timer);
+		l2cap_conn_clear_timer(conn, &conn->security_timer);
 		smp_chan_destroy(conn);
 	}
 }
@@ -999,7 +998,7 @@ int smp_distribute_keys(struct l2cap_conn *conn, __u8 force)
 
 	if (conn->hcon->out || force) {
 		clear_bit(HCI_CONN_LE_SMP_PEND, &conn->hcon->flags);
-		cancel_delayed_work_sync(&conn->security_timer);
+		l2cap_conn_clear_timer(conn, &conn->security_timer);
 		smp_chan_destroy(conn);
 	}
 
-- 
1.7.9.5

Re: [PATCHv1 1/2] Bluetooth: Add refcnt to l2cap_conn

[Gustavo Padovan]

Hi Andrei,

* Andrei Emeltchenko <Andrei.Emeltchenko.news@gmail.com> [2012-07-13 15:07:45 +0300]:

> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> 
> Adding kref to l2cap_conn helps to better handle racing when deleting
> l2cap_conn. Races are created when deleting conn from timeout and from
> the other execution path.
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  include/net/bluetooth/l2cap.h |    6 ++
>  net/bluetooth/l2cap_core.c    |  126 +++++++++++++++++++++++++++++++++--------
>  net/bluetooth/smp.c           |    7 +--
>  3 files changed, 111 insertions(+), 28 deletions(-)
> 
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index e5164ff..e8b65ae 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -572,6 +572,8 @@ struct l2cap_conn {
>  
>  	struct list_head	chan_l;
>  	struct mutex		chan_lock;
> +
> +	struct kref		kref;
>  };
>  
>  #define L2CAP_INFO_CL_MTU_REQ_SENT	0x01
> @@ -781,5 +783,9 @@ void l2cap_chan_set_defaults(struct l2cap_chan *chan);
>  int l2cap_ertm_init(struct l2cap_chan *chan);
>  void l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan);
>  void l2cap_chan_del(struct l2cap_chan *chan, int err);
> +void l2cap_conn_set_timer(struct l2cap_conn *conn, struct delayed_work *work,
> +			  long timeout);
> +bool l2cap_conn_clear_timer(struct l2cap_conn *conn,
> +			    struct delayed_work *work);
>  
>  #endif /* __L2CAP_H */
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 9fd0599..773500b 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -57,6 +57,68 @@ static void l2cap_send_disconn_req(struct l2cap_conn *conn,
>  
>  static void l2cap_tx(struct l2cap_chan *chan, struct l2cap_ctrl *control,
>  		    struct sk_buff_head *skbs, u8 event);
> +static void l2cap_conn_del(struct hci_conn *hcon, int err);
> +
> +/* ---- L2CAP connections ---- */
> +
> +static void l2cap_conn_get(struct l2cap_conn *conn)
> +{
> +	BT_DBG("conn %p orig refcnt %d", conn,
> +	       atomic_read(&conn->kref.refcount));
> +
> +	kref_get(&conn->kref);
> +}
> +
> +static void l2cap_conn_destroy(struct kref *kref)
> +{
> +	struct l2cap_conn *conn = container_of(kref, struct l2cap_conn, kref);
> +
> +	BT_DBG("conn %p", conn);
> +
> +	l2cap_conn_del(conn->hcon, 0);
> +}
> +
> +static int l2cap_conn_put(struct l2cap_conn *conn)
> +{
> +	/* conn might be NULL, was checked before in l2cap_conn_del */
> +	if (!conn) {
> +		BT_DBG("conn == NULL");
> +		return 1;

1? What does that mean?

> +	}
> +
> +	BT_DBG("conn %p orig refcnt %d", conn,
> +	       atomic_read(&conn->kref.refcount));
> +
> +	return kref_put(&conn->kref, &l2cap_conn_destroy);
> +}
> +
> +void l2cap_conn_set_timer(struct l2cap_conn *conn, struct delayed_work *work,
> +			  long timeout)
> +{
> +	BT_DBG("conn %p timeout %ld", conn, timeout);
> +
> +	/* If delayed work cancelled do not hold(conn)
> +	   since it is already done with previous set_timer */
> +	if (!cancel_delayed_work(work))
> +		l2cap_conn_get(conn);
> +
> +	schedule_delayed_work(work, timeout);
> +}
> +
> +bool l2cap_conn_clear_timer(struct l2cap_conn *conn, struct delayed_work *work)
> +{
> +	bool ret;
> +
> +	BT_DBG("conn %p", conn);
> +
> +	/* put(conn) if delayed work cancelled otherwise it
> +	   is done in delayed work function */
> +	ret = cancel_delayed_work(work);
> +	if (ret)
> +		l2cap_conn_put(conn);
> +
> +	return ret;
> +}
>  
>  /* ---- L2CAP channels ---- */
>  
> @@ -976,7 +1038,8 @@ static void l2cap_do_start(struct l2cap_chan *chan)
>  		conn->info_state |= L2CAP_INFO_FEAT_MASK_REQ_SENT;
>  		conn->info_ident = l2cap_get_ident(conn);
>  
> -		schedule_delayed_work(&conn->info_timer, L2CAP_INFO_TIMEOUT);
> +		l2cap_conn_set_timer(conn, &conn->info_timer,
> +				     L2CAP_INFO_TIMEOUT);
>  
>  		l2cap_send_cmd(conn, conn->info_ident,
>  					L2CAP_INFO_REQ, sizeof(req), &req);
> @@ -1258,12 +1321,14 @@ static void l2cap_conn_unreliable(struct l2cap_conn *conn, int err)
>  static void l2cap_info_timeout(struct work_struct *work)
>  {
>  	struct l2cap_conn *conn = container_of(work, struct l2cap_conn,
> -							info_timer.work);
> +					       info_timer.work);
>  
>  	conn->info_state |= L2CAP_INFO_FEAT_MASK_REQ_DONE;
>  	conn->info_ident = 0;
>  
>  	l2cap_conn_start(conn);
> +
> +	l2cap_conn_put(conn);
>  }
>  
>  static void l2cap_conn_del(struct hci_conn *hcon, int err)
> @@ -1297,13 +1362,8 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
>  
>  	hci_chan_del(conn->hchan);
>  
> -	if (conn->info_state & L2CAP_INFO_FEAT_MASK_REQ_SENT)
> -		cancel_delayed_work_sync(&conn->info_timer);
> -
> -	if (test_and_clear_bit(HCI_CONN_LE_SMP_PEND, &hcon->flags)) {
> -		cancel_delayed_work_sync(&conn->security_timer);
> +	if (test_and_clear_bit(HCI_CONN_LE_SMP_PEND, &hcon->flags))

So where are you cleaning these timers? I don't see it. You are change the
code flow here.

>  		smp_chan_destroy(conn);
> -	}
>  
>  	hcon->l2cap_data = NULL;
>  	kfree(conn);
> @@ -1312,14 +1372,11 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
>  static void security_timeout(struct work_struct *work)
>  {
>  	struct l2cap_conn *conn = container_of(work, struct l2cap_conn,
> -						security_timer.work);
> +					       security_timer.work);
>  
>  	BT_DBG("conn %p", conn);
>  
> -	if (test_and_clear_bit(HCI_CONN_LE_SMP_PEND, &conn->hcon->flags)) {
> -		smp_chan_destroy(conn);
> -		l2cap_conn_del(conn->hcon, ETIMEDOUT);
> -	}
> +	l2cap_conn_put(conn);

You are loosing the error code here (and in some other places in the this
patch). We need to carry the error code so we can do proper error report in
teardown()

>  }
>  
>  static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon, u8 status)
> @@ -1361,6 +1418,8 @@ static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon, u8 status)
>  
>  	INIT_LIST_HEAD(&conn->chan_l);
>  
> +	kref_init(&conn->kref);
> +
>  	if (hcon->type == LE_LINK)
>  		INIT_DELAYED_WORK(&conn->security_timer, security_timeout);
>  	else
> @@ -3318,8 +3377,8 @@ static inline int l2cap_command_rej(struct l2cap_conn *conn, struct l2cap_cmd_hd
>  		return 0;
>  
>  	if ((conn->info_state & L2CAP_INFO_FEAT_MASK_REQ_SENT) &&
> -					cmd->ident == conn->info_ident) {
> -		cancel_delayed_work(&conn->info_timer);
> +	    cmd->ident == conn->info_ident) {
> +		l2cap_conn_clear_timer(conn, &conn->info_timer);
>  
>  		conn->info_state |= L2CAP_INFO_FEAT_MASK_REQ_DONE;
>  		conn->info_ident = 0;
> @@ -3433,10 +3492,11 @@ sendresp:
>  		conn->info_state |= L2CAP_INFO_FEAT_MASK_REQ_SENT;
>  		conn->info_ident = l2cap_get_ident(conn);
>  
> -		schedule_delayed_work(&conn->info_timer, L2CAP_INFO_TIMEOUT);
> +		l2cap_conn_set_timer(conn, &conn->info_timer,
> +				     L2CAP_INFO_TIMEOUT);
>  
>  		l2cap_send_cmd(conn, conn->info_ident,
> -					L2CAP_INFO_REQ, sizeof(info), &info);
> +			       L2CAP_INFO_REQ, sizeof(info), &info);
>  	}
>  
>  	if (chan && !test_bit(CONF_REQ_SENT, &chan->conf_state) &&
> @@ -3889,7 +3949,7 @@ static inline int l2cap_information_rsp(struct l2cap_conn *conn, struct l2cap_cm
>  			conn->info_state & L2CAP_INFO_FEAT_MASK_REQ_DONE)
>  		return 0;
>  
> -	cancel_delayed_work(&conn->info_timer);
> +	l2cap_conn_clear_timer(conn, &conn->info_timer);
>  
>  	if (result != L2CAP_IR_SUCCESS) {
>  		conn->info_state |= L2CAP_INFO_FEAT_MASK_REQ_DONE;
> @@ -5277,8 +5337,10 @@ static void l2cap_recv_frame(struct l2cap_conn *conn, struct sk_buff *skb)
>  		break;
>  
>  	case L2CAP_CID_SMP:
> -		if (smp_sig_channel(conn, skb))
> -			l2cap_conn_del(conn->hcon, EACCES);
> +		if (smp_sig_channel(conn, skb)) {
> +			l2cap_conn_clear_timer(conn, &conn->security_timer);
> +			l2cap_conn_put(conn);
> +		}
>  		break;
>  
>  	default:
> @@ -5330,8 +5392,14 @@ int l2cap_connect_cfm(struct hci_conn *hcon, u8 status)
>  		conn = l2cap_conn_add(hcon, status);
>  		if (conn)
>  			l2cap_conn_ready(conn);
> -	} else
> -		l2cap_conn_del(hcon, bt_to_errno(status));
> +	} else {
> +		conn = hcon->l2cap_data;
> +
> +		if (hcon->type == LE_LINK)
> +			l2cap_conn_clear_timer(conn, &conn->security_timer);

Can you explain why this change is here?

	Gustavo

Re: [PATCHv1 1/2] Bluetooth: Add refcnt to l2cap_conn

[Andrei Emeltchenko]

Hi Gustavo,

On Tue, Jul 24, 2012 at 08:26:18PM -0300, Gustavo Padovan wrote:
...
> > +static int l2cap_conn_put(struct l2cap_conn *conn)
> > +{
> > +	/* conn might be NULL, was checked before in l2cap_conn_del */
> > +	if (!conn) {
> > +		BT_DBG("conn == NULL");
> > +		return 1;
> 
> 1? What does that mean?

this would mean that conn == NULL but I will make this function void.

> > +	}
> > +
> > +	BT_DBG("conn %p orig refcnt %d", conn,
> > +	       atomic_read(&conn->kref.refcount));
> > +
> > +	return kref_put(&conn->kref, &l2cap_conn_destroy);
> > +}
> > +
> > +void l2cap_conn_set_timer(struct l2cap_conn *conn, struct delayed_work *work,
> > +			  long timeout)
> > +{
> > +	BT_DBG("conn %p timeout %ld", conn, timeout);
> > +
> > +	/* If delayed work cancelled do not hold(conn)
> > +	   since it is already done with previous set_timer */
> > +	if (!cancel_delayed_work(work))
> > +		l2cap_conn_get(conn);
> > +
> > +	schedule_delayed_work(work, timeout);
> > +}
> > +
> > +bool l2cap_conn_clear_timer(struct l2cap_conn *conn, struct delayed_work *work)
> > +{
> > +	bool ret;
> > +
> > +	BT_DBG("conn %p", conn);
> > +
> > +	/* put(conn) if delayed work cancelled otherwise it
> > +	   is done in delayed work function */
> > +	ret = cancel_delayed_work(work);
> > +	if (ret)
> > +		l2cap_conn_put(conn);
> > +
> > +	return ret;
> > +}
> >  
> >  /* ---- L2CAP channels ---- */
> >  
> > @@ -976,7 +1038,8 @@ static void l2cap_do_start(struct l2cap_chan *chan)
> >  		conn->info_state |= L2CAP_INFO_FEAT_MASK_REQ_SENT;
> >  		conn->info_ident = l2cap_get_ident(conn);
> >  
> > -		schedule_delayed_work(&conn->info_timer, L2CAP_INFO_TIMEOUT);
> > +		l2cap_conn_set_timer(conn, &conn->info_timer,
> > +				     L2CAP_INFO_TIMEOUT);
> >  
> >  		l2cap_send_cmd(conn, conn->info_ident,
> >  					L2CAP_INFO_REQ, sizeof(req), &req);
> > @@ -1258,12 +1321,14 @@ static void l2cap_conn_unreliable(struct l2cap_conn *conn, int err)
> >  static void l2cap_info_timeout(struct work_struct *work)
> >  {
> >  	struct l2cap_conn *conn = container_of(work, struct l2cap_conn,
> > -							info_timer.work);
> > +					       info_timer.work);
> >  
> >  	conn->info_state |= L2CAP_INFO_FEAT_MASK_REQ_DONE;
> >  	conn->info_ident = 0;
> >  
> >  	l2cap_conn_start(conn);
> > +
> > +	l2cap_conn_put(conn);
> >  }
> >  
> >  static void l2cap_conn_del(struct hci_conn *hcon, int err)
> > @@ -1297,13 +1362,8 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
> >  
> >  	hci_chan_del(conn->hchan);
> >  
> > -	if (conn->info_state & L2CAP_INFO_FEAT_MASK_REQ_SENT)
> > -		cancel_delayed_work_sync(&conn->info_timer);
> > -
> > -	if (test_and_clear_bit(HCI_CONN_LE_SMP_PEND, &hcon->flags)) {
> > -		cancel_delayed_work_sync(&conn->security_timer);
> > +	if (test_and_clear_bit(HCI_CONN_LE_SMP_PEND, &hcon->flags))
> 
> So where are you cleaning these timers? I don't see it. You are change the
> code flow here.

Yes this change code flow. Those would be handled gracefully with refcounting.

> >  		smp_chan_destroy(conn);
> > -	}
> >  
> >  	hcon->l2cap_data = NULL;
> >  	kfree(conn);
> > @@ -1312,14 +1372,11 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
> >  static void security_timeout(struct work_struct *work)
> >  {
> >  	struct l2cap_conn *conn = container_of(work, struct l2cap_conn,
> > -						security_timer.work);
> > +					       security_timer.work);
> >  
> >  	BT_DBG("conn %p", conn);
> >  
> > -	if (test_and_clear_bit(HCI_CONN_LE_SMP_PEND, &conn->hcon->flags)) {
> > -		smp_chan_destroy(conn);
> > -		l2cap_conn_del(conn->hcon, ETIMEDOUT);
> > -	}
> > +	l2cap_conn_put(conn);
> 
> You are loosing the error code here (and in some other places in the this
> patch). We need to carry the error code so we can do proper error report in
> teardown()

Maybe we need to set error code somewhere here.

> >  }
> >  
> >  static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon, u8 status)
> > @@ -1361,6 +1418,8 @@ static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon, u8 status)
> >  
> >  	INIT_LIST_HEAD(&conn->chan_l);
> >  
> > +	kref_init(&conn->kref);
> > +`
> >  	if (hcon->type == LE_LINK)
> >  		INIT_DELAYED_WORK(&conn->security_timer, security_timeout);
> >  	else
> > @@ -3318,8 +3377,8 @@ static inline int l2cap_command_rej(struct l2cap_conn *conn, struct l2cap_cmd_hd
> >  		return 0;
> >  
> >  	if ((conn->info_state & L2CAP_INFO_FEAT_MASK_REQ_SENT) &&
> > -					cmd->ident == conn->info_ident) {
> > -		cancel_delayed_work(&conn->info_timer);
> > +	    cmd->ident == conn->info_ident) {
> > +		l2cap_conn_clear_timer(conn, &conn->info_timer);
> >  
> >  		conn->info_state |= L2CAP_INFO_FEAT_MASK_REQ_DONE;
> >  		conn->info_ident = 0;
> > @@ -3433,10 +3492,11 @@ sendresp:
> >  		conn->info_state |= L2CAP_INFO_FEAT_MASK_REQ_SENT;
> >  		conn->info_ident = l2cap_get_ident(conn);
> >  
> > -		schedule_delayed_work(&conn->info_timer, L2CAP_INFO_TIMEOUT);
> > +		l2cap_conn_set_timer(conn, &conn->info_timer,
> > +				     L2CAP_INFO_TIMEOUT);
> >  
> >  		l2cap_send_cmd(conn, conn->info_ident,
> > -					L2CAP_INFO_REQ, sizeof(info), &info);
> > +			       L2CAP_INFO_REQ, sizeof(info), &info);
> >  	}
> >  
> >  	if (chan && !test_bit(CONF_REQ_SENT, &chan->conf_state) &&
> > @@ -3889,7 +3949,7 @@ static inline int l2cap_information_rsp(struct l2cap_conn *conn, struct l2cap_cm
> >  			conn->info_state & L2CAP_INFO_FEAT_MASK_REQ_DONE)
> >  		return 0;
> >  
> > -	cancel_delayed_work(&conn->info_timer);
> > +	l2cap_conn_clear_timer(conn, &conn->info_timer);
> >  
> >  	if (result != L2CAP_IR_SUCCESS) {
> >  		conn->info_state |= L2CAP_INFO_FEAT_MASK_REQ_DONE;
> > @@ -5277,8 +5337,10 @@ static void l2cap_recv_frame(struct l2cap_conn *conn, struct sk_buff *skb)
> >  		break;
> >  
> >  	case L2CAP_CID_SMP:
> > -		if (smp_sig_channel(conn, skb))
> > -			l2cap_conn_del(conn->hcon, EACCES);
> > +		if (smp_sig_channel(conn, skb)) {
> > +			l2cap_conn_clear_timer(conn, &conn->security_timer);
> > +			l2cap_conn_put(conn);
> > +		}
> >  		break;
> >  
> >  	default:
> > @@ -5330,8 +5392,14 @@ int l2cap_connect_cfm(struct hci_conn *hcon, u8 status)
> >  		conn = l2cap_conn_add(hcon, status);
> >  		if (conn)
> >  			l2cap_conn_ready(conn);
> > -	} else
> > -		l2cap_conn_del(hcon, bt_to_errno(status));
> > +	} else {
> > +		conn = hcon->l2cap_data;
> > +
> > +		if (hcon->type == LE_LINK)
> > +			l2cap_conn_clear_timer(conn, &conn->security_timer);
> 
> Can you explain why this change is here?

IIRC this the result of changing l2cap_conn_del -> l2cap_conn_put

Best regards 
Andrei Emeltchenko 

[PATCHv1 2/2] Bluetooth: Lock l2cap chan in sockopt

[Andrei Emeltchenko]

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


Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 net/bluetooth/l2cap_sock.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index a4bb27e..373ce9c 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -275,6 +275,7 @@ static int l2cap_sock_getsockopt_old(struct socket *sock, int optname, char __us
 	if (get_user(len, optlen))
 		return -EFAULT;
 
+	l2cap_chan_lock(chan);
 	lock_sock(sk);
 
 	switch (optname) {
@@ -345,6 +346,8 @@ static int l2cap_sock_getsockopt_old(struct socket *sock, int optname, char __us
 	}
 
 	release_sock(sk);
+	l2cap_chan_unlock(chan);
+
 	return err;
 }
 
@@ -367,6 +370,7 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname, ch
 	if (get_user(len, optlen))
 		return -EFAULT;
 
+	l2cap_chan_lock(chan);
 	lock_sock(sk);
 
 	switch (optname) {
@@ -442,6 +446,8 @@ static int l2cap_sock_getsockopt(struct socket *sock, int level, int optname, ch
 	}
 
 	release_sock(sk);
+	l2cap_chan_unlock(chan);
+
 	return err;
 }
 
@@ -471,6 +477,7 @@ static int l2cap_sock_setsockopt_old(struct socket *sock, int optname, char __us
 
 	BT_DBG("sk %p", sk);
 
+	l2cap_chan_lock(chan);
 	lock_sock(sk);
 
 	switch (optname) {
@@ -556,6 +563,8 @@ static int l2cap_sock_setsockopt_old(struct socket *sock, int optname, char __us
 	}
 
 	release_sock(sk);
+	l2cap_chan_unlock(chan);
+
 	return err;
 }
 
@@ -577,6 +586,7 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname, ch
 	if (level != SOL_BLUETOOTH)
 		return -ENOPROTOOPT;
 
+	l2cap_chan_lock(chan);
 	lock_sock(sk);
 
 	switch (optname) {
@@ -729,6 +739,8 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname, ch
 	}
 
 	release_sock(sk);
+	l2cap_chan_unlock(chan);
+
 	return err;
 }
 
-- 
1.7.9.5

Re: [PATCHv1 2/2] Bluetooth: Lock l2cap chan in sockopt

[Gustavo Padovan]

* Andrei Emeltchenko <Andrei.Emeltchenko.news@gmail.com> [2012-07-13 15:07:46 +0300]:

> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> 
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/l2cap_sock.c |   12 ++++++++++++
>  1 file changed, 12 insertions(+)

where is the explanation of this patch? I'd like to know why it is needed.

	Gustavo

Re: [PATCHv1 2/2] Bluetooth: Lock l2cap chan in sockopt

[Andrei Emeltchenko]

On Tue, Jul 24, 2012 at 08:27:34PM -0300, Gustavo Padovan wrote:
> * Andrei Emeltchenko <Andrei.Emeltchenko.news@gmail.com> [2012-07-13 15:07:46 +0300]:
> 
> > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > 
> > 
> > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > ---
> >  net/bluetooth/l2cap_sock.c |   12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> 
> where is the explanation of this patch? I'd like to know why it is needed.

I think it may fix one my crash in l2cap_sock_getsockopt but I am not
completely sure (and do not remember).

[  745.735560] [5395] l2cap_sock_getsockopt: sk d8176800
[  745.746210] [5395] l2cap_sock_getsockopt_old: sk d8176800
[  745.760703] [5395] l2cap_sock_getsockopt: sk d8176800
[  745.767357] [5395] l2cap_sock_getsockopt_old: sk d8176800
[  745.772484] BUG: unable to handle kernel NULL pointer dereference at
(null)
[  745.774157] IP: [<f828dccc>] l2cap_sock_getsockopt+0x2ec/0x3f0
[bluetooth]
[  745.775077] *pde = 00000000 
[  745.776483] Oops: 0000 [#1] SMP 
[  745.776483] Modules linked in: bnep arc4 mac80211_hwsim mac80211
cfg80211 btusb bluetooth binfmt_misc snd_intel8x0 snd_ac97_codec ac97_bus
joydev ppdev snd_pcm snd_seq hid_generic psmouse snd_timer snd_seq_device
parport_pc snd serio_raw i2c_piix4 soundcore snd_page_alloc lp parport
usbhid hid ahci libahci e1000
[  745.776483] 
[  745.776483] Pid: 5395, comm: l2test Not tainted 3.5.0-rc1niko+ #123
innotek GmbH VirtualBox
[  745.776483] EIP: 0060:[<f828dccc>] EFLAGS: 00010246 CPU: 0
[  745.776483] EIP is at l2cap_sock_getsockopt+0x2ec/0x3f0 [bluetooth]
[  745.776483] EAX: 00000000 EBX: fffffff2 ECX: 00000006 EDX: d8176800
[  745.776483] ESI: d8176400 EDI: 00000002 EBP: e3ef3f48 ESP: e3ef3f04
[  745.776483]  DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
[  745.776483] CR0: 8005003b CR2: 00000000 CR3: 26a5d000 CR4: 000006d0
[  745.776483] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
[  745.776483] DR6: ffff0ff0 DR7: 00000400
[  745.776483] Process l2test (pid: 5395, ti=e3ef2000 task=df4c0000
task.ti=e3ef2000)
[  745.776483] Stack:
[  745.776483]  f82a04a0 f82997aa d8176800 00000000 00000006 d8176800
00000006 00000246
[  745.776483]  00000246 c1119cd4 00003f58 00000000 00000006 e3ef3f48
f8297960 ddb66780
[  745.776483]  00000006 e3ef3f6c c14815fb bfc9944e bfc99454 00000000
00000000 0000000f
[  745.776483] Call Trace:
[  745.776483]  [<c1119cd4>] ? might_fault+0x54/0xb0
[  745.776483]  [<c14815fb>] sys_getsockopt+0x5b/0xc0
[  745.776483]  [<c1481d99>] sys_socketcall+0x269/0x2e0
[  745.776483]  [<c12c0948>] ? trace_hardirqs_on_thunk+0xc/0x10
[  745.776483]  [<c15833df>] sysenter_do_call+0x12/0x38
[  745.776483] Code: 00 00 a8 01 0f 84 6c ff ff ff c7 44 24 2a 00 00 00 00
b9 06 00 00 00 66 c7 44 24 2e 00 00 8b 46 04 83 7c 24 18 06 0f 46 4c 24 18
<8b> 10 0f b7 52 14 66 89 54 24 2a 8b 00 0f b7 50 1c 0f b6 40 1e 
[  745.776483] EIP: [<f828dccc>] l2cap_sock_getsockopt+0x2ec/0x3f0
[bluetooth] SS:ESP 0068:e3ef3f04
[  745.776483] CR2: 0000000000000000
[  745.856959] ---[ end trace 7fb1d28495fb8a81 ]---
[  745.956316] [5395] l2cap_sock_release: sock ddb66780, sk d8176800
[  745.964074] [5395] l2cap_sock_shutdown: sock ddb66780, sk d8176800

Best regards 
Andrei Emeltchenko