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

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

[Cheng Jiang]

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);
 		disconnect_notify(device, reason);
 	}
 
-- 
2.25.1

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

[bluez.test.bot]

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

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

[Luiz Augusto von Dentz]

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


-- 
Luiz Augusto von Dentz

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

[Cheng Jiang]

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

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

[Luiz Augusto von Dentz]

Hi Cheng,

On Mon, Aug 26, 2024 at 10:30 AM Cheng Jiang <quic_chejiang@quicinc.com> wrote:
>
> 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.

Looks like you never send another version of this one, are you still
planning on having this fixed?

> >
> >
> >>                 disconnect_notify(device, reason);
> >>         }
> >>
> >> --
> >> 2.25.1
> >>
> >>
> >
> >
>


-- 
Luiz Augusto von Dentz

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

[Cheng Jiang]

Hi Luiz,

Sorry for the delay, will send the new patch soon. 


On 9/20/2024 3:53 AM, Luiz Augusto von Dentz wrote:
> Hi Cheng,
> 
> On Mon, Aug 26, 2024 at 10:30 AM Cheng Jiang <quic_chejiang@quicinc.com> wrote:
>>
>> 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.
> 
> Looks like you never send another version of this one, are you still
> planning on having this fixed?
> 
>>>
>>>
>>>>                 disconnect_notify(device, reason);
>>>>         }
>>>>
>>>> --
>>>> 2.25.1
>>>>
>>>>
>>>
>>>
>>
> 
>