Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200807142429.BlueZ.1.I588558f1e38231193d6e955a4008ce54712d5c89@changeid>
 <CABBYNZL4s151e-a+X0fEA73RM54aA--m+on7as4dLrJX0xevQQ@mail.gmail.com> <CAGPPCLDFUgPU+XqNPxUBgH9kKUSEb+DvFO0PhAKTUZUToXCi4Q@mail.gmail.com>
In-Reply-To: <CAGPPCLDFUgPU+XqNPxUBgH9kKUSEb+DvFO0PhAKTUZUToXCi4Q@mail.gmail.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Mon, 17 Aug 2020 13:52:30 -0700
Message-ID: <CABBYNZK_dvQPetyJ=fi_2u3bLzLe9bayBD4PZ7yL0KskN_2nhA@mail.gmail.com>
Subject: Re: [BlueZ PATCH] Disable auto-connect on cancel pair
To:     Manish Mandlik <mmandlik@google.com>
Cc:     Marcel Holtmann <marcel@holtmann.org>,
        Alain Michaud <alainm@chromium.org>,
        ChromeOS Bluetooth Upstreaming 
        <chromeos-bluetooth-upstreaming@chromium.org>,
        Sonny Sasaka <sonnysasaka@chromium.org>,
        "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

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


-- 
Luiz Augusto von Dentz