Bluetooth: Fix deadlock with settings and bt_hci_cmd_send_sync()

We can't do synchronous HCI command sending in any settings commit()
callback, since we don't know what context the callback is being called
from. One particular deadlock can happen if settings_load() is called from
the preemptible main thread:

main()
 |--> settings_load() (aquire settings_lock mutex)
       |-->commit callback A that defers to system wq
       |-->commit callback B that calls bt_hci_cmd_send_sync()

system wq from the previous deferral from within settings_load():
 |--> work item
       |--> settings_save_one()
             |--> attempt to aquire settings_lock mutex

In the above scenario, the bt_hci_cmd_send_sync() call from the main thread
depends on the system workqueue being processed (since that's what does HCI
command processing by default), while at the same time holding the settings
subsystem's mutex. At the same time, a system wq item tries to store
something into settings, however it deadlocks waiting for the settings
mutex.

The actual scenario that we have in the Bluetooth subsystem is where
"commit callback A" is commit_settings() in host/settings.c, and "commit
callback B" is keys_commit() in host/keys.c.

The solution to the deadlock is to take advantage of deferred bt_id_add()
handling which already exists, i.e. set a flag and deferre the actual
adding to the system workqueue.

Signed-off-by: Johan Hedberg <[email protected]>
(cherry picked from commit 39c2605)
Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Author: jhedberg

subsys/bluetooth/host/id.c

@@ -866,7 +866,7 @@ static void pending_id_update(struct bt_keys *keys, void *data)
 	}
 }
 
-static void bt_id_pending_keys_update_set(struct bt_keys *keys, uint8_t flag)
+void bt_id_pending_keys_update_set(struct bt_keys *keys, uint8_t flag)
 {
 	atomic_set_bit(bt_dev.flags, BT_DEV_ID_PENDING);
 	keys->state |= flag;

subsys/bluetooth/host/id.h

@@ -5,6 +5,8 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
+#include "keys.h"
+
 #define RPA_TIMEOUT_MS(_rpa_timeout) (_rpa_timeout * MSEC_PER_SEC)
 
 static inline bool bt_id_rpa_is_new(void)
@@ -44,4 +46,6 @@ int bt_id_set_private_addr(uint8_t id);
 
 void bt_id_pending_keys_update(void);
 
+void bt_id_pending_keys_update_set(struct bt_keys *keys, uint8_t flag);
+
 void bt_id_adv_limited_stopped(struct bt_le_ext_adv *adv);

subsys/bluetooth/host/keys.c

@@ -28,6 +28,7 @@
 #include "hci_core.h"
 #include "smp.h"
 #include "settings.h"
+#include "id.h"
 #include "keys.h"
 
 #define LOG_LEVEL CONFIG_BT_KEYS_LOG_LEVEL
@@ -426,11 +427,19 @@ static int keys_set(const char *name, size_t len_rd, settings_read_cb read_cb,
 	return 0;
 }
 
+static void add_id_cb(struct k_work *work)
+{
+	bt_id_pending_keys_update();
+}
+
+static K_WORK_DEFINE(add_id_work, add_id_cb);
+
 static void id_add(struct bt_keys *keys, void *user_data)
 {
 	__ASSERT_NO_MSG(keys != NULL);
 
-	bt_id_add(keys);
+	bt_id_pending_keys_update_set(keys, BT_KEYS_ID_PENDING_ADD);
+	k_work_submit(&add_id_work);
 }
 
 static int keys_commit(void)

tests/bluetooth/host/keys/mocks/id.c

@@ -8,3 +8,6 @@
 #include "mocks/id.h"
 
 DEFINE_FAKE_VOID_FUNC(bt_id_del, struct bt_keys *);
+DEFINE_FAKE_VOID_FUNC(bt_id_pending_keys_update);
+DEFINE_FAKE_VOID_FUNC(bt_id_pending_keys_update_set, struct bt_keys *, uint8_t);
+DEFINE_FAKE_VALUE_FUNC(int, k_work_submit, struct k_work *);

tests/bluetooth/host/keys/mocks/id.h

@@ -10,7 +10,13 @@
 #include <host/keys.h>
 
 /* List of fakes used by this unit tester */
-#define ID_FFF_FAKES_LIST(FAKE)        \
-		FAKE(bt_id_del)                \
+#define ID_FFF_FAKES_LIST(FAKE)                     \
+		FAKE(bt_id_del)                     \
+		FAKE(bt_id_pending_keys_update)     \
+		FAKE(bt_id_pending_keys_update_set) \
+		FAKE(k_work_submit)
 
 DECLARE_FAKE_VOID_FUNC(bt_id_del, struct bt_keys *);
+DECLARE_FAKE_VOID_FUNC(bt_id_pending_keys_update);
+DECLARE_FAKE_VOID_FUNC(bt_id_pending_keys_update_set, struct bt_keys *, uint8_t);
+DECLARE_FAKE_VALUE_FUNC(int, k_work_submit, struct k_work *);