[PATCHv2] Bluetooth: Add refcnt to l2cap_conn

[Andrei Emeltchenko <Andrei.Emeltchenko.news@gmail.com>]

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 3d31ecd..5abfe8a 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
@@ -779,5 +781,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 7aa862d..ee6ca78 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 ---- */
 
@@ -997,7 +1061,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);
@@ -1279,12 +1344,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)
@@ -1318,13 +1385,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);
@@ -1333,14 +1395,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)
@@ -1382,6 +1441,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
@@ -3337,8 +3398,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;
@@ -3452,10 +3513,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) &&
@@ -3908,7 +3970,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;
@@ -5291,8 +5353,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:
@@ -5344,8 +5408,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;
 }
@@ -5363,9 +5433,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;
 }
 
@@ -5395,10 +5470,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);
@@ -5497,6 +5575,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