[PATCH] Bluetooth: Honor name resolve evt regardless of discov state

[PATCH] Bluetooth: Honor name resolve evt regardless of discov state

[Archie Pusaka]

From: Archie Pusaka <apusaka@chromium.org>

Currently, we don't update the name resolving cache when receiving
a name resolve event if the discovery phase is not in the resolving
stage.

However, if the user connect to a device while we are still resolving
remote name for another device, discovery will be stopped, and because
we are no longer in the discovery resolving phase, the corresponding
remote name event will be ignored, and thus the device being resolved
will stuck in NAME_PENDING state.

If discovery is then restarted and then stopped, this will cause us to
try cancelling the name resolve of the same device again, which is
incorrect and might upset the controller.

Signed-off-by: Archie Pusaka <apusaka@chromium.org>
Reviewed-by: Ying Hsu <yinghsu@chromium.org>

---
The following steps are performed:
    (1) Prepare 2 classic peer devices that needs RNR. Put device A
        closer to DUT and device B (much) farther from DUT.
    (2) Remove all cache and previous connection from DUT
    (3) Put both peers into pairing mode, then start scanning on DUT
    (4) After ~8 sec, turn off peer B.
    *This is done so DUT can discover peer B (discovery time is 10s),
    but it hasn't started RNR. Peer is turned off to buy us the max
    time in the RNR phase (5s).
    (5) Immediately as device A is shown on UI, click to connect.
    *We thus know that the DUT is in the RNR phase and trying to
    resolve the name of peer B when we initiate connection to peer A.
    (6) Forget peer A.
    (7) Restart scan and stop scan.
    *Before the CL, stop scan is broken because we will try to cancel
    a nonexistent RNR
    (8) Restart scan again. Observe DUT can scan normally.


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

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 395c6479456f..95e145e278c9 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2453,6 +2453,16 @@ static void hci_check_pending_name(struct hci_dev *hdev, struct hci_conn *conn,
 	    !test_and_set_bit(HCI_CONN_MGMT_CONNECTED, &conn->flags))
 		mgmt_device_connected(hdev, conn, name, name_len);
 
+	e = hci_inquiry_cache_lookup_resolve(hdev, bdaddr, NAME_PENDING);
+
+	if (e) {
+		list_del(&e->list);
+
+		e->name_state = name ? NAME_KNOWN : NAME_NOT_KNOWN;
+		mgmt_remote_name(hdev, bdaddr, ACL_LINK, 0x00, e->data.rssi,
+				 name, name_len);
+	}
+
 	if (discov->state == DISCOVERY_STOPPED)
 		return;
 
@@ -2462,7 +2472,6 @@ static void hci_check_pending_name(struct hci_dev *hdev, struct hci_conn *conn,
 	if (discov->state != DISCOVERY_RESOLVING)
 		return;
 
-	e = hci_inquiry_cache_lookup_resolve(hdev, bdaddr, NAME_PENDING);
 	/* If the device was not found in a list of found devices names of which
 	 * are pending. there is no need to continue resolving a next name as it
 	 * will be done upon receiving another Remote Name Request Complete
@@ -2470,12 +2479,6 @@ static void hci_check_pending_name(struct hci_dev *hdev, struct hci_conn *conn,
 	if (!e)
 		return;
 
-	list_del(&e->list);
-
-	e->name_state = name ? NAME_KNOWN : NAME_NOT_KNOWN;
-	mgmt_remote_name(hdev, bdaddr, ACL_LINK, 0x00, e->data.rssi,
-			 name, name_len);
-
 	if (hci_resolve_next_name(hdev))
 		return;
 
-- 
2.37.1.595.g718a3a8f04-goog

RE: Bluetooth: Honor name resolve evt regardless of discov state

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1096 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=666528

---Test result---

Test Summary:
CheckPatch                    PASS      1.28 seconds
GitLint                       PASS      0.75 seconds
SubjectPrefix                 PASS      0.61 seconds
BuildKernel                   PASS      34.64 seconds
BuildKernel32                 PASS      29.78 seconds
Incremental Build with patchesPASS      42.19 seconds
TestRunner: Setup             PASS      488.42 seconds
TestRunner: l2cap-tester      PASS      16.73 seconds
TestRunner: bnep-tester       PASS      6.27 seconds
TestRunner: mgmt-tester       PASS      99.81 seconds
TestRunner: rfcomm-tester     PASS      9.64 seconds
TestRunner: sco-tester        PASS      9.39 seconds
TestRunner: smp-tester        PASS      9.45 seconds
TestRunner: userchan-tester   PASS      6.51 seconds



---
Regards,
Linux Bluetooth

Re: [PATCH] Bluetooth: Honor name resolve evt regardless of discov state

[Luiz Augusto von Dentz]

Hi Archie,

On Wed, Aug 10, 2022 at 1:47 AM Archie Pusaka <apusaka@google.com> wrote:
>
> From: Archie Pusaka <apusaka@chromium.org>
>
> Currently, we don't update the name resolving cache when receiving
> a name resolve event if the discovery phase is not in the resolving
> stage.
>
> However, if the user connect to a device while we are still resolving
> remote name for another device, discovery will be stopped, and because
> we are no longer in the discovery resolving phase, the corresponding
> remote name event will be ignored, and thus the device being resolved
> will stuck in NAME_PENDING state.
>
> If discovery is then restarted and then stopped, this will cause us to
> try cancelling the name resolve of the same device again, which is
> incorrect and might upset the controller.

Please add the Fixes tag.

> Signed-off-by: Archie Pusaka <apusaka@chromium.org>
> Reviewed-by: Ying Hsu <yinghsu@chromium.org>
>
> ---
> The following steps are performed:
>     (1) Prepare 2 classic peer devices that needs RNR. Put device A
>         closer to DUT and device B (much) farther from DUT.
>     (2) Remove all cache and previous connection from DUT
>     (3) Put both peers into pairing mode, then start scanning on DUT
>     (4) After ~8 sec, turn off peer B.
>     *This is done so DUT can discover peer B (discovery time is 10s),
>     but it hasn't started RNR. Peer is turned off to buy us the max
>     time in the RNR phase (5s).
>     (5) Immediately as device A is shown on UI, click to connect.
>     *We thus know that the DUT is in the RNR phase and trying to
>     resolve the name of peer B when we initiate connection to peer A.
>     (6) Forget peer A.
>     (7) Restart scan and stop scan.
>     *Before the CL, stop scan is broken because we will try to cancel
>     a nonexistent RNR
>     (8) Restart scan again. Observe DUT can scan normally.
>
>
>  net/bluetooth/hci_event.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 395c6479456f..95e145e278c9 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -2453,6 +2453,16 @@ static void hci_check_pending_name(struct hci_dev *hdev, struct hci_conn *conn,
>             !test_and_set_bit(HCI_CONN_MGMT_CONNECTED, &conn->flags))
>                 mgmt_device_connected(hdev, conn, name, name_len);
>
> +       e = hci_inquiry_cache_lookup_resolve(hdev, bdaddr, NAME_PENDING);
> +
> +       if (e) {
> +               list_del(&e->list);
> +
> +               e->name_state = name ? NAME_KNOWN : NAME_NOT_KNOWN;
> +               mgmt_remote_name(hdev, bdaddr, ACL_LINK, 0x00, e->data.rssi,
> +                                name, name_len);
> +       }
> +
>         if (discov->state == DISCOVERY_STOPPED)
>                 return;
>
> @@ -2462,7 +2472,6 @@ static void hci_check_pending_name(struct hci_dev *hdev, struct hci_conn *conn,
>         if (discov->state != DISCOVERY_RESOLVING)
>                 return;
>
> -       e = hci_inquiry_cache_lookup_resolve(hdev, bdaddr, NAME_PENDING);
>         /* If the device was not found in a list of found devices names of which
>          * are pending. there is no need to continue resolving a next name as it
>          * will be done upon receiving another Remote Name Request Complete
> @@ -2470,12 +2479,6 @@ static void hci_check_pending_name(struct hci_dev *hdev, struct hci_conn *conn,
>         if (!e)
>                 return;
>
> -       list_del(&e->list);
> -
> -       e->name_state = name ? NAME_KNOWN : NAME_NOT_KNOWN;
> -       mgmt_remote_name(hdev, bdaddr, ACL_LINK, 0x00, e->data.rssi,
> -                        name, name_len);
> -
>         if (hci_resolve_next_name(hdev))
>                 return;
>
> --
> 2.37.1.595.g718a3a8f04-goog
>


-- 
Luiz Augusto von Dentz

Re: [PATCH] Bluetooth: Honor name resolve evt regardless of discov state

[Archie Pusaka]

Hi Luiz,

On Thu, 11 Aug 2022 at 03:58, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Archie,
>
> On Wed, Aug 10, 2022 at 1:47 AM Archie Pusaka <apusaka@google.com> wrote:
> >
> > From: Archie Pusaka <apusaka@chromium.org>
> >
> > Currently, we don't update the name resolving cache when receiving
> > a name resolve event if the discovery phase is not in the resolving
> > stage.
> >
> > However, if the user connect to a device while we are still resolving
> > remote name for another device, discovery will be stopped, and because
> > we are no longer in the discovery resolving phase, the corresponding
> > remote name event will be ignored, and thus the device being resolved
> > will stuck in NAME_PENDING state.
> >
> > If discovery is then restarted and then stopped, this will cause us to
> > try cancelling the name resolve of the same device again, which is
> > incorrect and might upset the controller.
>
> Please add the Fixes tag.

Unfortunately I don't know when was the issue introduced, I don't even
know whether this is a recent issue or an old one.
Looking back, this part of the code has stayed this way since 2012.
Do I still need to add the fixes tag? If so, does it have to be accurate?

>
> > Signed-off-by: Archie Pusaka <apusaka@chromium.org>
> > Reviewed-by: Ying Hsu <yinghsu@chromium.org>
> >
> > ---
> > The following steps are performed:
> >     (1) Prepare 2 classic peer devices that needs RNR. Put device A
> >         closer to DUT and device B (much) farther from DUT.
> >     (2) Remove all cache and previous connection from DUT
> >     (3) Put both peers into pairing mode, then start scanning on DUT
> >     (4) After ~8 sec, turn off peer B.
> >     *This is done so DUT can discover peer B (discovery time is 10s),
> >     but it hasn't started RNR. Peer is turned off to buy us the max
> >     time in the RNR phase (5s).
> >     (5) Immediately as device A is shown on UI, click to connect.
> >     *We thus know that the DUT is in the RNR phase and trying to
> >     resolve the name of peer B when we initiate connection to peer A.
> >     (6) Forget peer A.
> >     (7) Restart scan and stop scan.
> >     *Before the CL, stop scan is broken because we will try to cancel
> >     a nonexistent RNR
> >     (8) Restart scan again. Observe DUT can scan normally.
> >
> >
> >  net/bluetooth/hci_event.c | 17 ++++++++++-------
> >  1 file changed, 10 insertions(+), 7 deletions(-)
> >
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index 395c6479456f..95e145e278c9 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -2453,6 +2453,16 @@ static void hci_check_pending_name(struct hci_dev *hdev, struct hci_conn *conn,
> >             !test_and_set_bit(HCI_CONN_MGMT_CONNECTED, &conn->flags))
> >                 mgmt_device_connected(hdev, conn, name, name_len);
> >
> > +       e = hci_inquiry_cache_lookup_resolve(hdev, bdaddr, NAME_PENDING);
> > +
> > +       if (e) {
> > +               list_del(&e->list);
> > +
> > +               e->name_state = name ? NAME_KNOWN : NAME_NOT_KNOWN;
> > +               mgmt_remote_name(hdev, bdaddr, ACL_LINK, 0x00, e->data.rssi,
> > +                                name, name_len);
> > +       }
> > +
> >         if (discov->state == DISCOVERY_STOPPED)
> >                 return;
> >
> > @@ -2462,7 +2472,6 @@ static void hci_check_pending_name(struct hci_dev *hdev, struct hci_conn *conn,
> >         if (discov->state != DISCOVERY_RESOLVING)
> >                 return;
> >
> > -       e = hci_inquiry_cache_lookup_resolve(hdev, bdaddr, NAME_PENDING);
> >         /* If the device was not found in a list of found devices names of which
> >          * are pending. there is no need to continue resolving a next name as it
> >          * will be done upon receiving another Remote Name Request Complete
> > @@ -2470,12 +2479,6 @@ static void hci_check_pending_name(struct hci_dev *hdev, struct hci_conn *conn,
> >         if (!e)
> >                 return;
> >
> > -       list_del(&e->list);
> > -
> > -       e->name_state = name ? NAME_KNOWN : NAME_NOT_KNOWN;
> > -       mgmt_remote_name(hdev, bdaddr, ACL_LINK, 0x00, e->data.rssi,
> > -                        name, name_len);
> > -
> >         if (hci_resolve_next_name(hdev))
> >                 return;
> >
> > --
> > 2.37.1.595.g718a3a8f04-goog
> >
>
>
> --
> Luiz Augusto von Dentz

Thanks,
Archie

Re: [PATCH] Bluetooth: Honor name resolve evt regardless of discov state

[Luiz Augusto von Dentz]

Hi Archie,

On Thu, Aug 11, 2022 at 12:00 AM Archie Pusaka <apusaka@google.com> wrote:
>
> Hi Luiz,
>
> On Thu, 11 Aug 2022 at 03:58, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Archie,
> >
> > On Wed, Aug 10, 2022 at 1:47 AM Archie Pusaka <apusaka@google.com> wrote:
> > >
> > > From: Archie Pusaka <apusaka@chromium.org>
> > >
> > > Currently, we don't update the name resolving cache when receiving
> > > a name resolve event if the discovery phase is not in the resolving
> > > stage.
> > >
> > > However, if the user connect to a device while we are still resolving
> > > remote name for another device, discovery will be stopped, and because
> > > we are no longer in the discovery resolving phase, the corresponding
> > > remote name event will be ignored, and thus the device being resolved
> > > will stuck in NAME_PENDING state.
> > >
> > > If discovery is then restarted and then stopped, this will cause us to
> > > try cancelling the name resolve of the same device again, which is
> > > incorrect and might upset the controller.
> >
> > Please add the Fixes tag.
>
> Unfortunately I don't know when was the issue introduced, I don't even
> know whether this is a recent issue or an old one.
> Looking back, this part of the code has stayed this way since 2012.
> Do I still need to add the fixes tag? If so, does it have to be accurate?

Hmm I thought this was related to some recent changes of RNR, we will
need to trace back with git blame, note it important to have the fixes
tag so we can add this change to stable kernels and if that is really
since 2012 that have this bug it probably even more important since it
might be applied to more stable versions.

> >
> > > Signed-off-by: Archie Pusaka <apusaka@chromium.org>
> > > Reviewed-by: Ying Hsu <yinghsu@chromium.org>
> > >
> > > ---
> > > The following steps are performed:
> > >     (1) Prepare 2 classic peer devices that needs RNR. Put device A
> > >         closer to DUT and device B (much) farther from DUT.
> > >     (2) Remove all cache and previous connection from DUT
> > >     (3) Put both peers into pairing mode, then start scanning on DUT
> > >     (4) After ~8 sec, turn off peer B.
> > >     *This is done so DUT can discover peer B (discovery time is 10s),
> > >     but it hasn't started RNR. Peer is turned off to buy us the max
> > >     time in the RNR phase (5s).
> > >     (5) Immediately as device A is shown on UI, click to connect.
> > >     *We thus know that the DUT is in the RNR phase and trying to
> > >     resolve the name of peer B when we initiate connection to peer A.
> > >     (6) Forget peer A.
> > >     (7) Restart scan and stop scan.
> > >     *Before the CL, stop scan is broken because we will try to cancel
> > >     a nonexistent RNR
> > >     (8) Restart scan again. Observe DUT can scan normally.
> > >
> > >
> > >  net/bluetooth/hci_event.c | 17 ++++++++++-------
> > >  1 file changed, 10 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > > index 395c6479456f..95e145e278c9 100644
> > > --- a/net/bluetooth/hci_event.c
> > > +++ b/net/bluetooth/hci_event.c
> > > @@ -2453,6 +2453,16 @@ static void hci_check_pending_name(struct hci_dev *hdev, struct hci_conn *conn,
> > >             !test_and_set_bit(HCI_CONN_MGMT_CONNECTED, &conn->flags))
> > >                 mgmt_device_connected(hdev, conn, name, name_len);
> > >
> > > +       e = hci_inquiry_cache_lookup_resolve(hdev, bdaddr, NAME_PENDING);
> > > +
> > > +       if (e) {
> > > +               list_del(&e->list);
> > > +
> > > +               e->name_state = name ? NAME_KNOWN : NAME_NOT_KNOWN;
> > > +               mgmt_remote_name(hdev, bdaddr, ACL_LINK, 0x00, e->data.rssi,
> > > +                                name, name_len);
> > > +       }
> > > +
> > >         if (discov->state == DISCOVERY_STOPPED)
> > >                 return;
> > >
> > > @@ -2462,7 +2472,6 @@ static void hci_check_pending_name(struct hci_dev *hdev, struct hci_conn *conn,
> > >         if (discov->state != DISCOVERY_RESOLVING)
> > >                 return;
> > >
> > > -       e = hci_inquiry_cache_lookup_resolve(hdev, bdaddr, NAME_PENDING);
> > >         /* If the device was not found in a list of found devices names of which
> > >          * are pending. there is no need to continue resolving a next name as it
> > >          * will be done upon receiving another Remote Name Request Complete
> > > @@ -2470,12 +2479,6 @@ static void hci_check_pending_name(struct hci_dev *hdev, struct hci_conn *conn,
> > >         if (!e)
> > >                 return;
> > >
> > > -       list_del(&e->list);
> > > -
> > > -       e->name_state = name ? NAME_KNOWN : NAME_NOT_KNOWN;
> > > -       mgmt_remote_name(hdev, bdaddr, ACL_LINK, 0x00, e->data.rssi,
> > > -                        name, name_len);
> > > -
> > >         if (hci_resolve_next_name(hdev))
> > >                 return;
> > >
> > > --
> > > 2.37.1.595.g718a3a8f04-goog
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz
>
> Thanks,
> Archie



-- 
Luiz Augusto von Dentz