From 8451ad010e26562d4a4c7d0d135c70c4325ee6b5 Mon Sep 17 00:00:00 2001 From: Emil Lenngren Date: Mon, 19 Dec 2016 19:21:19 +0000 Subject: [PATCH] Fix BLE white list issues Since Bluetooth 4.2 and errata ESR08 there may not be more than one connection between two LE device addresses. Also the stack assumes there is at maximum one connection to the same address. This patch makes sure there are no connected devices in the white list when a connection attempt is started. Since some (even 4.2) controllers don't handle this correctly, currently this method is used regardless of controller in this patch. When the maximum L2CAP connections were reached and a new connection was established to a device using auto connect, the stack hung and would no longer create new connections until Bluetooth was restarted, since the state change to BLE_CONN_IDLE was forgotten. This patch resets the state correctly, and also never initiates a connection unless there is space to avoid connect-disconnect loop. There were also bugs in the background_connections hash map; memory was not freed when an element was erased and an incorrect hash function which used the pointer to a bd addr instead of the bd addr itself which basically meant that elements were never removed. This patch removes the dynamic memory allocation and uses a correct hash function. There was a bug that might lead to that the white list was filled beyond its maximum, due to the counter was updated on the HCI command complete event, which might run too late. Now the space is instead calculated based on what commands have been sent to the controller. The address type of the address added to the white list must also be tracked, otherwise it might be updated due to a BLE scan, and later the wrong address is removed from the white list. This patch fixes this. (Preferably 49-bit bd addrs should be used as identifier through the whole stack but we're not there yet.) There was a queue of size 10 with pending white list operations. That queue got full if there was initially 10 devices in the white list, then the 10 devices were removed and immediately after 10 other devices were added. This patch removes the queue altogether by instead syncing against the background_connections hash map. Bug: https://code.google.com/p/android/issues/detail?id=219910 Test: stress-testing with a bunch of BLE devices and inspecting HCI log Change-Id: I78de654ffbea5f4962a189caf984f7f2934e8fbe --- stack/btm/btm_ble_bgconn.cc | 203 +++++++++++++++++++++++++----------------- stack/btm/btm_ble_gap.cc | 9 +- stack/btm/btm_ble_int.h | 1 + stack/btm/btm_ble_int_types.h | 6 -- stack/l2cap/l2c_ble.cc | 3 + stack/l2cap/l2c_int.h | 1 + stack/l2cap/l2c_utils.cc | 16 ++++ 7 files changed, 145 insertions(+), 94 deletions(-) diff --git a/stack/btm/btm_ble_bgconn.cc b/stack/btm/btm_ble_bgconn.cc index 319fbfaa5..41854424c 100644 --- a/stack/btm/btm_ble_bgconn.cc +++ b/stack/btm/btm_ble_bgconn.cc @@ -50,39 +50,63 @@ static void btm_resume_wl_activity(tBTM_BLE_WL_STATE wl_state); // TODO: Move all of this to controller/le/background_list or similar? typedef struct background_connection_t { bt_bdaddr_t address; + uint8_t addr_type; + + bool in_controller_wl; + uint8_t addr_type_in_wl; + + bool pending_removal; } background_connection_t; -struct KeyEqual { - bool operator()(const bt_bdaddr_t* x, const bt_bdaddr_t* y) const { - return bdaddr_equals(x, y); +struct BgConnHash { + bool operator()(const bt_bdaddr_t& x) const { + const uint8_t* a = x.address; + return a[0] ^ (a[1] << 8) ^ (a[2] << 16) ^ (a[3] << 24) ^ a[4] ^ + (a[5] << 8); + } +}; + +struct BgConnKeyEqual { + bool operator()(const bt_bdaddr_t& x, const bt_bdaddr_t& y) const { + return bdaddr_equals(&x, &y); } }; -static std::unordered_map, KeyEqual> +static std::unordered_map background_connections; -static void background_connection_add(bt_bdaddr_t* address) { +static void background_connection_add(uint8_t addr_type, bt_bdaddr_t* address) { CHECK(address); - auto map_iter = background_connections.find(address); + auto map_iter = background_connections.find(*address); if (map_iter == background_connections.end()) { - background_connection_t* connection = - (background_connection_t*)osi_calloc(sizeof(background_connection_t)); - connection->address = *address; - background_connections[&(connection->address)] = connection; + background_connections[*address] = + background_connection_t{*address, addr_type, false, 0, false}; + } else { + background_connection_t* connection = &map_iter->second; + connection->addr_type = addr_type; + connection->pending_removal = false; } } static void background_connection_remove(bt_bdaddr_t* address) { - background_connections.erase(address); + auto map_iter = background_connections.find(*address); + if (map_iter != background_connections.end()) { + if (map_iter->second.in_controller_wl) { + map_iter->second.pending_removal = true; + } else { + background_connections.erase(map_iter); + } + } } static void background_connections_clear() { background_connections.clear(); } static bool background_connections_pending() { - for (const auto& map_el : background_connections) { - background_connection_t* connection = map_el.second; + for (auto& map_el : background_connections) { + background_connection_t* connection = &map_el.second; + if (connection->pending_removal) continue; const bool connected = BTM_IsAclConnectionUp(connection->address.address, BT_TRANSPORT_LE); if (!connected) { @@ -92,6 +116,14 @@ static bool background_connections_pending() { return false; } +static int background_connections_count() { + int count = 0; + for (auto& map_el : background_connections) { + if (!map_el.second.pending_removal) ++count; + } + return count; +} + /******************************************************************************* * * Function btm_update_scanner_filter_policy @@ -117,6 +149,34 @@ void btm_update_scanner_filter_policy(tBTM_BLE_SFP scan_policy) { p_inq->scan_type, (uint16_t)scan_interval, (uint16_t)scan_window, btm_cb.ble_ctr_cb.addr_mgnt_cb.own_addr_type, scan_policy); } + +/******************************************************************************* + * + * Function btm_ble_bgconn_cancel_if_disconnected + * + * Description If a device has been disconnected, it must be re-added to + * the white list. If needed, this function cancels a pending + * initiate command in order to trigger restart of the initiate + * command which in turn updates the white list. + * + * Parameters bd_addr: updated device + * + ******************************************************************************/ +void btm_ble_bgconn_cancel_if_disconnected(BD_ADDR bd_addr) { + if (btm_cb.ble_ctr_cb.conn_state != BLE_BG_CONN) return; + + bt_bdaddr_t addr = *(bt_bdaddr_t*)bd_addr; + + auto map_it = background_connections.find(addr); + if (map_it != background_connections.end()) { + background_connection_t* connection = &map_it->second; + if (!connection->in_controller_wl && !connection->pending_removal && + !BTM_IsAclConnectionUp(bd_addr, BT_TRANSPORT_LE)) { + btm_ble_start_auto_conn(false); + } + } +} + /******************************************************************************* * * Function btm_add_dev_to_controller @@ -132,30 +192,29 @@ bool btm_add_dev_to_controller(bool to_add, BD_ADDR bd_addr) { if (to_add) { if (p_dev_rec->ble.ble_addr_type == BLE_ADDR_PUBLIC || !BTM_BLE_IS_RESOLVE_BDA(bd_addr)) { - btsnd_hcic_ble_add_white_list(p_dev_rec->ble.ble_addr_type, bd_addr); + background_connection_add(p_dev_rec->ble.ble_addr_type, + (bt_bdaddr_t*)bd_addr); started = true; p_dev_rec->ble.in_controller_list |= BTM_WHITE_LIST_BIT; } else if (memcmp(p_dev_rec->ble.static_addr, bd_addr, BD_ADDR_LEN) != 0 && memcmp(p_dev_rec->ble.static_addr, dummy_bda, BD_ADDR_LEN) != 0) { - btsnd_hcic_ble_add_white_list(p_dev_rec->ble.static_addr_type, - p_dev_rec->ble.static_addr); + background_connection_add(p_dev_rec->ble.static_addr_type, + (bt_bdaddr_t*)p_dev_rec->ble.static_addr); started = true; p_dev_rec->ble.in_controller_list |= BTM_WHITE_LIST_BIT; } } else { if (p_dev_rec->ble.ble_addr_type == BLE_ADDR_PUBLIC || !BTM_BLE_IS_RESOLVE_BDA(bd_addr)) { - btsnd_hcic_ble_remove_from_white_list(p_dev_rec->ble.ble_addr_type, - bd_addr); + background_connection_remove((bt_bdaddr_t*)bd_addr); started = true; } if (memcmp(p_dev_rec->ble.static_addr, dummy_bda, BD_ADDR_LEN) != 0 && memcmp(p_dev_rec->ble.static_addr, bd_addr, BD_ADDR_LEN) != 0) { - btsnd_hcic_ble_remove_from_white_list(p_dev_rec->ble.static_addr_type, - p_dev_rec->ble.static_addr); + background_connection_remove((bt_bdaddr_t*)p_dev_rec->ble.static_addr); started = true; } @@ -166,9 +225,11 @@ bool btm_add_dev_to_controller(bool to_add, BD_ADDR bd_addr) { */ uint8_t addr_type = BTM_IS_PUBLIC_BDA(bd_addr) ? BLE_ADDR_PUBLIC : BLE_ADDR_RANDOM; - btsnd_hcic_ble_remove_from_white_list(addr_type, bd_addr); started = true; - if (to_add) btsnd_hcic_ble_add_white_list(addr_type, bd_addr); + if (to_add) + background_connection_add(addr_type, (bt_bdaddr_t*)bd_addr); + else + background_connection_remove((bt_bdaddr_t*)bd_addr); } return started; @@ -181,45 +242,37 @@ bool btm_add_dev_to_controller(bool to_add, BD_ADDR bd_addr) { * removing) ******************************************************************************/ bool btm_execute_wl_dev_operation(void) { - tBTM_BLE_WL_OP* p_dev_op = btm_cb.ble_ctr_cb.wl_op_q; - uint8_t i = 0; - bool rt = true; - - for (i = 0; i < BTM_BLE_MAX_BG_CONN_DEV_NUM && rt; i++, p_dev_op++) { - if (p_dev_op->in_use) { - rt = btm_add_dev_to_controller(p_dev_op->to_add, p_dev_op->bd_addr); - memset(p_dev_op, 0, sizeof(tBTM_BLE_WL_OP)); + // handle removals first to avoid filling up controller's white list + for (auto map_it = background_connections.begin(); + map_it != background_connections.end();) { + background_connection_t* connection = &map_it->second; + if (connection->pending_removal) { + btsnd_hcic_ble_remove_from_white_list(connection->addr_type_in_wl, + connection->address.address); + map_it = background_connections.erase(map_it); } else - break; - } - return rt; -} -/******************************************************************************* - * - * Function btm_enq_wl_dev_operation - * - * Description enqueue the pending whitelist device operation (loading or - * removing). - ******************************************************************************/ -void btm_enq_wl_dev_operation(bool to_add, BD_ADDR bd_addr) { - tBTM_BLE_WL_OP* p_dev_op = btm_cb.ble_ctr_cb.wl_op_q; - uint8_t i = 0; - - for (i = 0; i < BTM_BLE_MAX_BG_CONN_DEV_NUM; i++, p_dev_op++) { - if (p_dev_op->in_use && !memcmp(p_dev_op->bd_addr, bd_addr, BD_ADDR_LEN)) { - p_dev_op->to_add = to_add; - return; - } else if (!p_dev_op->in_use) - break; + ++map_it; } - if (i != BTM_BLE_MAX_BG_CONN_DEV_NUM) { - p_dev_op->in_use = true; - p_dev_op->to_add = to_add; - memcpy(p_dev_op->bd_addr, bd_addr, BD_ADDR_LEN); - } else { - BTM_TRACE_ERROR("max pending WL operation reached, discard"); + for (auto& map_el : background_connections) { + background_connection_t* connection = &map_el.second; + const bool connected = + BTM_IsAclConnectionUp(connection->address.address, BT_TRANSPORT_LE); + if (!connection->in_controller_wl && !connected) { + btsnd_hcic_ble_add_white_list(connection->addr_type, + connection->address.address); + connection->in_controller_wl = true; + connection->addr_type_in_wl = connection->addr_type; + } else if (connection->in_controller_wl && connected) { + /* Bluetooth Core 4.2 as well as ESR08 disallows more than one + connection between two LE addresses. Not all controllers handle this + correctly, therefore we must make sure connected devices are not in + the white list when bg connection attempt is active. */ + btsnd_hcic_ble_remove_from_white_list(connection->addr_type_in_wl, + connection->address.address); + connection->in_controller_wl = false; + } } - return; + return true; } /******************************************************************************* @@ -233,18 +286,15 @@ void btm_enq_wl_dev_operation(bool to_add, BD_ADDR bd_addr) { bool btm_update_dev_to_white_list(bool to_add, BD_ADDR bd_addr) { tBTM_BLE_CB* p_cb = &btm_cb.ble_ctr_cb; - if (to_add && p_cb->white_list_avail_size == 0) { + if (to_add && + background_connections_count() == + controller_get_interface()->get_ble_white_list_size()) { BTM_TRACE_ERROR("%s Whitelist full, unable to add device", __func__); return false; } - if (to_add) - background_connection_add((bt_bdaddr_t*)bd_addr); - else - background_connection_remove((bt_bdaddr_t*)bd_addr); - btm_suspend_wl_activity(p_cb->wl_state); - btm_enq_wl_dev_operation(to_add, bd_addr); + btm_add_dev_to_controller(to_add, bd_addr); btm_resume_wl_activity(p_cb->wl_state); return true; } @@ -271,15 +321,10 @@ void btm_ble_clear_white_list(void) { ******************************************************************************/ void btm_ble_clear_white_list_complete(uint8_t* p_data, UNUSED_ATTR uint16_t evt_len) { - tBTM_BLE_CB* p_cb = &btm_cb.ble_ctr_cb; uint8_t status; - BTM_TRACE_EVENT("btm_ble_clear_white_list_complete"); STREAM_TO_UINT8(status, p_data); - - if (status == HCI_SUCCESS) - p_cb->white_list_avail_size = - controller_get_interface()->get_ble_white_list_size(); + BTM_TRACE_EVENT("%s status=%d", __func__, status); } /******************************************************************************* @@ -291,7 +336,6 @@ void btm_ble_clear_white_list_complete(uint8_t* p_data, ******************************************************************************/ void btm_ble_white_list_init(uint8_t white_list_size) { BTM_TRACE_DEBUG("%s white_list_size = %d", __func__, white_list_size); - btm_cb.ble_ctr_cb.white_list_avail_size = white_list_size; } /******************************************************************************* @@ -303,7 +347,6 @@ void btm_ble_white_list_init(uint8_t white_list_size) { ******************************************************************************/ void btm_ble_add_2_white_list_complete(uint8_t status) { BTM_TRACE_EVENT("%s status=%d", __func__, status); - if (status == HCI_SUCCESS) --btm_cb.ble_ctr_cb.white_list_avail_size; } /******************************************************************************* @@ -316,7 +359,6 @@ void btm_ble_add_2_white_list_complete(uint8_t status) { void btm_ble_remove_from_white_list_complete(uint8_t* p, UNUSED_ATTR uint16_t evt_len) { BTM_TRACE_EVENT("%s status=%d", __func__, *p); - if (*p == HCI_SUCCESS) ++btm_cb.ble_ctr_cb.white_list_avail_size; } void btm_send_hci_create_connection( @@ -380,9 +422,11 @@ bool btm_ble_start_auto_conn(bool start) { if (controller_get_interface()->supports_ble_2m_phy()) phy |= PHY_LE_2M; if (controller_get_interface()->supports_ble_coded_phy()) phy |= PHY_LE_CODED; + BTM_TRACE_EVENT("%s start=%d", __func__, start); + if (start) { if (p_cb->conn_state == BLE_CONN_IDLE && background_connections_pending() && - btm_ble_topology_check(BTM_BLE_STATE_INIT)) { + btm_ble_topology_check(BTM_BLE_STATE_INIT) && l2cu_can_allocate_lcb()) { p_cb->wl_state |= BTM_BLE_WL_INIT; btm_execute_wl_dev_operation(); @@ -470,9 +514,6 @@ static void btm_suspend_wl_activity(tBTM_BLE_WL_STATE wl_state) { if (wl_state & BTM_BLE_WL_INIT) { btm_ble_start_auto_conn(false); } - if (wl_state & BTM_BLE_WL_ADV) { - btm_ble_stop_adv(); - } } /******************************************************************************* * @@ -485,10 +526,6 @@ static void btm_suspend_wl_activity(tBTM_BLE_WL_STATE wl_state) { ******************************************************************************/ static void btm_resume_wl_activity(tBTM_BLE_WL_STATE wl_state) { btm_ble_resume_bg_conn(); - - if (wl_state & BTM_BLE_WL_ADV) { - btm_ble_start_adv(); - } } /******************************************************************************* * diff --git a/stack/btm/btm_ble_gap.cc b/stack/btm/btm_ble_gap.cc index caf610f3e..be12e6117 100644 --- a/stack/btm/btm_ble_gap.cc +++ b/stack/btm/btm_ble_gap.cc @@ -2417,10 +2417,6 @@ tBTM_STATUS btm_ble_start_adv(void) { /* enable resolving list is desired */ btm_ble_enable_resolving_list_for_platform(BTM_BLE_RL_ADV); #endif - if (p_cb->afp != AP_SCAN_CONN_ALL) { - btm_execute_wl_dev_operation(); - btm_cb.ble_ctr_cb.wl_state |= BTM_BLE_WL_ADV; - } btsnd_hcic_ble_set_adv_enable(BTM_BLE_ADV_ENABLE); p_cb->adv_mode = BTM_BLE_ADV_ENABLE; @@ -2445,7 +2441,6 @@ tBTM_STATUS btm_ble_stop_adv(void) { p_cb->fast_adv_on = false; p_cb->adv_mode = BTM_BLE_ADV_DISABLE; - btm_cb.ble_ctr_cb.wl_state &= ~BTM_BLE_WL_ADV; /* clear all adv states */ btm_ble_clear_topology_mask(BTM_BLE_STATE_ALL_ADV_MASK); @@ -2675,6 +2670,10 @@ void btm_ble_update_mode_operation(uint8_t link_role, BD_ADDR bd_addr, btm_cb.ble_ctr_cb.inq_var.connectable_mode); } + /* in case of disconnected, we must cancel bgconn and restart + in order to add back device to white list in order to reconnect */ + btm_ble_bgconn_cancel_if_disconnected(bd_addr); + /* when no connection is attempted, and controller is not rejecting last request due to resource limitation, start next direct connection or background diff --git a/stack/btm/btm_ble_int.h b/stack/btm/btm_ble_int.h index e645994b1..f7da237ea 100644 --- a/stack/btm/btm_ble_int.h +++ b/stack/btm/btm_ble_int.h @@ -138,6 +138,7 @@ extern void btm_ble_update_mode_operation(uint8_t link_role, BD_ADDR bda, uint8_t status); extern bool btm_execute_wl_dev_operation(void); extern void btm_ble_update_link_topology_mask(uint8_t role, bool increase); +extern void btm_ble_bgconn_cancel_if_disconnected(BD_ADDR bd_addr); /* direct connection utility */ extern bool btm_send_pending_direct_conn(void); diff --git a/stack/btm/btm_ble_int_types.h b/stack/btm/btm_ble_int_types.h index 51b51184e..3e16fea14 100644 --- a/stack/btm/btm_ble_int_types.h +++ b/stack/btm/btm_ble_int_types.h @@ -174,8 +174,6 @@ typedef struct { alarm_t* refresh_raddr_timer; } tBTM_LE_RANDOM_CB; -#define BTM_BLE_MAX_BG_CONN_DEV_NUM 10 - typedef struct { uint16_t min_conn_int; uint16_t max_conn_int; @@ -194,7 +192,6 @@ typedef struct { /* white list using state as a bit mask */ #define BTM_BLE_WL_IDLE 0 #define BTM_BLE_WL_INIT 1 -#define BTM_BLE_WL_ADV 4 typedef uint8_t tBTM_BLE_WL_STATE; /* resolving list using state as a bit mask */ @@ -300,7 +297,6 @@ typedef struct { uint32_t scan_win; /* white list information */ - uint8_t white_list_avail_size; tBTM_BLE_WL_STATE wl_state; fixed_queue_t* conn_pending_q; @@ -321,8 +317,6 @@ typedef struct { tBTM_BLE_RL_STATE rl_state; /* Resolving list state */ #endif - tBTM_BLE_WL_OP wl_op_q[BTM_BLE_MAX_BG_CONN_DEV_NUM]; - /* current BLE link state */ tBTM_BLE_STATE_MASK cur_states; /* bit mask of tBTM_BLE_STATE */ uint8_t link_count[2]; /* total link count master and slave*/ diff --git a/stack/l2cap/l2c_ble.cc b/stack/l2cap/l2c_ble.cc index d15a87a23..07f810564 100644 --- a/stack/l2cap/l2c_ble.cc +++ b/stack/l2cap/l2c_ble.cc @@ -292,6 +292,7 @@ void l2cble_scanner_conn_comp(uint16_t handle, BD_ADDR bda, tBLE_ADDR_TYPE type, if (!p_lcb) { btm_sec_disconnect(handle, HCI_ERR_NO_CONNECTION); L2CAP_TRACE_ERROR("l2cble_scanner_conn_comp - failed to allocate LCB"); + btm_ble_set_conn_st(BLE_CONN_IDLE); return; } else { if (!l2cu_initialize_fixed_ccb( @@ -300,12 +301,14 @@ void l2cble_scanner_conn_comp(uint16_t handle, BD_ADDR bda, tBLE_ADDR_TYPE type, .fixed_chnl_opts)) { btm_sec_disconnect(handle, HCI_ERR_NO_CONNECTION); L2CAP_TRACE_WARNING("l2cble_scanner_conn_comp - LCB but no CCB"); + btm_ble_set_conn_st(BLE_CONN_IDLE); return; } } } else if (p_lcb->link_state != LST_CONNECTING) { L2CAP_TRACE_ERROR("L2CAP got BLE scanner conn_comp in bad state: %d", p_lcb->link_state); + btm_ble_set_conn_st(BLE_CONN_IDLE); return; } alarm_cancel(p_lcb->l2c_lcb_timer); diff --git a/stack/l2cap/l2c_int.h b/stack/l2cap/l2c_int.h index d6758bbc2..c01e958d3 100644 --- a/stack/l2cap/l2c_int.h +++ b/stack/l2cap/l2c_int.h @@ -580,6 +580,7 @@ extern void l2c_process_held_packets(bool timed_out); /* Functions provided by l2c_utils.cc *********************************** */ +extern bool l2cu_can_allocate_lcb(void); extern tL2C_LCB* l2cu_allocate_lcb(BD_ADDR p_bd_addr, bool is_bonding, tBT_TRANSPORT transport); extern bool l2cu_start_post_bond_timer(uint16_t handle); diff --git a/stack/l2cap/l2c_utils.cc b/stack/l2cap/l2c_utils.cc index f6728d6bc..ba4ba3f6a 100644 --- a/stack/l2cap/l2c_utils.cc +++ b/stack/l2cap/l2c_utils.cc @@ -43,6 +43,22 @@ extern fixed_queue_t* btu_general_alarm_queue; /******************************************************************************* * + * Function l2cu_can_allocate_lcb + * + * Description Look for an unused LCB + * + * Returns true if there is space for one more lcb + * + ******************************************************************************/ +bool l2cu_can_allocate_lcb(void) { + for (int i = 0; i < MAX_L2CAP_LINKS; i++) { + if (!l2cb.lcb_pool[i].in_use) return true; + } + return false; +} + +/******************************************************************************* + * * Function l2cu_allocate_lcb * * Description Look for an unused LCB -- 2.11.0