Re: [PATCH] gatt: Fix double att_disconnected issue on disconnection

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Yunhan,
On Fri, Oct 26, 2018 at 5:00 AM Yunhan Wang <yunhanw@xxxxxxxxxx> wrote:
>
> Hi, Luiz
>
> On Thu, Oct 25, 2018 at 2:41 PM Luiz Augusto von Dentz
> <luiz.dentz@xxxxxxxxx> wrote:
> >
> > Hi Yunhan,
> > On Fri, Oct 26, 2018 at 12:06 AM Yunhan Wang <yunhanw@xxxxxxxxxx> wrote:
> > >
> > > Hi, Luiz
> > >
> > > I am using latest bluez master without any change for this issue, I
> > > think I am not missing any changes....The issue is there.
> >
> > Then we have a problem on bt_att, but that is tracking if the handler
> > is removed so I wonder how it is still reproducible for you.
> >
> It is reproducible using real ble dongles, It is also reproducible
> using btvirt.....
> Using btvirt -L -l2  and bluetoothctl
>
> Following the below instructions, when central issue ble disconnection
> to peripheral, the bluetoothd would crash as I show before.
>
> Peripheral:
>
> [bluetooth]# select 00:AA:01:01:00:24
> Controller 00:AA:01:01:00:24 N0001 [default]
> [bluetooth]# system-alias N0001
> Changing N0001 succeeded
> [bluetooth]# power on
> Changing power on succeeded
> [bluetooth]# name N0001
> [bluetooth]# uuids FEAF
> [bluetooth]# discoverable on
> [bluetooth]# back
> [bluetooth]# register-service 0000feaf-0000-1000-8000-00805f9b34fb
> [NEW] Primary Service
> /org/bluez/app/service0x562f48a31860
> 0000feaf-0000-1000-8000-00805f9b34fb
> Nest Labs Inc.
> [/org/bluez/app/service0x562f48a31860] Primary (yes/no): yees
> Invalid option: yees
> [DEL] Primary Service
> /org/bluez/app/service0x562f48a31860
> 0000feaf-0000-1000-8000-00805f9b34fb
> Nest Labs Inc.
> [bluetooth]# register-service 0000feaf-0000-1000-8000-00805f9b34fb
> [NEW] Primary Service
> /org/bluez/app/service0x562f48a34e70
> 0000feaf-0000-1000-8000-00805f9b34fb
> Nest Labs Inc.
> [/org/bluez/app/service0x562f48a34e70] Primary (yes/no): yes
>
> [bluetooth]# register-characteristic
> 18ee2ef5-263d-4559-959f-4f9c429f9d11 read,write
> [NEW] Characteristic
> /org/bluez/app/service0x562f48a34e70/chrc0x562f48a437c0
> 18ee2ef5-263d-4559-959f-4f9c429f9d11
> Vendor specific
> [/org/bluez/app/service0x562f48a34e70/chrc0x562f48a437c0] Enter value: 1
>
> [bluetooth]# register-application
> [CHG] Controller 00:AA:01:01:00:24 UUIDs: 00001800-0000-1000-8000-00805f9b34fb
> [CHG] Controller 00:AA:01:01:00:24 UUIDs: 00001801-0000-1000-8000-00805f9b34fb
> [CHG] Controller 00:AA:01:01:00:24 UUIDs: 0000110e-0000-1000-8000-00805f9b34fb
> [CHG] Controller 00:AA:01:01:00:24 UUIDs: 00001200-0000-1000-8000-00805f9b34fb
> [CHG] Controller 00:AA:01:01:00:24 UUIDs: 0000110c-0000-1000-8000-00805f9b34fb
> [CHG] Controller 00:AA:01:01:00:24 UUIDs: 0000feaf-0000-1000-8000-00805f9b34fb
> Application registered
> [CHG] Controller 00:AA:01:01:00:24 UUIDs: 00001800-0000-1000-8000-00805f9b34fb
> [CHG] Controller 00:AA:01:01:00:24 UUIDs: 00001801-0000-1000-8000-00805f9b34fb
> [CHG] Controller 00:AA:01:01:00:24 UUIDs: 0000110e-0000-1000-8000-00805f9b34fb
> [CHG] Controller 00:AA:01:01:00:24 UUIDs: 00001200-0000-1000-8000-00805f9b34fb
> [CHG] Controller 00:AA:01:01:00:24 UUIDs: 0000110c-0000-1000-8000-00805f9b34fb
> [CHG] Controller 00:AA:01:01:00:24 UUIDs: 0000feaf-0000-1000-8000-00805f9b34fb
> [bluetooth]# back
>
> [bluetooth]# advertise peripheral
> [CHG] Controller 00:AA:01:01:00:24 SupportedInstances: 0x04
> [CHG] Controller 00:AA:01:01:00:24 ActiveInstances: 0x01
> Advertising object registered
> UUID: (FEAF)
> Tx Power: off
> LocalName: N0001
> Apperance: off
> Discoverable: on
> [CHG] Controller 00:AA:01:00:00:23 Powered: yes
> [CHG] Controller 00:AA:01:00:00:23 Discovering: yes
> [CHG] Controller 00:AA:01:00:00:23 Discovering: no
> [CHG] Controller 00:AA:01:00:00:23 Discovering: yes
> [CHG] Device 00:AA:01:00:00:23 Connected: yes
> [NEW] Primary Service
> /org/bluez/hci2/dev_00_AA_01_00_00_23/service0006
> 00001801-0000-1000-8000-00805f9b34fb
> Generic Attribute Profile
> [NEW] Characteristic
> /org/bluez/hci2/dev_00_AA_01_00_00_23/service0006/char0007
> 00002a05-0000-1000-8000-00805f9b34fb
> Service Changed
> [NEW] Descriptor
> /org/bluez/hci2/dev_00_AA_01_00_00_23/service0006/char0007/desc0009
> 00002902-0000-1000-8000-00805f9b34fb
> Client Characteristic Configuration
> [CHG] Device 00:AA:01:00:00:23 ServicesResolved: yes
>
>
> Central:
>
> [bluetooth]# select 00:AA:01:00:00:23
> Discovery stopped
> [bluetooth]# scan on
> Discovery started
> [CHG] Controller 00:AA:01:00:00:23 Discovering: yes
> [bluetooth]# connect 00:AA:01:01:00:24
> Attempting to connect to 00:AA:01:01:00:24
> [CHG] Device 00:AA:01:01:00:24 Connected: yes
> Connection successful
> [CHG] Device 00:AA:01:01:00:24 UUIDs: 00001800-0000-1000-8000-00805f9b34fb
> [CHG] Device 00:AA:01:01:00:24 UUIDs: 00001801-0000-1000-8000-00805f9b34fb
> [NEW] Primary Service
> /org/bluez/hci1/dev_00_AA_01_01_00_24/service0006
> 00001801-0000-1000-8000-00805f9b34fb
> Generic Attribute Profile
> [NEW] Characteristic
> /org/bluez/hci1/dev_00_AA_01_01_00_24/service0006/char0007
> 00002a05-0000-1000-8000-00805f9b34fb
> Service Changed
> [NEW] Descriptor
> /org/bluez/hci1/dev_00_AA_01_01_00_24/service0006/char0007/desc0009
> 00002902-0000-1000-8000-00805f9b34fb
> Client Characteristic Configuration
> [NEW] Primary Service
> /org/bluez/hci1/dev_00_AA_01_01_00_24/service000d
> 0000feaf-0000-1000-8000-00805f9b34fb
> Nest Labs Inc.
> [NEW] Characteristic
> /org/bluez/hci1/dev_00_AA_01_01_00_24/service000d/char000e
> 18ee2ef5-263d-4559-959f-4f9c429f9d11
> Vendor specific
> [CHG] Device 00:AA:01:01:00:24 UUIDs: 00001800-0000-1000-8000-00805f9b34fb
> [CHG] Device 00:AA:01:01:00:24 UUIDs: 00001801-0000-1000-8000-00805f9b34fb
> [CHG] Device 00:AA:01:01:00:24 UUIDs: 0000feaf-0000-1000-8000-00805f9b34fb
> [CHG] Device 00:AA:01:01:00:24 ServicesResolved: yes
>
> [N0001]# disconnect 00:AA:01:01:00:24
> Attempting to disconnect from 00:AA:01:01:00:24
> [CHG] Device 00:AA:01:01:00:24 ServicesResolved: no
> Successful disconnected

It should be fixed now, the problem was that the bt_gatt_server was
set to NULL already thus the bt_att_unregister_disconnect did nothing,
Ive might have tested a version were I passed bt_att directly but
later I changed to use bt_gatt_server to access the bt_att instance
from btd_device.

>
> > > It is not for chrome os. I am currently trying bring up bluez version
> > > from commit in Jan 19 11:37:07 2018 to latest master in Open weave
> > > project(https://github.com/openweave/openweave-core/blob/master/repos.conf),
> > > where we are using BLE for weave pairing in iot products, and create
> > > two GATT characteristics for Tx and Rx and the TCP-like control
> > > protocol  to control BLE packet flow. Periodically I would sync Bluez
> > > revision in openweave against Bluez Upstream.
> >
> > All major mobile OS support LE L2CAP CoC channels, no idea why
> > companies want to keep using GATT for emulating serial like
> > communication special when L2CAP does have support for fragmentation
> > and flow control.
> >
> Yes, L2CAP do have support for fragmentation and flow control, but for
> some platforms, it may not have bluez, and its L2CAP is not good, then
> GATT layer fragmentation and flow control is needed. In addition, the
> L2CAP API is available on neither Android nor iOS when we did this
> implementation in the past...then GATT layer fragmentation and flow
> control is also needed.

Sure, though moving to L2CAP is a lot simpler... anyway it was just a
recommendation given that L2CAP is now supported.

> Thanks
> Best wishes
> Yunhan
> > > Thanks
> > > Best wishes
> > > Yunhan
> > >
> > > On Thu, Oct 25, 2018 at 1:22 PM Luiz Augusto von Dentz
> > > <luiz.dentz@xxxxxxxxx> wrote:
> > > >
> > > > Hi Yunhan,
> > > >
> > > > On Thu, Oct 25, 2018 at 9:24 PM Luiz Augusto von Dentz
> > > > <luiz.dentz@xxxxxxxxx> wrote:
> > > > >
> > > > > Hi Yunhan,
> > > > >
> > > > > We might be better of removing the handler altogether and just leave device.c handler instead then.
> > > > >
> > > > > On Thu, 25 Oct 2018, 20:19 Yunhan Wang, <yunhanw@xxxxxxxxxx> wrote:
> > > > >>
> > > > >> Hi, Luiz
> > > > >>
> > > > >> Actually before I submit my patch, I tried your way to unregister the
> > > > >> handler, it is failing. That is why I use random address check in
> > > > >> btd_gatt_database_att_disconnected to workaround this issue.
> > > > >>
> > > > >> Thanks
> > > > >> Best wishes
> > > > >> Yunhan
> > > > >>
> > > > >>
> > > > >>
> > > > >> On Thu, Oct 25, 2018 at 10:49 AM Yunhan Wang <yunhanw@xxxxxxxxxx> wrote:
> > > > >> >
> > > > >> > Hi, Luiz
> > > > >> >
> > > > >> > Just have a test with your patch in master branch, both crashes are
> > > > >> > still there, and att_disconnected has been called for two times even
> > > > >> > though unregistering the handler....
> > > >
> > > > Actually you may be missing the following patch:
> > > >
> > > > commit 261cf78db4be79a0f7d44798a57730b159c9be91
> > > > Author: Luiz Augusto von Dentz <luiz.von.dentz@xxxxxxxxx>
> > > > Date:   Mon Oct 23 14:13:59 2017 +0300
> > > >
> > > >     shared/att: Fix crash when calling disconnect handlers
> > > >
> > > > This is quite old btw, what version is Chrome OS shipping?
> > > >
> > > > >> > Thanks
> > > > >> > Best wishes
> > > > >> > Yunhan
> > > > >> >
> > > > >> > Program received signal SIGSEGV, Segmentation fault.
> > > > >> > btd_adapter_find_device (adapter=0x72657664612f6372,
> > > > >> > dst=dst@entry=0x555555872998, bdaddr_type=0 '\000')
> > > > >> >     at bluez/repo/src/adapter.c:845
> > > > >> > 845 list = g_slist_find_custom(adapter->devices, &addr,
> > > > >> > (gdb) bt
> > > > >> > #0  btd_adapter_find_device (adapter=0x72657664612f6372,
> > > > >> > dst=dst@entry=0x555555872998, bdaddr_type=0 '\000')
> > > > >> >     at bluez/repo/src/adapter.c:845
> > > > >> > #1  0x00005555555ab890 in att_disconnected (err=<optimized out>,
> > > > >> > user_data=0x555555872990)
> > > > >> >     at bluez/repo/src/gatt-database.c:329
> > > > >> > #2  0x00005555555eaba8 in queue_foreach (queue=0x55555585de60,
> > > > >> > function=function@entry=0x5555555ee5f0 <disconn_handler>,
> > > > >> > user_data=0x68)
> > > > >> >     at bluez/repo/src/shared/queue.c:220
> > > > >> > #3  0x00005555555ef819 in disconnect_cb (io=<optimized out>,
> > > > >> > user_data=0x555555869d50)
> > > > >> >     at bluez/repo/src/shared/att.c:592
> > > > >> > #4  0x00005555555f89a3 in watch_callback (channel=<optimized out>,
> > > > >> > cond=<optimized out>, user_data=<optimized out>)
> > > > >> >     at bluez/repo/src/shared/io-glib.c:170
> > > > >> > #5  0x00007ffff7b0fe35 in g_main_context_dispatch () from
> > > > >> > /lib/x86_64-linux-gnu/libglib-2.0.so.0
> > > > >> > #6  0x00007ffff7b10200 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
> > > > >> > #7  0x00007ffff7b10512 in g_main_loop_run () from
> > > > >> > /lib/x86_64-linux-gnu/libglib-2.0.so.0
> > > > >> > #8  0x0000555555572238 in main (argc=<optimized out>, argv=<optimized
> > > > >> > out>) at bluez/repo/src/main.c:808
> > > > >> >
> > > > >> >
> > > > >> > Program received signal SIGSEGV, Segmentation fault.
> > > > >> > queue_remove (queue=0x30, data=data@entry=0x555555873740) at
> > > > >> > bluez/repo/src/shared/queue.c:256
> > > > >> > 256 for (entry = queue->head, prev = NULL; entry;
> > > > >> > (gdb) bt
> > > > >> > #0  queue_remove (queue=0x30, data=data@entry=0x555555873740) at
> > > > >> > bluez/repo/src/shared/queue.c:256
> > > > >> > #1  0x00005555555ab8c5 in att_disconnected (err=<optimized out>,
> > > > >> > user_data=0x555555873740)
> > > > >> >     at bluez/repo/src/gatt-database.c:350
> > > > >> > #2  0x00005555555eabb8 in queue_foreach (queue=0x55555586e670,
> > > > >> > function=function@entry=0x5555555ee600 <disconn_handler>,
> > > > >> > user_data=0x68)
> > > > >> >     at bluez/repo/src/shared/queue.c:220
> > > > >> > #3  0x00005555555ef829 in disconnect_cb (io=<optimized out>,
> > > > >> > user_data=0x555555865f50)
> > > > >> >     at bluez/repo/src/shared/att.c:592
> > > > >> > #4  0x00005555555f89b3 in watch_callback (channel=<optimized out>,
> > > > >> > cond=<optimized out>, user_data=<optimized out>)
> > > > >> >     at bluez/repo/src/shared/io-glib.c:170
> > > > >> > #5  0x00007ffff7b0fe35 in g_main_context_dispatch () from
> > > > >> > /lib/x86_64-linux-gnu/libglib-2.0.so.0
> > > > >> > #6  0x00007ffff7b10200 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
> > > > >> > #7  0x00007ffff7b10512 in g_main_loop_run () from
> > > > >> > /lib/x86_64-linux-gnu/libglib-2.0.so.0
> > > > >> > #8  0x0000555555572238 in main (argc=<optimized out>, argv=<optimized
> > > > >> > out>) at  bluez/repo/src/main.c:808
> > > > >> > On Thu, Oct 25, 2018 at 2:20 AM Luiz Augusto von Dentz
> > > > >> > <luiz.dentz@xxxxxxxxx> wrote:
> > > > >> > >
> > > > >> > > Hi Yunhan,
> > > > >> > >
> > > > >> > > On Thu, Oct 25, 2018 at 4:47 AM Yunhan Wang <yunhanw@xxxxxxxxxx> wrote:
> > > > >> > > >
> > > > >> > > > Hi, Luiz
> > > > >> > > >
> > > > >> > > > I am observing the multiple crashes when doing BLE disconnection using
> > > > >> > > > latest bluez master..It looks like the two att_disconnect are
> > > > >> > > > triggered from your last gatt commit.. Please help take a look at this
> > > > >> > > > workaround and comments.. the better solution might be to figure out
> > > > >> > > > how to handle the disconnection along with random address and public
> > > > >> > > > address together regarding the previous issue, Gatt: Subscriptions are
> > > > >> > > > not cleared after disconnection from a temporary device
> > > > >> > >
> > > > >> > > Ive pushed a similar fix, it should remove the handler before calling
> > > > >> > > att_disconnected.
> > > > >> > >
> > > > >> > > > Thanks
> > > > >> > > > Best wishes
> > > > >> > > > Yunhan
> > > > >> > > > On Wed, Oct 24, 2018 at 5:42 PM yunhanw <yunhanw@xxxxxxxxxx> wrote:
> > > > >> > > > >
> > > > >> > > > > When BLE disconnection happens, att_disconnect is triggered from two locations, the new added location is gatt_server_cleanup, it would cause several blueetoothd crashes. This bus is introduced from commit 634f0a6e1125af8d5959bff119d9336a8d81c028, where gatt fix, gatt subscriptions are not cleared after disconnection from a temporary device with private/random address. In order to workaround this issue, btd_gatt_database_att_disconnected can only be triggered when address type is random, and for others, it can continue to use original disconnect code path.
> > > > >> > > > >
> > > > >> > > > >     crash 1
> > > > >> > > > >     Program received signal SIGSEGV, Segmentation fault.
> > > > >> > > > >     queue_remove (queue=0x30, data=data@entry=0x555555872a40) at /repo/src/shared/queue.c:256
> > > > >> > > > >     256     for (entry = queue->head, prev = NULL; entry;
> > > > >> > > > >     (gdb) backtrace
> > > > >> > > > >         at /bluez/repo/src/gatt-database.c:350
> > > > >> > > > >         at bluez/repo/src/shared/queue.c:220
> > > > >> > > > >         at bluez/repo/src/shared/att.c:592
> > > > >> > > > >         at bluez/repo/src/shared/io-glib.c:170
> > > > >> > > > >
> > > > >> > > > >     crash 2
> > > > >> > > > >         at bluez/repo/src/shared/queue.c:220
> > > > >> > > > >         at bluez/repo/src/shared/att.c:592
> > > > >> > > > >         at bluez/repo/src/shared/io-glib.c:170
> > > > >> > > > >
> > > > >> > > > >     (gdb) print state->db->adapter
> > > > >> > > > >     Cannot access memory at address 0x61672f6269727474
> > > > >> > > > > ---
> > > > >> > > > >  src/gatt-database.c | 2 ++
> > > > >> > > > >  1 file changed, 2 insertions(+)
> > > > >> > > > >
> > > > >> > > > > diff --git a/src/gatt-database.c b/src/gatt-database.c
> > > > >> > > > > index 783b692d5..2f0eb83b5 100644
> > > > >> > > > > --- a/src/gatt-database.c
> > > > >> > > > > +++ b/src/gatt-database.c
> > > > >> > > > > @@ -3365,6 +3365,8 @@ void btd_gatt_database_att_disconnected(struct btd_gatt_database *database,
> > > > >> > > > >
> > > > >> > > > >         addr = device_get_address(device);
> > > > >> > > > >         type = btd_device_get_bdaddr_type(device);
> > > > >> > > > > +    if (type != BDADDR_LE_RANDOM)
> > > > >> > > > > +        return;
> > > > >> > > > >
> > > > >> > > > >         state = find_device_state(database, addr, type);
> > > > >> > > > >         if (!state)
> > > > >> > > > > --
> > > > >> > > > > 2.19.1.568.g152ad8e336-goog
> > > > >> > > > >
> > > > >> > >
> > > > >> > >
> > > > >> > >
> > > > >> > > --
> > > > >> > > Luiz Augusto von Dentz
> > > >
> > > >
> > > >
> > > > --
> > > > Luiz Augusto von Dentz
> >
> >
> >
> > --
> > Luiz Augusto von Dentz



-- 
Luiz Augusto von Dentz




[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux