Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210323141653.1.I53e6be1f7df0be198b7e55ae9fc45c7f5760132d@changeid>
 <8E70C497-BDCE-471F-9ECD-790E2FE3B024@holtmann.org>
In-Reply-To: <8E70C497-BDCE-471F-9ECD-790E2FE3B024@holtmann.org>
From:   Daniel Winkler <danielwinkler@google.com>
Date:   Wed, 24 Mar 2021 11:49:02 -0700
Message-ID: <CAP2xMbseUhKhxdw93Q320euOdUZ39GtpiHNy49m_p0U7j58u8Q@mail.gmail.com>
Subject: Re: [PATCH] Bluetooth: Always call advertising disable before setting params
To:     Marcel Holtmann <marcel@holtmann.org>
Cc:     linux-bluetooth <linux-bluetooth@vger.kernel.org>,
        CrosBT Upstreaming <chromeos-bluetooth-upstreaming@chromium.org>,
        Miao-chen Chou <mcchou@chromium.org>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Johan Hedberg <johan.hedberg@gmail.com>,
        Luiz Augusto von Dentz <luiz.dentz@gmail.com>,
        LKML <linux-kernel@vger.kernel.org>,
        "open list:NETWORKING [GENERAL]" <netdev@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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> wrot=
e:
>
> Hi Daniel,
>
> > In __hci_req_enable_advertising, the HCI_LE_ADV hdev flag is temporaril=
y
> > 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 b=
e
> > very temporarily out-of-sync. According to the spec, there is no harm i=
n
> > 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_req=
uest *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 call=
ed
> > +      * whether or not there is an active advertisement.
> > +      */
> > +     __hci_req_disable_advertising(req);
>
> can you include a btmon trace that shows that we don=E2=80=99t get a HCI =
error. Since if we get one, then the complete request will fail. And that h=
as further side effects.
>
> Regards
>
> Marcel
>