diff mbox series

[Bluez,v1] adapter: Remove temporary devices before power off

Message ID 20210106172647.Bluez.v1.1.If4056891c866780eba7b210887c6db468b8b55e9@changeid (mailing list archive)
State Accepted
Delegated to: Luiz Von Dentz
Headers show
Series [Bluez,v1] adapter: Remove temporary devices before power off | expand

Commit Message

Archie Pusaka Jan. 6, 2021, 9:26 a.m. UTC
From: Archie Pusaka <apusaka@chromium.org>

If adapter is powered off when a currently connected device is
being removed, there is a possibility that we haven't finish waiting
for the disconnection but the adapter is already powered down.

When this happens, the kernel would fail to clean the device's
information, for example the pairing information. This causes
disagreement between the user space and the kernel about whether the
device is already paired, because the device is successfully removed
from the user space's perspective.

This patch enforces the removal of such devices before allowing the
adapter to power off.
---

 src/adapter.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

Comments

bluez.test.bot@gmail.com Jan. 6, 2021, 9:51 a.m. UTC | #1
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=409877

---Test result---

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

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

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

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



---
Regards,
Linux Bluetooth
Luiz Augusto von Dentz Jan. 6, 2021, 6:24 p.m. UTC | #2
Hi Archie,

On Wed, Jan 6, 2021 at 1:27 AM Archie Pusaka <apusaka@google.com> wrote:
>
> From: Archie Pusaka <apusaka@chromium.org>
>
> If adapter is powered off when a currently connected device is
> being removed, there is a possibility that we haven't finish waiting
> for the disconnection but the adapter is already powered down.
>
> When this happens, the kernel would fail to clean the device's
> information, for example the pairing information. This causes
> disagreement between the user space and the kernel about whether the
> device is already paired, because the device is successfully removed
> from the user space's perspective.
>
> This patch enforces the removal of such devices before allowing the
> adapter to power off.
> ---
>
>  src/adapter.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/src/adapter.c b/src/adapter.c
> index ec6a6a64c5..92d1cb2232 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -517,6 +517,7 @@ static void adapter_stop(struct btd_adapter *adapter);
>  static void trigger_passive_scanning(struct btd_adapter *adapter);
>  static bool set_mode(struct btd_adapter *adapter, uint16_t opcode,
>                                                         uint8_t mode);
> +static void remove_temporary_devices(struct btd_adapter *adapter);

I would have the function above declared just before the first user
that way we don't have to use a forward declaration like above.

>  static void settings_changed(struct btd_adapter *adapter, uint32_t settings)
>  {
> @@ -622,6 +623,8 @@ static bool set_mode(struct btd_adapter *adapter, uint16_t opcode,
>         switch (mode) {
>         case MGMT_OP_SET_POWERED:
>                 setting = MGMT_SETTING_POWERED;
> +               if (!mode)
> +                       remove_temporary_devices(adapter);
>                 break;
>         case MGMT_OP_SET_CONNECTABLE:
>                 setting = MGMT_SETTING_CONNECTABLE;
> @@ -2888,8 +2891,10 @@ static void property_set_mode(struct btd_adapter *adapter, uint32_t setting,
>                 param = &mode;
>                 len = sizeof(mode);
>
> -               if (!mode)
> +               if (!mode) {
>                         clear_discoverable(adapter);
> +                       remove_temporary_devices(adapter);
> +               }
>
>                 break;
>         case MGMT_SETTING_DISCOVERABLE:
> @@ -5304,6 +5309,19 @@ static void remove_discovery_list(struct btd_adapter *adapter)
>         adapter->discovery_list = NULL;
>  }
>
> +static void remove_temporary_devices(struct btd_adapter *adapter)
> +{
> +       GSList *l, *next;
> +
> +       for (l = adapter->devices; l; l = next) {
> +               struct btd_device *dev = l->data;
> +
> +               next = g_slist_next(l);
> +               if (device_is_temporary(dev))
> +                       btd_adapter_remove_device(adapter, dev);
> +       }
> +}
> +
>  static void adapter_free(gpointer user_data)
>  {
>         struct btd_adapter *adapter = user_data;
> --
> 2.29.2.729.g45daf8777d-goog
>
Luiz Augusto von Dentz Jan. 6, 2021, 7:05 p.m. UTC | #3
Hi Archie,

On Wed, Jan 6, 2021 at 10:24 AM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Archie,
>
> On Wed, Jan 6, 2021 at 1:27 AM Archie Pusaka <apusaka@google.com> wrote:
> >
> > From: Archie Pusaka <apusaka@chromium.org>
> >
> > If adapter is powered off when a currently connected device is
> > being removed, there is a possibility that we haven't finish waiting
> > for the disconnection but the adapter is already powered down.
> >
> > When this happens, the kernel would fail to clean the device's
> > information, for example the pairing information. This causes
> > disagreement between the user space and the kernel about whether the
> > device is already paired, because the device is successfully removed
> > from the user space's perspective.
> >
> > This patch enforces the removal of such devices before allowing the
> > adapter to power off.
> > ---
> >
> >  src/adapter.c | 20 +++++++++++++++++++-
> >  1 file changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/adapter.c b/src/adapter.c
> > index ec6a6a64c5..92d1cb2232 100644
> > --- a/src/adapter.c
> > +++ b/src/adapter.c
> > @@ -517,6 +517,7 @@ static void adapter_stop(struct btd_adapter *adapter);
> >  static void trigger_passive_scanning(struct btd_adapter *adapter);
> >  static bool set_mode(struct btd_adapter *adapter, uint16_t opcode,
> >                                                         uint8_t mode);
> > +static void remove_temporary_devices(struct btd_adapter *adapter);
>
> I would have the function above declared just before the first user
> that way we don't have to use a forward declaration like above.
>
> >  static void settings_changed(struct btd_adapter *adapter, uint32_t settings)
> >  {
> > @@ -622,6 +623,8 @@ static bool set_mode(struct btd_adapter *adapter, uint16_t opcode,
> >         switch (mode) {
> >         case MGMT_OP_SET_POWERED:
> >                 setting = MGMT_SETTING_POWERED;
> > +               if (!mode)
> > +                       remove_temporary_devices(adapter);
> >                 break;
> >         case MGMT_OP_SET_CONNECTABLE:
> >                 setting = MGMT_SETTING_CONNECTABLE;
> > @@ -2888,8 +2891,10 @@ static void property_set_mode(struct btd_adapter *adapter, uint32_t setting,
> >                 param = &mode;
> >                 len = sizeof(mode);
> >
> > -               if (!mode)
> > +               if (!mode) {
> >                         clear_discoverable(adapter);
> > +                       remove_temporary_devices(adapter);
> > +               }
> >
> >                 break;
> >         case MGMT_SETTING_DISCOVERABLE:
> > @@ -5304,6 +5309,19 @@ static void remove_discovery_list(struct btd_adapter *adapter)
> >         adapter->discovery_list = NULL;
> >  }
> >
> > +static void remove_temporary_devices(struct btd_adapter *adapter)
> > +{
> > +       GSList *l, *next;
> > +
> > +       for (l = adapter->devices; l; l = next) {
> > +               struct btd_device *dev = l->data;
> > +
> > +               next = g_slist_next(l);
> > +               if (device_is_temporary(dev))
> > +                       btd_adapter_remove_device(adapter, dev);
> > +       }
> > +}
> > +
> >  static void adapter_free(gpointer user_data)
> >  {
> >         struct btd_adapter *adapter = user_data;
> > --
> > 2.29.2.729.g45daf8777d-goog
> >
>
>
> --
> Luiz Augusto von Dentz

Applied, thanks. I went ahead and did the changes myself.
diff mbox series

Patch

diff --git a/src/adapter.c b/src/adapter.c
index ec6a6a64c5..92d1cb2232 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -517,6 +517,7 @@  static void adapter_stop(struct btd_adapter *adapter);
 static void trigger_passive_scanning(struct btd_adapter *adapter);
 static bool set_mode(struct btd_adapter *adapter, uint16_t opcode,
 							uint8_t mode);
+static void remove_temporary_devices(struct btd_adapter *adapter);
 
 static void settings_changed(struct btd_adapter *adapter, uint32_t settings)
 {
@@ -622,6 +623,8 @@  static bool set_mode(struct btd_adapter *adapter, uint16_t opcode,
 	switch (mode) {
 	case MGMT_OP_SET_POWERED:
 		setting = MGMT_SETTING_POWERED;
+		if (!mode)
+			remove_temporary_devices(adapter);
 		break;
 	case MGMT_OP_SET_CONNECTABLE:
 		setting = MGMT_SETTING_CONNECTABLE;
@@ -2888,8 +2891,10 @@  static void property_set_mode(struct btd_adapter *adapter, uint32_t setting,
 		param = &mode;
 		len = sizeof(mode);
 
-		if (!mode)
+		if (!mode) {
 			clear_discoverable(adapter);
+			remove_temporary_devices(adapter);
+		}
 
 		break;
 	case MGMT_SETTING_DISCOVERABLE:
@@ -5304,6 +5309,19 @@  static void remove_discovery_list(struct btd_adapter *adapter)
 	adapter->discovery_list = NULL;
 }
 
+static void remove_temporary_devices(struct btd_adapter *adapter)
+{
+	GSList *l, *next;
+
+	for (l = adapter->devices; l; l = next) {
+		struct btd_device *dev = l->data;
+
+		next = g_slist_next(l);
+		if (device_is_temporary(dev))
+			btd_adapter_remove_device(adapter, dev);
+	}
+}
+
 static void adapter_free(gpointer user_data)
 {
 	struct btd_adapter *adapter = user_data;