diff mbox series

[BlueZ] Disable auto-connect on cancel pair

Message ID 20200807142429.BlueZ.1.I588558f1e38231193d6e955a4008ce54712d5c89@changeid (mailing list archive)
State Accepted
Delegated to: Luiz Von Dentz
Headers show
Series [BlueZ] Disable auto-connect on cancel pair | expand

Commit Message

Manish Mandlik Aug. 7, 2020, 9:24 p.m. UTC
While pairing process is in progress, service discovery starts in the
background. If HOG profile is detected, auto connect is enabled for
that device. This causes future advertisement from that device to
trigger a pairing even if the user has already cancelled the pairing.

Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>

Signed-off-by: Manish Mandlik <mmandlik@google.com>
---

 src/device.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Luiz Augusto von Dentz Aug. 17, 2020, 5:11 p.m. UTC | #1
Hi Manish,

On Fri, Aug 7, 2020 at 2:24 PM Manish Mandlik <mmandlik@google.com> wrote:
>
> While pairing process is in progress, service discovery starts in the
> background. If HOG profile is detected, auto connect is enabled for
> that device. This causes future advertisement from that device to
> trigger a pairing even if the user has already cancelled the pairing.

So it looks like something is not right if the user cancel the pairing
process I would expect the device to be removed if this happens when
setting up a new device or at least call Disconnect method which would
disable auto_connect if the device is not trusted.

> Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
>
> Signed-off-by: Manish Mandlik <mmandlik@google.com>
> ---
>
>  src/device.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/src/device.c b/src/device.c
> index 470596ee4..ab5bb123e 100644
> --- a/src/device.c
> +++ b/src/device.c
> @@ -2870,6 +2870,15 @@ static void device_cancel_bonding(struct btd_device *device, uint8_t status)
>         if (!bonding)
>                 return;
>
> +       /* Auto connect may get enabled during the service discovery even
> +        * before the pairing process completes. In such case, disable it
> +        * when the user has cancelled the pairing process.
> +        */
> +       if (device->auto_connect) {
> +               device->disable_auto_connect = TRUE;
> +               device_set_auto_connect(device, FALSE);
> +       }

BlueZ has the trusted property so upper layer can actually flag if the
device is trusted regardless if it is paired on not, so this seems out
of place or we should at least check if if the device has been marked
as trusted.
>         ba2str(&device->bdaddr, addr);
>         DBG("Canceling bonding request for %s", addr);
>
> --
> 2.28.0.236.gb10cc79966-goog
>


--
Luiz Augusto von Dentz
Luiz Augusto von Dentz Aug. 17, 2020, 8:52 p.m. UTC | #2
Hi Manish,

On Mon, Aug 17, 2020 at 12:37 PM Manish Mandlik <mmandlik@google.com> wrote:
>
> Hi Luiz,
>
> On Mon, Aug 17, 2020 at 10:11 AM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote:
>>
>> Hi Manish,
>>
>> On Fri, Aug 7, 2020 at 2:24 PM Manish Mandlik <mmandlik@google.com> wrote:
>> >
>> > While pairing process is in progress, service discovery starts in the
>> > background. If HOG profile is detected, auto connect is enabled for
>> > that device. This causes future advertisement from that device to
>> > trigger a pairing even if the user has already cancelled the pairing.
>>
>> So it looks like something is not right if the user cancel the pairing
>> process I would expect the device to be removed if this happens when
>> setting up a new device or at least call Disconnect method which would
>> disable auto_connect if the device is not trusted.
>
>
> Earlier there was another patch related to pairing cancel issue: https://patchwork.kernel.org/patch/11608271/
> This patch terminates the link when the user cancels the pairing process. Once the link is terminated, the device is removed by disconnected_callback()->dev_disconnected()->btd_adapter_remove_device().
>
> However, the device remove code-path doesn't check for or disable autoconnect. In the current bluez code, autoconnect is disabled only if the disconnect is initiated by the user (in dev_disconnect()).
>
> So, instead of disabling autoconnect while cancel pair, do you think we should disable it during device remove (in device_remove())?

I'm not following you on this part, is there something preventing the
device to be removed? If the devices end up being removed so does its
autoconnect state, if that is not happening then that is the culprit
here and should be fixed.

> Or should we disable it in the cancel pair code-path, but with a check if the device is trusted or not as per your suggestion:
>
>         if (device->auto_connect && !device->trusted) {
>                 device->disable_auto_connect = TRUE;
>                 device_set_auto_connect(device, FALSE);
>         }
>
> Please advise.
>
> Thank you.
>
>>
>> > Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
>> >
>> > Signed-off-by: Manish Mandlik <mmandlik@google.com>
>> > ---
>> >
>> >  src/device.c | 9 +++++++++
>> >  1 file changed, 9 insertions(+)
>> >
>> > diff --git a/src/device.c b/src/device.c
>> > index 470596ee4..ab5bb123e 100644
>> > --- a/src/device.c
>> > +++ b/src/device.c
>> > @@ -2870,6 +2870,15 @@ static void device_cancel_bonding(struct btd_device *device, uint8_t status)
>> >         if (!bonding)
>> >                 return;
>> >
>> > +       /* Auto connect may get enabled during the service discovery even
>> > +        * before the pairing process completes. In such case, disable it
>> > +        * when the user has cancelled the pairing process.
>> > +        */
>> > +       if (device->auto_connect) {
>> > +               device->disable_auto_connect = TRUE;
>> > +               device_set_auto_connect(device, FALSE);
>> > +       }
>>
>> BlueZ has the trusted property so upper layer can actually flag if the
>> device is trusted regardless if it is paired on not, so this seems out
>> of place or we should at least check if if the device has been marked
>> as trusted.
>> >         ba2str(&device->bdaddr, addr);
>> >         DBG("Canceling bonding request for %s", addr);
>> >
>> > --
>> > 2.28.0.236.gb10cc79966-goog
>> >
>>
>>
>> --
>> Luiz Augusto von Dentz
diff mbox series

Patch

diff --git a/src/device.c b/src/device.c
index 470596ee4..ab5bb123e 100644
--- a/src/device.c
+++ b/src/device.c
@@ -2870,6 +2870,15 @@  static void device_cancel_bonding(struct btd_device *device, uint8_t status)
 	if (!bonding)
 		return;
 
+	/* Auto connect may get enabled during the service discovery even
+	 * before the pairing process completes. In such case, disable it
+	 * when the user has cancelled the pairing process.
+	 */
+	if (device->auto_connect) {
+		device->disable_auto_connect = TRUE;
+		device_set_auto_connect(device, FALSE);
+	}
+
 	ba2str(&device->bdaddr, addr);
 	DBG("Canceling bonding request for %s", addr);