Re: [Bluez PATCH v1] adapter: Don't remove device if adapter is powered off

[Archie Pusaka <apusaka@google.com>]

Hi Luiz,

Sorry for being unclear.


On Tue, 5 Jan 2021 at 03:17, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Archie,
>
> On Mon, Jan 4, 2021 at 11:09 AM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Archie,
> >
> > On Mon, Dec 28, 2020 at 10:34 PM Archie Pusaka <apusaka@google.com> wrote:
> > >
> > > From: Archie Pusaka <apusaka@chromium.org>
> > >
> > > If adapter is powered off when a device is being removed, there is a
> > > possibility that the kernel couldn't clean the device's information,
> > > for example the pairing information. This causes the kernel to
> > > disagree with the user space about whether the device is paired.
> > >
> > > Therefore, to avoid discrepancy we must not proceed to remove the
> > > device within the user space as well.
> >
> > This sounds like we have a bug in the kernel, aren't we calling
> > btd_adapter_remove_bonding or is that failing if the adapter is not
> > powered? Hmm it does like it:
> >
> > This command can only be used when the controller is powered.
> >
> > > Reviewed-by: Daniel Winkler <danielwinkler@google.com>
> > > ---
> > >
> > >  src/adapter.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > >
> > > diff --git a/src/adapter.c b/src/adapter.c
> > > index ec6a6a64c5..a2abc46706 100644
> > > --- a/src/adapter.c
> > > +++ b/src/adapter.c
> > > @@ -1238,6 +1238,14 @@ void btd_adapter_remove_device(struct btd_adapter *adapter,
> > >  {
> > >         GList *l;
> > >
> > > +       /* Test if adapter is or will be powered off.
> > > +        * This is to prevent removing the device information only on user
> > > +        * space, but failing to do so on the kernel.
> > > +        */
> > > +       if (!(adapter->current_settings & MGMT_SETTING_POWERED) ||
> > > +                       (adapter->pending_settings & MGMT_SETTING_POWERED))
> > > +               return;
> >
> > We might need to return an error here so we can reply with an error on
> > Adapter.RemoveDevice.
>
Should be unnecessary due to the check you mentioned below.

> After some investigation it looks like there is already a similar check:
>
> https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/adapter.c#n3238
>
> That perhaps needs to updated or perhaps this is the result of the
> device being set to temporary which sets a timer to remove the device
> and then in the meantime the adapter is powered off? In that case
> perhaps we should clean up the devices set as temporary.
>
The problem occurs when the device is paired and is currently
connected, then Adapter.RemoveDevice is called. This would make us
disconnect the device first before actually removing the device.
During this disconnection phase, there is a chance the adapter is
powered off.

If this happens, we would still attempt to remove the device anyway.
No problem on the user space, but it will fail on the kernel side (as
per the API, it requires adapter to be on). The check you mentioned is
unfortunately not executed during this phase.

About the timer, I didn't have the exact issue you described, but this
version of patch might have other problems with it, because the timer
would still be running when we do the early return from
btd_adapter_remove_device. Although nothing will happen if the timer
runs out when the power is still off (we would just do the early
return again), but it might be unpleasant if the adapter is re-powered
and the timer kicks off to remove the device.

> > >         adapter->connect_list = g_slist_remove(adapter->connect_list, dev);
> > >
> > >         adapter->devices = g_slist_remove(adapter->devices, dev);
> > > --
> > > 2.29.2.729.g45daf8777d-goog
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz

Thanks,
Archie