Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bluetooth: gatt: CCC cfg not flushed if device was previously paired #22514

Closed
ivan98ams opened this issue Feb 5, 2020 · 11 comments · Fixed by #22700
Closed

Bluetooth: gatt: CCC cfg not flushed if device was previously paired #22514

ivan98ams opened this issue Feb 5, 2020 · 11 comments · Fixed by #22700
Assignees
Labels
area: Bluetooth bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug

Comments

@ivan98ams
Copy link
Contributor

ivan98ams commented Feb 5, 2020

Describe the bug
The CCC cfg is not freed/flushed when a paired device disconnects from the peripheral; however, the ccc_cfg_changed() callback of the descriptor gets called with value = 0. This leads to the CCC cfg buffer becoming full, the warning bt_gatt: No space to store CCC cfg to be raised, and no more subscriptions to be allowed.

While debugging, I confirmed that the CCC cfg array was indeed full of identical addresses with notifications active.

To Reproduce
Steps to reproduce the behavior:

  1. Use examples peripheral_hr and central_hr and make them pair after subscription (I have only tested it using fixed passkey pairing and legacy oob pairing)
  2. Make it such that the peripheral removes pairing information upon disconnection
  3. Flash devices
  4. Upon pairing complete, disconnect/reset the central
  5. Repeat step 4 until <wrn> bt_gatt: No space to store CCC cfg is raised (For me it took 4 cycles on one project, and 2 cycles on another one)

Expected behavior
The connect - subscribe - disconnect cycle should have kept going perpetually

Screenshots or console output

*** Booting Zephyr OS build zephyr-v2.1.0-2046-g71dff17a3703  ***
Bluetooth initialized
Advertising successfully started
Passkey 12
Security changed level 4
Pairing Complete
Disconnected (reason 0x16)
Unpaired
<inf> hrs: HRS notifications disabled
........
Connected
<inf> hrs: HRS notifications enabled
Passkey 12
Security changed level 4
Pairing Complete
Disconnected (reason 0x16)
Unpaired!
<inf> hrs: HRS notifications disabled
Connected
<wrn> bt_gatt: No space to store CCC cfg

Environment (please complete the following information):

  • Platform NRF52 PCA10040
  • Current head of master d1fa27c

Peripheral configuration:

CONFIG_BT=y
CONFIG_BT_DEBUG_LOG=y
CONFIG_BT_SMP=y
CONFIG_BT_PERIPHERAL=y
CONFIG_BT_GATT_DIS=y
CONFIG_BT_GATT_DIS_PNP=n
CONFIG_BT_GATT_BAS=y
CONFIG_BT_GATT_HRS=y
CONFIG_BT_DEVICE_NAME="Zephyr Heartrate Sensor"
CONFIG_BT_DEVICE_APPEARANCE=833

CONFIG_BT_FIXED_PASSKEY=y
@ivan98ams ivan98ams added the bug The issue is a bug, or the PR is fixing a bug label Feb 5, 2020
@Vudentz
Copy link
Contributor

Vudentz commented Feb 5, 2020

The callback is indicating 0 when there is no active connection so the application can stop notifying, this seems to be a regression where a new slot is taken everytime.

@ivan98ams
Copy link
Contributor Author

The callback is indicating 0 when there is no active connection so the application can stop notifying, this seems to be a regression where a new slot is taken everytime.

@Vudentz yes, this is what is happening. I checked the array in debug mode and it was full of identical addresses, i updated the description

@Vudentz
Copy link
Contributor

Vudentz commented Feb 5, 2020

find_ccc_cfg is never able to find the original cfg then?

@ivan98ams
Copy link
Contributor Author

ivan98ams commented Feb 5, 2020

find_ccc_cfg is never able to find the original cfg then?

Yes that’s what happens, and then they are just repeated

@jhedberg jhedberg added the priority: medium Medium impact/importance bug label Feb 5, 2020
@Vudentz
Copy link
Contributor

Vudentz commented Feb 5, 2020

I was not able to reproduce with the shell and central_hr, I did:

setup: 2 btvirt instances using native_posix

> hrs simulate on
uart:~$ hrs simulate on
Registering HRS Service
Connected: 00:aa:01:01:00:24 (public)
Start HRS simulation
[00:00:11.140,000] <inf> hrs: HRS notifications enabled
uart:~$ bt security 2
Security changed: 00:aa:01:01:00:24 (public) level 2
uart:~$ bt disconnect
[00:01:20.950,000] <inf> hrs: HRS notifications disabled
Disconnected: 00:aa:01:01:00:24 (public) (reason 0x13)
Connected: 00:aa:01:01:00:24 (public)
[00:01:21.010,000] <inf> hrs: HRS notifications enabled
uart:~$ bt disconnect
[00:01:56.860,000] <inf> hrs: HRS notifications disabled
Disconnected: 00:aa:01:01:00:24 (public) (reason 0x13)
Connected: 00:aa:01:01:00:24 (public)
[00:01:56.950,000] <inf> hrs: HRS notifications enabled
uart:~$ bt disconnect
[00:02:03.880,000] <inf> hrs: HRS notifications disabled
Disconnected: 00:aa:01:01:00:24 (public) (reason 0x13)
Connected: 00:aa:01:01:00:24 (public)
[00:02:03.940,000] <inf> hrs: HRS notifications enabled

*I will send a patch that updates the shell to be able to connect to central_hr since that is expecting an UUID.

@Vudentz
Copy link
Contributor

Vudentz commented Feb 5, 2020

Hmm, there is a bug in the central_hr though, it is not tracking if it has been subscribed already perhaps that has something to do with the problem.

@ivan98ams
Copy link
Contributor Author

Hmm, there is a bug in the central_hr though, it is not tracking if it has been subscribed already perhaps that has something to do with the problem.

In my test I reset the central between cycles, so it is not affected if it was already subscribed in the past

@ivan98ams
Copy link
Contributor Author

ivan98ams commented Feb 6, 2020

Also @Vudentz I just realized that you didnt do bt clear to unpair the devices. right now I cannot test it using the shell test project itself (no right platform atm), but by enabling the bt_shell on the pheripheral_hr project I was able to again reproduce the bug using the following commands:

uart:~$ bt init
Bluetooth initialized
uart:~$ bt advertise on
Advertising started
Connected: 53:b0:88:44:5c:92 (random)
[00:00:43.762,176] <inf> hrs: HRS notifications enabled
uart:~$ bt security 2
Security changed: 53:b0:88:44:5c:92 (random) level 2
Identity resolved 53:b0:88:44:5c:92 (random) -> 38:80:df:12:5c:4d (public)
Disconnected: 38:80:df:12:5c:4d (public) (reason 0x13)
[00:01:13.158,355] <inf> hrs: HRS notifications disabled
uart:~$ bt clear all
Pairings successfully cleared
Connected: 53:b0:88:44:5c:92 (random)
[00:01:42.066,223] <inf> hrs: HRS notifications enabled
uart:~$ bt security 2
Security changed: 53:b0:88:44:5c:92 (random) level 2
Identity resolved 53:b0:88:44:5c:92 (random) -> 38:80:df:12:5c:4d (public)
Disconnected: 38:80:df:12:5c:4d (public) (reason 0x13)
[00:01:59.665,069] <inf> hrs: HRS notifications disabled
uart:~$ bt clear all
Pairings successfully cleared
Connected: 53:b0:88:44:5c:92 (random)
[00:02:11.762,023] <wrn> bt_gatt: No space to store CCC cfg
[00:02:24.429,260] <wrn> bt_gatt: No space to store CCC cfg
[00:02:27.012,969] <wrn> bt_gatt: No space to store CCC cfg
[00:02:29.889,160] <wrn> bt_gatt: No space to store CCC cfg
  • Please notice that the disconnect is coming externally, and not from the commands themselves.
  • Also please notice that i'm unpairing and repairing each time

This time i was doing the connect/subscribe operations of the central using the NRF app to remove any possible bugs introduced by the central_hr project

@Vudentz
Copy link
Contributor

Vudentz commented Feb 10, 2020

Check if the following patch helps:

diff --git a/subsys/bluetooth/host/gatt.c b/subsys/bluetooth/host/gatt.c
index a191725290..b57d31af21 100644
--- a/subsys/bluetooth/host/gatt.c
+++ b/subsys/bluetooth/host/gatt.c
@@ -4159,7 +4159,7 @@ static u8_t remove_peer_from_attr(const struct bt_gatt_attr *attr,
        /* Check if there is a cfg for the peer */
        cfg = ccc_find_cfg(ccc, addr_with_id->addr, addr_with_id->id);
        if (cfg) {
-               memset(cfg, 0, sizeof(*cfg));
+               clear_ccc_cfg(cfg);
        }
 
        return BT_GATT_ITER_CONTINUE;

@ivan98ams
Copy link
Contributor Author

ivan98ams commented Feb 10, 2020

No help with the patch :/, I looked a bit more into the code and the registers during debug and found the following:

  • When paired, the address in ccc cfg is updated to the private identity of the remote device upon disconnect
  • When bt_unpair is called, the gatt information is not cleared unless BT_SETTINGS is enabled (not the case yet for any my test projects), so the function remove_peer_from_attr is actually never called
  • When the central device connects and subscribes to the peripheral again, ccc_find_cfg compares the stored private identity address with the random address that the central just used for connection(we haven't repaired yet at this point), therefore the function always returns null, and a new ccfg slot is occupied, but never cleared. Also shows why the array is full of the same address

@Vudentz Is there a particular reason why unpair only calls bt_gatt_clear when BT_SETTINGS is enabled? I see you authored that line in the code

@Vudentz
Copy link
Contributor

Vudentz commented Feb 10, 2020

So this is a combination of:

  • BT_SETTINGS being disabled
  • bt_unpair being called while disconnected

I guess we didn't have bt_unpair when we introduced bt_gatt_clear, but you are right that we should probably clear the ccc no matter if BT_SETTINGS is enabled or not.

Vudentz added a commit to Vudentz/zephyr that referenced this issue Feb 13, 2020
GATT data shall not be considered conditional to BT_SETTINGS since
the data is stored in RAM it must also be cleared when unpairing.

Fixes zephyrproject-rtos#22514

Signed-off-by: Luiz Augusto von Dentz <[email protected]>
jhedberg pushed a commit that referenced this issue Feb 14, 2020
GATT data shall not be considered conditional to BT_SETTINGS since
the data is stored in RAM it must also be cleared when unpairing.

Fixes #22514

Signed-off-by: Luiz Augusto von Dentz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Bluetooth bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants