[PATCH] Bluetooth: Always call advertising disable before setting params

In __hci_req_enable_advertising, the HCI_LE_ADV hdev flag is temporarily
cleared to allow the random address to be set, which exposes a race
condition when an advertisement is configured immediately (<10ms) after
software rotation starts to refresh an advertisement.

In normal operation, the HCI_LE_ADV flag is updated as follows:

1. adv_timeout_expire is called, HCI_LE_ADV gets cleared in
   __hci_req_enable_advertising, but hci_req configures an enable
   request
2. hci_req is run, enable callback re-sets HCI_LE_ADV flag

However, in this race condition, the following occurs:

1. adv_timeout_expire is called, HCI_LE_ADV gets cleared in
   __hci_req_enable_advertising, but hci_req configures an enable
   request
2. add_advertising is called, which also calls
   __hci_req_enable_advertising. Because HCI_LE_ADV was cleared in Step
   1, no "disable" command is queued.
3. hci_req for adv_timeout_expire is run, which enables advertising and
   re-sets HCI_LE_ADV
4. hci_req for add_advertising is run, but because no "disable" command
   was queued, we try to set advertising parameters while advertising is
   active, causing a Command Disallowed error, failing the registration.

To resolve the issue, this patch removes the check for the HCI_LE_ADV
flag, and always queues the "disable" request, since HCI_LE_ADV could be
very temporarily out-of-sync. According to the spec, there is no harm in
calling "disable" when advertising is not active.

Reviewed-by: Miao-chen Chou <redacted>
Signed-off-by: Daniel Winkler <redacted>
---

 net/bluetooth/hci_request.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
index 8ace5d34b01efe..2b4b99f4cedf21 100644
--- a/net/bluetooth/hci_request.c
+++ b/net/bluetooth/hci_request.c

@@ -1547,8 +1547,10 @@ void __hci_req_enable_advertising(struct hci_request *req)
 	if (!is_advertising_allowed(hdev, connectable))
 		return;
 
-	if (hci_dev_test_flag(hdev, HCI_LE_ADV))
-		__hci_req_disable_advertising(req);
+	/* Request that the controller stop advertising. This can be called
+	 * whether or not there is an active advertisement.
+	 */
+	__hci_req_disable_advertising(req);
 
 	/* Clear the HCI_LE_ADV bit temporarily so that the
 	 * hci_update_random_address knows that it's safe to go ahead

-- 
2.31.0.291.g576ba9dcdaf-goog

Hi Daniel,

can you include a btmon trace that shows that we don’t get a HCI error. Since if we get one, then the complete request will fail. And that has further side effects.

Regards

Marcel

Hello Marcel,

Thank you for the feedback. I have just sent a V2 with a btmon snippet
showing the HCI Set Advertising Parameters "Command Disallowed"
failure that occurs as a result of this issue. I tried to provide some
annotation for context. Please take a look.

Thanks!
Daniel


On Wed, Mar 24, 2021 at 12:06 AM Marcel Holtmann [off-list ref] wrote:

Hi Daniel,

quoted

In __hci_req_enable_advertising, the HCI_LE_ADV hdev flag is temporarily
cleared to allow the random address to be set, which exposes a race
condition when an advertisement is configured immediately (<10ms) after
software rotation starts to refresh an advertisement.

In normal operation, the HCI_LE_ADV flag is updated as follows:

1. adv_timeout_expire is called, HCI_LE_ADV gets cleared in
  __hci_req_enable_advertising, but hci_req configures an enable
  request
2. hci_req is run, enable callback re-sets HCI_LE_ADV flag

However, in this race condition, the following occurs:

1. adv_timeout_expire is called, HCI_LE_ADV gets cleared in
  __hci_req_enable_advertising, but hci_req configures an enable
  request
2. add_advertising is called, which also calls
  __hci_req_enable_advertising. Because HCI_LE_ADV was cleared in Step
  1, no "disable" command is queued.
3. hci_req for adv_timeout_expire is run, which enables advertising and
  re-sets HCI_LE_ADV
4. hci_req for add_advertising is run, but because no "disable" command
  was queued, we try to set advertising parameters while advertising is
  active, causing a Command Disallowed error, failing the registration.

To resolve the issue, this patch removes the check for the HCI_LE_ADV
flag, and always queues the "disable" request, since HCI_LE_ADV could be
very temporarily out-of-sync. According to the spec, there is no harm in
calling "disable" when advertising is not active.

Reviewed-by: Miao-chen Chou <redacted>
Signed-off-by: Daniel Winkler <redacted>
---

net/bluetooth/hci_request.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
index 8ace5d34b01efe..2b4b99f4cedf21 100644
--- a/net/bluetooth/hci_request.c
+++ b/net/bluetooth/hci_request.c

@@ -1547,8 +1547,10 @@ void __hci_req_enable_advertising(struct hci_request *req)
      if (!is_advertising_allowed(hdev, connectable))
              return;

-     if (hci_dev_test_flag(hdev, HCI_LE_ADV))
-             __hci_req_disable_advertising(req);
+     /* Request that the controller stop advertising. This can be called
+      * whether or not there is an active advertisement.
+      */
+     __hci_req_disable_advertising(req);

can you include a btmon trace that shows that we don’t get a HCI error. Since if we get one, then the complete request will fail. And that has further side effects.

Regards

Marcel