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

Fix commissioning problem due to retransmissions #1207

Merged
merged 1 commit into from Jan 27, 2017

Conversation

Projects
None yet
4 participants
@hubertmis
Member

hubertmis commented Jan 25, 2017

We had a commissioning problem on a nrf52 chip.

nrf52 802.15.4 driver is running from IRQ context and it uses the rx queue. That's why it queues all retransmitted messages during commissioning procedure. Probably you didn't notice this problem using TI, because it's driver does not have rx queues and it drops DTLS retransmissions during commissioning procedure.

We didn't implement crypto hardware acceleration in nrf52 driver yet and some procedures take more than 8 seconds.

Because of that typical commissioning procedure looks like following diagram:

    Joiner                        Commissioner
(1) |      Client Hello           |
    |---------------------------->|
    |                             |
(2) |      Hello Verify Request   |
    |<----------------------------|
    |                             |
(3) |      Client Hello           |
    |---------------------------->|
8 s |                             |
(4) |      Client Hello           |  retransmission
    |---------------------------->|
    |                             |
(5) |      Server Hello           |
    |      Server Key Exchange    |
    |      Server Hello Done      |
    |<----------------------------|
    |                             |
(6) |      Server Hello           |  response to retransmission
    |      Server Key Exchange    |
    |      Server Hello Done      |
    |<----------------------------|
8 s |                             |
(7) |      Server Hello           |  retransmission
    |      Server Key Exchange    |
    |      Server Hello Done      |
    |<----------------------------|
    |                             |
(8) |      Client Key Exchange    |
    |      Change Cipher Spec     |
    | Encrypted Handshake Message |
    |---------------------------->|
    |                             |
(9) |      Change Cipher Spec     |
    | Encrypted Handshake Message |
    |      Application Data       |
    |<----------------------------|
    |                             |
(10)|      Client Key Exchange    |  retransmission
    |      Change Cipher Spec     |
    | Encrypted Handshake Message |
    |---------------------------->|

We found 2 problems:

  1. Joiner queues retransmissions (6) and (7) during reply (8) preparation. After message flight (8) is sent Joiner receives queued retransmissions, but it removes only 1 record from it's queue (instead of all 6 records: (6) - Server Hello, (6) - Server Key Exchange, (6) - Server Hello Done, (7) - Server Hello, (7) - Server Key Exchange, (7) - Server Hello Done). When Joiner receives (9) it cannot parse it due to old records in the queue.
  2. Commissioner does not retransmit message flight (9) when it receives retransmission (10). According to RFC 6347: 4.2.4 it should retransmit to avoid dead lock.

This PR solves both problems. And it makes commissioning procedure robust.

@jwhui jwhui added bug P2 labels Jan 25, 2017

@jwhui

This comment has been minimized.

Member

jwhui commented Jan 25, 2017

@hubertmis, thanks for raising this issue!

Note that the OpenThread project has a policy where imported third-party code in third_party is pristine. In particular, third_party/mbedtls/repo is a pristine copy of mbedtls-2.4.1.

The strongly preferred method of updating third-party code is by upstreaming changes directly to the third-party project itself. Another option is to put any modifications into third_party/mbedtls. For example, you could include a patch file that is applied as part of the make process or the modified file itself.

@codecov-io

This comment has been minimized.

codecov-io commented Jan 26, 2017

Current coverage is 73.04% (diff: 100%)

Merging #1207 into master will decrease coverage by 0.31%

@@             master      #1207   diff @@
==========================================
  Files           128        127     -1   
  Lines         17702      17693     -9   
  Methods        2502       2498     -4   
  Messages          0          0          
  Branches       2141       2141          
==========================================
- Hits          12986      12924    -62   
- Misses         3909       3949    +40   
- Partials        807        820    +13   

Powered by Codecov. Last update 19b62df...16d36a9

@hubertmis

This comment has been minimized.

Member

hubertmis commented Jan 26, 2017

I've created a PR in the mbedtls upstream ARMmbed/mbedtls#772
and a patch file that is automatically applied by autotools.

@jwhui jwhui requested a review from librasungirl Jan 26, 2017

@librasungirl

LGTM 👍

@jwhui

jwhui approved these changes Jan 27, 2017

LGTM 👍

@jwhui jwhui merged commit f820783 into openthread:master Jan 27, 2017

3 checks passed

cla/google All necessary CLAs are signed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@hubertmis hubertmis deleted the NordicPlayground:pr/fix-commissioning-problem-due-to-retransmissions branch Jan 27, 2017

amejias added a commit to Zolertia/openthread that referenced this pull request Mar 8, 2017

vaas-krish pushed a commit to vaas-krish/openthread that referenced this pull request May 23, 2017

Merge pull request openthread#221 in TPS/openthread from feature/gith…
…ub_61138b to master

* commit '089d6de362f0b31d207fc1a709afb66505aea117':
  #include misc.h for otPlatAssertFail. (openthread#1223)
  Apply patch to mbedtls source files during parallel build. (openthread#1221)
  Fix REED behavior on migration (openthread#1179)
  Add patch to mbedtls that fixes commissioning problem with retransmitted messages. (openthread#1207)
  Reset receive frame counters on master key change. (openthread#1218)
  Do not include CoAP Payload Marker when there is no payload. (openthread#1214)
  ncp: Remove deprecated flen encoding. (openthread#1213)
  Make mNumFreeBuffers unsigned (openthread#1204)
  NCP Usage of otLinkRaw API (openthread#1205)
  Process Link and MLE Frame Counter TLVs in Child Update Response. (openthread#1209)
  Separate route cost from link cost to allow routing when the link is lost. (openthread#1187)
  Initialize mLastPartitionRouterIdSequence and mLastPartitionId. (openthread#1208)
  NcpBase: Counter for out of order received spinel frames (openthread#1210)
  NcpBase: Adding get handler for `MAC_EXTENDED_ADDR` spinel property (openthread#1212)
  Hdlc: Remove unused include file "message.h" (openthread#1211)
  THCI: Commit with new assigned Activetimestamp if any instead of default value. (openthread#1206)
  Use syslog for logging on posix. (openthread#1203)
  Fix logic governing DataRequest PollTimer. (openthread#1192)
  enable mac security for fragmented mle message (openthread#1202)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment