During the L2CAP socket shutdown, the LE connection is not terminated
as expected. This bug can be reproduced using l2test tool. Once the
LE connection is established, kill l2test and the LE connection will
not terminate.
This patch fixes hci_conn_disconnect function so it is able to
terminate LE connections.
Signed-off-by: Andre Guedes <[email protected]>
---
net/bluetooth/hci_conn.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 25bfce0..0492949 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -250,6 +250,7 @@ static void hci_conn_disconnect(struct hci_conn *conn)
switch (conn->type) {
case ACL_LINK:
+ case LE_LINK:
hci_acl_disconn(conn, reason);
break;
case AMP_LINK:
--
1.8.1.1
Hi Andre,
* Andre Guedes <[email protected]> [2013-01-29 19:59:56 -0300]:
> This patch reduces the critical section protected by sco_conn_lock in
> sco_conn_ready function. The lock is acquired only when it is really
> needed.
>
> This patch fixes the following lockdep warning which is generated
> when the host terminates a SCO connection.
>
> Today, this warning is a false positive. There is no way those
> two threads reported by lockdep are running at the same time since
> hdev->workqueue (where rx_work is queued) is single-thread. However,
> if somehow this behavior is changed in future, we will have a
> potential deadlock.
>
> ======================================================
> [ INFO: possible circular locking dependency detected ]
> 3.8.0-rc1+ #7 Not tainted
> -------------------------------------------------------
> kworker/u:1H/1018 is trying to acquire lock:
> (&(&conn->lock)->rlock){+.+...}, at: [<ffffffffa0033ba6>] sco_chan_del+0x66/0x190 [bluetooth]
>
> but task is already holding lock:
> (slock-AF_BLUETOOTH-BTPROTO_SCO){+.+...}, at: [<ffffffffa0033d5a>] sco_conn_del+0x8a/0xe0 [bluetooth]
>
> which lock already depends on the new lock.
>
> the existing dependency chain (in reverse order) is:
>
> -> #1 (slock-AF_BLUETOOTH-BTPROTO_SCO){+.+...}:
> [<ffffffff81083011>] lock_acquire+0xb1/0xe0
> [<ffffffff813efd01>] _raw_spin_lock+0x41/0x80
> [<ffffffffa003436e>] sco_connect_cfm+0xbe/0x350 [bluetooth]
> [<ffffffffa0015d6c>] hci_event_packet+0xd3c/0x29b0 [bluetooth]
> [<ffffffffa0004583>] hci_rx_work+0x133/0x870 [bluetooth]
> [<ffffffff8104d65f>] process_one_work+0x2bf/0x4f0
> [<ffffffff81050022>] worker_thread+0x2b2/0x3e0
> [<ffffffff81056021>] kthread+0xd1/0xe0
> [<ffffffff813f14bc>] ret_from_fork+0x7c/0xb0
>
> -> #0 (&(&conn->lock)->rlock){+.+...}:
> [<ffffffff81082215>] __lock_acquire+0x1465/0x1c70
> [<ffffffff81083011>] lock_acquire+0xb1/0xe0
> [<ffffffff813efd01>] _raw_spin_lock+0x41/0x80
> [<ffffffffa0033ba6>] sco_chan_del+0x66/0x190 [bluetooth]
> [<ffffffffa0033d6d>] sco_conn_del+0x9d/0xe0 [bluetooth]
> [<ffffffffa0034653>] sco_disconn_cfm+0x53/0x60 [bluetooth]
> [<ffffffffa000fef3>] hci_disconn_complete_evt.isra.54+0x363/0x3c0 [bluetooth]
> [<ffffffffa00150f7>] hci_event_packet+0xc7/0x29b0 [bluetooth]
> [<ffffffffa0004583>] hci_rx_work+0x133/0x870 [bluetooth]
> [<ffffffff8104d65f>] process_one_work+0x2bf/0x4f0
> [<ffffffff81050022>] worker_thread+0x2b2/0x3e0
> [<ffffffff81056021>] kthread+0xd1/0xe0
> [<ffffffff813f14bc>] ret_from_fork+0x7c/0xb0
>
> other info that might help us debug this:
>
> Possible unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(slock-AF_BLUETOOTH-BTPROTO_SCO);
> lock(&(&conn->lock)->rlock);
> lock(slock-AF_BLUETOOTH-BTPROTO_SCO);
> lock(&(&conn->lock)->rlock);
>
> *** DEADLOCK ***
>
> 4 locks held by kworker/u:1H/1018:
> #0: (hdev->name#2){.+.+.+}, at: [<ffffffff8104d5f8>] process_one_work+0x258/0x4f0
> #1: ((&hdev->rx_work)){+.+.+.}, at: [<ffffffff8104d5f8>] process_one_work+0x258/0x4f0
> #2: (&hdev->lock){+.+.+.}, at: [<ffffffffa000fbe9>] hci_disconn_complete_evt.isra.54+0x59/0x3c0 [bluetooth]
> #3: (slock-AF_BLUETOOTH-BTPROTO_SCO){+.+...}, at: [<ffffffffa0033d5a>] sco_conn_del+0x8a/0xe0 [bluetooth]
>
> stack backtrace:
> Pid: 1018, comm: kworker/u:1H Not tainted 3.8.0-rc1+ #7
> Call Trace:
> [<ffffffff813e92f9>] print_circular_bug+0x1fb/0x20c
> [<ffffffff81082215>] __lock_acquire+0x1465/0x1c70
> [<ffffffff81083011>] lock_acquire+0xb1/0xe0
> [<ffffffffa0033ba6>] ? sco_chan_del+0x66/0x190 [bluetooth]
> [<ffffffff813efd01>] _raw_spin_lock+0x41/0x80
> [<ffffffffa0033ba6>] ? sco_chan_del+0x66/0x190 [bluetooth]
> [<ffffffffa0033ba6>] sco_chan_del+0x66/0x190 [bluetooth]
> [<ffffffffa0033d6d>] sco_conn_del+0x9d/0xe0 [bluetooth]
> [<ffffffffa0034653>] sco_disconn_cfm+0x53/0x60 [bluetooth]
> [<ffffffffa000fef3>] hci_disconn_complete_evt.isra.54+0x363/0x3c0 [bluetooth]
> [<ffffffffa000fbd0>] ? hci_disconn_complete_evt.isra.54+0x40/0x3c0 [bluetooth]
> [<ffffffffa00150f7>] hci_event_packet+0xc7/0x29b0 [bluetooth]
> [<ffffffff81202e90>] ? __dynamic_pr_debug+0x80/0x90
> [<ffffffff8133ff7d>] ? kfree_skb+0x2d/0x40
> [<ffffffffa0021644>] ? hci_send_to_monitor+0x1a4/0x1c0 [bluetooth]
> [<ffffffffa0004583>] hci_rx_work+0x133/0x870 [bluetooth]
> [<ffffffff8104d5f8>] ? process_one_work+0x258/0x4f0
> [<ffffffff8104d65f>] process_one_work+0x2bf/0x4f0
> [<ffffffff8104d5f8>] ? process_one_work+0x258/0x4f0
> [<ffffffff8104fdc1>] ? worker_thread+0x51/0x3e0
> [<ffffffffa0004450>] ? hci_tx_work+0x800/0x800 [bluetooth]
> [<ffffffff81050022>] worker_thread+0x2b2/0x3e0
> [<ffffffff8104fd70>] ? busy_worker_rebind_fn+0x100/0x100
> [<ffffffff81056021>] kthread+0xd1/0xe0
> [<ffffffff81055f50>] ? flush_kthread_worker+0xc0/0xc0
> [<ffffffff813f14bc>] ret_from_fork+0x7c/0xb0
> [<ffffffff81055f50>] ? flush_kthread_worker+0xc0/0xc0
>
> Signed-off-by: Andre Guedes <[email protected]>
> ---
>
> This lockdep warning has been around for a long time. I could test
> until Linux 3.4, but it seems it is older than that. However, in
> current bluetooth-next, this warning has been masked by commit
> 53502d69be49e3dd5bc95ab0f2deeaea260bd617 which introduced a bug in
> SCO socket shutdown routine.
>
> The bug in SCO socket shutdown routine has been fixed by patch 02/05
> from this patchset.
>
> net/bluetooth/sco.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
Patch has been applied to bluetooth-next. Thanks.
Gustavo
Hi Andre,
> This patch does a trivial refactoring in hci_conn_disconnect function.
>
> Signed-off-by: Andre Guedes <[email protected]>
> ---
> net/bluetooth/hci_conn.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 28cfa72..4925a02 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -249,15 +249,12 @@ static void hci_conn_disconnect(struct hci_conn *conn)
> __u8 reason = hci_proto_disconn_ind(conn);
>
> switch (conn->type) {
> - case ACL_LINK:
> - case LE_LINK:
> - case SCO_LINK:
> - case ESCO_LINK:
> - hci_acl_disconn(conn, reason);
> - break;
> case AMP_LINK:
> hci_amp_disconn(conn, reason);
> break;
> + default:
> + hci_acl_disconn(conn, reason);
> + break;
> }
just make it one patch that fixes all cases. Trying to make 3 patches
out of this is just confusing. Rather write a proper commit message that
explains it all.
Regards
Marcel
This patch does a trivial refactoring in hci_conn_disconnect function.
Signed-off-by: Andre Guedes <[email protected]>
---
net/bluetooth/hci_conn.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 28cfa72..4925a02 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -249,15 +249,12 @@ static void hci_conn_disconnect(struct hci_conn *conn)
__u8 reason = hci_proto_disconn_ind(conn);
switch (conn->type) {
- case ACL_LINK:
- case LE_LINK:
- case SCO_LINK:
- case ESCO_LINK:
- hci_acl_disconn(conn, reason);
- break;
case AMP_LINK:
hci_amp_disconn(conn, reason);
break;
+ default:
+ hci_acl_disconn(conn, reason);
+ break;
}
}
--
1.8.1.1
As hci_acl_disconn function basically sends the HCI Disconnect Command
and it is used to disconnect ACL, SCO and LE links, renaming it to
hci_disconnect is more suitable.
Signed-off-by: Andre Guedes <[email protected]>
---
include/net/bluetooth/hci_core.h | 2 +-
net/bluetooth/hci_conn.c | 4 ++--
net/bluetooth/hci_core.c | 2 +-
net/bluetooth/hci_event.c | 4 ++--
4 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 90cf75a..787d3b9 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -574,7 +574,7 @@ static inline struct hci_conn *hci_conn_hash_lookup_state(struct hci_dev *hdev,
return NULL;
}
-void hci_acl_disconn(struct hci_conn *conn, __u8 reason);
+void hci_disconnect(struct hci_conn *conn, __u8 reason);
void hci_setup_sync(struct hci_conn *conn, __u16 handle);
void hci_sco_setup(struct hci_conn *conn, __u8 status);
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 4925a02..b9f9016 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -117,7 +117,7 @@ static void hci_acl_create_connection_cancel(struct hci_conn *conn)
hci_send_cmd(conn->hdev, HCI_OP_CREATE_CONN_CANCEL, sizeof(cp), &cp);
}
-void hci_acl_disconn(struct hci_conn *conn, __u8 reason)
+void hci_disconnect(struct hci_conn *conn, __u8 reason)
{
struct hci_cp_disconnect cp;
@@ -253,7 +253,7 @@ static void hci_conn_disconnect(struct hci_conn *conn)
hci_amp_disconn(conn, reason);
break;
default:
- hci_acl_disconn(conn, reason);
+ hci_disconnect(conn, reason);
break;
}
}
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 618ca1a..388c456 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2398,7 +2398,7 @@ static void hci_link_tx_to(struct hci_dev *hdev, __u8 type)
if (c->type == type && c->sent) {
BT_ERR("%s killing stalled connection %pMR",
hdev->name, &c->dst);
- hci_acl_disconn(c, HCI_ERROR_REMOTE_USER_TERM);
+ hci_disconnect(c, HCI_ERROR_REMOTE_USER_TERM);
}
}
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index d4fcba6..5c78480 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2399,7 +2399,7 @@ static void hci_encrypt_change_evt(struct hci_dev *hdev, struct sk_buff *skb)
clear_bit(HCI_CONN_ENCRYPT_PEND, &conn->flags);
if (ev->status && conn->state == BT_CONNECTED) {
- hci_acl_disconn(conn, HCI_ERROR_AUTH_FAILURE);
+ hci_disconnect(conn, HCI_ERROR_AUTH_FAILURE);
hci_conn_put(conn);
goto unlock;
}
@@ -3472,7 +3472,7 @@ static void hci_key_refresh_complete_evt(struct hci_dev *hdev,
clear_bit(HCI_CONN_ENCRYPT_PEND, &conn->flags);
if (ev->status && conn->state == BT_CONNECTED) {
- hci_acl_disconn(conn, HCI_ERROR_AUTH_FAILURE);
+ hci_disconnect(conn, HCI_ERROR_AUTH_FAILURE);
hci_conn_put(conn);
goto unlock;
}
--
1.8.1.1
This patch reduces the critical section protected by sco_conn_lock in
sco_conn_ready function. The lock is acquired only when it is really
needed.
This patch fixes the following lockdep warning which is generated
when the host terminates a SCO connection.
Today, this warning is a false positive. There is no way those
two threads reported by lockdep are running at the same time since
hdev->workqueue (where rx_work is queued) is single-thread. However,
if somehow this behavior is changed in future, we will have a
potential deadlock.
======================================================
[ INFO: possible circular locking dependency detected ]
3.8.0-rc1+ #7 Not tainted
-------------------------------------------------------
kworker/u:1H/1018 is trying to acquire lock:
(&(&conn->lock)->rlock){+.+...}, at: [<ffffffffa0033ba6>] sco_chan_del+0x66/0x190 [bluetooth]
but task is already holding lock:
(slock-AF_BLUETOOTH-BTPROTO_SCO){+.+...}, at: [<ffffffffa0033d5a>] sco_conn_del+0x8a/0xe0 [bluetooth]
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (slock-AF_BLUETOOTH-BTPROTO_SCO){+.+...}:
[<ffffffff81083011>] lock_acquire+0xb1/0xe0
[<ffffffff813efd01>] _raw_spin_lock+0x41/0x80
[<ffffffffa003436e>] sco_connect_cfm+0xbe/0x350 [bluetooth]
[<ffffffffa0015d6c>] hci_event_packet+0xd3c/0x29b0 [bluetooth]
[<ffffffffa0004583>] hci_rx_work+0x133/0x870 [bluetooth]
[<ffffffff8104d65f>] process_one_work+0x2bf/0x4f0
[<ffffffff81050022>] worker_thread+0x2b2/0x3e0
[<ffffffff81056021>] kthread+0xd1/0xe0
[<ffffffff813f14bc>] ret_from_fork+0x7c/0xb0
-> #0 (&(&conn->lock)->rlock){+.+...}:
[<ffffffff81082215>] __lock_acquire+0x1465/0x1c70
[<ffffffff81083011>] lock_acquire+0xb1/0xe0
[<ffffffff813efd01>] _raw_spin_lock+0x41/0x80
[<ffffffffa0033ba6>] sco_chan_del+0x66/0x190 [bluetooth]
[<ffffffffa0033d6d>] sco_conn_del+0x9d/0xe0 [bluetooth]
[<ffffffffa0034653>] sco_disconn_cfm+0x53/0x60 [bluetooth]
[<ffffffffa000fef3>] hci_disconn_complete_evt.isra.54+0x363/0x3c0 [bluetooth]
[<ffffffffa00150f7>] hci_event_packet+0xc7/0x29b0 [bluetooth]
[<ffffffffa0004583>] hci_rx_work+0x133/0x870 [bluetooth]
[<ffffffff8104d65f>] process_one_work+0x2bf/0x4f0
[<ffffffff81050022>] worker_thread+0x2b2/0x3e0
[<ffffffff81056021>] kthread+0xd1/0xe0
[<ffffffff813f14bc>] ret_from_fork+0x7c/0xb0
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(slock-AF_BLUETOOTH-BTPROTO_SCO);
lock(&(&conn->lock)->rlock);
lock(slock-AF_BLUETOOTH-BTPROTO_SCO);
lock(&(&conn->lock)->rlock);
*** DEADLOCK ***
4 locks held by kworker/u:1H/1018:
#0: (hdev->name#2){.+.+.+}, at: [<ffffffff8104d5f8>] process_one_work+0x258/0x4f0
#1: ((&hdev->rx_work)){+.+.+.}, at: [<ffffffff8104d5f8>] process_one_work+0x258/0x4f0
#2: (&hdev->lock){+.+.+.}, at: [<ffffffffa000fbe9>] hci_disconn_complete_evt.isra.54+0x59/0x3c0 [bluetooth]
#3: (slock-AF_BLUETOOTH-BTPROTO_SCO){+.+...}, at: [<ffffffffa0033d5a>] sco_conn_del+0x8a/0xe0 [bluetooth]
stack backtrace:
Pid: 1018, comm: kworker/u:1H Not tainted 3.8.0-rc1+ #7
Call Trace:
[<ffffffff813e92f9>] print_circular_bug+0x1fb/0x20c
[<ffffffff81082215>] __lock_acquire+0x1465/0x1c70
[<ffffffff81083011>] lock_acquire+0xb1/0xe0
[<ffffffffa0033ba6>] ? sco_chan_del+0x66/0x190 [bluetooth]
[<ffffffff813efd01>] _raw_spin_lock+0x41/0x80
[<ffffffffa0033ba6>] ? sco_chan_del+0x66/0x190 [bluetooth]
[<ffffffffa0033ba6>] sco_chan_del+0x66/0x190 [bluetooth]
[<ffffffffa0033d6d>] sco_conn_del+0x9d/0xe0 [bluetooth]
[<ffffffffa0034653>] sco_disconn_cfm+0x53/0x60 [bluetooth]
[<ffffffffa000fef3>] hci_disconn_complete_evt.isra.54+0x363/0x3c0 [bluetooth]
[<ffffffffa000fbd0>] ? hci_disconn_complete_evt.isra.54+0x40/0x3c0 [bluetooth]
[<ffffffffa00150f7>] hci_event_packet+0xc7/0x29b0 [bluetooth]
[<ffffffff81202e90>] ? __dynamic_pr_debug+0x80/0x90
[<ffffffff8133ff7d>] ? kfree_skb+0x2d/0x40
[<ffffffffa0021644>] ? hci_send_to_monitor+0x1a4/0x1c0 [bluetooth]
[<ffffffffa0004583>] hci_rx_work+0x133/0x870 [bluetooth]
[<ffffffff8104d5f8>] ? process_one_work+0x258/0x4f0
[<ffffffff8104d65f>] process_one_work+0x2bf/0x4f0
[<ffffffff8104d5f8>] ? process_one_work+0x258/0x4f0
[<ffffffff8104fdc1>] ? worker_thread+0x51/0x3e0
[<ffffffffa0004450>] ? hci_tx_work+0x800/0x800 [bluetooth]
[<ffffffff81050022>] worker_thread+0x2b2/0x3e0
[<ffffffff8104fd70>] ? busy_worker_rebind_fn+0x100/0x100
[<ffffffff81056021>] kthread+0xd1/0xe0
[<ffffffff81055f50>] ? flush_kthread_worker+0xc0/0xc0
[<ffffffff813f14bc>] ret_from_fork+0x7c/0xb0
[<ffffffff81055f50>] ? flush_kthread_worker+0xc0/0xc0
Signed-off-by: Andre Guedes <[email protected]>
---
This lockdep warning has been around for a long time. I could test
until Linux 3.4, but it seems it is older than that. However, in
current bluetooth-next, this warning has been masked by commit
53502d69be49e3dd5bc95ab0f2deeaea260bd617 which introduced a bug in
SCO socket shutdown routine.
The bug in SCO socket shutdown routine has been fixed by patch 02/05
from this patchset.
net/bluetooth/sco.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 531a93d..e435641 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -900,8 +900,6 @@ static void sco_conn_ready(struct sco_conn *conn)
BT_DBG("conn %p", conn);
- sco_conn_lock(conn);
-
if (sk) {
sco_sock_clear_timer(sk);
bh_lock_sock(sk);
@@ -909,9 +907,13 @@ static void sco_conn_ready(struct sco_conn *conn)
sk->sk_state_change(sk);
bh_unlock_sock(sk);
} else {
+ sco_conn_lock(conn);
+
parent = sco_get_sock_listen(conn->src);
- if (!parent)
- goto done;
+ if (!parent) {
+ sco_conn_unlock(conn);
+ return;
+ }
bh_lock_sock(parent);
@@ -919,7 +921,8 @@ static void sco_conn_ready(struct sco_conn *conn)
BTPROTO_SCO, GFP_ATOMIC);
if (!sk) {
bh_unlock_sock(parent);
- goto done;
+ sco_conn_unlock(conn);
+ return;
}
sco_sock_init(sk, parent);
@@ -939,10 +942,9 @@ static void sco_conn_ready(struct sco_conn *conn)
parent->sk_data_ready(parent, 1);
bh_unlock_sock(parent);
- }
-done:
- sco_conn_unlock(conn);
+ sco_conn_unlock(conn);
+ }
}
/* ----- SCO interface with lower layer (HCI) ----- */
--
1.8.1.1
During the SCO socket shutdown, the connection is not terminated a
expected. This bug can be reproduced using scotest tool. Once the
connection is established, kill scotest and the connection will not
terminate.
This patch fixes hci_conn_disconnect function so it is able to
terminate SCO connections.
Signed-off-by: Andre Guedes <[email protected]>
---
net/bluetooth/hci_conn.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 0492949..28cfa72 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -251,6 +251,8 @@ static void hci_conn_disconnect(struct hci_conn *conn)
switch (conn->type) {
case ACL_LINK:
case LE_LINK:
+ case SCO_LINK:
+ case ESCO_LINK:
hci_acl_disconn(conn, reason);
break;
case AMP_LINK:
--
1.8.1.1