nimble/store: Fix nimble store behavior when CCCDs exceed static defined limit #790

prasad-alatkar · 2020-03-31T18:49:28Z

Issue reference: #788

Description:
In cases where CCCDs are exceeding MYNEWT_VAL(BLE_STORE_MAX_CCCDS) (overflow event), we can end up in continuous while loop.

Change list:

In ble_gap_unpair_oldest_peer changed return value from 0 to BLE_HS_ENOENT when there is no entry found.
Modified ble_store_util_status_rr to address overflow event of CCCDs.

Previous to this PR, if CCCDs used to exceed the MAX_CCCDs then in ble_store_write call there used to come OVERFLOW event, which used to call ble_store_delete_oldest_peer, now here as we do not have any peer to be deleted, we should have identified that we need to make space for CCCDs and not peer_sec or our_sec. The PR tries to address this issue.

h2zero · 2020-03-31T22:53:03Z

Glad you built a proper fix and hope it gets merged.

I still question why we store cccd data for devices that aren’t bonded but that’s a separate issue.

prasad-alatkar · 2020-04-01T04:54:01Z

@h2zero That is a valid point, will update if I find anything. Maybe @rymanluk can explain it to us.

rymanluk

Thanks for this fix.

Just as aside note, ble_store_util_status_rr() is just an example code. In most of cases application shall provide its own handling of store events.

Anyway, we should improve it, and it is good catch with CCCD. Please check my comments.

nimble/host/src/ble_gap.c

rymanluk · 2020-04-03T07:15:12Z

nimble/host/src/ble_store_util.c

    switch (event->event_code) {
    case BLE_STORE_EVENT_OVERFLOW:
        switch (event->overflow.obj_type) {
            case BLE_STORE_OBJ_TYPE_OUR_SEC:
            case BLE_STORE_OBJ_TYPE_PEER_SEC:
            case BLE_STORE_OBJ_TYPE_CCCD:
-                return ble_gap_unpair_oldest_peer();


I believe that for CCCD we should have different handling and if we want to unpair older peer we should not unpair peer for which we store CCCD. If there is nothing to unpair just return the error.

In the CCCD event there is and ble_addr we should use to make sure that one is no unpaired.

@rymanluk agreed !! I have made relevant changes. For CCCD, if ble_gap_unpair_oldest_peer is unable to delete any peer, we continue to find other peers occupying CCCDs in storage. I have also modified callback function (ble_store_util_iter_peer_cccd) to skip the current (i.e. for which CCCDs need to be stored) peer entry.

…bonded peer exist

rymanluk · 2020-04-04T20:05:13Z

nimble/host/src/ble_store_util.c

+        case BLE_STORE_OBJ_TYPE_PEER_SEC:
+            return ble_gap_unpair_oldest_peer();
+        case BLE_STORE_OBJ_TYPE_CCCD:
+            if ((rc = ble_gap_unpair_oldest_peer()) == BLE_HS_ENOENT) {


I was thinking to call here other version of unpair_oldest which would take as a parameter ble_addr_t of device which we don't want to unpair.
If the only device which could be unpair is the one provided, then unpair is not done and this function returns error.

In such a case we know that we cannot store CCC and there is nothing we can do more here. All the other code is not needed then.

@rymanluk That is what I have done actually in ble_store_util_subscribed_cccds !! What I think is ble_gap_unpair_oldest_peer is not anyway going to unpair current peer (the one for which we want to make space), so introducing similar function with ble_addr_t as input won't be really that helpful. Please let me know your thoughts on this.

The issue I believe is only confined to CCCDs, so ble_gap_unpair_oldest_peer()) == BLE_HS_ENOENT will make sure that we do not have any bonded devices and still we are out of storage space, which can be used to conclude that we need to clean old CCCDs (except the current one).

@prasad-alatkar are you sure that ble_gap_unpair_oldest_peer() will not unpair current peer? For me it looks it will.

I believe @rymanluk is correct, if the current peer exceeds the cccd limit it will likely unpair that peer and disconnect. I like the do nothing if full approach here.

@rymanluk @h2zero , disregard my earlier comment, yes current peer will be unpaired, and we should simply return error in that case.

I was thinking of adding utility function (ble_store_clean_old_cccd) which will delete CCCDs of all unbonded peers. This can be called first in case of CCCD overflow and if it fails to delete peer then we can continue with modified ble_gap_unpair_oldest_peer(). Please let me know your views on it.

I believe that ble_store_util_delete_peer() deletes all what we need.

Hi @rymanluk I think for some peers CCCD will exist but peer_sec and our_sec won't exist (Ref: subscribe-event ble_gatts_clt_cfg_access). So we need mechanism to clean these stored CCCDs, that is why I have introduced ble_store_clean_old_cccd() function.

rymanluk

We are almost there :) Couple of comments.

General comment - please keep commit message title short (up to 70) and then use commit message body if needed.

rymanluk · 2020-04-09T07:56:40Z

nimble/host/src/ble_gap.c

+    }
+
+    for (i = 0; i < num_peers; i++) {
+        if (memcmp(curr_peer, &oldest_peer_id_addr[i], sizeof (ble_addr_t)) != 0) {


I would use here ble_addr_cmp()

rymanluk · 2020-04-09T07:59:13Z

nimble/host/src/ble_gap.c

+        }
+    }
+
+    if (i < num_peers) {


I would do it little simpler.

if (i >= num_peers) { return BLE_HS_ENOMEM; } return ble_gap_unpair(&oldest_peer_id_addr[i]);

rymanluk · 2020-04-09T08:01:06Z

nimble/host/src/ble_gap.c

+int
+ble_gap_unpair_oldest_except_curr(const ble_addr_t *curr_peer)
+{
+    ble_addr_t oldest_peer_id_addr[MYNEWT_VAL(BLE_STORE_MAX_BONDS)];


nitpick: please rename to peer_id_addrs

rymanluk · 2020-04-09T08:10:36Z

nimble/host/include/host/ble_gap.h

+ *                              A BLE host core return code on unexpected
+ *                                  error.
+ */
+int ble_gap_unpair_oldest_except_curr(const ble_addr_t *curr_peer);


let us call it ble_gap_unpair_oldest_except () and change the parameter name to peer_addr.
This functionality is not really dedicated only to current device. You might want to have device which you don't want to unpair no matter what :)

rymanluk · 2020-04-09T08:14:31Z

nimble/host/src/ble_store_util.c

+            return ble_gap_unpair_oldest_peer();
+        case BLE_STORE_OBJ_TYPE_CCCD:
+            /* Try to remove unbonded CCCDs first */
+            if ((rc = ble_store_clean_old_cccds((void *) &event->overflow.value->cccd.peer_addr)) == BLE_HS_ENOENT) {


I don't think we need to clean unbodend CCCDs as we do not keep such in the storage. Unless I miss something, but CCCDs are stored only for bonded devices, and once unbonded, we do clean CCCDs. If it does not work like this (I believe it does) then we have problem somewhere else.

In this case we just need to call the new function and that is it. Nothing else is needed.

Hi @rymanluk Please let me know if I am missing something. As per my understanding CCCDs for unbonded devices can get stored as ble_gatts_clt_cfg_access --> ble_store_write_cccd --> ble_store_write(BLE_STORE_OBJ_TYPE_CCCD, store_value).

Edit:
Please disregard my earlier comment, missed on cccd_value.flags == 0 check.

I don't think we need to clean unbodend CCCDs as we do not keep such in the storage. Unless I miss something, but CCCDs are stored only for bonded devices, and once unbonded, we do clean CCCDs. If it does not work like this (I believe it does) then we have problem somewhere else.

In this case we just need to call the new function and that is it. Nothing else is needed.

@rymanluk CCCD's do get stored if the devices pair but one or both of them does not accept bonding, so cleaning those up makes sense in this case. Although it would be better if that never happened to begin with.

@h2zero actually CCCD's do get stored when devices are bonded. That happens after pairing is completed and both devices have set bonding flag.

If you do see an issue that CCCDs are stored but device is not actually bonded, please report it as an issue.

rymanluk

Looks good, one minor comment.

rymanluk · 2020-04-16T11:59:19Z

nimble/host/src/ble_store_util.c

+            return ble_gap_unpair_oldest_peer();
+        case BLE_STORE_OBJ_TYPE_CCCD:
+            /* Try unpairing oldest peer except current peer */
+            return ble_gap_unpair_oldest_except((void *) &event->overflow.value->cccd.peer_addr);


nitpick: (void *) seems to be not needed here.

- Add supporting API to skip input peer while unpairing oldest peer

apache-mynewt-bot · 2020-04-16T15:34:36Z

Style check summary

No suggestions at this time!

rymanluk

looks good to me. Thanks

Change list: - Reduces the size of the compiled binary, PR: espressif/esp-nimble#6 - Null pointer check, PR: apache/mynewt-nimble#701 - Pairing procedure abort on unexpected req: apache/mynewt-nimble#710 - Fix conn flags after pairing: apache/mynewt-nimble#730 - Remove notification for update process timeout (Vol 6, Part B, section 5.2 ): apache/mynewt-nimble#782 - CCCD fix : apache/mynewt-nimble#790 and apache/mynewt-nimble#804 - Host based Privacy (RPA) fix: espressif/esp-nimble#7 Closes espressif/esp-nimble#10 Closes #4413

… (backport v3.3) Change list: - Reduces the size of the compiled binary, PR: espressif/esp-nimble#6 - Null pointer check, PR: apache/mynewt-nimble#701 - Pairing procedure abort on unexpected req: apache/mynewt-nimble#710 - Fix conn flags after pairing: apache/mynewt-nimble#730 - Remove notification for update process timeout (Vol 6, Part B, section 5.2 ): apache/mynewt-nimble#782 - CCCD fix : apache/mynewt-nimble#790 and apache/mynewt-nimble#804 - Host based Privacy (RPA) fix: espressif/esp-nimble#7 Closes espressif/esp-nimble#10 Closes espressif/esp-idf#4413

… (backport v4.0) Change list: - Reduces the size of the compiled binary, PR: espressif/esp-nimble#6 - Null pointer check, PR: apache/mynewt-nimble#701 - Pairing procedure abort on unexpected req: apache/mynewt-nimble#710 - Fix conn flags after pairing: apache/mynewt-nimble#730 - Remove notification for update process timeout (Vol 6, Part B, section 5.2 ): apache/mynewt-nimble#782 - CCCD fix : apache/mynewt-nimble#790 and apache/mynewt-nimble#804 - Host based Privacy (RPA) fix: espressif/esp-nimble#7 Closes espressif/esp-nimble#10 Closes #4413

… (backport v4.1) Change list: - Reduces the size of the compiled binary, PR: espressif/esp-nimble#6 - Null pointer check, PR: apache/mynewt-nimble#701 - Pairing procedure abort on unexpected req: apache/mynewt-nimble#710 - Fix conn flags after pairing: apache/mynewt-nimble#730 - Remove notification for update process timeout (Vol 6, Part B, section 5.2 ): apache/mynewt-nimble#782 - CCCD fix : apache/mynewt-nimble#790 and apache/mynewt-nimble#804 - Host based Privacy (RPA) fix: espressif/esp-nimble#7 Closes espressif/esp-nimble#10 Closes #4413

rymanluk requested changes Apr 3, 2020

View reviewed changes

nimble/host: Fix return code in ble_gap_unpair_oldest_peer when no …

9cb4412

…bonded peer exist

prasad-alatkar force-pushed the fix/ble_store_overflow_handling branch 4 times, most recently from 1f5ad85 to d2012a8 Compare April 3, 2020 16:44

rymanluk reviewed Apr 4, 2020

View reviewed changes

prasad-alatkar force-pushed the fix/ble_store_overflow_handling branch 5 times, most recently from 286109e to cd7a848 Compare April 8, 2020 05:51

rymanluk requested changes Apr 9, 2020

View reviewed changes

prasad-alatkar force-pushed the fix/ble_store_overflow_handling branch from cd7a848 to 4f38ac7 Compare April 16, 2020 11:42

rymanluk reviewed Apr 16, 2020

View reviewed changes

nimble/store: Fix store behavior when CCCDs exceed maximum limit

bb9303a

- Add supporting API to skip input peer while unpairing oldest peer

prasad-alatkar force-pushed the fix/ble_store_overflow_handling branch from 4f38ac7 to bb9303a Compare April 16, 2020 14:11

rymanluk approved these changes Apr 17, 2020

View reviewed changes

rymanluk merged commit 6661a51 into apache:master Apr 17, 2020

ravibhagavandas mentioned this pull request Apr 24, 2020

NimBLE AFR update and provision to use IRAM memory as 8-bit addressable memory region aws/amazon-freertos#1713

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nimble/store: Fix nimble store behavior when CCCDs exceed static defined limit #790

nimble/store: Fix nimble store behavior when CCCDs exceed static defined limit #790

prasad-alatkar commented Mar 31, 2020 •

edited

Loading

h2zero commented Mar 31, 2020

prasad-alatkar commented Apr 1, 2020

rymanluk left a comment

rymanluk Apr 3, 2020

prasad-alatkar Apr 3, 2020

rymanluk Apr 4, 2020

prasad-alatkar Apr 6, 2020

rymanluk Apr 6, 2020

h2zero Apr 7, 2020

prasad-alatkar Apr 7, 2020

rymanluk Apr 8, 2020

prasad-alatkar Apr 8, 2020

rymanluk left a comment

rymanluk Apr 9, 2020

prasad-alatkar Apr 9, 2020

rymanluk Apr 9, 2020

rymanluk Apr 9, 2020

rymanluk Apr 9, 2020

prasad-alatkar Apr 9, 2020

rymanluk Apr 9, 2020

prasad-alatkar Apr 9, 2020 •

edited

Loading

h2zero Apr 13, 2020

rymanluk Apr 14, 2020 •

edited

Loading

rymanluk left a comment

rymanluk Apr 16, 2020

apache-mynewt-bot commented Apr 16, 2020

rymanluk left a comment

nimble/store: Fix nimble store behavior when CCCDs exceed static defined limit #790

nimble/store: Fix nimble store behavior when CCCDs exceed static defined limit #790

Conversation

prasad-alatkar commented Mar 31, 2020 • edited Loading

h2zero commented Mar 31, 2020

prasad-alatkar commented Apr 1, 2020

rymanluk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rymanluk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prasad-alatkar Apr 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rymanluk Apr 14, 2020 • edited Loading

Choose a reason for hiding this comment

rymanluk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apache-mynewt-bot commented Apr 16, 2020

Style check summary

No suggestions at this time!

rymanluk left a comment

Choose a reason for hiding this comment

prasad-alatkar commented Mar 31, 2020 •

edited

Loading

prasad-alatkar Apr 9, 2020 •

edited

Loading

rymanluk Apr 14, 2020 •

edited

Loading