You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mynewt.apache.org by GitBox <gi...@apache.org> on 2020/08/31 20:19:00 UTC

[GitHub] [mynewt-nimble] haukepetersen opened a new pull request #856: controller: properly seed srand() during initialization

haukepetersen opened a new pull request #856:
URL: https://github.com/apache/mynewt-nimble/pull/856


   This one took me a long time to find and it had devastating effects: the STDLIBs pseudo random number generator was not properly seeded by the controller. As the comment in the code, wisely starting with `XXX`, perfectly stated: `This really doesn't belong here, as the address probably has not been set yet.`. At least when using the NimBLE controller with RIOT, that comment is absolutely true... So when running multiple nodes, all use the same seed making their random number identical.
   
   When running my deployment using 15 nRF52xxx nodes in a multi-hop network, I observed the following faulty behaviors:
   - application data was duplicated and received on nodes that did not have a direct connection to nodes sending this data
   - L2CAP COC flow control getting out of sync: RX or TX credit counts exceeding their initial limits
   
   As far as I can tell by now, this was all caused by multiple connections using the same access addresses, in term caused by all nodes using the same seed value.
   
   Now this PR fixes the seeding issue by simply using the hardware random number generator to seed the pseudo random number generator. This way it should be guaranteed, that nodes always use different seed values. Also the runtime-overhead should be acceptable, since this is only called once during controller initialization.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [mynewt-nimble] apache-mynewt-bot removed a comment on pull request #856: controller: properly seed srand() during initialization

Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot removed a comment on pull request #856:
URL: https://github.com/apache/mynewt-nimble/pull/856#issuecomment-684018264


   
   <!-- style-bot -->
   
   ## Style check summary
   
   #### No suggestions at this time!
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [mynewt-nimble] andrzej-kaczmarek commented on pull request #856: controller: properly seed srand() during initialization

Posted by GitBox <gi...@apache.org>.
andrzej-kaczmarek commented on pull request #856:
URL: https://github.com/apache/mynewt-nimble/pull/856#issuecomment-690321682


   @haukepetersen #863 is merged so CI should be happy if you rebase it now


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [mynewt-nimble] haukepetersen commented on a change in pull request #856: controller: properly seed srand() during initialization

Posted by GitBox <gi...@apache.org>.
haukepetersen commented on a change in pull request #856:
URL: https://github.com/apache/mynewt-nimble/pull/856#discussion_r481031056



##########
File path: nimble/controller/src/ble_ll.c
##########
@@ -1682,15 +1663,17 @@ ble_ll_init(void)
     features |= BLE_LL_FEAT_ISO_HOST_SUPPORT;
 #endif
 
+    lldata->ll_supp_features = features;
+
     /* Initialize random number generation */
     ble_ll_rand_init();
-
-    /* XXX: This really doesn't belong here, as the address probably has not
-     * been set yet.
-     */
-    ble_ll_seed_prng();
-
-    lldata->ll_supp_features = features;
+    /* Start the random number generator */
+    ble_ll_rand_start();
+    /* Use the random number generator to seed the STDLIBs pseudo-number
+     * generator */
+    unsigned seed;

Review comment:
       done




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [mynewt-nimble] haukepetersen merged pull request #856: controller: properly seed srand() during initialization

Posted by GitBox <gi...@apache.org>.
haukepetersen merged pull request #856:
URL: https://github.com/apache/mynewt-nimble/pull/856


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [mynewt-nimble] sjanc commented on pull request #856: controller: properly seed srand() during initialization

Posted by GitBox <gi...@apache.org>.
sjanc commented on pull request #856:
URL: https://github.com/apache/mynewt-nimble/pull/856#issuecomment-690261340


   this needs to wait until https://github.com/apache/mynewt-nimble/pull/863 is merged
   otherwise unit tests will fail


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [mynewt-nimble] apache-mynewt-bot commented on pull request #856: controller: properly seed srand() during initialization

Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot commented on pull request #856:
URL: https://github.com/apache/mynewt-nimble/pull/856#issuecomment-684018264


   
   <!-- style-bot -->
   
   ## Style check summary
   
   #### No suggestions at this time!
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [mynewt-nimble] haukepetersen commented on pull request #856: controller: properly seed srand() during initialization

Posted by GitBox <gi...@apache.org>.
haukepetersen commented on pull request #856:
URL: https://github.com/apache/mynewt-nimble/pull/856#issuecomment-690617724


   done :-)


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [mynewt-nimble] apache-mynewt-bot commented on pull request #856: controller: properly seed srand() during initialization

Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot commented on pull request #856:
URL: https://github.com/apache/mynewt-nimble/pull/856#issuecomment-684781314


   
   <!-- style-bot -->
   
   ## Style check summary
   
   #### No suggestions at this time!
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [mynewt-nimble] apache-mynewt-bot removed a comment on pull request #856: controller: properly seed srand() during initialization

Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot removed a comment on pull request #856:
URL: https://github.com/apache/mynewt-nimble/pull/856#issuecomment-684781314


   
   <!-- style-bot -->
   
   ## Style check summary
   
   #### No suggestions at this time!
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [mynewt-nimble] haukepetersen commented on pull request #856: controller: properly seed srand() during initialization

Posted by GitBox <gi...@apache.org>.
haukepetersen commented on pull request #856:
URL: https://github.com/apache/mynewt-nimble/pull/856#issuecomment-688136277


   @sjanc any chance you could verify the current version? Would be nice to have this in :-)


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [mynewt-nimble] haukepetersen commented on pull request #856: controller: properly seed srand() during initialization

Posted by GitBox <gi...@apache.org>.
haukepetersen commented on pull request #856:
URL: https://github.com/apache/mynewt-nimble/pull/856#issuecomment-690716218


   all green -> go.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [mynewt-nimble] haukepetersen commented on pull request #856: controller: properly seed srand() during initialization

Posted by GitBox <gi...@apache.org>.
haukepetersen commented on pull request #856:
URL: https://github.com/apache/mynewt-nimble/pull/856#issuecomment-684727659


   addressed comment.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [mynewt-nimble] apache-mynewt-bot commented on pull request #856: controller: properly seed srand() during initialization

Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot commented on pull request #856:
URL: https://github.com/apache/mynewt-nimble/pull/856#issuecomment-690626739


   
   <!-- style-bot -->
   
   ## Style check summary
   
   #### No suggestions at this time!
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [mynewt-nimble] sjanc commented on a change in pull request #856: controller: properly seed srand() during initialization

Posted by GitBox <gi...@apache.org>.
sjanc commented on a change in pull request #856:
URL: https://github.com/apache/mynewt-nimble/pull/856#discussion_r481012643



##########
File path: nimble/controller/src/ble_ll.c
##########
@@ -1682,15 +1663,17 @@ ble_ll_init(void)
     features |= BLE_LL_FEAT_ISO_HOST_SUPPORT;
 #endif
 
+    lldata->ll_supp_features = features;
+
     /* Initialize random number generation */
     ble_ll_rand_init();
-
-    /* XXX: This really doesn't belong here, as the address probably has not
-     * been set yet.
-     */
-    ble_ll_seed_prng();
-
-    lldata->ll_supp_features = features;
+    /* Start the random number generator */
+    ble_ll_rand_start();
+    /* Use the random number generator to seed the STDLIBs pseudo-number
+     * generator */
+    unsigned seed;

Review comment:
       good catch!
   nitpick : declare seed at function top :)




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org