From: Andrei Emeltchenko <[email protected]>
Adding kref to l2cap_conn helps to better handle racing when deleting
l2cap_conn.
Signed-off-by: Andrei Emeltchenko <[email protected]>
---
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
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 <[email protected]>
> >---
> >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
Hi Andrei -
On Thu, 21 Jun 2012, Andrei Emeltchenko wrote:
> From: Andrei Emeltchenko <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
From: Andrei Emeltchenko <[email protected]>
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 <[email protected]>
---
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
Andrei -
On Thu, 7 Jun 2012, Andrei Emeltchenko wrote:
> From: Andrei Emeltchenko <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
Any thoughts about this patch?
On Thu, Jun 07, 2012 at 10:04:57AM +0300, Andrei Emeltchenko wrote:
> From: Andrei Emeltchenko <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Andrei Emeltchenko <[email protected]>
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 <[email protected]>
---
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