* [PATCH 1/4] Bluetooth: Remove rfcomm session refcnt
2012-08-11 18:47 [PATCH 0/4] Bluetooth: Details of rfcomm fixes - remove session refcnt Dean Jenkins
@ 2012-08-11 18:47 ` Dean Jenkins
2012-08-11 18:47 ` [PATCH 2/4] Bluetooth: Return rfcomm session pointers to avoid freed session Dean Jenkins
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: Dean Jenkins @ 2012-08-11 18:47 UTC (permalink / raw)
To: linux-bluetooth
The rfcomm session refcnt is unworkable as it is hard
to predict the program flow during abnormal protocol conditions
so is easy for the refcnt to be out-of-sync. In addition, high
processor loading can cause the run-time thread order to change
resulting in malfunctioning of the session refcnt eg. premature
session deletion or double session deletion resulting in
kernel crashes.
Therefore, rely on the rfcomm session state machine to absolutely
delete the rfcomm session at the right time. The rfcomm session
state machine is controlled by the DLC data channel connection
states. The rfcomm session is created when a DLC data channel is
required. The rfcomm session is deleted when all DLC data channels
are closed or in abnormal conditions when the socket is BT_CLOSED.
There are 4 connection / disconnection rfcomm session scenarios:
host initiated: host disconnected
host initiated: remote disconnected
remote initiated: host disconnected
remote initiated: remote disconnected
The connection procedures are independent of the disconnection
procedures. Strangely, the session refcnt was applying special
treatment so erroneously combining connection and disconnection
events.
Signed-off-by: Dean Jenkins <djenkins@mvista.com>
---
include/net/bluetooth/rfcomm.h | 6 -----
net/bluetooth/rfcomm/core.c | 56 +++++-----------------------------------
2 files changed, 6 insertions(+), 56 deletions(-)
diff --git a/include/net/bluetooth/rfcomm.h b/include/net/bluetooth/rfcomm.h
index e2e3eca..7afd419 100644
--- a/include/net/bluetooth/rfcomm.h
+++ b/include/net/bluetooth/rfcomm.h
@@ -158,7 +158,6 @@ struct rfcomm_session {
struct timer_list timer;
unsigned long state;
unsigned long flags;
- atomic_t refcnt;
int initiator;
/* Default DLC parameters */
@@ -276,11 +275,6 @@ static inline void rfcomm_dlc_unthrottle(struct rfcomm_dlc *d)
void rfcomm_session_getaddr(struct rfcomm_session *s, bdaddr_t *src,
bdaddr_t *dst);
-static inline void rfcomm_session_hold(struct rfcomm_session *s)
-{
- atomic_inc(&s->refcnt);
-}
-
/* ---- RFCOMM sockets ---- */
struct sockaddr_rc {
sa_family_t rc_family;
diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
index 8a60238..b0805c1 100644
--- a/net/bluetooth/rfcomm/core.c
+++ b/net/bluetooth/rfcomm/core.c
@@ -122,12 +122,6 @@ static inline void rfcomm_schedule(void)
wake_up_process(rfcomm_thread);
}
-static inline void rfcomm_session_put(struct rfcomm_session *s)
-{
- if (atomic_dec_and_test(&s->refcnt))
- rfcomm_session_del(s);
-}
-
/* ---- RFCOMM FCS computation ---- */
/* reversed, 8-bit, poly=0x07 */
@@ -263,16 +257,15 @@ static void rfcomm_session_set_timer(struct rfcomm_session *s, long timeout)
{
BT_DBG("session %p state %ld timeout %ld", s, s->state, timeout);
- if (!mod_timer(&s->timer, jiffies + timeout))
- rfcomm_session_hold(s);
+ mod_timer(&s->timer, jiffies + timeout);
}
static void rfcomm_session_clear_timer(struct rfcomm_session *s)
{
BT_DBG("session %p state %ld", s, s->state);
- if (timer_pending(&s->timer) && del_timer(&s->timer))
- rfcomm_session_put(s);
+ if (timer_pending(&s->timer))
+ del_timer(&s->timer);
}
/* ---- RFCOMM DLCs ---- */
@@ -350,8 +343,6 @@ static void rfcomm_dlc_link(struct rfcomm_session *s, struct rfcomm_dlc *d)
{
BT_DBG("dlc %p session %p", d, s);
- rfcomm_session_hold(s);
-
rfcomm_session_clear_timer(s);
rfcomm_dlc_hold(d);
list_add(&d->list, &s->dlcs);
@@ -370,8 +361,6 @@ static void rfcomm_dlc_unlink(struct rfcomm_dlc *d)
if (list_empty(&s->dlcs))
rfcomm_session_set_timer(s, RFCOMM_IDLE_TIMEOUT);
-
- rfcomm_session_put(s);
}
static struct rfcomm_dlc *rfcomm_dlc_get(struct rfcomm_session *s, u8 dlci)
@@ -631,9 +620,6 @@ static void rfcomm_session_del(struct rfcomm_session *s)
list_del(&s->list);
- if (state == BT_CONNECTED)
- rfcomm_send_disc(s, 0);
-
rfcomm_session_clear_timer(s);
sock_release(s->sock);
kfree(s);
@@ -665,8 +651,6 @@ static void rfcomm_session_close(struct rfcomm_session *s, int err)
BT_DBG("session %p state %ld err %d", s, s->state, err);
- rfcomm_session_hold(s);
-
s->state = BT_CLOSED;
/* Close all dlcs */
@@ -676,8 +660,7 @@ static void rfcomm_session_close(struct rfcomm_session *s, int err)
__rfcomm_dlc_close(d, err);
}
- rfcomm_session_clear_timer(s);
- rfcomm_session_put(s);
+ rfcomm_session_del(s);
}
static struct rfcomm_session *rfcomm_session_create(bdaddr_t *src,
@@ -1164,18 +1147,7 @@ static int rfcomm_recv_ua(struct rfcomm_session *s, u8 dlci)
break;
case BT_DISCONN:
- /* rfcomm_session_put is called later so don't do
- * anything here otherwise we will mess up the session
- * reference counter:
- *
- * (a) when we are the initiator dlc_unlink will drive
- * the reference counter to 0 (there is no initial put
- * after session_add)
- *
- * (b) when we are not the initiator rfcomm_rx_process
- * will explicitly call put to balance the initial hold
- * done after session add.
- */
+ rfcomm_session_del(s);
break;
}
}
@@ -1875,12 +1847,8 @@ static inline void rfcomm_process_rx(struct rfcomm_session *s)
kfree_skb(skb);
}
- if (sk->sk_state == BT_CLOSED) {
- if (!s->initiator)
- rfcomm_session_put(s);
-
+ if (sk->sk_state == BT_CLOSED)
rfcomm_session_close(s, sk->sk_err);
- }
}
static inline void rfcomm_accept_connection(struct rfcomm_session *s)
@@ -1905,8 +1873,6 @@ static inline void rfcomm_accept_connection(struct rfcomm_session *s)
s = rfcomm_session_add(nsock, BT_OPEN);
if (s) {
- rfcomm_session_hold(s);
-
/* We should adjust MTU on incoming sessions.
* L2CAP MTU minus UIH header and FCS. */
s->mtu = min(l2cap_pi(nsock->sk)->chan->omtu,
@@ -1954,7 +1920,6 @@ static inline void rfcomm_process_sessions(void)
if (test_and_clear_bit(RFCOMM_TIMED_OUT, &s->flags)) {
s->state = BT_DISCONN;
rfcomm_send_disc(s, 0);
- rfcomm_session_put(s);
continue;
}
@@ -1963,8 +1928,6 @@ static inline void rfcomm_process_sessions(void)
continue;
}
- rfcomm_session_hold(s);
-
switch (s->state) {
case BT_BOUND:
rfcomm_check_connection(s);
@@ -1976,8 +1939,6 @@ static inline void rfcomm_process_sessions(void)
}
rfcomm_process_dlcs(s);
-
- rfcomm_session_put(s);
}
rfcomm_unlock();
@@ -2027,7 +1988,6 @@ static int rfcomm_add_listener(bdaddr_t *ba)
if (!s)
goto failed;
- rfcomm_session_hold(s);
return 0;
failed:
sock_release(sock);
@@ -2085,8 +2045,6 @@ static void rfcomm_security_cfm(struct hci_conn *conn, u8 status, u8 encrypt)
if (!s)
return;
- rfcomm_session_hold(s);
-
list_for_each_safe(p, n, &s->dlcs) {
d = list_entry(p, struct rfcomm_dlc, list);
@@ -2118,8 +2076,6 @@ static void rfcomm_security_cfm(struct hci_conn *conn, u8 status, u8 encrypt)
set_bit(RFCOMM_AUTH_REJECT, &d->flags);
}
- rfcomm_session_put(s);
-
rfcomm_schedule();
}
--
1.7.10.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/4] Bluetooth: Return rfcomm session pointers to avoid freed session
2012-08-11 18:47 [PATCH 0/4] Bluetooth: Details of rfcomm fixes - remove session refcnt Dean Jenkins
2012-08-11 18:47 ` [PATCH 1/4] Bluetooth: Remove rfcomm " Dean Jenkins
@ 2012-08-11 18:47 ` Dean Jenkins
2012-08-11 18:47 ` [PATCH 3/4] Bluetooth: Avoid rfcomm_session_timeout using freed pointer Dean Jenkins
2012-08-11 18:47 ` [PATCH 4/4] Bluetooth: On socket shutdown check rfcomm session and DLC exists Dean Jenkins
3 siblings, 0 replies; 9+ messages in thread
From: Dean Jenkins @ 2012-08-11 18:47 UTC (permalink / raw)
To: linux-bluetooth
Unfortunately, the design retains copies of the s rfcomm session
pointer in various code blocks and this invites the reuse of a
freed session pointer.
Therefore, return the rfcomm session pointer back up the call stack
to avoid reusing a freed rfcomm session pointer. When the rfcomm
session is deleted, NULL is passed up the call stack.
Signed-off-by: Dean Jenkins <djenkins@mvista.com>
---
net/bluetooth/rfcomm/core.c | 88 +++++++++++++++++++++++++++----------------
1 file changed, 55 insertions(+), 33 deletions(-)
diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
index b0805c1..24d4d3c 100644
--- a/net/bluetooth/rfcomm/core.c
+++ b/net/bluetooth/rfcomm/core.c
@@ -83,7 +83,7 @@ static struct rfcomm_session *rfcomm_session_create(bdaddr_t *src,
u8 sec_level,
int *err);
static struct rfcomm_session *rfcomm_session_get(bdaddr_t *src, bdaddr_t *dst);
-static void rfcomm_session_del(struct rfcomm_session *s);
+static struct rfcomm_session *rfcomm_session_del(struct rfcomm_session *s);
/* ---- RFCOMM frame parsing macros ---- */
#define __get_dlci(b) ((b & 0xfc) >> 2)
@@ -612,9 +612,13 @@ static struct rfcomm_session *rfcomm_session_add(struct socket *sock, int state)
return s;
}
-static void rfcomm_session_del(struct rfcomm_session *s)
+static struct rfcomm_session *rfcomm_session_del(struct rfcomm_session *s)
{
- int state = s->state;
+ int state;
+
+ BUG_ON(s == NULL);
+
+ state = s->state;
BT_DBG("session %p state %ld", s, s->state);
@@ -626,6 +630,8 @@ static void rfcomm_session_del(struct rfcomm_session *s)
if (state != BT_LISTEN)
module_put(THIS_MODULE);
+
+ return NULL;
}
static struct rfcomm_session *rfcomm_session_get(bdaddr_t *src, bdaddr_t *dst)
@@ -644,11 +650,14 @@ static struct rfcomm_session *rfcomm_session_get(bdaddr_t *src, bdaddr_t *dst)
return NULL;
}
-static void rfcomm_session_close(struct rfcomm_session *s, int err)
+static struct rfcomm_session *rfcomm_session_close(struct rfcomm_session *s,
+ int err)
{
struct rfcomm_dlc *d;
struct list_head *p, *n;
+ BUG_ON(s == NULL);
+
BT_DBG("session %p state %ld err %d", s, s->state, err);
s->state = BT_CLOSED;
@@ -660,7 +669,7 @@ static void rfcomm_session_close(struct rfcomm_session *s, int err)
__rfcomm_dlc_close(d, err);
}
- rfcomm_session_del(s);
+ return rfcomm_session_del(s);
}
static struct rfcomm_session *rfcomm_session_create(bdaddr_t *src,
@@ -712,8 +721,7 @@ static struct rfcomm_session *rfcomm_session_create(bdaddr_t *src,
if (*err == 0 || *err == -EINPROGRESS)
return s;
- rfcomm_session_del(s);
- return NULL;
+ return rfcomm_session_del(s);
failed:
sock_release(sock);
@@ -1102,7 +1110,7 @@ static void rfcomm_make_uih(struct sk_buff *skb, u8 addr)
}
/* ---- RFCOMM frame reception ---- */
-static int rfcomm_recv_ua(struct rfcomm_session *s, u8 dlci)
+static struct rfcomm_session *rfcomm_recv_ua(struct rfcomm_session *s, u8 dlci)
{
BT_DBG("session %p state %ld dlci %d", s, s->state, dlci);
@@ -1111,7 +1119,7 @@ static int rfcomm_recv_ua(struct rfcomm_session *s, u8 dlci)
struct rfcomm_dlc *d = rfcomm_dlc_get(s, dlci);
if (!d) {
rfcomm_send_dm(s, dlci);
- return 0;
+ return s;
}
switch (d->state) {
@@ -1147,14 +1155,14 @@ static int rfcomm_recv_ua(struct rfcomm_session *s, u8 dlci)
break;
case BT_DISCONN:
- rfcomm_session_del(s);
+ s = rfcomm_session_del(s);
break;
}
}
- return 0;
+ return s;
}
-static int rfcomm_recv_dm(struct rfcomm_session *s, u8 dlci)
+static struct rfcomm_session *rfcomm_recv_dm(struct rfcomm_session *s, u8 dlci)
{
int err = 0;
@@ -1179,12 +1187,13 @@ static int rfcomm_recv_dm(struct rfcomm_session *s, u8 dlci)
err = ECONNRESET;
s->state = BT_CLOSED;
- rfcomm_session_close(s, err);
+ s = rfcomm_session_close(s, err);
}
- return 0;
+ return s;
}
-static int rfcomm_recv_disc(struct rfcomm_session *s, u8 dlci)
+static struct rfcomm_session *rfcomm_recv_disc(struct rfcomm_session *s,
+ u8 dlci)
{
int err = 0;
@@ -1214,10 +1223,10 @@ static int rfcomm_recv_disc(struct rfcomm_session *s, u8 dlci)
err = ECONNRESET;
s->state = BT_CLOSED;
- rfcomm_session_close(s, err);
+ s = rfcomm_session_close(s, err);
}
- return 0;
+ return s;
}
void rfcomm_dlc_accept(struct rfcomm_dlc *d)
@@ -1638,11 +1647,18 @@ drop:
return 0;
}
-static int rfcomm_recv_frame(struct rfcomm_session *s, struct sk_buff *skb)
+static struct rfcomm_session *rfcomm_recv_frame(struct rfcomm_session *s,
+ struct sk_buff *skb)
{
struct rfcomm_hdr *hdr = (void *) skb->data;
u8 type, dlci, fcs;
+ if (!s) {
+ /* no session, so free socket data */
+ kfree_skb(skb);
+ return s;
+ }
+
dlci = __get_dlci(hdr->addr);
type = __get_type(hdr->ctrl);
@@ -1653,7 +1669,7 @@ static int rfcomm_recv_frame(struct rfcomm_session *s, struct sk_buff *skb)
if (__check_fcs(skb->data, type, fcs)) {
BT_ERR("bad checksum in packet");
kfree_skb(skb);
- return -EILSEQ;
+ return s;
}
if (__test_ea(hdr->len))
@@ -1669,22 +1685,23 @@ static int rfcomm_recv_frame(struct rfcomm_session *s, struct sk_buff *skb)
case RFCOMM_DISC:
if (__test_pf(hdr->ctrl))
- rfcomm_recv_disc(s, dlci);
+ s = rfcomm_recv_disc(s, dlci);
break;
case RFCOMM_UA:
if (__test_pf(hdr->ctrl))
- rfcomm_recv_ua(s, dlci);
+ s = rfcomm_recv_ua(s, dlci);
break;
case RFCOMM_DM:
- rfcomm_recv_dm(s, dlci);
+ s = rfcomm_recv_dm(s, dlci);
break;
case RFCOMM_UIH:
- if (dlci)
- return rfcomm_recv_data(s, dlci, __test_pf(hdr->ctrl), skb);
-
+ if (dlci) {
+ rfcomm_recv_data(s, dlci, __test_pf(hdr->ctrl), skb);
+ return s;
+ }
rfcomm_recv_mcc(s, skb);
break;
@@ -1693,7 +1710,7 @@ static int rfcomm_recv_frame(struct rfcomm_session *s, struct sk_buff *skb)
break;
}
kfree_skb(skb);
- return 0;
+ return s;
}
/* ---- Connection and data processing ---- */
@@ -1775,6 +1792,8 @@ static inline void rfcomm_process_dlcs(struct rfcomm_session *s)
struct rfcomm_dlc *d;
struct list_head *p, *n;
+ BUG_ON(s == NULL);
+
BT_DBG("session %p state %ld", s, s->state);
list_for_each_safe(p, n, &s->dlcs) {
@@ -1830,7 +1849,7 @@ static inline void rfcomm_process_dlcs(struct rfcomm_session *s)
}
}
-static inline void rfcomm_process_rx(struct rfcomm_session *s)
+static inline struct rfcomm_session *rfcomm_process_rx(struct rfcomm_session *s)
{
struct socket *sock = s->sock;
struct sock *sk = sock->sk;
@@ -1842,13 +1861,15 @@ static inline void rfcomm_process_rx(struct rfcomm_session *s)
while ((skb = skb_dequeue(&sk->sk_receive_queue))) {
skb_orphan(skb);
if (!skb_linearize(skb))
- rfcomm_recv_frame(s, skb);
+ s = rfcomm_recv_frame(s, skb);
else
kfree_skb(skb);
}
- if (sk->sk_state == BT_CLOSED)
- rfcomm_session_close(s, sk->sk_err);
+ if (s && (sk->sk_state == BT_CLOSED))
+ s = rfcomm_session_close(s, sk->sk_err);
+
+ return s;
}
static inline void rfcomm_accept_connection(struct rfcomm_session *s)
@@ -1902,7 +1923,7 @@ static inline void rfcomm_check_connection(struct rfcomm_session *s)
case BT_CLOSED:
s->state = BT_CLOSED;
- rfcomm_session_close(s, sk->sk_err);
+ s = rfcomm_session_close(s, sk->sk_err);
break;
}
}
@@ -1934,11 +1955,12 @@ static inline void rfcomm_process_sessions(void)
break;
default:
- rfcomm_process_rx(s);
+ s = rfcomm_process_rx(s);
break;
}
- rfcomm_process_dlcs(s);
+ if (s)
+ rfcomm_process_dlcs(s);
}
rfcomm_unlock();
--
1.7.10.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/4] Bluetooth: Avoid rfcomm_session_timeout using freed pointer
2012-08-11 18:47 [PATCH 0/4] Bluetooth: Details of rfcomm fixes - remove session refcnt Dean Jenkins
2012-08-11 18:47 ` [PATCH 1/4] Bluetooth: Remove rfcomm " Dean Jenkins
2012-08-11 18:47 ` [PATCH 2/4] Bluetooth: Return rfcomm session pointers to avoid freed session Dean Jenkins
@ 2012-08-11 18:47 ` Dean Jenkins
2012-08-21 18:56 ` Gustavo Padovan
2012-08-11 18:47 ` [PATCH 4/4] Bluetooth: On socket shutdown check rfcomm session and DLC exists Dean Jenkins
3 siblings, 1 reply; 9+ messages in thread
From: Dean Jenkins @ 2012-08-11 18:47 UTC (permalink / raw)
To: linux-bluetooth
rfcomm_session_timeout() protects the scenario of the remote
Bluetooth device failing to send a DISC on the rfcomm control
channel after the last data DLC channel has been closed.
There is a race condition between the timer expiring causing
rfcomm_session_timeout() to run and the rfcomm session being
deleted. If the rfcomm session is deleted then
rfcomm_session_timeout() would use a freed rfcomm session
pointer resulting in a potential kernel crash or memory corruption.
Note the timer is cleared before the rfcomm session is deleted
by del_timer() so the circumstances for a failure to occur are
as follows:
rfcomm_session_timeout() needs to be executing before the
del_timer() is called to clear the timer but
rfcomm_session_timeout() needs to be delayed from using the
rfcomm session pointer until after the session has been deleted.
Therefore, there is a very small window of opportunity for failure.
The solution is to use del_timer_sync() instead of del_timer()
as this ensures that rfcomm_session_timeout() is not running
when del_timer_sync() returns. This means that it is not
possible for rfcomm_session_timeout() to run after the rfcomm
session has been deleted.
Signed-off-by: Dean Jenkins <djenkins@mvista.com>
---
net/bluetooth/rfcomm/core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
index 24d4d3c..c7921fd 100644
--- a/net/bluetooth/rfcomm/core.c
+++ b/net/bluetooth/rfcomm/core.c
@@ -264,8 +264,8 @@ static void rfcomm_session_clear_timer(struct rfcomm_session *s)
{
BT_DBG("session %p state %ld", s, s->state);
- if (timer_pending(&s->timer))
- del_timer(&s->timer);
+ /* ensure rfcomm_session_timeout() is not running past this point */
+ del_timer_sync(&s->timer);
}
/* ---- RFCOMM DLCs ---- */
--
1.7.10.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/4] Bluetooth: Avoid rfcomm_session_timeout using freed pointer
2012-08-11 18:47 ` [PATCH 3/4] Bluetooth: Avoid rfcomm_session_timeout using freed pointer Dean Jenkins
@ 2012-08-21 18:56 ` Gustavo Padovan
2012-08-30 15:36 ` Dean Jenkins
0 siblings, 1 reply; 9+ messages in thread
From: Gustavo Padovan @ 2012-08-21 18:56 UTC (permalink / raw)
To: Dean Jenkins; +Cc: linux-bluetooth
Hi Dean,
* Dean Jenkins <djenkins@mvista.com> [2012-08-11 19:47:09 +0100]:
> rfcomm_session_timeout() protects the scenario of the remote
> Bluetooth device failing to send a DISC on the rfcomm control
> channel after the last data DLC channel has been closed.
>
> There is a race condition between the timer expiring causing
> rfcomm_session_timeout() to run and the rfcomm session being
> deleted. If the rfcomm session is deleted then
> rfcomm_session_timeout() would use a freed rfcomm session
> pointer resulting in a potential kernel crash or memory corruption.
> Note the timer is cleared before the rfcomm session is deleted
> by del_timer() so the circumstances for a failure to occur are
> as follows:
>
> rfcomm_session_timeout() needs to be executing before the
> del_timer() is called to clear the timer but
> rfcomm_session_timeout() needs to be delayed from using the
> rfcomm session pointer until after the session has been deleted.
> Therefore, there is a very small window of opportunity for failure.
>
> The solution is to use del_timer_sync() instead of del_timer()
> as this ensures that rfcomm_session_timeout() is not running
> when del_timer_sync() returns. This means that it is not
> possible for rfcomm_session_timeout() to run after the rfcomm
> session has been deleted.
>
> Signed-off-by: Dean Jenkins <djenkins@mvista.com>
> ---
> net/bluetooth/rfcomm/core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
> index 24d4d3c..c7921fd 100644
> --- a/net/bluetooth/rfcomm/core.c
> +++ b/net/bluetooth/rfcomm/core.c
> @@ -264,8 +264,8 @@ static void rfcomm_session_clear_timer(struct rfcomm_session *s)
> {
> BT_DBG("session %p state %ld", s, s->state);
>
> - if (timer_pending(&s->timer))
> - del_timer(&s->timer);
> + /* ensure rfcomm_session_timeout() is not running past this point */
> + del_timer_sync(&s->timer);
I'm not happy of the idea of let the stack broken between patches 2 and 3
(this one). As you said if we use del_timer_sync() we don't need rfcnt here,
can you add this as a first patch, maybe? and after this continue to remove
the rest of the refcount code?
Gustavo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/4] Bluetooth: Avoid rfcomm_session_timeout using freed pointer
2012-08-21 18:56 ` Gustavo Padovan
@ 2012-08-30 15:36 ` Dean Jenkins
0 siblings, 0 replies; 9+ messages in thread
From: Dean Jenkins @ 2012-08-30 15:36 UTC (permalink / raw)
To: Gustavo Padovan, Dean Jenkins, linux-bluetooth
Hi Gustavo,
On 21 August 2012 19:56, Gustavo Padovan <gustavo@padovan.org> wrote:
> Hi Dean,
>
> * Dean Jenkins <djenkins@mvista.com> [2012-08-11 19:47:09 +0100]:
>
>> rfcomm_session_timeout() protects the scenario of the remote
>> Bluetooth device failing to send a DISC on the rfcomm control
>> channel after the last data DLC channel has been closed.
>>
>> There is a race condition between the timer expiring causing
>> rfcomm_session_timeout() to run and the rfcomm session being
>> deleted. If the rfcomm session is deleted then
>> rfcomm_session_timeout() would use a freed rfcomm session
>> pointer resulting in a potential kernel crash or memory corruption.
>> Note the timer is cleared before the rfcomm session is deleted
>> by del_timer() so the circumstances for a failure to occur are
>> as follows:
>>
>> rfcomm_session_timeout() needs to be executing before the
>> del_timer() is called to clear the timer but
>> rfcomm_session_timeout() needs to be delayed from using the
>> rfcomm session pointer until after the session has been deleted.
>> Therefore, there is a very small window of opportunity for failure.
>>
>> The solution is to use del_timer_sync() instead of del_timer()
>> as this ensures that rfcomm_session_timeout() is not running
>> when del_timer_sync() returns. This means that it is not
>> possible for rfcomm_session_timeout() to run after the rfcomm
>> session has been deleted.
>
>>
>> Signed-off-by: Dean Jenkins <djenkins@mvista.com>
>> ---
>> net/bluetooth/rfcomm/core.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
>> index 24d4d3c..c7921fd 100644
>> --- a/net/bluetooth/rfcomm/core.c
>> +++ b/net/bluetooth/rfcomm/core.c
>> @@ -264,8 +264,8 @@ static void rfcomm_session_clear_timer(struct rfcomm_session *s)
>> {
>> BT_DBG("session %p state %ld", s, s->state);
>>
>> - if (timer_pending(&s->timer))
>> - del_timer(&s->timer);
>> + /* ensure rfcomm_session_timeout() is not running past this point */
>> + del_timer_sync(&s->timer);
>
> I'm not happy of the idea of let the stack broken between patches 2 and 3
> (this one). As you said if we use del_timer_sync() we don't need rfcnt here,
> can you add this as a first patch, maybe? and after this continue to remove
> the rest of the refcount code?
>
> Gustavo
Thanks for your feedback. Sorry for the delay in getting back to you.
My gmail failed to filter out your reply.
OK, I'll start with a patch for del_timer_sync(). This can be a
standalone patch.
For the removal of the refcnt, do you propose 1 patch to remove the
refcnt AND to manage the rfcomm session pointer ? I have doubts now
because you don't wish the stack to be broken between patches. Perhaps
it is possible to add a patch to manage the rfcomm session pointer
with the refcnt in place so we have "belt and braces" then have a
patch to remove the refcnt as the last thing to do.
I am open to suggestions.
Thanks,
Regards,
Dean
--
Dean Jenkins
Embedded Software Engineer
Professional Services UK/EMEA
MontaVista Software, LLC
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 4/4] Bluetooth: On socket shutdown check rfcomm session and DLC exists
2012-08-11 18:47 [PATCH 0/4] Bluetooth: Details of rfcomm fixes - remove session refcnt Dean Jenkins
` (2 preceding siblings ...)
2012-08-11 18:47 ` [PATCH 3/4] Bluetooth: Avoid rfcomm_session_timeout using freed pointer Dean Jenkins
@ 2012-08-11 18:47 ` Dean Jenkins
2012-08-21 18:50 ` Gustavo Padovan
3 siblings, 1 reply; 9+ messages in thread
From: Dean Jenkins @ 2012-08-11 18:47 UTC (permalink / raw)
To: linux-bluetooth
A race condition exists between near simultaneous asynchronous
DLC data channel disconnection requests from the host and remote device.
This causes the socket layer to request a socket shutdown at the same time
the rfcomm core is processing the disconnect request from the remote
device.
The socket layer retains a copy of a struct rfcomm_dlc d pointer.
The d pointer refers to a copy of a struct rfcomm_session.
When the socket layer thread performs a socket shutdown, the thread
may wait on a rfcomm lock in rfcomm_dlc_close(). This means that
whilst the thread waits, the rfcomm_session and/or rfcomm_dlc structures
pointed to by d maybe freed due to rfcomm core handling. Consequently,
when the rfcomm lock becomes available and the thread runs, a
malfunction could occur as a freed rfcomm_session pointer and/or a
freed d pointer will be erroneously reused.
Therefore, after the rfcomm lock is acquired, check that the struct
rfcomm_session is still valid by searching the rfcomm session list.
If the session is valid then validate the d pointer by searching the
rfcomm session list of active DLCs for the rfcomm_dlc structure
pointed by d.
If active DLCs exist when the rfcomm session is terminating,
avoid a memory leak of rfcomm_dlc structures by ensuring that
rfcomm_session_close() is used instead of rfcomm_session_del().
Signed-off-by: Dean Jenkins <djenkins@mvista.com>
---
net/bluetooth/rfcomm/core.c | 29 ++++++++++++++++++++++++++---
1 file changed, 26 insertions(+), 3 deletions(-)
diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
index c7921fd..1a7db34 100644
--- a/net/bluetooth/rfcomm/core.c
+++ b/net/bluetooth/rfcomm/core.c
@@ -496,11 +496,34 @@ static int __rfcomm_dlc_close(struct rfcomm_dlc *d, int err)
int rfcomm_dlc_close(struct rfcomm_dlc *d, int err)
{
- int r;
+ int r = 0;
+ struct rfcomm_dlc *d_list;
+ struct rfcomm_session *s, *s_list;
+ struct list_head *p, *n, *p2, *n2;
rfcomm_lock();
- r = __rfcomm_dlc_close(d, err);
+ s = d->session;
+ if (s) {
+ /* check the session still exists after waiting on the mutex */
+ list_for_each_safe(p, n, &session_list) {
+ s_list = list_entry(p, struct rfcomm_session, list);
+ if (s == s_list) {
+ /* check the dlc still exists */
+ /* after waiting on the mutex */
+ list_for_each_safe(p2, n2, &s->dlcs) {
+ d_list = list_entry(p2,
+ struct rfcomm_dlc,
+ list);
+ if (d == d_list) {
+ r = __rfcomm_dlc_close(d, err);
+ break;
+ }
+ }
+ break;
+ }
+ }
+ }
rfcomm_unlock();
return r;
@@ -1155,7 +1178,7 @@ static struct rfcomm_session *rfcomm_recv_ua(struct rfcomm_session *s, u8 dlci)
break;
case BT_DISCONN:
- s = rfcomm_session_del(s);
+ s = rfcomm_session_close(s, ECONNRESET);
break;
}
}
--
1.7.10.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 4/4] Bluetooth: On socket shutdown check rfcomm session and DLC exists
2012-08-11 18:47 ` [PATCH 4/4] Bluetooth: On socket shutdown check rfcomm session and DLC exists Dean Jenkins
@ 2012-08-21 18:50 ` Gustavo Padovan
2012-08-30 15:40 ` Dean Jenkins
0 siblings, 1 reply; 9+ messages in thread
From: Gustavo Padovan @ 2012-08-21 18:50 UTC (permalink / raw)
To: Dean Jenkins; +Cc: linux-bluetooth
Hi Dean,
* Dean Jenkins <djenkins@mvista.com> [2012-08-11 19:47:10 +0100]:
> A race condition exists between near simultaneous asynchronous
> DLC data channel disconnection requests from the host and remote device.
> This causes the socket layer to request a socket shutdown at the same time
> the rfcomm core is processing the disconnect request from the remote
> device.
>
> The socket layer retains a copy of a struct rfcomm_dlc d pointer.
> The d pointer refers to a copy of a struct rfcomm_session.
> When the socket layer thread performs a socket shutdown, the thread
> may wait on a rfcomm lock in rfcomm_dlc_close(). This means that
> whilst the thread waits, the rfcomm_session and/or rfcomm_dlc structures
> pointed to by d maybe freed due to rfcomm core handling. Consequently,
> when the rfcomm lock becomes available and the thread runs, a
> malfunction could occur as a freed rfcomm_session pointer and/or a
> freed d pointer will be erroneously reused.
>
> Therefore, after the rfcomm lock is acquired, check that the struct
> rfcomm_session is still valid by searching the rfcomm session list.
> If the session is valid then validate the d pointer by searching the
> rfcomm session list of active DLCs for the rfcomm_dlc structure
> pointed by d.
>
> If active DLCs exist when the rfcomm session is terminating,
> avoid a memory leak of rfcomm_dlc structures by ensuring that
> rfcomm_session_close() is used instead of rfcomm_session_del().
>
> Signed-off-by: Dean Jenkins <djenkins@mvista.com>
> ---
> net/bluetooth/rfcomm/core.c | 29 ++++++++++++++++++++++++++---
> 1 file changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
> index c7921fd..1a7db34 100644
> --- a/net/bluetooth/rfcomm/core.c
> +++ b/net/bluetooth/rfcomm/core.c
> @@ -496,11 +496,34 @@ static int __rfcomm_dlc_close(struct rfcomm_dlc *d, int err)
>
> int rfcomm_dlc_close(struct rfcomm_dlc *d, int err)
> {
> - int r;
> + int r = 0;
> + struct rfcomm_dlc *d_list;
> + struct rfcomm_session *s, *s_list;
> + struct list_head *p, *n, *p2, *n2;
>
> rfcomm_lock();
>
> - r = __rfcomm_dlc_close(d, err);
> + s = d->session;
> + if (s) {
Please invert the check here, and add a goto unlock;. There too many
indentation levels here.
> + /* check the session still exists after waiting on the mutex */
> + list_for_each_safe(p, n, &session_list) {
> + s_list = list_entry(p, struct rfcomm_session, list);
please use list_for_each_entry(), the _safe version seems to not be needed.
> + if (s == s_list) {
> + /* check the dlc still exists */
> + /* after waiting on the mutex */
> + list_for_each_safe(p2, n2, &s->dlcs) {
> + d_list = list_entry(p2,
> + struct rfcomm_dlc,
> + list);
and here you can use rfcomm_dlc_get()
Gustavo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 4/4] Bluetooth: On socket shutdown check rfcomm session and DLC exists
2012-08-21 18:50 ` Gustavo Padovan
@ 2012-08-30 15:40 ` Dean Jenkins
0 siblings, 0 replies; 9+ messages in thread
From: Dean Jenkins @ 2012-08-30 15:40 UTC (permalink / raw)
To: Gustavo Padovan, Dean Jenkins, linux-bluetooth
Hi Gustavo,
On 21 August 2012 19:50, Gustavo Padovan <gustavo@padovan.org> wrote:
> Hi Dean,
>
> * Dean Jenkins <djenkins@mvista.com> [2012-08-11 19:47:10 +0100]:
>
>> A race condition exists between near simultaneous asynchronous
>> DLC data channel disconnection requests from the host and remote device.
>> This causes the socket layer to request a socket shutdown at the same time
>> the rfcomm core is processing the disconnect request from the remote
>> device.
>>
>> The socket layer retains a copy of a struct rfcomm_dlc d pointer.
>> The d pointer refers to a copy of a struct rfcomm_session.
>> When the socket layer thread performs a socket shutdown, the thread
>> may wait on a rfcomm lock in rfcomm_dlc_close(). This means that
>> whilst the thread waits, the rfcomm_session and/or rfcomm_dlc structures
>> pointed to by d maybe freed due to rfcomm core handling. Consequently,
>> when the rfcomm lock becomes available and the thread runs, a
>> malfunction could occur as a freed rfcomm_session pointer and/or a
>> freed d pointer will be erroneously reused.
>>
>> Therefore, after the rfcomm lock is acquired, check that the struct
>> rfcomm_session is still valid by searching the rfcomm session list.
>> If the session is valid then validate the d pointer by searching the
>> rfcomm session list of active DLCs for the rfcomm_dlc structure
>> pointed by d.
>>
>> If active DLCs exist when the rfcomm session is terminating,
>> avoid a memory leak of rfcomm_dlc structures by ensuring that
>> rfcomm_session_close() is used instead of rfcomm_session_del().
>>
>> Signed-off-by: Dean Jenkins <djenkins@mvista.com>
>> ---
>> net/bluetooth/rfcomm/core.c | 29 ++++++++++++++++++++++++++---
>> 1 file changed, 26 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/bluetooth/rfcomm/core.c b/net/bluetooth/rfcomm/core.c
>> index c7921fd..1a7db34 100644
>> --- a/net/bluetooth/rfcomm/core.c
>> +++ b/net/bluetooth/rfcomm/core.c
>> @@ -496,11 +496,34 @@ static int __rfcomm_dlc_close(struct rfcomm_dlc *d, int err)
>>
>> int rfcomm_dlc_close(struct rfcomm_dlc *d, int err)
>> {
>> - int r;
>> + int r = 0;
>> + struct rfcomm_dlc *d_list;
>> + struct rfcomm_session *s, *s_list;
>> + struct list_head *p, *n, *p2, *n2;
>>
>> rfcomm_lock();
>>
>> - r = __rfcomm_dlc_close(d, err);
>> + s = d->session;
>> + if (s) {
>
> Please invert the check here, and add a goto unlock;. There too many
> indentation levels here.
>
>> + /* check the session still exists after waiting on the mutex */
>> + list_for_each_safe(p, n, &session_list) {
>> + s_list = list_entry(p, struct rfcomm_session, list);
>
> please use list_for_each_entry(), the _safe version seems to not be needed.
>
>> + if (s == s_list) {
>> + /* check the dlc still exists */
>> + /* after waiting on the mutex */
>> + list_for_each_safe(p2, n2, &s->dlcs) {
>> + d_list = list_entry(p2,
>> + struct rfcomm_dlc,
>> + list);
>
>
> and here you can use rfcomm_dlc_get()
>
> Gustavo
Thanks for your feedback. Sorry for the delay getting back to you. I
blame gmail.
OK, I will take a look at redoing the patch as per your suggestion.
Thanks,
Regards,
Dean
--
Dean Jenkins
Embedded Software Engineer
Professional Services UK/EMEA
MontaVista Software, LLC
^ permalink raw reply [flat|nested] 9+ messages in thread