diff mbox series

[v2] adapter: Cancel the service authorization when remote is disconnected

Message ID 20240826090044.560142-1-quic_chejiang@quicinc.com (mailing list archive)
State New
Headers show
Series [v2] adapter: Cancel the service authorization when remote is disconnected | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/CheckPatch success CheckPatch PASS
tedd_an/GitLint success Gitlint PASS
tedd_an/BuildEll success Build ELL PASS
tedd_an/BluezMake success Bluez Make PASS
tedd_an/MakeCheck success Bluez Make Check PASS
tedd_an/MakeDistcheck success Make Distcheck PASS
tedd_an/CheckValgrind success Check Valgrind PASS
tedd_an/CheckSmatch success CheckSparse PASS
tedd_an/bluezmakeextell success Make External ELL PASS
tedd_an/IncrementalBuild success Incremental Build PASS
tedd_an/ScanBuild success Scan Build PASS

Commit Message

Cheng Jiang Aug. 26, 2024, 9 a.m. UTC 
If the remote device drops the connection before DUT confirm the
service authorization, the DUT still must wait for service
authorization timeout before processing future request.

Cancel the service authorization request when connection is dropped.
---
 src/adapter.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Comments

bluez.test.bot@gmail.com Aug. 26, 2024, 10:43 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=883234

---Test result---

Test Summary:
CheckPatch                    PASS      0.41 seconds
GitLint                       PASS      0.27 seconds
BuildEll                      PASS      24.55 seconds
BluezMake                     PASS      1728.48 seconds
MakeCheck                     PASS      13.32 seconds
MakeDistcheck                 PASS      182.69 seconds
CheckValgrind                 PASS      254.20 seconds
CheckSmatch                   PASS      355.43 seconds
bluezmakeextell               PASS      119.41 seconds
IncrementalBuild              PASS      1507.74 seconds
ScanBuild                     PASS      1003.07 seconds



---
Regards,
Linux Bluetooth
Luiz Augusto von Dentz Aug. 26, 2024, 1:43 p.m. UTC | #2 
Hi Cheng,

On Mon, Aug 26, 2024 at 5:01 AM Cheng Jiang <quic_chejiang@quicinc.com> wrote:
>
> If the remote device drops the connection before DUT confirm the
> service authorization, the DUT still must wait for service
> authorization timeout before processing future request.
>
> Cancel the service authorization request when connection is dropped.
> ---
>  src/adapter.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/src/adapter.c b/src/adapter.c
> index 245de4456..3ad000425 100644
> --- a/src/adapter.c
> +++ b/src/adapter.c
> @@ -8502,6 +8502,25 @@ static void disconnect_notify(struct btd_device *dev, uint8_t reason)
>         }
>  }
>
> +static void adapter_cancel_service_auth(struct btd_adapter *adapter,
> +                                       struct btd_device *device)
> +{
> +       struct service_auth *auth;
> +       GList *l;
> +
> +       auth = NULL;
> +       for (l = adapter->auths->head; l != NULL; l = l->next) {
> +               auth = l->data;
> +               if (auth->device == device)
> +                       break;
> +       }
> +       if (auth != NULL) {
> +               DBG("Cancel pending auth: %s", auth->uuid);
> +               g_queue_remove(auth->adapter->auths, auth);
> +               service_auth_cancel(auth);
> +       }
> +}
> +
>  static void dev_disconnected(struct btd_adapter *adapter,
>                                         const struct mgmt_addr_info *addr,
>                                         uint8_t reason)
> @@ -8516,6 +8535,7 @@ static void dev_disconnected(struct btd_adapter *adapter,
>         device = btd_adapter_find_device(adapter, &addr->bdaddr, addr->type);
>         if (device) {
>                 adapter_remove_connection(adapter, device, addr->type);
> +               adapter_cancel_service_auth(adapter, device);

This shall probably be placed together with
device_cancel_authentication in adapter_remove_connection but we need
to track in what bearer the authorization is for and remove all
authorization requests like in btd_adapter_remove_device:

    l = adapter->auths->head;
    while (l != NULL) {
        struct service_auth *auth = l->data;
        GList *next = g_list_next(l);

        if (auth->device != dev) {
            l = next;
            continue;
        }

        g_queue_delete_link(adapter->auths, l);
        l = next;

        service_auth_cancel(auth);
   }

I'd probably move the above code to a helper function so it can be
called from different places, as for doing this on a per bearer basis
would probably need to add bearer information to btd_service but I
guess that can be treated separately.


>                 disconnect_notify(device, reason);
>         }
>
> --
> 2.25.1
>
>
Cheng Jiang Aug. 26, 2024, 2:30 p.m. UTC | #3 
Hi Luiz,

Thank you for your feedback. Please check the comment inline. 

On 8/26/2024 9:43 PM, Luiz Augusto von Dentz wrote:
> Hi Cheng,
> 
> On Mon, Aug 26, 2024 at 5:01 AM Cheng Jiang <quic_chejiang@quicinc.com> wrote:
>>
>> If the remote device drops the connection before DUT confirm the
>> service authorization, the DUT still must wait for service
>> authorization timeout before processing future request.
>>
>> Cancel the service authorization request when connection is dropped.
>> ---
>>  src/adapter.c | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/src/adapter.c b/src/adapter.c
>> index 245de4456..3ad000425 100644
>> --- a/src/adapter.c
>> +++ b/src/adapter.c
>> @@ -8502,6 +8502,25 @@ static void disconnect_notify(struct btd_device *dev, uint8_t reason)
>>         }
>>  }
>>
>> +static void adapter_cancel_service_auth(struct btd_adapter *adapter,
>> +                                       struct btd_device *device)
>> +{
>> +       struct service_auth *auth;
>> +       GList *l;
>> +
>> +       auth = NULL;
>> +       for (l = adapter->auths->head; l != NULL; l = l->next) {
>> +               auth = l->data;
>> +               if (auth->device == device)
>> +                       break;
>> +       }
>> +       if (auth != NULL) {
>> +               DBG("Cancel pending auth: %s", auth->uuid);
>> +               g_queue_remove(auth->adapter->auths, auth);
>> +               service_auth_cancel(auth);
>> +       }
>> +}
>> +
>>  static void dev_disconnected(struct btd_adapter *adapter,
>>                                         const struct mgmt_addr_info *addr,
>>                                         uint8_t reason)
>> @@ -8516,6 +8535,7 @@ static void dev_disconnected(struct btd_adapter *adapter,
>>         device = btd_adapter_find_device(adapter, &addr->bdaddr, addr->type);
>>         if (device) {
>>                 adapter_remove_connection(adapter, device, addr->type);
>> +               adapter_cancel_service_auth(adapter, device);
> 
> This shall probably be placed together with
> device_cancel_authentication in adapter_remove_connection but we need
> to track in what bearer the authorization is for and remove all
> authorization requests like in btd_adapter_remove_device:
Yes. It can be put in device_cancel_authentication, but the condition to call
this function in adapter_remove_connection need change. device_is_authenticating
is only true when request, notify or confirm pincode/passkey.
As for bearer, the service authorize is only happened on BDADDR_BREDR. So it 
can be called when the address type is BDADDR_BREDR in dev_disconnected. 
> 
>     l = adapter->auths->head;
>     while (l != NULL) {
>         struct service_auth *auth = l->data;
>         GList *next = g_list_next(l);
> 
>         if (auth->device != dev) {
>             l = next;
>             continue;
>         }
> 
>         g_queue_delete_link(adapter->auths, l);
>         l = next;
> 
>         service_auth_cancel(auth);
>    }
> 
> I'd probably move the above code to a helper function so it can be
> called from different places, as for doing this on a per bearer basis
> would probably need to add bearer information to btd_service but I
> guess that can be treated separately.
Yes, it's great. Need use the above code to cancel all pending authorize. 
> 
> 
>>                 disconnect_notify(device, reason);
>>         }
>>
>> --
>> 2.25.1
>>
>>
> 
>
diff mbox series

Patch

diff --git a/src/adapter.c b/src/adapter.c
index 245de4456..3ad000425 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -8502,6 +8502,25 @@  static void disconnect_notify(struct btd_device *dev, uint8_t reason)
 	}
 }
 
+static void adapter_cancel_service_auth(struct btd_adapter *adapter,
+					struct btd_device *device)
+{
+	struct service_auth *auth;
+	GList *l;
+
+	auth = NULL;
+	for (l = adapter->auths->head; l != NULL; l = l->next) {
+		auth = l->data;
+		if (auth->device == device)
+			break;
+	}
+	if (auth != NULL) {
+		DBG("Cancel pending auth: %s", auth->uuid);
+		g_queue_remove(auth->adapter->auths, auth);
+		service_auth_cancel(auth);
+	}
+}
+
 static void dev_disconnected(struct btd_adapter *adapter,
 					const struct mgmt_addr_info *addr,
 					uint8_t reason)
@@ -8516,6 +8535,7 @@  static void dev_disconnected(struct btd_adapter *adapter,
 	device = btd_adapter_find_device(adapter, &addr->bdaddr, addr->type);
 	if (device) {
 		adapter_remove_connection(adapter, device, addr->type);
+		adapter_cancel_service_auth(adapter, device);
 		disconnect_notify(device, reason);
 	}