| Message ID | 20201229143408.Bluez.v1.1.I7978a075910600058245dc6891c614cf4c7b004e@changeid |
|---|---|
| State | New |
| Headers | show |
| Series | [Bluez,v1] adapter: Don't remove device if adapter is powered off | expand |
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
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
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
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
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
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);