| Message ID | 20210323141653.1.I53e6be1f7df0be198b7e55ae9fc45c7f5760132d@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Bluetooth: Always call advertising disable before setting params | expand |
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
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 >
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