[PATCH 0/2] Bluetooth: Avoid centralized adv handle tracking for extended features

[PATCH 0/2] Bluetooth: Avoid centralized adv handle tracking for extended features

[Daniel Winkler]

Hi Maintainers,

This series addresses a race condition where an advertisement
registration can conflict with a software rotation advertisement
refresh. I found that this issue was only occurring with the new
extended MGMT advertising interface. A bad use of the
hdev->cur_adv_instance caused every new instance to be immediately sent
to the controller rather than queued for software rotation, opening a
path for the race to occur.

This series improves the way new extended advertising hci callbacks
track the relevant adv handle, removing the need for the
cur_adv_instance use. In a separate patch, the incorrect usage of
cur_adv_instance is removed, to align the extended MGMT commands to the
original add_advertising usage. The series was tested on both extended
and non-extended bluetooth controllers to confirm that the race
condition is resolved, and that multi- and single-advertising automated
test scenarios are still successful.

Thanks in advance,
Daniel


Daniel Winkler (2):
  Bluetooth: Use ext adv handle from requests in CCs
  Bluetooth: Do not set cur_adv_instance in adv param MGMT request

 net/bluetooth/hci_event.c | 16 +++++++---------
 net/bluetooth/mgmt.c      |  1 -
 2 files changed, 7 insertions(+), 10 deletions(-)

-- 
2.31.0.208.g409f899ff0-goog

[PATCH 1/2] Bluetooth: Use ext adv handle from requests in CCs

[Daniel Winkler]

Some extended advertising hci command complete events are still using
hdev->cur_adv_instance to map the request to the correct advertisement
handle. However, with extended advertising, "current instance" doesn't
make sense as we can have multiple concurrent advertisements. This
change switches these command complete handlers to use the advertising
handle from the request/event, to ensure we will always use the correct
advertising handle regardless of the state of hdev->cur_adv_instance.

This change is tested on hatch and kefka chromebooks and run through
single- and multi-advertising automated tests to confirm callbacks
report tx power to the correct advertising handle, etc.

Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
Signed-off-by: Daniel Winkler <danielwinkler@google.com>
---

 net/bluetooth/hci_event.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 593eafa282e31a..016b2999f21957 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1212,12 +1212,11 @@ static void hci_cc_le_set_adv_set_random_addr(struct hci_dev *hdev,
 
 	hci_dev_lock(hdev);
 
-	if (!hdev->cur_adv_instance) {
+	if (!cp->handle) {
 		/* Store in hdev for instance 0 (Set adv and Directed advs) */
 		bacpy(&hdev->random_addr, &cp->bdaddr);
 	} else {
-		adv_instance = hci_find_adv_instance(hdev,
-						     hdev->cur_adv_instance);
+		adv_instance = hci_find_adv_instance(hdev, cp->handle);
 		if (adv_instance)
 			bacpy(&adv_instance->random_addr, &cp->bdaddr);
 	}
@@ -1778,17 +1777,16 @@ static void hci_cc_set_ext_adv_param(struct hci_dev *hdev, struct sk_buff *skb)
 
 	hci_dev_lock(hdev);
 	hdev->adv_addr_type = cp->own_addr_type;
-	if (!hdev->cur_adv_instance) {
+	if (!cp->handle) {
 		/* Store in hdev for instance 0 */
 		hdev->adv_tx_power = rp->tx_power;
 	} else {
-		adv_instance = hci_find_adv_instance(hdev,
-						     hdev->cur_adv_instance);
+		adv_instance = hci_find_adv_instance(hdev, cp->handle);
 		if (adv_instance)
 			adv_instance->tx_power = rp->tx_power;
 	}
 	/* Update adv data as tx power is known now */
-	hci_req_update_adv_data(hdev, hdev->cur_adv_instance);
+	hci_req_update_adv_data(hdev, cp->handle);
 
 	hci_dev_unlock(hdev);
 }
@@ -5308,12 +5306,12 @@ static void hci_le_ext_adv_term_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		if (hdev->adv_addr_type != ADDR_LE_DEV_RANDOM)
 			return;
 
-		if (!hdev->cur_adv_instance) {
+		if (!ev->handle) {
 			bacpy(&conn->resp_addr, &hdev->random_addr);
 			return;
 		}
 
-		adv_instance = hci_find_adv_instance(hdev, hdev->cur_adv_instance);
+		adv_instance = hci_find_adv_instance(hdev, ev->handle);
 		if (adv_instance)
 			bacpy(&conn->resp_addr, &adv_instance->random_addr);
 	}
-- 
2.31.0.208.g409f899ff0-goog

[PATCH 2/2] Bluetooth: Do not set cur_adv_instance in adv param MGMT request

[Daniel Winkler]

We set hdev->cur_adv_instance in the adv param MGMT request to allow the
callback to the hci param request to set the tx power to the correct
instance. Now that the callbacks use the advertising handle from the hci
request (as they should), this workaround is no longer necessary.

Furthermore, this change resolves a race condition that is more
prevalent when using the extended advertising MGMT calls - if
hdev->cur_adv_instance is set in the params request, then when the data
request is called, we believe our new instance is already active. This
treats it as an update and immediately schedules the instance with the
controller, which has a potential race with the software rotation adv
update. By not setting hdev->cur_adv_instance too early, the new
instance is queued as it should be, to be used when the rotation comes
around again.

This change is tested on harrison peak to confirm that it resolves the
race condition on registration, and that there is no regression in
single- and multi-advertising automated tests.

Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
Signed-off-by: Daniel Winkler <danielwinkler@google.com>
---

 net/bluetooth/mgmt.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 09e099c419f251..59f8016c486626 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -7979,7 +7979,6 @@ static int add_ext_adv_params(struct sock *sk, struct hci_dev *hdev,
 		goto unlock;
 	}
 
-	hdev->cur_adv_instance = cp->instance;
 	/* Submit request for advertising params if ext adv available */
 	if (ext_adv_capable(hdev)) {
 		hci_req_init(&req, hdev);
-- 
2.31.0.208.g409f899ff0-goog

Re: [PATCH 0/2] Bluetooth: Avoid centralized adv handle tracking for extended features

[Marcel Holtmann]

Hi Daniel,

> This series addresses a race condition where an advertisement
> registration can conflict with a software rotation advertisement
> refresh. I found that this issue was only occurring with the new
> extended MGMT advertising interface. A bad use of the
> hdev->cur_adv_instance caused every new instance to be immediately sent
> to the controller rather than queued for software rotation, opening a
> path for the race to occur.
> 
> This series improves the way new extended advertising hci callbacks
> track the relevant adv handle, removing the need for the
> cur_adv_instance use. In a separate patch, the incorrect usage of
> cur_adv_instance is removed, to align the extended MGMT commands to the
> original add_advertising usage. The series was tested on both extended
> and non-extended bluetooth controllers to confirm that the race
> condition is resolved, and that multi- and single-advertising automated
> test scenarios are still successful.
> 
> Thanks in advance,
> Daniel
> 
> 
> Daniel Winkler (2):
>  Bluetooth: Use ext adv handle from requests in CCs
>  Bluetooth: Do not set cur_adv_instance in adv param MGMT request
> 
> net/bluetooth/hci_event.c | 16 +++++++---------
> net/bluetooth/mgmt.c      |  1 -
> 2 files changed, 7 insertions(+), 10 deletions(-)

both patches have been applied to bluetooth-next tree.

Regards

Marcel