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

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

[Archie Pusaka]

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.

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;
+
 	adapter->connect_list = g_slist_remove(adapter->connect_list, dev);
 
 	adapter->devices = g_slist_remove(adapter->devices, dev);
-- 
2.29.2.729.g45daf8777d-goog

RE: [Bluez,v1] adapter: Don't remove device if adapter is powered off

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 557 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=407063

---Test result---

##############################
Test: CheckPatch - PASS

##############################
Test: CheckGitLint - PASS

##############################
Test: CheckBuild - PASS

##############################
Test: MakeCheck - PASS



---
Regards,
Linux Bluetooth

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

[Luiz Augusto von Dentz]

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.

>         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

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

[Luiz Augusto von Dentz]

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.

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.

> >         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

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

[Archie Pusaka]

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

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

[Luiz Augusto von Dentz]

Hi Archie,

On Mon, Jan 4, 2021 at 7:17 PM Archie Pusaka <apusaka@google.com> wrote:
>
> 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.

Right, Id guess if RemoveDevice is called and either way we end up
powering off the adapter I suppose we want the device removed either
way thus why I suggested cleaning up the temporary devices before
powering down.

> > > >         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



-- 
Luiz Augusto von Dentz

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

[Archie Pusaka]

Hi Luiz,

Thanks for the suggestion. I had sent another patch (titled "adapter:
Remove temporary devices before power off") which implements your
idea.
That should make this patch obsolete.

Thanks,
Archie

On Wed, 6 Jan 2021 at 02:33, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Archie,
>
> On Mon, Jan 4, 2021 at 7:17 PM Archie Pusaka <apusaka@google.com> wrote:
> >
> > 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.
>
> Right, Id guess if RemoveDevice is called and either way we end up
> powering off the adapter I suppose we want the device removed either
> way thus why I suggested cleaning up the temporary devices before
> powering down.
>
> > > > >         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
>
>
>
> --
> Luiz Augusto von Dentz