[RFC] Bluetooth: Add refcnt to l2cap_conn

[RFC] 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.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 include/net/bluetooth/l2cap.h |    6 +++
 net/bluetooth/l2cap_core.c    |  101 ++++++++++++++++++++++++++++++++++-------
 net/bluetooth/smp.c           |    7 ++-
 3 files changed, 94 insertions(+), 20 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 eb7094c..b0943f7 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -57,6 +57,64 @@ 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", conn);
+
+	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)
+{
+	BT_DBG("conn %p", conn);
+
+	/* conn might be NULL, was checked before in l2cap_conn_del */
+	if (!conn)
+		return 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 ---- */
 
@@ -1321,11 +1379,6 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
 	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);
-		smp_chan_destroy(conn);
-	}
-
 	hcon->l2cap_data = NULL;
 	kfree(conn);
 }
@@ -1333,9 +1386,9 @@ 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);
 
-	l2cap_conn_del(conn->hcon, ETIMEDOUT);
+	l2cap_conn_put(conn);
 }
 
 static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon, u8 status)
@@ -1377,10 +1430,14 @@ static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon, u8 status)
 
 	INIT_LIST_HEAD(&conn->chan_l);
 
-	if (hcon->type == LE_LINK)
+	kref_init(&conn->kref);
+
+	if (hcon->type == LE_LINK) {
+		l2cap_conn_get(conn);
 		INIT_DELAYED_WORK(&conn->security_timer, security_timeout);
-	else
+	} else {
 		INIT_DELAYED_WORK(&conn->info_timer, l2cap_info_timeout);
+	}
 
 	conn->disc_reason = HCI_ERROR_REMOTE_USER_TERM;
 
@@ -5286,8 +5343,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:
@@ -5331,7 +5390,7 @@ int l2cap_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr)
 
 int l2cap_connect_cfm(struct hci_conn *hcon, u8 status)
 {
-	struct l2cap_conn *conn;
+	struct l2cap_conn *conn = hcon->l2cap_data;
 
 	BT_DBG("hcon %p bdaddr %s status %d", hcon, batostr(&hcon->dst), status);
 
@@ -5339,8 +5398,12 @@ 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 {
+		if (hcon->type == LE_LINK)
+			l2cap_conn_clear_timer(conn, &conn->security_timer);
+
+		l2cap_conn_put(conn);
+	}
 
 	return 0;
 }
@@ -5358,9 +5421,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;
 }
 
@@ -5393,7 +5461,8 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
 	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);
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index ff4835b..7aaf849 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);
 	}
 }
@@ -996,7 +995,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

[PATCHv1] 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>
---
	PATCHv1: fixes issues with LE

 include/net/bluetooth/l2cap.h |    6 +++
 net/bluetooth/l2cap_core.c    |   94 +++++++++++++++++++++++++++++++++++------
 net/bluetooth/smp.c           |    7 ++-
 3 files changed, 91 insertions(+), 16 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 eb7094c..d8c320b 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -57,6 +57,64 @@ 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", conn);
+
+	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)
+{
+	BT_DBG("conn %p", conn);
+
+	/* conn might be NULL, was checked before in l2cap_conn_del */
+	if (!conn)
+		return 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 ---- */
 
@@ -1321,10 +1379,8 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
 	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,9 +1389,9 @@ 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);
 
-	l2cap_conn_del(conn->hcon, ETIMEDOUT);
+	l2cap_conn_put(conn);
 }
 
 static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon, u8 status)
@@ -1377,6 +1433,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
@@ -5286,8 +5344,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:
@@ -5331,7 +5391,7 @@ int l2cap_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr)
 
 int l2cap_connect_cfm(struct hci_conn *hcon, u8 status)
 {
-	struct l2cap_conn *conn;
+	struct l2cap_conn *conn = hcon->l2cap_data;
 
 	BT_DBG("hcon %p bdaddr %s status %d", hcon, batostr(&hcon->dst), status);
 
@@ -5339,8 +5399,12 @@ 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 {
+		if (hcon->type == LE_LINK)
+			l2cap_conn_clear_timer(conn, &conn->security_timer);
+
+		l2cap_conn_put(conn);
+	}
 
 	return 0;
 }
@@ -5358,9 +5422,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;
 }
 
@@ -5393,7 +5462,8 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
 	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);
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index ff4835b..7aaf849 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);
 	}
 }
@@ -996,7 +995,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] Bluetooth: Add refcnt to l2cap_conn

[Andrei Emeltchenko]

Any thoughts about this patch?

On Thu, Jun 07, 2012 at 10:04:57AM +0300, Andrei Emeltchenko wrote:
> 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>
> ---
> 	PATCHv1: fixes issues with LE
> 
>  include/net/bluetooth/l2cap.h |    6 +++
>  net/bluetooth/l2cap_core.c    |   94 +++++++++++++++++++++++++++++++++++------
>  net/bluetooth/smp.c           |    7 ++-
>  3 files changed, 91 insertions(+), 16 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 eb7094c..d8c320b 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -57,6 +57,64 @@ 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", conn);
> +
> +	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)
> +{
> +	BT_DBG("conn %p", conn);
> +
> +	/* conn might be NULL, was checked before in l2cap_conn_del */
> +	if (!conn)
> +		return 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 ---- */
>  
> @@ -1321,10 +1379,8 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
>  	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,9 +1389,9 @@ 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);
>  
> -	l2cap_conn_del(conn->hcon, ETIMEDOUT);
> +	l2cap_conn_put(conn);
>  }
>  
>  static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon, u8 status)
> @@ -1377,6 +1433,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
> @@ -5286,8 +5344,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:
> @@ -5331,7 +5391,7 @@ int l2cap_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr)
>  
>  int l2cap_connect_cfm(struct hci_conn *hcon, u8 status)
>  {
> -	struct l2cap_conn *conn;
> +	struct l2cap_conn *conn = hcon->l2cap_data;
>  
>  	BT_DBG("hcon %p bdaddr %s status %d", hcon, batostr(&hcon->dst), status);
>  
> @@ -5339,8 +5399,12 @@ 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 {
> +		if (hcon->type == LE_LINK)
> +			l2cap_conn_clear_timer(conn, &conn->security_timer);
> +
> +		l2cap_conn_put(conn);
> +	}
>  
>  	return 0;
>  }
> @@ -5358,9 +5422,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;
>  }
>  
> @@ -5393,7 +5462,8 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
>  	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);
> diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
> index ff4835b..7aaf849 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);
>  	}
>  }
> @@ -996,7 +995,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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv1] Bluetooth: Add refcnt to l2cap_conn

[Mat Martineau]

Andrei -

On Thu, 7 Jun 2012, Andrei Emeltchenko wrote:

> 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>
> ---
> 	PATCHv1: fixes issues with LE
>
> include/net/bluetooth/l2cap.h |    6 +++
> net/bluetooth/l2cap_core.c    |   94 +++++++++++++++++++++++++++++++++++------
> net/bluetooth/smp.c           |    7 ++-
> 3 files changed, 91 insertions(+), 16 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 eb7094c..d8c320b 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -57,6 +57,64 @@ 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", conn);
> +
> +	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)
> +{
> +	BT_DBG("conn %p", conn);
> +
> +	/* conn might be NULL, was checked before in l2cap_conn_del */
> +	if (!conn)
> +		return 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 ---- */
>
> @@ -1321,10 +1379,8 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
> 	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,9 +1389,9 @@ 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);
>
> -	l2cap_conn_del(conn->hcon, ETIMEDOUT);
> +	l2cap_conn_put(conn);
> }
>
> static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon, u8 status)
> @@ -1377,6 +1433,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
> @@ -5286,8 +5344,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:
> @@ -5331,7 +5391,7 @@ int l2cap_connect_ind(struct hci_dev *hdev, bdaddr_t *bdaddr)
>
> int l2cap_connect_cfm(struct hci_conn *hcon, u8 status)
> {
> -	struct l2cap_conn *conn;
> +	struct l2cap_conn *conn = hcon->l2cap_data;
>
> 	BT_DBG("hcon %p bdaddr %s status %d", hcon, batostr(&hcon->dst), status);
>
> @@ -5339,8 +5399,12 @@ 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 {
> +		if (hcon->type == LE_LINK)
> +			l2cap_conn_clear_timer(conn, &conn->security_timer);
> +
> +		l2cap_conn_put(conn);
> +	}
>
> 	return 0;
> }
> @@ -5358,9 +5422,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;
> }
>
> @@ -5393,7 +5462,8 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
> 	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);
> diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
> index ff4835b..7aaf849 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);
> 	}
> }
> @@ -996,7 +995,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

I think it's a very good thing to add reference counting, thanks for 
doing this work.

I'd like to see the code be more strict about matching the gets & puts 
up with actual assignments to l2cap_conn pointers:

  * Make sure one reference count is held for hcon->l2cap_data (your 
patch already does this)
  * l2cap_conn_put when setting hcon->l2cap_data to NULL
  * l2cap_conn_get when assigning l2cap_chan->conn
  * l2cap_conn_put when setting l2cap_chan->conn to NULL or freeing 
l2cap_chan
  * Use l2cap_conn_get/put whenever there's a reference to an 
l2cap_conn struct in an active timer.  You set this up for the 
security_timer, but info_timer could use it too.

Right now, l2cap_conn_del is only called when the reference count goes 
to 0.  If l2cap_conn reference counts are added to l2cap_chan, then 
the reference count wouldn't be 0 if the ACL was forced to close, and 
the l2cap_chans would not get cleaned up.  I think it would help to 
make l2cap_conn_del clean up the channels and connection state, and 
then have l2cap_conn_destroy free the structure.  l2cap_conn_destroy 
would only have to call l2cap_conn_del if it hadn't been done yet.


Regards,

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

[PATCHv2] 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 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

Re: [PATCHv2] Bluetooth: Add refcnt to l2cap_conn

[Mat Martineau]

Hi Andrei -

On Thu, 21 Jun 2012, Andrei Emeltchenko wrote:

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

This v2 patch adds reference counting for the info timer.  Are there 
any other changes compared to v1?

I'm still concerned that this "reference counting" does not count 
every reference.  It only counts references used by the timers and in 
l2cap_security_cfm.  This is a fragile approach - as the code evolves, 
it's not clear what the rules are for using reference counting with 
l2cap_conn.  I think the most maintainable and robust approach is to 
make the rule "Count every reference" (including those in hci_conn and 
l2cap_chan).  What rules do you think are best for reference counting 
in this case?  It might be good to include that information in the 
commit message or comments.

Regards,

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [PATCHv2] Bluetooth: Add refcnt to l2cap_conn

[Andrei Emeltchenko]

Hi Mat,

On Fri, Jun 22, 2012 at 03:09:08PM -0700, Mat Martineau wrote:
> >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(-)
> >
> 
> This v2 patch adds reference counting for the info timer.  Are there
> any other changes compared to v1?
> 
> I'm still concerned that this "reference counting" does not count
> every reference.  It only counts references used by the timers and
> in l2cap_security_cfm.  This is a fragile approach - as the code
> evolves, it's not clear what the rules are for using reference
> counting with l2cap_conn.  I think the most maintainable and robust
> approach is to make the rule "Count every reference" (including
> those in hci_conn and l2cap_chan).  

Yes this should actually evolve to this.

> What rules do you think are best
> for reference counting in this case?  It might be good to include
> that information in the commit message or comments.

I am thinking to add also reference counting for each l2cap_chan created
in l2cap_conn.

Best regards 
Andrei Emeltchenko