* [PATCH] Bluetooth: Always call advertising disable before setting params
@ 2021-03-23 21:18 Daniel Winkler
2021-03-24 7:06 ` Marcel Holtmann
0 siblings, 1 reply; 3+ messages in thread
From: Daniel Winkler @ 2021-03-23 21:18 UTC (permalink / raw)
To: linux-bluetooth
Cc: chromeos-bluetooth-upstreaming, Daniel Winkler, Miao-chen Chou,
David S. Miller, Jakub Kicinski, Johan Hedberg,
Luiz Augusto von Dentz, Marcel Holtmann, linux-kernel, netdev
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 <mcchou@chromium.org>
Signed-off-by: Daniel Winkler <danielwinkler@google.com>
---
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] Bluetooth: Always call advertising disable before setting params
2021-03-23 21:18 [PATCH] Bluetooth: Always call advertising disable before setting params Daniel Winkler
@ 2021-03-24 7:06 ` Marcel Holtmann
2021-03-24 18:49 ` Daniel Winkler
0 siblings, 1 reply; 3+ messages in thread
From: Marcel Holtmann @ 2021-03-24 7:06 UTC (permalink / raw)
To: Daniel Winkler
Cc: linux-bluetooth, CrosBT Upstreaming, Miao-chen Chou,
David S. Miller, Jakub Kicinski, Johan Hedberg,
Luiz Augusto von Dentz, LKML, netdev
Hi Daniel,
> 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 <mcchou@chromium.org>
> Signed-off-by: Daniel Winkler <danielwinkler@google.com>
> ---
>
> 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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Bluetooth: Always call advertising disable before setting params
2021-03-24 7:06 ` Marcel Holtmann
@ 2021-03-24 18:49 ` Daniel Winkler
0 siblings, 0 replies; 3+ messages in thread
From: Daniel Winkler @ 2021-03-24 18:49 UTC (permalink / raw)
To: Marcel Holtmann
Cc: linux-bluetooth, CrosBT Upstreaming, Miao-chen Chou,
David S. Miller, Jakub Kicinski, Johan Hedberg,
Luiz Augusto von Dentz, LKML, open list:NETWORKING [GENERAL]
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 <marcel@holtmann.org> wrote:
>
> Hi Daniel,
>
> > 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 <mcchou@chromium.org>
> > Signed-off-by: Daniel Winkler <danielwinkler@google.com>
> > ---
> >
> > 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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-03-24 18:50 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-23 21:18 [PATCH] Bluetooth: Always call advertising disable before setting params Daniel Winkler
2021-03-24 7:06 ` Marcel Holtmann
2021-03-24 18:49 ` Daniel Winkler
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.